[PATCH] gfs2: Get rid of I_MUTEX_QUOTA usage

[PATCH] gfs2: Get rid of I_MUTEX_QUOTA usage

[Jan Kara]

GFS2 uses i_mutex on its system quota inode to synchronize writes to
quota file. Since this is an internal inode to GFS2 (not part of directory
hiearchy or visible by user) we are safe to define locking rules for it. So
let's just get it its own locking class to make it clear.

CC: Steven Whitehouse <swhiteho@redhat.com>
Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/gfs2/ops_fstype.c |    8 ++++++++
 fs/gfs2/quota.c      |    2 +-
 2 files changed, 9 insertions(+), 1 deletions(-)

 So this is probably the simplest what can be done for GFS2 (actually OCFS2
does the same for internal system files). Compile tested only. Steven?

diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
index 6f3a18f..ae8f225 100644
--- a/fs/gfs2/ops_fstype.c
+++ b/fs/gfs2/ops_fstype.c
@@ -19,6 +19,7 @@
 #include <linux/mount.h>
 #include <linux/gfs2_ondisk.h>
 #include <linux/quotaops.h>
+#include <linux/lockdep.h>
 
 #include "gfs2.h"
 #include "incore.h"
@@ -767,6 +768,7 @@ fail:
 	return error;
 }
 
+static struct lock_class_key gfs2_quota_imutex_key;
 
 static int init_inodes(struct gfs2_sbd *sdp, int undo)
 {
@@ -804,6 +806,12 @@ static int init_inodes(struct gfs2_sbd *sdp, int undo)
 		fs_err(sdp, "can't get quota file inode: %d\n", error);
 		goto fail_rindex;
 	}
+	/*
+	 * i_mutex on quota files is special. Since this inode is hidden system
+	 * file, we are safe to define locking ourselves.
+	 */
+	lockdep_set_class(&sdp->sd_quota_inode->i_mutex,
+			  &gfs2_quota_imutex_key);
 
 	error = gfs2_rindex_update(sdp);
 	if (error)
diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index 6019da3..970598b 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -772,7 +772,7 @@ static int do_sync(unsigned int num_qd, struct gfs2_quota_data **qda)
 		return -ENOMEM;
 
 	sort(qda, num_qd, sizeof(struct gfs2_quota_data *), sort_qd, NULL);
-	mutex_lock_nested(&ip->i_inode.i_mutex, I_MUTEX_QUOTA);
+	mutex_lock(&ip->i_inode.i_mutex);
 	for (qx = 0; qx < num_qd; qx++) {
 		error = gfs2_glock_nq_init(qda[qx]->qd_gl, LM_ST_EXCLUSIVE,
 					   GL_NOCACHE, &ghs[qx]);
-- 
1.7.1

Re: [PATCH] gfs2: Get rid of I_MUTEX_QUOTA usage

[Steven Whitehouse]

Hi,

On Thu, 2012-04-26 at 15:20 +0200, Jan Kara wrote:
> GFS2 uses i_mutex on its system quota inode to synchronize writes to
> quota file. Since this is an internal inode to GFS2 (not part of directory
> hiearchy or visible by user) we are safe to define locking rules for it. So
> let's just get it its own locking class to make it clear.
> 
It is visible to the user if the gfs2 metadata filesystem is mounted.
That used to be the way in which the quotas were set by gfs2_quota.
However that is really considered obsolete now, so maybe we don't have
to support that interface any more... the gfs2_quota package is no
longer in distros as the generic quota package does the job for us these
days.

I'd just been looking at using sd_quota_mutex instead of the inode
mutex, but run into that same issue,

Steve.


> CC: Steven Whitehouse <swhiteho@redhat.com>
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/gfs2/ops_fstype.c |    8 ++++++++
>  fs/gfs2/quota.c      |    2 +-
>  2 files changed, 9 insertions(+), 1 deletions(-)
> 
>  So this is probably the simplest what can be done for GFS2 (actually OCFS2
> does the same for internal system files). Compile tested only. Steven?
> 
> diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> index 6f3a18f..ae8f225 100644
> --- a/fs/gfs2/ops_fstype.c
> +++ b/fs/gfs2/ops_fstype.c
> @@ -19,6 +19,7 @@
>  #include <linux/mount.h>
>  #include <linux/gfs2_ondisk.h>
>  #include <linux/quotaops.h>
> +#include <linux/lockdep.h>
>  
>  #include "gfs2.h"
>  #include "incore.h"
> @@ -767,6 +768,7 @@ fail:
>  	return error;
>  }
>  
> +static struct lock_class_key gfs2_quota_imutex_key;
>  
>  static int init_inodes(struct gfs2_sbd *sdp, int undo)
>  {
> @@ -804,6 +806,12 @@ static int init_inodes(struct gfs2_sbd *sdp, int undo)
>  		fs_err(sdp, "can't get quota file inode: %d\n", error);
>  		goto fail_rindex;
>  	}
> +	/*
> +	 * i_mutex on quota files is special. Since this inode is hidden system
> +	 * file, we are safe to define locking ourselves.
> +	 */
> +	lockdep_set_class(&sdp->sd_quota_inode->i_mutex,
> +			  &gfs2_quota_imutex_key);
>  
>  	error = gfs2_rindex_update(sdp);
>  	if (error)
> diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> index 6019da3..970598b 100644
> --- a/fs/gfs2/quota.c
> +++ b/fs/gfs2/quota.c
> @@ -772,7 +772,7 @@ static int do_sync(unsigned int num_qd, struct gfs2_quota_data **qda)
>  		return -ENOMEM;
>  
>  	sort(qda, num_qd, sizeof(struct gfs2_quota_data *), sort_qd, NULL);
> -	mutex_lock_nested(&ip->i_inode.i_mutex, I_MUTEX_QUOTA);
> +	mutex_lock(&ip->i_inode.i_mutex);
>  	for (qx = 0; qx < num_qd; qx++) {
>  		error = gfs2_glock_nq_init(qda[qx]->qd_gl, LM_ST_EXCLUSIVE,
>  					   GL_NOCACHE, &ghs[qx]);

Re: [PATCH] gfs2: Get rid of I_MUTEX_QUOTA usage

[Jan Kara]

  Hello,

On Thu 26-04-12 14:28:39, Steven Whitehouse wrote:
> On Thu, 2012-04-26 at 15:20 +0200, Jan Kara wrote:
> > GFS2 uses i_mutex on its system quota inode to synchronize writes to
> > quota file. Since this is an internal inode to GFS2 (not part of directory
> > hiearchy or visible by user) we are safe to define locking rules for it. So
> > let's just get it its own locking class to make it clear.
> > 
> It is visible to the user if the gfs2 metadata filesystem is mounted.
> That used to be the way in which the quotas were set by gfs2_quota.
> However that is really considered obsolete now, so maybe we don't have
> to support that interface any more... the gfs2_quota package is no
> longer in distros as the generic quota package does the job for us these
> days.
  Ah, OK. But then if users could write to (or even truncate?) the quota
file, wasn't it really deadlockable? mutex_lock_nested would silence
lockdep but the deadlock won't change. Another advantage of my change is
that if there is some problem, lockdep will warn about it because all
places which end up taking i_mutex on quota file will use the same locking
class. Using of lockdep subclasses (i.e. the _nested variant of mutex_lock)
essentially tells lockdep - I promise that uses of the lock tagged as
one locking subclass cannot interact with uses tagged as another locking
class. 

								Honza

> > CC: Steven Whitehouse <swhiteho@redhat.com>
> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/gfs2/ops_fstype.c |    8 ++++++++
> >  fs/gfs2/quota.c      |    2 +-
> >  2 files changed, 9 insertions(+), 1 deletions(-)
> > 
> >  So this is probably the simplest what can be done for GFS2 (actually OCFS2
> > does the same for internal system files). Compile tested only. Steven?
> > 
> > diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> > index 6f3a18f..ae8f225 100644
> > --- a/fs/gfs2/ops_fstype.c
> > +++ b/fs/gfs2/ops_fstype.c
> > @@ -19,6 +19,7 @@
> >  #include <linux/mount.h>
> >  #include <linux/gfs2_ondisk.h>
> >  #include <linux/quotaops.h>
> > +#include <linux/lockdep.h>
> >  
> >  #include "gfs2.h"
> >  #include "incore.h"
> > @@ -767,6 +768,7 @@ fail:
> >  	return error;
> >  }
> >  
> > +static struct lock_class_key gfs2_quota_imutex_key;
> >  
> >  static int init_inodes(struct gfs2_sbd *sdp, int undo)
> >  {
> > @@ -804,6 +806,12 @@ static int init_inodes(struct gfs2_sbd *sdp, int undo)
> >  		fs_err(sdp, "can't get quota file inode: %d\n", error);
> >  		goto fail_rindex;
> >  	}
> > +	/*
> > +	 * i_mutex on quota files is special. Since this inode is hidden system
> > +	 * file, we are safe to define locking ourselves.
> > +	 */
> > +	lockdep_set_class(&sdp->sd_quota_inode->i_mutex,
> > +			  &gfs2_quota_imutex_key);
> >  
> >  	error = gfs2_rindex_update(sdp);
> >  	if (error)
> > diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> > index 6019da3..970598b 100644
> > --- a/fs/gfs2/quota.c
> > +++ b/fs/gfs2/quota.c
> > @@ -772,7 +772,7 @@ static int do_sync(unsigned int num_qd, struct gfs2_quota_data **qda)
> >  		return -ENOMEM;
> >  
> >  	sort(qda, num_qd, sizeof(struct gfs2_quota_data *), sort_qd, NULL);
> > -	mutex_lock_nested(&ip->i_inode.i_mutex, I_MUTEX_QUOTA);
> > +	mutex_lock(&ip->i_inode.i_mutex);
> >  	for (qx = 0; qx < num_qd; qx++) {
> >  		error = gfs2_glock_nq_init(qda[qx]->qd_gl, LM_ST_EXCLUSIVE,
> >  					   GL_NOCACHE, &ghs[qx]);
> 
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH] gfs2: Get rid of I_MUTEX_QUOTA usage

[Steven Whitehouse]

Hi,

On Thu, 2012-04-26 at 15:47 +0200, Jan Kara wrote:
> Hello,
> 
> On Thu 26-04-12 14:28:39, Steven Whitehouse wrote:
> > On Thu, 2012-04-26 at 15:20 +0200, Jan Kara wrote:
> > > GFS2 uses i_mutex on its system quota inode to synchronize writes to
> > > quota file. Since this is an internal inode to GFS2 (not part of directory
> > > hiearchy or visible by user) we are safe to define locking rules for it. So
> > > let's just get it its own locking class to make it clear.
> > > 
> > It is visible to the user if the gfs2 metadata filesystem is mounted.
> > That used to be the way in which the quotas were set by gfs2_quota.
> > However that is really considered obsolete now, so maybe we don't have
> > to support that interface any more... the gfs2_quota package is no
> > longer in distros as the generic quota package does the job for us these
> > days.
>   Ah, OK. But then if users could write to (or even truncate?) the quota
> file, wasn't it really deadlockable? mutex_lock_nested would silence
> lockdep but the deadlock won't change. Another advantage of my change is
> that if there is some problem, lockdep will warn about it because all
> places which end up taking i_mutex on quota file will use the same locking
> class. Using of lockdep subclasses (i.e. the _nested variant of mutex_lock)
> essentially tells lockdep - I promise that uses of the lock tagged as
> one locking subclass cannot interact with uses tagged as another locking
> class. 
> 
> 								Honza
> 
It has been fairly well tested over a period of time, so I'm pretty
confident that it does work at the moment. The locking is not that
obvious though and could certainly do with a clean up. I'd prefer to
clean up the locking than just silence the messages.

I am taking a look at it currently to see what we might do. It may take
me a little while to get something together though, bearing in mind all
the possible cases that need checking. If we can use sd_quota_mutex
everywhere, then that should at least reduce the complexity and remove
the need for I_MUTEX_QUOTA, so thats my preferred solution if possible.
Otherwise I'm open to suggestions,

Steve.

> > > CC: Steven Whitehouse <swhiteho@redhat.com>
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> > >  fs/gfs2/ops_fstype.c |    8 ++++++++
> > >  fs/gfs2/quota.c      |    2 +-
> > >  2 files changed, 9 insertions(+), 1 deletions(-)
> > > 
> > >  So this is probably the simplest what can be done for GFS2 (actually OCFS2
> > > does the same for internal system files). Compile tested only. Steven?
> > > 
> > > diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> > > index 6f3a18f..ae8f225 100644
> > > --- a/fs/gfs2/ops_fstype.c
> > > +++ b/fs/gfs2/ops_fstype.c
> > > @@ -19,6 +19,7 @@
> > >  #include <linux/mount.h>
> > >  #include <linux/gfs2_ondisk.h>
> > >  #include <linux/quotaops.h>
> > > +#include <linux/lockdep.h>
> > >  
> > >  #include "gfs2.h"
> > >  #include "incore.h"
> > > @@ -767,6 +768,7 @@ fail:
> > >  	return error;
> > >  }
> > >  
> > > +static struct lock_class_key gfs2_quota_imutex_key;
> > >  
> > >  static int init_inodes(struct gfs2_sbd *sdp, int undo)
> > >  {
> > > @@ -804,6 +806,12 @@ static int init_inodes(struct gfs2_sbd *sdp, int undo)
> > >  		fs_err(sdp, "can't get quota file inode: %d\n", error);
> > >  		goto fail_rindex;
> > >  	}
> > > +	/*
> > > +	 * i_mutex on quota files is special. Since this inode is hidden system
> > > +	 * file, we are safe to define locking ourselves.
> > > +	 */
> > > +	lockdep_set_class(&sdp->sd_quota_inode->i_mutex,
> > > +			  &gfs2_quota_imutex_key);
> > >  
> > >  	error = gfs2_rindex_update(sdp);
> > >  	if (error)
> > > diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> > > index 6019da3..970598b 100644
> > > --- a/fs/gfs2/quota.c
> > > +++ b/fs/gfs2/quota.c
> > > @@ -772,7 +772,7 @@ static int do_sync(unsigned int num_qd, struct gfs2_quota_data **qda)
> > >  		return -ENOMEM;
> > >  
> > >  	sort(qda, num_qd, sizeof(struct gfs2_quota_data *), sort_qd, NULL);
> > > -	mutex_lock_nested(&ip->i_inode.i_mutex, I_MUTEX_QUOTA);
> > > +	mutex_lock(&ip->i_inode.i_mutex);
> > >  	for (qx = 0; qx < num_qd; qx++) {
> > >  		error = gfs2_glock_nq_init(qda[qx]->qd_gl, LM_ST_EXCLUSIVE,
> > >  					   GL_NOCACHE, &ghs[qx]);
> > 
> > 

[Cluster-devel] [PATCH] gfs2: Get rid of I_MUTEX_QUOTA usage

[Steven Whitehouse]

Hi,

On Thu, 2012-04-26 at 15:47 +0200, Jan Kara wrote:
> Hello,
> 
> On Thu 26-04-12 14:28:39, Steven Whitehouse wrote:
> > On Thu, 2012-04-26 at 15:20 +0200, Jan Kara wrote:
> > > GFS2 uses i_mutex on its system quota inode to synchronize writes to
> > > quota file. Since this is an internal inode to GFS2 (not part of directory
> > > hiearchy or visible by user) we are safe to define locking rules for it. So
> > > let's just get it its own locking class to make it clear.
> > > 
> > It is visible to the user if the gfs2 metadata filesystem is mounted.
> > That used to be the way in which the quotas were set by gfs2_quota.
> > However that is really considered obsolete now, so maybe we don't have
> > to support that interface any more... the gfs2_quota package is no
> > longer in distros as the generic quota package does the job for us these
> > days.
>   Ah, OK. But then if users could write to (or even truncate?) the quota
> file, wasn't it really deadlockable? mutex_lock_nested would silence
> lockdep but the deadlock won't change. Another advantage of my change is
> that if there is some problem, lockdep will warn about it because all
> places which end up taking i_mutex on quota file will use the same locking
> class. Using of lockdep subclasses (i.e. the _nested variant of mutex_lock)
> essentially tells lockdep - I promise that uses of the lock tagged as
> one locking subclass cannot interact with uses tagged as another locking
> class. 
> 
> 								Honza
> 
It has been fairly well tested over a period of time, so I'm pretty
confident that it does work at the moment. The locking is not that
obvious though and could certainly do with a clean up. I'd prefer to
clean up the locking than just silence the messages.

I am taking a look at it currently to see what we might do. It may take
me a little while to get something together though, bearing in mind all
the possible cases that need checking. If we can use sd_quota_mutex
everywhere, then that should at least reduce the complexity and remove
the need for I_MUTEX_QUOTA, so thats my preferred solution if possible.
Otherwise I'm open to suggestions,

Steve.

> > > CC: Steven Whitehouse <swhiteho@redhat.com>
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> > >  fs/gfs2/ops_fstype.c |    8 ++++++++
> > >  fs/gfs2/quota.c      |    2 +-
> > >  2 files changed, 9 insertions(+), 1 deletions(-)
> > > 
> > >  So this is probably the simplest what can be done for GFS2 (actually OCFS2
> > > does the same for internal system files). Compile tested only. Steven?
> > > 
> > > diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> > > index 6f3a18f..ae8f225 100644
> > > --- a/fs/gfs2/ops_fstype.c
> > > +++ b/fs/gfs2/ops_fstype.c
> > > @@ -19,6 +19,7 @@
> > >  #include <linux/mount.h>
> > >  #include <linux/gfs2_ondisk.h>
> > >  #include <linux/quotaops.h>
> > > +#include <linux/lockdep.h>
> > >  
> > >  #include "gfs2.h"
> > >  #include "incore.h"
> > > @@ -767,6 +768,7 @@ fail:
> > >  	return error;
> > >  }
> > >  
> > > +static struct lock_class_key gfs2_quota_imutex_key;
> > >  
> > >  static int init_inodes(struct gfs2_sbd *sdp, int undo)
> > >  {
> > > @@ -804,6 +806,12 @@ static int init_inodes(struct gfs2_sbd *sdp, int undo)
> > >  		fs_err(sdp, "can't get quota file inode: %d\n", error);
> > >  		goto fail_rindex;
> > >  	}
> > > +	/*
> > > +	 * i_mutex on quota files is special. Since this inode is hidden system
> > > +	 * file, we are safe to define locking ourselves.
> > > +	 */
> > > +	lockdep_set_class(&sdp->sd_quota_inode->i_mutex,
> > > +			  &gfs2_quota_imutex_key);
> > >  
> > >  	error = gfs2_rindex_update(sdp);
> > >  	if (error)
> > > diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> > > index 6019da3..970598b 100644
> > > --- a/fs/gfs2/quota.c
> > > +++ b/fs/gfs2/quota.c
> > > @@ -772,7 +772,7 @@ static int do_sync(unsigned int num_qd, struct gfs2_quota_data **qda)
> > >  		return -ENOMEM;
> > >  
> > >  	sort(qda, num_qd, sizeof(struct gfs2_quota_data *), sort_qd, NULL);
> > > -	mutex_lock_nested(&ip->i_inode.i_mutex, I_MUTEX_QUOTA);
> > > +	mutex_lock(&ip->i_inode.i_mutex);
> > >  	for (qx = 0; qx < num_qd; qx++) {
> > >  		error = gfs2_glock_nq_init(qda[qx]->qd_gl, LM_ST_EXCLUSIVE,
> > >  					   GL_NOCACHE, &ghs[qx]);
> > 
> > 

Re: [PATCH] gfs2: Get rid of I_MUTEX_QUOTA usage

[Jan Kara]

  Hi,

On Thu 26-04-12 15:55:33, Steven Whitehouse wrote:
> On Thu, 2012-04-26 at 15:47 +0200, Jan Kara wrote:
> > Hello,
> > 
> > On Thu 26-04-12 14:28:39, Steven Whitehouse wrote:
> > > On Thu, 2012-04-26 at 15:20 +0200, Jan Kara wrote:
> > > > GFS2 uses i_mutex on its system quota inode to synchronize writes to
> > > > quota file. Since this is an internal inode to GFS2 (not part of directory
> > > > hiearchy or visible by user) we are safe to define locking rules for it. So
> > > > let's just get it its own locking class to make it clear.
> > > > 
> > > It is visible to the user if the gfs2 metadata filesystem is mounted.
> > > That used to be the way in which the quotas were set by gfs2_quota.
> > > However that is really considered obsolete now, so maybe we don't have
> > > to support that interface any more... the gfs2_quota package is no
> > > longer in distros as the generic quota package does the job for us these
> > > days.
> >   Ah, OK. But then if users could write to (or even truncate?) the quota
> > file, wasn't it really deadlockable? mutex_lock_nested would silence
> > lockdep but the deadlock won't change. Another advantage of my change is
> > that if there is some problem, lockdep will warn about it because all
> > places which end up taking i_mutex on quota file will use the same locking
> > class. Using of lockdep subclasses (i.e. the _nested variant of mutex_lock)
> > essentially tells lockdep - I promise that uses of the lock tagged as
> > one locking subclass cannot interact with uses tagged as another locking
> > class. 
> > 
> > 								Honza
> > 
> It has been fairly well tested over a period of time, so I'm pretty
> confident that it does work at the moment. The locking is not that
> obvious though and could certainly do with a clean up. I'd prefer to
> clean up the locking than just silence the messages.
> 
> I am taking a look at it currently to see what we might do. It may take
> me a little while to get something together though, bearing in mind all
> the possible cases that need checking. If we can use sd_quota_mutex
> everywhere, then that should at least reduce the complexity and remove
> the need for I_MUTEX_QUOTA, so thats my preferred solution if possible.
> Otherwise I'm open to suggestions,
  Ok, sounds good.

								Honza

> > > > CC: Steven Whitehouse <swhiteho@redhat.com>
> > > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > > ---
> > > >  fs/gfs2/ops_fstype.c |    8 ++++++++
> > > >  fs/gfs2/quota.c      |    2 +-
> > > >  2 files changed, 9 insertions(+), 1 deletions(-)
> > > > 
> > > >  So this is probably the simplest what can be done for GFS2 (actually OCFS2
> > > > does the same for internal system files). Compile tested only. Steven?
> > > > 
> > > > diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> > > > index 6f3a18f..ae8f225 100644
> > > > --- a/fs/gfs2/ops_fstype.c
> > > > +++ b/fs/gfs2/ops_fstype.c
> > > > @@ -19,6 +19,7 @@
> > > >  #include <linux/mount.h>
> > > >  #include <linux/gfs2_ondisk.h>
> > > >  #include <linux/quotaops.h>
> > > > +#include <linux/lockdep.h>
> > > >  
> > > >  #include "gfs2.h"
> > > >  #include "incore.h"
> > > > @@ -767,6 +768,7 @@ fail:
> > > >  	return error;
> > > >  }
> > > >  
> > > > +static struct lock_class_key gfs2_quota_imutex_key;
> > > >  
> > > >  static int init_inodes(struct gfs2_sbd *sdp, int undo)
> > > >  {
> > > > @@ -804,6 +806,12 @@ static int init_inodes(struct gfs2_sbd *sdp, int undo)
> > > >  		fs_err(sdp, "can't get quota file inode: %d\n", error);
> > > >  		goto fail_rindex;
> > > >  	}
> > > > +	/*
> > > > +	 * i_mutex on quota files is special. Since this inode is hidden system
> > > > +	 * file, we are safe to define locking ourselves.
> > > > +	 */
> > > > +	lockdep_set_class(&sdp->sd_quota_inode->i_mutex,
> > > > +			  &gfs2_quota_imutex_key);
> > > >  
> > > >  	error = gfs2_rindex_update(sdp);
> > > >  	if (error)
> > > > diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> > > > index 6019da3..970598b 100644
> > > > --- a/fs/gfs2/quota.c
> > > > +++ b/fs/gfs2/quota.c
> > > > @@ -772,7 +772,7 @@ static int do_sync(unsigned int num_qd, struct gfs2_quota_data **qda)
> > > >  		return -ENOMEM;
> > > >  
> > > >  	sort(qda, num_qd, sizeof(struct gfs2_quota_data *), sort_qd, NULL);
> > > > -	mutex_lock_nested(&ip->i_inode.i_mutex, I_MUTEX_QUOTA);
> > > > +	mutex_lock(&ip->i_inode.i_mutex);
> > > >  	for (qx = 0; qx < num_qd; qx++) {
> > > >  		error = gfs2_glock_nq_init(qda[qx]->qd_gl, LM_ST_EXCLUSIVE,
> > > >  					   GL_NOCACHE, &ghs[qx]);
> > > 
> > > 
> 
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[Cluster-devel] [PATCH] gfs2: Get rid of I_MUTEX_QUOTA usage

[Jan Kara]

  Hi,

On Thu 26-04-12 15:55:33, Steven Whitehouse wrote:
> On Thu, 2012-04-26 at 15:47 +0200, Jan Kara wrote:
> > Hello,
> > 
> > On Thu 26-04-12 14:28:39, Steven Whitehouse wrote:
> > > On Thu, 2012-04-26 at 15:20 +0200, Jan Kara wrote:
> > > > GFS2 uses i_mutex on its system quota inode to synchronize writes to
> > > > quota file. Since this is an internal inode to GFS2 (not part of directory
> > > > hiearchy or visible by user) we are safe to define locking rules for it. So
> > > > let's just get it its own locking class to make it clear.
> > > > 
> > > It is visible to the user if the gfs2 metadata filesystem is mounted.
> > > That used to be the way in which the quotas were set by gfs2_quota.
> > > However that is really considered obsolete now, so maybe we don't have
> > > to support that interface any more... the gfs2_quota package is no
> > > longer in distros as the generic quota package does the job for us these
> > > days.
> >   Ah, OK. But then if users could write to (or even truncate?) the quota
> > file, wasn't it really deadlockable? mutex_lock_nested would silence
> > lockdep but the deadlock won't change. Another advantage of my change is
> > that if there is some problem, lockdep will warn about it because all
> > places which end up taking i_mutex on quota file will use the same locking
> > class. Using of lockdep subclasses (i.e. the _nested variant of mutex_lock)
> > essentially tells lockdep - I promise that uses of the lock tagged as
> > one locking subclass cannot interact with uses tagged as another locking
> > class. 
> > 
> > 								Honza
> > 
> It has been fairly well tested over a period of time, so I'm pretty
> confident that it does work at the moment. The locking is not that
> obvious though and could certainly do with a clean up. I'd prefer to
> clean up the locking than just silence the messages.
> 
> I am taking a look at it currently to see what we might do. It may take
> me a little while to get something together though, bearing in mind all
> the possible cases that need checking. If we can use sd_quota_mutex
> everywhere, then that should at least reduce the complexity and remove
> the need for I_MUTEX_QUOTA, so thats my preferred solution if possible.
> Otherwise I'm open to suggestions,
  Ok, sounds good.

								Honza

> > > > CC: Steven Whitehouse <swhiteho@redhat.com>
> > > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > > ---
> > > >  fs/gfs2/ops_fstype.c |    8 ++++++++
> > > >  fs/gfs2/quota.c      |    2 +-
> > > >  2 files changed, 9 insertions(+), 1 deletions(-)
> > > > 
> > > >  So this is probably the simplest what can be done for GFS2 (actually OCFS2
> > > > does the same for internal system files). Compile tested only. Steven?
> > > > 
> > > > diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> > > > index 6f3a18f..ae8f225 100644
> > > > --- a/fs/gfs2/ops_fstype.c
> > > > +++ b/fs/gfs2/ops_fstype.c
> > > > @@ -19,6 +19,7 @@
> > > >  #include <linux/mount.h>
> > > >  #include <linux/gfs2_ondisk.h>
> > > >  #include <linux/quotaops.h>
> > > > +#include <linux/lockdep.h>
> > > >  
> > > >  #include "gfs2.h"
> > > >  #include "incore.h"
> > > > @@ -767,6 +768,7 @@ fail:
> > > >  	return error;
> > > >  }
> > > >  
> > > > +static struct lock_class_key gfs2_quota_imutex_key;
> > > >  
> > > >  static int init_inodes(struct gfs2_sbd *sdp, int undo)
> > > >  {
> > > > @@ -804,6 +806,12 @@ static int init_inodes(struct gfs2_sbd *sdp, int undo)
> > > >  		fs_err(sdp, "can't get quota file inode: %d\n", error);
> > > >  		goto fail_rindex;
> > > >  	}
> > > > +	/*
> > > > +	 * i_mutex on quota files is special. Since this inode is hidden system
> > > > +	 * file, we are safe to define locking ourselves.
> > > > +	 */
> > > > +	lockdep_set_class(&sdp->sd_quota_inode->i_mutex,
> > > > +			  &gfs2_quota_imutex_key);
> > > >  
> > > >  	error = gfs2_rindex_update(sdp);
> > > >  	if (error)
> > > > diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> > > > index 6019da3..970598b 100644
> > > > --- a/fs/gfs2/quota.c
> > > > +++ b/fs/gfs2/quota.c
> > > > @@ -772,7 +772,7 @@ static int do_sync(unsigned int num_qd, struct gfs2_quota_data **qda)
> > > >  		return -ENOMEM;
> > > >  
> > > >  	sort(qda, num_qd, sizeof(struct gfs2_quota_data *), sort_qd, NULL);
> > > > -	mutex_lock_nested(&ip->i_inode.i_mutex, I_MUTEX_QUOTA);
> > > > +	mutex_lock(&ip->i_inode.i_mutex);
> > > >  	for (qx = 0; qx < num_qd; qx++) {
> > > >  		error = gfs2_glock_nq_init(qda[qx]->qd_gl, LM_ST_EXCLUSIVE,
> > > >  					   GL_NOCACHE, &ghs[qx]);
> > > 
> > > 
> 
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR