[PATCH 0/6] Get rid of I_MUTEX_QUOTA use

[PATCH 0/6] Get rid of I_MUTEX_QUOTA use

[Jan Kara]

  Hello,

  this patch set changes generic quota code (and filesystems relying on it)
so that they don't use special locking class I_MUTEX_QUOTA. The only remaining
user of I_MUTEX_QUOTA is GFS2. If noone has objections, I can merge these
patches through my tree or Bruce, you can take them as your work depends
on these changes.

								Honza

[PATCH 1/6] ext3: Remove i_mutex use from ext3_quota_write()

[Jan Kara]

We don't need i_mutex in ext3_quota_write() because writes to quota file
are serialized by dqio_mutex anyway. Changes to quota files outside of quota
code are forbidded and enforced by NOATIME and IMMUTABLE bits.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext3/super.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/fs/ext3/super.c b/fs/ext3/super.c
index cf0b592..7c08c93 100644
--- a/fs/ext3/super.c
+++ b/fs/ext3/super.c
@@ -3000,7 +3000,6 @@ static ssize_t ext3_quota_write(struct super_block *sb, int type,
 			(unsigned long long)off, (unsigned long long)len);
 		return -EIO;
 	}
-	mutex_lock_nested(&inode->i_mutex, I_MUTEX_QUOTA);
 	bh = ext3_bread(handle, inode, blk, 1, &err);
 	if (!bh)
 		goto out;
@@ -3024,10 +3023,8 @@ static ssize_t ext3_quota_write(struct super_block *sb, int type,
 	}
 	brelse(bh);
 out:
-	if (err) {
-		mutex_unlock(&inode->i_mutex);
+	if (err)
 		return err;
-	}
 	if (inode->i_size < off + len) {
 		i_size_write(inode, off + len);
 		EXT3_I(inode)->i_disksize = inode->i_size;
@@ -3035,7 +3032,6 @@ out:
 	inode->i_version++;
 	inode->i_mtime = inode->i_ctime = CURRENT_TIME;
 	ext3_mark_inode_dirty(handle, inode);
-	mutex_unlock(&inode->i_mutex);
 	return len;
 }
 
-- 
1.7.1

[PATCH 2/6] ext4: Remove i_mutex use from ext4_quota_write()

[Jan Kara]

We don't need i_mutex in ext4_quota_write() because writes to quota file
are serialized by dqio_mutex anyway. Changes to quota files outside of quota
code are forbidded and enforced by NOATIME and IMMUTABLE bits.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext4/super.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index 6da1935..14965ca 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -4742,7 +4742,6 @@ static ssize_t ext4_quota_write(struct super_block *sb, int type,
 		return -EIO;
 	}
 
-	mutex_lock_nested(&inode->i_mutex, I_MUTEX_QUOTA);
 	bh = ext4_bread(handle, inode, blk, 1, &err);
 	if (!bh)
 		goto out;
@@ -4758,16 +4757,13 @@ static ssize_t ext4_quota_write(struct super_block *sb, int type,
 	err = ext4_handle_dirty_metadata(handle, NULL, bh);
 	brelse(bh);
 out:
-	if (err) {
-		mutex_unlock(&inode->i_mutex);
+	if (err)
 		return err;
-	}
 	if (inode->i_size < off + len) {
 		i_size_write(inode, off + len);
 		EXT4_I(inode)->i_disksize = inode->i_size;
 		ext4_mark_inode_dirty(handle, inode);
 	}
-	mutex_unlock(&inode->i_mutex);
 	return len;
 }
 
-- 
1.7.1

[PATCH 3/6] reiserfs: Remove i_mutex use from reiserfs_quota_write()

[Jan Kara]

We don't need i_mutex in reiserfs_quota_write() because writes to quota file
are serialized by dqio_mutex anyway. Changes to quota files outside of quota
code are forbidded and enforced by NOATIME and IMMUTABLE bits.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/reiserfs/super.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/fs/reiserfs/super.c b/fs/reiserfs/super.c
index 8b7616e..c07b7d7 100644
--- a/fs/reiserfs/super.c
+++ b/fs/reiserfs/super.c
@@ -2270,7 +2270,6 @@ static ssize_t reiserfs_quota_write(struct super_block *sb, int type,
 			(unsigned long long)off, (unsigned long long)len);
 		return -EIO;
 	}
-	mutex_lock_nested(&inode->i_mutex, I_MUTEX_QUOTA);
 	while (towrite > 0) {
 		tocopy = sb->s_blocksize - offset < towrite ?
 		    sb->s_blocksize - offset : towrite;
@@ -2302,16 +2301,13 @@ static ssize_t reiserfs_quota_write(struct super_block *sb, int type,
 		blk++;
 	}
 out:
-	if (len == towrite) {
-		mutex_unlock(&inode->i_mutex);
+	if (len == towrite)
 		return err;
-	}
 	if (inode->i_size < off + len - towrite)
 		i_size_write(inode, off + len - towrite);
 	inode->i_version++;
 	inode->i_mtime = inode->i_ctime = CURRENT_TIME;
 	mark_inode_dirty(inode);
-	mutex_unlock(&inode->i_mutex);
 	return len - towrite;
 }
 
-- 
1.7.1

[PATCH 4/6] ext2: Remove i_mutex use from ext2_quota_write()

[Jan Kara]

We don't need i_mutex in ext2_quota_write() because writes to quota file
are serialized by dqio_mutex anyway. Changes to quota files outside of quota
code are forbidded and enforced by NOATIME and IMMUTABLE bits.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/ext2/super.c |    6 +-----
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/fs/ext2/super.c b/fs/ext2/super.c
index e1025c7..1a6fb52 100644
--- a/fs/ext2/super.c
+++ b/fs/ext2/super.c
@@ -1441,7 +1441,6 @@ static ssize_t ext2_quota_write(struct super_block *sb, int type,
 	struct buffer_head tmp_bh;
 	struct buffer_head *bh;
 
-	mutex_lock_nested(&inode->i_mutex, I_MUTEX_QUOTA);
 	while (towrite > 0) {
 		tocopy = sb->s_blocksize - offset < towrite ?
 				sb->s_blocksize - offset : towrite;
@@ -1471,16 +1470,13 @@ static ssize_t ext2_quota_write(struct super_block *sb, int type,
 		blk++;
 	}
 out:
-	if (len == towrite) {
-		mutex_unlock(&inode->i_mutex);
+	if (len == towrite)
 		return err;
-	}
 	if (inode->i_size < off+len-towrite)
 		i_size_write(inode, off+len-towrite);
 	inode->i_version++;
 	inode->i_mtime = inode->i_ctime = CURRENT_TIME;
 	mark_inode_dirty(inode);
-	mutex_unlock(&inode->i_mutex);
 	return len - towrite;
 }
 
-- 
1.7.1

[PATCH 5/6] quota: Use precomputed value of sb_dqopt in dquot_quota_sync

[Jan Kara]

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/quota/dquot.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index d69a1d1..1b7e0fc 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -638,7 +638,7 @@ int dquot_quota_sync(struct super_block *sb, int type, int wait)
 	dqstats_inc(DQST_SYNCS);
 	mutex_unlock(&dqopt->dqonoff_mutex);
 
-	if (!wait || (sb_dqopt(sb)->flags & DQUOT_QUOTA_SYS_FILE))
+	if (!wait || (dqopt->flags & DQUOT_QUOTA_SYS_FILE))
 		return 0;
 
 	/* This is not very clever (and fast) but currently I don't know about
@@ -652,18 +652,18 @@ int dquot_quota_sync(struct super_block *sb, int type, int wait)
 	 * Now when everything is written we can discard the pagecache so
 	 * that userspace sees the changes.
 	 */
-	mutex_lock(&sb_dqopt(sb)->dqonoff_mutex);
+	mutex_lock(&dqopt->dqonoff_mutex);
 	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
 		if (type != -1 && cnt != type)
 			continue;
 		if (!sb_has_quota_active(sb, cnt))
 			continue;
-		mutex_lock_nested(&sb_dqopt(sb)->files[cnt]->i_mutex,
+		mutex_lock_nested(&dqopt->files[cnt]->i_mutex,
 				  I_MUTEX_QUOTA);
-		truncate_inode_pages(&sb_dqopt(sb)->files[cnt]->i_data, 0);
-		mutex_unlock(&sb_dqopt(sb)->files[cnt]->i_mutex);
+		truncate_inode_pages(&dqopt->files[cnt]->i_data, 0);
+		mutex_unlock(&dqopt->files[cnt]->i_mutex);
 	}
-	mutex_unlock(&sb_dqopt(sb)->dqonoff_mutex);
+	mutex_unlock(&dqopt->dqonoff_mutex);
 
 	return 0;
 }
-- 
1.7.1

[PATCH 6/6] quota: Get rid of nested I_MUTEX_QUOTA locking subclass

[Jan Kara]

So far i_mutex was ranking above dqonoff_mutex and i_mutex on quota files
was special and ranking below dqonoff_mutex (and several other locks).
However there's no real need for i_mutex on quota files to be special.
IO on quota files is serialized by dqio_mutex anyway so we don't need to
take i_mutex when writing to quota files. Other places where we take i_mutex
on quota file can accomodate standard i_mutex lock ranking, we only need
to change the lock ranking to be dqonoff_mutex > i_mutex which is a matter
of changing documentation because there's no place which would enforce
ordering in the other direction.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/quota/dquot.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
index 1b7e0fc..d4ff981 100644
--- a/fs/quota/dquot.c
+++ b/fs/quota/dquot.c
@@ -116,15 +116,15 @@
  * spinlock to internal buffers before writing.
  *
  * Lock ordering (including related VFS locks) is the following:
- *   i_mutex > dqonoff_sem > journal_lock > dqptr_sem > dquot->dq_lock >
+ *   dqonoff_mutex > i_mutex > journal_lock > dqptr_sem > dquot->dq_lock >
  *   dqio_mutex
+ * dqonoff_mutex > i_mutex comes from dquot_quota_sync, dquot_enable, etc.
  * The lock ordering of dqptr_sem imposed by quota code is only dqonoff_sem >
  * dqptr_sem. But filesystem has to count with the fact that functions such as
  * dquot_alloc_space() acquire dqptr_sem and they usually have to be called
  * from inside a transaction to keep filesystem consistency after a crash. Also
  * filesystems usually want to do some IO on dquot from ->mark_dirty which is
  * called with dqptr_sem held.
- * i_mutex on quota files is special (it's below dqio_mutex)
  */
 
 static __cacheline_aligned_in_smp DEFINE_SPINLOCK(dq_list_lock);
@@ -658,8 +658,7 @@ int dquot_quota_sync(struct super_block *sb, int type, int wait)
 			continue;
 		if (!sb_has_quota_active(sb, cnt))
 			continue;
-		mutex_lock_nested(&dqopt->files[cnt]->i_mutex,
-				  I_MUTEX_QUOTA);
+		mutex_lock(&dqopt->files[cnt]->i_mutex);
 		truncate_inode_pages(&dqopt->files[cnt]->i_data, 0);
 		mutex_unlock(&dqopt->files[cnt]->i_mutex);
 	}
@@ -2037,8 +2036,7 @@ int dquot_disable(struct super_block *sb, int type, unsigned int flags)
 			/* If quota was reenabled in the meantime, we have
 			 * nothing to do */
 			if (!sb_has_quota_loaded(sb, cnt)) {
-				mutex_lock_nested(&toputinode[cnt]->i_mutex,
-						  I_MUTEX_QUOTA);
+				mutex_lock(&toputinode[cnt]->i_mutex);
 				toputinode[cnt]->i_flags &= ~(S_IMMUTABLE |
 				  S_NOATIME | S_NOQUOTA);
 				truncate_inode_pages(&toputinode[cnt]->i_data,
@@ -2133,7 +2131,7 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id,
 		/* We don't want quota and atime on quota files (deadlocks
 		 * possible) Also nobody should write to the file - we use
 		 * special IO operations which ignore the immutable bit. */
-		mutex_lock_nested(&inode->i_mutex, I_MUTEX_QUOTA);
+		mutex_lock(&inode->i_mutex);
 		oldflags = inode->i_flags & (S_NOATIME | S_IMMUTABLE |
 					     S_NOQUOTA);
 		inode->i_flags |= S_NOQUOTA | S_NOATIME | S_IMMUTABLE;
@@ -2180,7 +2178,7 @@ out_file_init:
 	iput(inode);
 out_lock:
 	if (oldflags != -1) {
-		mutex_lock_nested(&inode->i_mutex, I_MUTEX_QUOTA);
+		mutex_lock(&inode->i_mutex);
 		/* Set the flags back (in the case of accidental quotaon()
 		 * on a wrong file we don't want to mess up the flags) */
 		inode->i_flags &= ~(S_NOATIME | S_NOQUOTA | S_IMMUTABLE);
-- 
1.7.1

Re: [PATCH 0/6] Get rid of I_MUTEX_QUOTA use

[J. Bruce Fields]

On Wed, Apr 25, 2012 at 10:57:41PM +0200, Jan Kara wrote:
> 
>   Hello,
> 
>   this patch set changes generic quota code (and filesystems relying on it)
> so that they don't use special locking class I_MUTEX_QUOTA. The only remaining
> user of I_MUTEX_QUOTA is GFS2. If noone has objections, I can merge these
> patches through my tree or Bruce, you can take them as your work depends
> on these changes.

My changes will likely need to go through Al, and depend on GFS2 being
fixed as well.

So it's up to Al.  Best might be to just merge it through your tree and
let us sort it out in the next merge window?

--b.

Re: [PATCH 0/6] Get rid of I_MUTEX_QUOTA use

[Jan Kara]

On Wed 25-04-12 17:25:32, J. Bruce Fields wrote:
> On Wed, Apr 25, 2012 at 10:57:41PM +0200, Jan Kara wrote:
> > 
> >   Hello,
> > 
> >   this patch set changes generic quota code (and filesystems relying on it)
> > so that they don't use special locking class I_MUTEX_QUOTA. The only remaining
> > user of I_MUTEX_QUOTA is GFS2. If noone has objections, I can merge these
> > patches through my tree or Bruce, you can take them as your work depends
> > on these changes.
> 
> My changes will likely need to go through Al, and depend on GFS2 being
> fixed as well.
> 
> So it's up to Al.  Best might be to just merge it through your tree and
> let us sort it out in the next merge window?
  OK, I've added patches to my tree for now. I also sent a patch for GFS2
but after some investigation that's mostly a cleanup grade and Steven can
merge it independently. Quota inode in GFS2 is hidden system file so there
should be no locking problems with it wrt your patches.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 0/6] Get rid of I_MUTEX_QUOTA use

[J. Bruce Fields]

On Thu, Apr 26, 2012 at 03:26:08PM +0200, Jan Kara wrote:
> On Wed 25-04-12 17:25:32, J. Bruce Fields wrote:
> > On Wed, Apr 25, 2012 at 10:57:41PM +0200, Jan Kara wrote:
> > > 
> > >   Hello,
> > > 
> > >   this patch set changes generic quota code (and filesystems relying on it)
> > > so that they don't use special locking class I_MUTEX_QUOTA. The only remaining
> > > user of I_MUTEX_QUOTA is GFS2. If noone has objections, I can merge these
> > > patches through my tree or Bruce, you can take them as your work depends
> > > on these changes.
> > 
> > My changes will likely need to go through Al, and depend on GFS2 being
> > fixed as well.
> > 
> > So it's up to Al.  Best might be to just merge it through your tree and
> > let us sort it out in the next merge window?
>   OK, I've added patches to my tree for now. I also sent a patch for GFS2
> but after some investigation that's mostly a cleanup grade and Steven can
> merge it independently. Quota inode in GFS2 is hidden system file so there
> should be no locking problems with it wrt your patches.

OK.  In that case I suppose I'd like Al to take patches 1-4 of the 5 I
posted most recently (followups to
http://mid.gmane.org/20120425152041.GA751@fieldses.org), probably after
your fixes go in.  I'll resend if necessary after they go in.

(I'm also going ahead and pushing out the rest of my delegation patches,
to be merged for 3.5; that solves most of my delegation problem, leaving
just a race in rename to be closed by those 4 patches.)

--b.

Re: [PATCH 0/6] Get rid of I_MUTEX_QUOTA use

[Jan Kara]

On Fri 27-04-12 09:47:52, J. Bruce Fields wrote:
> On Thu, Apr 26, 2012 at 03:26:08PM +0200, Jan Kara wrote:
> > On Wed 25-04-12 17:25:32, J. Bruce Fields wrote:
> > > On Wed, Apr 25, 2012 at 10:57:41PM +0200, Jan Kara wrote:
> > > > 
> > > >   Hello,
> > > > 
> > > >   this patch set changes generic quota code (and filesystems relying on it)
> > > > so that they don't use special locking class I_MUTEX_QUOTA. The only remaining
> > > > user of I_MUTEX_QUOTA is GFS2. If noone has objections, I can merge these
> > > > patches through my tree or Bruce, you can take them as your work depends
> > > > on these changes.
> > > 
> > > My changes will likely need to go through Al, and depend on GFS2 being
> > > fixed as well.
> > > 
> > > So it's up to Al.  Best might be to just merge it through your tree and
> > > let us sort it out in the next merge window?
> >   OK, I've added patches to my tree for now. I also sent a patch for GFS2
> > but after some investigation that's mostly a cleanup grade and Steven can
> > merge it independently. Quota inode in GFS2 is hidden system file so there
> > should be no locking problems with it wrt your patches.
> 
> OK.  In that case I suppose I'd like Al to take patches 1-4 of the 5 I
> posted most recently (followups to
> http://mid.gmane.org/20120425152041.GA751@fieldses.org), probably after
> your fixes go in.  I'll resend if necessary after they go in.
  Yeah. But also please rename I_MUTEX_QUOTA to something better describing
what's going on...

> (I'm also going ahead and pushing out the rest of my delegation patches,
> to be merged for 3.5; that solves most of my delegation problem, leaving
> just a race in rename to be closed by those 4 patches.)

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: [PATCH 0/6] Get rid of I_MUTEX_QUOTA use

[J. Bruce Fields]

On Mon, Apr 30, 2012 at 03:54:34PM +0200, Jan Kara wrote:
> On Fri 27-04-12 09:47:52, J. Bruce Fields wrote:
> > On Thu, Apr 26, 2012 at 03:26:08PM +0200, Jan Kara wrote:
> > > On Wed 25-04-12 17:25:32, J. Bruce Fields wrote:
> > > > On Wed, Apr 25, 2012 at 10:57:41PM +0200, Jan Kara wrote:
> > > > > 
> > > > >   Hello,
> > > > > 
> > > > >   this patch set changes generic quota code (and filesystems relying on it)
> > > > > so that they don't use special locking class I_MUTEX_QUOTA. The only remaining
> > > > > user of I_MUTEX_QUOTA is GFS2. If noone has objections, I can merge these
> > > > > patches through my tree or Bruce, you can take them as your work depends
> > > > > on these changes.
> > > > 
> > > > My changes will likely need to go through Al, and depend on GFS2 being
> > > > fixed as well.
> > > > 
> > > > So it's up to Al.  Best might be to just merge it through your tree and
> > > > let us sort it out in the next merge window?
> > >   OK, I've added patches to my tree for now. I also sent a patch for GFS2
> > > but after some investigation that's mostly a cleanup grade and Steven can
> > > merge it independently. Quota inode in GFS2 is hidden system file so there
> > > should be no locking problems with it wrt your patches.
> > 
> > OK.  In that case I suppose I'd like Al to take patches 1-4 of the 5 I
> > posted most recently (followups to
> > http://mid.gmane.org/20120425152041.GA751@fieldses.org), probably after
> > your fixes go in.  I'll resend if necessary after they go in.
>   Yeah. But also please rename I_MUTEX_QUOTA to something better describing
> what's going on...

Yes.  My uncreative replacement is I_MUTEX_NONDIR2 ("a second
non-directory object"), but I'd welcome any more elegant suggestion.

--b.