[Cluster-devel] [PATCH] Fix freeze of cluster-2.03.11

[Cluster-devel] [PATCH] Fix freeze of cluster-2.03.11

[Kadlecsik Jozsef]

Hi,

I reported on the linux-cluster mailing list (see thread
https://www.redhat.com/archives/linux-cluster/2009-March/msg00176.html)
that version cluster-2.03.11 freezes.

As Wendy Cheng pointed out, the reason for this is that the filesystem API 
of the kernel changed and "put_inode" was replaced by "drop_inode": while 
"put_inode" was called without holding any lock, "drop_inode" is called 
under inode_lock held but the called gfs_sync_page_i may block
(https://www.redhat.com/archives/linux-cluster/2009-April/msg00060.html).

The patch below (applied in gfs-kernel/src/gfs/) fixes the bug by 
releasing the lock before calling gfs_sync_page_i and locking it back 
after the function call:

--- gfs-orig/ops_super.c	2009-01-22 13:33:51.000000000 +0100
+++ gfs/ops_super.c	2009-04-06 13:07:06.000000000 +0200
@@ -9,6 +9,7 @@
 #include <linux/statfs.h>
 #include <linux/seq_file.h>
 #include <linux/mount.h>
+#include <linux/writeback.h>
 
 #include "gfs.h"
 #include "dio.h"
@@ -68,8 +69,11 @@
 	if (ip &&
 	    !inode->i_nlink &&
 	    S_ISREG(inode->i_mode) &&
-	    !sdp->sd_args.ar_localcaching)
+	    !sdp->sd_args.ar_localcaching) {
+	    	spin_unlock(&inode_lock);
 		gfs_sync_page_i(inode, DIO_START | DIO_WAIT);
+		spin_lock(&inode_lock);
+	}
 	generic_drop_inode(inode);
 }
 

Additionally, the line

EXPORT_SYMBOL(inode_lock);

must be added to fs/inode.c in order to the inode_lock symbol get 
exported.

Best regards,
Jozsef
--
E-mail : kadlec at mail.kfki.hu, kadlec at blackhole.kfki.hu
PGP key: http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address: KFKI Research Institute for Particle and Nuclear Physics
         H-1525 Budapest 114, POB. 49, Hungary

[Cluster-devel] [PATCH] Fix freeze of cluster-2.03.11

[Fabio M. Di Nitto]

Hi,

On Mon, 2009-04-20 at 14:27 +0200, Kadlecsik Jozsef wrote:
> Hi,
> 
> I reported on the linux-cluster mailing list (see thread
> https://www.redhat.com/archives/linux-cluster/2009-March/msg00176.html)
> that version cluster-2.03.11 freezes.
> 
> As Wendy Cheng pointed out, the reason for this is that the filesystem API 
> of the kernel changed and "put_inode" was replaced by "drop_inode": while 
> "put_inode" was called without holding any lock, "drop_inode" is called 
> under inode_lock held but the called gfs_sync_page_i may block
> (https://www.redhat.com/archives/linux-cluster/2009-April/msg00060.html).
> 
> The patch below (applied in gfs-kernel/src/gfs/) fixes the bug by 
> releasing the lock before calling gfs_sync_page_i and locking it back 
> after the function call:
> 
> --- gfs-orig/ops_super.c	2009-01-22 13:33:51.000000000 +0100
> +++ gfs/ops_super.c	2009-04-06 13:07:06.000000000 +0200
> @@ -9,6 +9,7 @@
>  #include <linux/statfs.h>
>  #include <linux/seq_file.h>
>  #include <linux/mount.h>
> +#include <linux/writeback.h>
>  
>  #include "gfs.h"
>  #include "dio.h"
> @@ -68,8 +69,11 @@
>  	if (ip &&
>  	    !inode->i_nlink &&
>  	    S_ISREG(inode->i_mode) &&
> -	    !sdp->sd_args.ar_localcaching)
> +	    !sdp->sd_args.ar_localcaching) {
> +	    	spin_unlock(&inode_lock);
>  		gfs_sync_page_i(inode, DIO_START | DIO_WAIT);
> +		spin_lock(&inode_lock);
> +	}
>  	generic_drop_inode(inode);
>  }
>  
> 
> Additionally, the line
> 
> EXPORT_SYMBOL(inode_lock);
> 
> must be added to fs/inode.c in order to the inode_lock symbol get 
> exported.

thanks a lot for you effort but we will need to find another solution.

it's been a long time since gfs1 required extra exported symbols from
the kernel and asking distributions to do that again is not really an
option (not sure if there any other option either).

I'll let the gfs guys look at it.

Thanks again!
Fabio

[Cluster-devel] [PATCH] Fix freeze of cluster-2.03.11

[Steven Whitehouse]

Hi,

On Mon, 2009-04-20 at 15:14 +0200, Fabio M. Di Nitto wrote:
> Hi,
> 
> On Mon, 2009-04-20 at 14:27 +0200, Kadlecsik Jozsef wrote:
> > Hi,
> > 
> > I reported on the linux-cluster mailing list (see thread
> > https://www.redhat.com/archives/linux-cluster/2009-March/msg00176.html)
> > that version cluster-2.03.11 freezes.
> > 
> > As Wendy Cheng pointed out, the reason for this is that the filesystem API 
> > of the kernel changed and "put_inode" was replaced by "drop_inode": while 
> > "put_inode" was called without holding any lock, "drop_inode" is called 
> > under inode_lock held but the called gfs_sync_page_i may block
> > (https://www.redhat.com/archives/linux-cluster/2009-April/msg00060.html).
> >
Its not a replacement as such, they are different operations.
->put_inode() was called on each and every iput() (despite what the docs
said, as Christoph noted) whereas ->drop_inode() is only called when the
inode is being removed from the cache.

The purpose of drop inode is to allow the fs to override the decision
about whether the inode needs to be dallocated (->delete_inode()) or not
(->clear_inode()).

Christoph's patch is here:

http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=33dcdac2df54e66c447ae03f58c95c7251aa5649

>  
> > The patch below (applied in gfs-kernel/src/gfs/) fixes the bug by 
> > releasing the lock before calling gfs_sync_page_i and locking it back 
> > after the function call:
> > 
> > --- gfs-orig/ops_super.c	2009-01-22 13:33:51.000000000 +0100
> > +++ gfs/ops_super.c	2009-04-06 13:07:06.000000000 +0200
> > @@ -9,6 +9,7 @@
> >  #include <linux/statfs.h>
> >  #include <linux/seq_file.h>
> >  #include <linux/mount.h>
> > +#include <linux/writeback.h>
> >  
> >  #include "gfs.h"
> >  #include "dio.h"
> > @@ -68,8 +69,11 @@
> >  	if (ip &&
> >  	    !inode->i_nlink &&
> >  	    S_ISREG(inode->i_mode) &&
> > -	    !sdp->sd_args.ar_localcaching)
> > +	    !sdp->sd_args.ar_localcaching) {
> > +	    	spin_unlock(&inode_lock);
> >  		gfs_sync_page_i(inode, DIO_START | DIO_WAIT);
> > +		spin_lock(&inode_lock);
> > +	}
> >  	generic_drop_inode(inode);
> >  }
> >
You can't do this in this particular way because you must set the
inode's state before releasing the inode_lock. That is done in
generic_drop_inode() and the functions that it calls.

Not to mention which, this doesn't seem to make any sense. It appears to
be trying to write the inode's data back to disk, but if we've got to
this point in the inode's life cycle, there should be nothing to write.

Since its only called when i_nlink is 0, thats even more strange since
(a) it could be moved to ->delete_inode() and (b) why do we care about
forcing out dirty data for an inode thats being unlinked anyway....

This patch raises more questions than it answers I think.

>   
> > 
> > Additionally, the line
> > 
> > EXPORT_SYMBOL(inode_lock);
> > 
> > must be added to fs/inode.c in order to the inode_lock symbol get 
> > exported.
> 
> thanks a lot for you effort but we will need to find another solution.
> 
> it's been a long time since gfs1 required extra exported symbols from
> the kernel and asking distributions to do that again is not really an
> option (not sure if there any other option either).
> 
> I'll let the gfs guys look at it.
> 
> Thanks again!
> Fabio
> 
Yes, we can't have any of our own exports now (since gfs isn't in the
upstream kernel), as per the ruling from the Fedora people,

Steve.

[Cluster-devel] [PATCH] Fix freeze of cluster-2.03.11

[Kadlecsik Jozsef]

Hi,

On Mon, 20 Apr 2009, Steven Whitehouse wrote:

> On Mon, 2009-04-20 at 15:14 +0200, Fabio M. Di Nitto wrote:
> > 
> > On Mon, 2009-04-20 at 14:27 +0200, Kadlecsik Jozsef wrote:
> > > 
> > > I reported on the linux-cluster mailing list (see thread
> > > https://www.redhat.com/archives/linux-cluster/2009-March/msg00176.html)
> > > that version cluster-2.03.11 freezes.
> > > 
> > > As Wendy Cheng pointed out, the reason for this is that the filesystem API 
> > > of the kernel changed and "put_inode" was replaced by "drop_inode": while 
> > > "put_inode" was called without holding any lock, "drop_inode" is called 
> > > under inode_lock held but the called gfs_sync_page_i may block
> > > (https://www.redhat.com/archives/linux-cluster/2009-April/msg00060.html).
> > >
> Its not a replacement as such, they are different operations.
> ->put_inode() was called on each and every iput() (despite what the docs
> said, as Christoph noted) whereas ->drop_inode() is only called when the
> inode is being removed from the cache.

I knew I worded it incorrectly ;-).
 
> The purpose of drop inode is to allow the fs to override the decision
> about whether the inode needs to be dallocated (->delete_inode()) or not
> (->clear_inode()).
> 
> Christoph's patch is here:
> 
> http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=33dcdac2df54e66c447ae03f58c95c7251aa5649
> 
> >  
> > > The patch below (applied in gfs-kernel/src/gfs/) fixes the bug by 
> > > releasing the lock before calling gfs_sync_page_i and locking it back 
> > > after the function call:
> > > 
> > > --- gfs-orig/ops_super.c	2009-01-22 13:33:51.000000000 +0100
> > > +++ gfs/ops_super.c	2009-04-06 13:07:06.000000000 +0200
> > > @@ -9,6 +9,7 @@
> > >  #include <linux/statfs.h>
> > >  #include <linux/seq_file.h>
> > >  #include <linux/mount.h>
> > > +#include <linux/writeback.h>
> > >  
> > >  #include "gfs.h"
> > >  #include "dio.h"
> > > @@ -68,8 +69,11 @@
> > >  	if (ip &&
> > >  	    !inode->i_nlink &&
> > >  	    S_ISREG(inode->i_mode) &&
> > > -	    !sdp->sd_args.ar_localcaching)
> > > +	    !sdp->sd_args.ar_localcaching) {
> > > +	    	spin_unlock(&inode_lock);
> > >  		gfs_sync_page_i(inode, DIO_START | DIO_WAIT);
> > > +		spin_lock(&inode_lock);
> > > +	}
> > >  	generic_drop_inode(inode);
> > >  }
> > >
> You can't do this in this particular way because you must set the
> inode's state before releasing the inode_lock. That is done in
> generic_drop_inode() and the functions that it calls.
> 
> Not to mention which, this doesn't seem to make any sense. It appears to
> be trying to write the inode's data back to disk, but if we've got to
> this point in the inode's life cycle, there should be nothing to write.
> 
> Since its only called when i_nlink is 0, thats even more strange since
> (a) it could be moved to ->delete_inode() and (b) why do we care about
> forcing out dirty data for an inode thats being unlinked anyway....
> 
> This patch raises more questions than it answers I think.

Do you imply it is superfluous to call gfs_sync_page_i at all, and in 
consequence there's no need for a gfs-specific ->drop_inode function?

Just for the record: without unlocking/locking, the node having a mailmain 
queue over GFS got frozen in a few seconds as mailman queue manager was 
started. With the patch above it ran smoothly for a longer (test) period.

Best regards,
Jozsef
--
E-mail : kadlec at mail.kfki.hu, kadlec at blackhole.kfki.hu
PGP key: http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address: KFKI Research Institute for Particle and Nuclear Physics
         H-1525 Budapest 114, POB. 49, Hungary

[Cluster-devel] [PATCH] Fix freeze of cluster-2.03.11

[Steven Whitehouse]

Hi,

On Mon, 2009-04-20 at 18:36 +0200, Kadlecsik Jozsef wrote:
> Hi,
> 
> On Mon, 20 Apr 2009, Steven Whitehouse wrote:
> 
> > On Mon, 2009-04-20 at 15:14 +0200, Fabio M. Di Nitto wrote:
> > > 
> > > On Mon, 2009-04-20 at 14:27 +0200, Kadlecsik Jozsef wrote:
> > > > 
> > > > I reported on the linux-cluster mailing list (see thread
> > > > https://www.redhat.com/archives/linux-cluster/2009-March/msg00176.html)
> > > > that version cluster-2.03.11 freezes.
> > > > 
> > > > As Wendy Cheng pointed out, the reason for this is that the filesystem API 
> > > > of the kernel changed and "put_inode" was replaced by "drop_inode": while 
> > > > "put_inode" was called without holding any lock, "drop_inode" is called 
> > > > under inode_lock held but the called gfs_sync_page_i may block
> > > > (https://www.redhat.com/archives/linux-cluster/2009-April/msg00060.html).
> > > >
> > Its not a replacement as such, they are different operations.
> > ->put_inode() was called on each and every iput() (despite what the docs
> > said, as Christoph noted) whereas ->drop_inode() is only called when the
> > inode is being removed from the cache.
> 
> I knew I worded it incorrectly ;-).
>  
> > The purpose of drop inode is to allow the fs to override the decision
> > about whether the inode needs to be dallocated (->delete_inode()) or not
> > (->clear_inode()).
> > 
> > Christoph's patch is here:
> > 
> > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=33dcdac2df54e66c447ae03f58c95c7251aa5649
> > 
> > >  
> > > > The patch below (applied in gfs-kernel/src/gfs/) fixes the bug by 
> > > > releasing the lock before calling gfs_sync_page_i and locking it back 
> > > > after the function call:
> > > > 
> > > > --- gfs-orig/ops_super.c	2009-01-22 13:33:51.000000000 +0100
> > > > +++ gfs/ops_super.c	2009-04-06 13:07:06.000000000 +0200
> > > > @@ -9,6 +9,7 @@
> > > >  #include <linux/statfs.h>
> > > >  #include <linux/seq_file.h>
> > > >  #include <linux/mount.h>
> > > > +#include <linux/writeback.h>
> > > >  
> > > >  #include "gfs.h"
> > > >  #include "dio.h"
> > > > @@ -68,8 +69,11 @@
> > > >  	if (ip &&
> > > >  	    !inode->i_nlink &&
> > > >  	    S_ISREG(inode->i_mode) &&
> > > > -	    !sdp->sd_args.ar_localcaching)
> > > > +	    !sdp->sd_args.ar_localcaching) {
> > > > +	    	spin_unlock(&inode_lock);
> > > >  		gfs_sync_page_i(inode, DIO_START | DIO_WAIT);
> > > > +		spin_lock(&inode_lock);
> > > > +	}
> > > >  	generic_drop_inode(inode);
> > > >  }
> > > >
> > You can't do this in this particular way because you must set the
> > inode's state before releasing the inode_lock. That is done in
> > generic_drop_inode() and the functions that it calls.
> > 
> > Not to mention which, this doesn't seem to make any sense. It appears to
> > be trying to write the inode's data back to disk, but if we've got to
> > this point in the inode's life cycle, there should be nothing to write.
> > 
> > Since its only called when i_nlink is 0, thats even more strange since
> > (a) it could be moved to ->delete_inode() and (b) why do we care about
> > forcing out dirty data for an inode thats being unlinked anyway....
> > 
> > This patch raises more questions than it answers I think.
> 
> Do you imply it is superfluous to call gfs_sync_page_i at all, and in 
> consequence there's no need for a gfs-specific ->drop_inode function?
> 
Probably there is no need for it. Its not at all obvious what the
gfs_sync_page_i was supposed to do, so its tricky to see that its not
needed at all. I think we can be fairly certain that it shouldn't be in
->drop_inode() though, and that if it is needed, it can go into
->delete_inode()

> Just for the record: without unlocking/locking, the node having a mailmain 
> queue over GFS got frozen in a few seconds as mailman queue manager was 
> started. With the patch above it ran smoothly for a longer (test) period.
> 
> Best regards,
> Jozsef
Yes, it doesn't surprise me that you'd see lockups without your patch.
To be on the safe side, try moving the call to gfs_sync_page_i into
->delete_inode so that you can do it after the inode lock has already
been dropped (and after the state has been set correctly too). It won't
harm anything to have that around, but it might slow things down a bit,

Steve.

[Cluster-devel] [PATCH] Fix freeze of cluster-2.03.11

[Kadlecsik Jozsef]

On Tue, 21 Apr 2009, Steven Whitehouse wrote:

> Yes, it doesn't surprise me that you'd see lockups without your patch.
> To be on the safe side, try moving the call to gfs_sync_page_i into
> ->delete_inode so that you can do it after the inode lock has already
> been dropped (and after the state has been set correctly too). It won't
> harm anything to have that around, but it might slow things down a bit,

What do you think about this patch?

--- gfs-orig/ops_super.c	2009-01-22 13:33:51.000000000 +0100
+++ gfs/ops_super.c	2009-04-22 13:51:06.000000000 +0200
@@ -49,7 +49,7 @@
 }
 
 /**
- * gfs_drop_inode - drop an inode
+ * gfs_delete_inode - delete an inode
  * @inode: The inode
  *
  * If i_nlink is zero, any dirty data for the inode is thrown away.
@@ -58,19 +58,19 @@
  */
 
 static void
-gfs_drop_inode(struct inode *inode)
+gfs_delete_inode(struct inode *inode)
 {
 	struct gfs_sbd *sdp = get_v2sdp(inode->i_sb);
 	struct gfs_inode *ip = get_v2ip(inode);
 
-	atomic_inc(&sdp->sd_ops_super);
-
 	if (ip &&
 	    !inode->i_nlink &&
 	    S_ISREG(inode->i_mode) &&
 	    !sdp->sd_args.ar_localcaching)
 		gfs_sync_page_i(inode, DIO_START | DIO_WAIT);
-	generic_drop_inode(inode);
+
+	truncate_inode_pages(&inode->i_data, 0);
+	clear_inode(inode);
 }
 
 /**
@@ -443,7 +443,7 @@
 
 struct super_operations gfs_super_ops = {
 	.write_inode = gfs_write_inode,
-	.drop_inode = gfs_drop_inode,
+	.delete_inode = gfs_delete_inode,
 	.put_super = gfs_put_super,
 	.write_super = gfs_write_super,
 	.write_super_lockfs = gfs_write_super_lockfs,

I'll be able to test it (or it's successor) around the next week.

Best regards,
Jozsef
--
E-mail : kadlec at mail.kfki.hu, kadlec at blackhole.kfki.hu
PGP key: http://www.kfki.hu/~kadlec/pgp_public_key.txt
Address: KFKI Research Institute for Particle and Nuclear Physics
         H-1525 Budapest 114, POB. 49, Hungary

[Cluster-devel] [PATCH] Fix freeze of cluster-2.03.11

[Steven Whitehouse]

Hi,

On Wed, 2009-04-22 at 13:55 +0200, Kadlecsik Jozsef wrote:
> On Tue, 21 Apr 2009, Steven Whitehouse wrote:
> 
> > Yes, it doesn't surprise me that you'd see lockups without your patch.
> > To be on the safe side, try moving the call to gfs_sync_page_i into
> > ->delete_inode so that you can do it after the inode lock has already
> > been dropped (and after the state has been set correctly too). It won't
> > harm anything to have that around, but it might slow things down a bit,
> 
> What do you think about this patch?
> 
> --- gfs-orig/ops_super.c	2009-01-22 13:33:51.000000000 +0100
> +++ gfs/ops_super.c	2009-04-22 13:51:06.000000000 +0200
> @@ -49,7 +49,7 @@
>  }
>  
>  /**
> - * gfs_drop_inode - drop an inode
> + * gfs_delete_inode - delete an inode
>   * @inode: The inode
>   *
>   * If i_nlink is zero, any dirty data for the inode is thrown away.
> @@ -58,19 +58,19 @@
>   */
>  
>  static void
> -gfs_drop_inode(struct inode *inode)
> +gfs_delete_inode(struct inode *inode)
>  {
>  	struct gfs_sbd *sdp = get_v2sdp(inode->i_sb);
>  	struct gfs_inode *ip = get_v2ip(inode);
>  
> -	atomic_inc(&sdp->sd_ops_super);
> -
>  	if (ip &&
>  	    !inode->i_nlink &&
That looks much better, but you don't need to test for !inode->i_nlink
as this function only gets called if that is the case.

>  	    S_ISREG(inode->i_mode) &&
>  	    !sdp->sd_args.ar_localcaching)
>  		gfs_sync_page_i(inode, DIO_START | DIO_WAIT);
I still wonder whether gfs2_sync_page_i is really needed here at all,
but its probably safer to keep it for now I guess.

> -	generic_drop_inode(inode);
> +
> +	truncate_inode_pages(&inode->i_data, 0);
> +	clear_inode(inode);
>  }
>  
>  /**
> @@ -443,7 +443,7 @@
>  
>  struct super_operations gfs_super_ops = {
>  	.write_inode = gfs_write_inode,
> -	.drop_inode = gfs_drop_inode,
> +	.delete_inode = gfs_delete_inode,
>  	.put_super = gfs_put_super,
>  	.write_super = gfs_write_super,
>  	.write_super_lockfs = gfs_write_super_lockfs,
> 
> I'll be able to test it (or it's successor) around the next week.
> 
> Best regards,
> Jozsef

Ok, if we here some positive test results, then we can put this in.
Thanks,

Steve.