Skip to contentSkip to conversation

rename locking

PATCH In review J. Bruce Fields 6 participants 13 messages May 31, 2012 6 messages not in the archive

Subsystems (6)
Documentation VFS NFS client NFS server +2 more subsystems
lore
Share & tools
lore

J. Bruce Fields

author
#0 lore raw

This is a resend of the rename locking patches. With the quota locking cleaned up, it looks safe to move ext4's locking into common code and then use that.

Steven Whitehouse has another plan for gfs2, but last I talked to him was OK with Jan Kara's fix for now.

--b.

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1338429016-20119-1-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

author
#2 lore raw

PATCH 2/6 vfs: fix outdated i_mutex_lock_class documentation

From: "J. Bruce Fields" <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>
---
 include/linux/fs.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
include/linux/fs.h +2 -1
index 038076b..5584a29 100644
@@ -861,7 +861,8 @@ static inline int inode_unhashed(struct inode *inode)
  * 0: the object of the current VFS operation
  * 1: parent
  * 2: child/target
- * 3: quota file
+ * 3: xattr
+ * 4: quota file
  *
  * The locking order between these classes is
  * parent -> child -> normal -> xattr -> quota
Signature · 6 lines
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 1338429016-20119-3-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

#4 lore raw

Re: [PATCH 2/6] vfs: fix outdated i_mutex_lock_class documentation

Quoted 12 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>
---
 include/linux/fs.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
include/linux/fs.h +0 -0
index 038076b..5584a29 100644

include/linux/fs.h static inline int inode_unhashed(struct inode *inode) · line 865

Quoted 6 lines
include/linux/fs.h +2 -1
@@ -861,7 +861,8 @@ static inline int inode_unhashed(struct inode *inode)
  * 0: the object of the current VFS operation
  * 1: parent
  * 2: child/target
- * 3: quota file
+ * 3: xattr
+ * 4: quota file

This isn't correct anymore, is it? Just remove references to quota in this
comment...

include/linux/fs.h static inline int inode_unhashed(struct inode *inode) · line 868

Quoted 3 lines
include/linux/fs.h +0 -0
  *
  * The locking order between these classes is
  * parent -> child -> normal -> xattr -> quota

Honza

Signature · 6 lines
Jan Kara <[email protected]>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20120531185111.GA13647-+0h/O2h83AeN3ZZ/[email protected]

#6 lore raw

Re: [PATCH 2/6] vfs: fix outdated i_mutex_lock_class documentation

Quoted 8 lines · Jan Kara
Quoted 7 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>
---
 include/linux/fs.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Quoted 13 lines

include/linux/fs.h static inline int inode_unhashed(struct inode *inode) · line 865

Quoted 11 lines
include/linux/fs.h +2 -1
index 038076b..5584a29 100644
@@ -861,7 +861,8 @@ static inline int inode_unhashed(struct inode *inode)
  * 0: the object of the current VFS operation
  * 1: parent
  * 2: child/target
- * 3: quota file
+ * 3: xattr
+ * 4: quota file

This isn't correct anymore, is it? Just remove references to quota in this
comment...

Oh, now I've noticed patch 6/6. Maybe this patch and 6/6 should be just
joined into one?

Honza

Signature · 6 lines
Jan Kara <[email protected]>
SUSE Labs, CR
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): 20120531185439.GB13647-+0h/O2h83AeN3ZZ/[email protected]

#8 lore raw

Re: [PATCH 2/6] vfs: fix outdated i_mutex_lock_class documentation

Quoted 22 lines · Jan Kara
Quoted 8 lines · Jan Kara
Quoted 7 lines · J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

Signed-off-by J. Bruce Fields <[email protected]>
---
 include/linux/fs.h |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Quoted 13 lines

include/linux/fs.h static inline int inode_unhashed(struct inode *inode) · line 865

Quoted 11 lines
include/linux/fs.h +2 -1
index 038076b..5584a29 100644
@@ -861,7 +861,8 @@ static inline int inode_unhashed(struct inode *inode)
  * 0: the object of the current VFS operation
  * 1: parent
  * 2: child/target
- * 3: quota file
+ * 3: xattr
+ * 4: quota file

This isn't correct anymore, is it? Just remove references to quota in this
comment...

Quoted 2 lines

Oh, now I've noticed patch 6/6. Maybe this patch and 6/6 should be just
joined into one?

Sure.--b.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

A message that is not in the archive (sent off-list or not yet received): [email protected]

#10 lore raw

PATCH 6/6 vfs: rename I_MUTEX_QUOTA now that it's not used for quotas

From: "J. Bruce Fields" <[email protected]>

I_MUTEX_QUOTA is now just being used whenever we want to lock two non-directories. So the name isn't right. I_MUTEX_NONDIR2 isn't especially elegant but it's the best I could think of.

Also fix some outdated documentation.

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/inode.c         |    4 ++--
 include/linux/fs.h |    9 ++++++---
 2 files changed, 8 insertions(+), 5 deletions(-)
Quoted 5 lines · bfields
Quoted 2 lines · Jan Kara

Oh, now I've noticed patch 6/6. Maybe this patch and 6/6 should be just
joined into one?

Sure.--b.

So, we'd omit patch 2 and replace the previous patch 6 by this. The other patches are unchanged.

fs/inode.c +2 -2
index 0272ac3..9c828ab 100644
@@ -979,11 +979,11 @@ void lock_two_nondirectories(struct inode *inode1, struct inode *inode2)
 		mutex_lock(&inode1->i_mutex);
 	else if (inode1 < inode2) {
 		mutex_lock(&inode1->i_mutex);
-		mutex_lock_nested(&inode2->i_mutex, I_MUTEX_QUOTA);
+		mutex_lock_nested(&inode2->i_mutex, I_MUTEX_NONDIR2);
 
 	} else {
 		mutex_lock(&inode2->i_mutex);
-		mutex_lock_nested(&inode1->i_mutex, I_MUTEX_QUOTA);
+		mutex_lock_nested(&inode1->i_mutex, I_MUTEX_NONDIR2);
 	}
 }
 EXPORT_SYMBOL(lock_two_nondirectories);
include/linux/fs.h +6 -3
index 09434db..f0b5a5d 100644
@@ -861,10 +861,13 @@ static inline int inode_unhashed(struct inode *inode)
  * 0: the object of the current VFS operation
  * 1: parent
  * 2: child/target
- * 3: quota file
+ * 3: xattr
+ * 4: second non-directory
+ * The last is for certain operations (such as rename) which lock two
+ * non-directories at once.
  *
  * The locking order between these classes is
- * parent -> child -> normal -> xattr -> quota
+ * parent -> child -> normal -> xattr -> second non-directory
  */
 enum inode_i_mutex_lock_class
 {

@@ -872,7 +875,7 @@ enum inode_i_mutex_lock_class
 	I_MUTEX_PARENT,
 	I_MUTEX_CHILD,
 	I_MUTEX_XATTR,
-	I_MUTEX_QUOTA
+	I_MUTEX_NONDIR2
 };
 
 void lock_two_nondirectories(struct inode *, struct inode*);
Signature · 6 lines
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#11 lore raw

PATCH 4/6 vfs: don't use PARENT/CHILD lock classes for non-directories

In reply to a message not in the archive

From: "J. Bruce Fields" <[email protected]>

Reserve I_MUTEX_PARENT and I_MUTEX_CHILD for locking of actual directories.

(Also I_MUTEX_QUOTA isn't really a meaningful name for this locking class any more; fixed in a later patch.)

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/inode.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
fs/inode.c +4 -4
index e48a96f..0272ac3 100644
@@ -978,12 +978,12 @@ void lock_two_nondirectories(struct inode *inode1, struct inode *inode2)
 	if (inode1 == inode2 || inode2 == NULL)
 		mutex_lock(&inode1->i_mutex);
 	else if (inode1 < inode2) {
-		mutex_lock_nested(&inode1->i_mutex, I_MUTEX_PARENT);
-		mutex_lock_nested(&inode2->i_mutex, I_MUTEX_CHILD);
+		mutex_lock(&inode1->i_mutex);
+		mutex_lock_nested(&inode2->i_mutex, I_MUTEX_QUOTA);
 
 	} else {
-		mutex_lock_nested(&inode2->i_mutex, I_MUTEX_PARENT);
-		mutex_lock_nested(&inode1->i_mutex, I_MUTEX_CHILD);
+		mutex_lock(&inode2->i_mutex);
+		mutex_lock_nested(&inode1->i_mutex, I_MUTEX_QUOTA);
 	}
 }
 EXPORT_SYMBOL(lock_two_nondirectories);
Signature · 6 lines
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#12 lore raw

PATCH 6/6 vfs: rename I_MUTEX_QUOTA now that it's not used for quotas

In reply to a message not in the archive

From: "J. Bruce Fields" <[email protected]>

I_MUTEX_QUOTA is now just being used whenever we want to lock two non-directories. So the name isn't right. I_MUTEX_NONDIR2 isn't especially elegant but it's the best I could think of.

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/inode.c         |    4 ++--
 include/linux/fs.h |    8 +++++---
 2 files changed, 7 insertions(+), 5 deletions(-)
fs/inode.c +2 -2
index 0272ac3..9c828ab 100644
@@ -979,11 +979,11 @@ void lock_two_nondirectories(struct inode *inode1, struct inode *inode2)
 		mutex_lock(&inode1->i_mutex);
 	else if (inode1 < inode2) {
 		mutex_lock(&inode1->i_mutex);
-		mutex_lock_nested(&inode2->i_mutex, I_MUTEX_QUOTA);
+		mutex_lock_nested(&inode2->i_mutex, I_MUTEX_NONDIR2);
 
 	} else {
 		mutex_lock(&inode2->i_mutex);
-		mutex_lock_nested(&inode1->i_mutex, I_MUTEX_QUOTA);
+		mutex_lock_nested(&inode1->i_mutex, I_MUTEX_NONDIR2);
 	}
 }
 EXPORT_SYMBOL(lock_two_nondirectories);
include/linux/fs.h +5 -3
index 308c2a0..f0b5a5d 100644
@@ -862,10 +862,12 @@ static inline int inode_unhashed(struct inode *inode)
  * 1: parent
  * 2: child/target
  * 3: xattr
- * 4: quota file
+ * 4: second non-directory
+ * The last is for certain operations (such as rename) which lock two
+ * non-directories at once.
  *
  * The locking order between these classes is
- * parent -> child -> normal -> xattr -> quota
+ * parent -> child -> normal -> xattr -> second non-directory
  */
 enum inode_i_mutex_lock_class
 {

@@ -873,7 +875,7 @@ enum inode_i_mutex_lock_class
 	I_MUTEX_PARENT,
 	I_MUTEX_CHILD,
 	I_MUTEX_XATTR,
-	I_MUTEX_QUOTA
+	I_MUTEX_NONDIR2
 };
 
 void lock_two_nondirectories(struct inode *, struct inode*);
Signature · 6 lines
1.7.9.5

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
#13 lore raw

PATCH 1/6 gfs2: Get rid of I_MUTEX_QUOTA usage

In reply to J. Bruce Fields

From: Jan Kara <[email protected]>

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 <[email protected]>
Signed-off-by Jan Kara <[email protected]>
Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/gfs2/ops_fstype.c |    8 ++++++++
 fs/gfs2/quota.c      |    2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)
fs/gfs2/ops_fstype.c +8 -0
index b8c250f..c714ef7 100644
@@ -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"

@@ -766,6 +767,7 @@ fail:
 	return error;
 }
 
+static struct lock_class_key gfs2_quota_imutex_key;
 
 static int init_inodes(struct gfs2_sbd *sdp, int undo)
 {

@@ -803,6 +805,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)
fs/gfs2/quota.c +1 -1
index b97178e..8ed70e7 100644
@@ -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]);
Signature · 1 line
1.7.9.5

A message that is not in the archive (sent off-list or not yet received): 1338429016-20119-2-git-send-email-bfields-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org

Acked-by
#15 lore raw

Re: [PATCH 1/6] gfs2: Get rid of I_MUTEX_QUOTA usage

Hi,

Acked-by Steven Whitehouse <[email protected]>

Eventually we should be able to drop i_mutex from here entirely, but that proved more problematic that I'd expected, so this is a good interim fix,

Steve.

Quoted 53 lines · J. Bruce Fields

From: Jan Kara <[email protected]>

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 <[email protected]>
Signed-off-by Jan Kara <[email protected]>
Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/gfs2/ops_fstype.c |    8 ++++++++
 fs/gfs2/quota.c      |    2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)
fs/gfs2/ops_fstype.c +8 -0
index b8c250f..c714ef7 100644
@@ -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"

@@ -766,6 +767,7 @@ fail:
 	return error;
 }
 
+static struct lock_class_key gfs2_quota_imutex_key;
 
 static int init_inodes(struct gfs2_sbd *sdp, int undo)
 {

@@ -803,6 +805,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)
fs/gfs2/quota.c +0 -0
index b97178e..8ed70e7 100644

fs/gfs2/quota.c static int do_sync(unsigned int num_qd, struct gfs2_quota_data **qda) · line 778

Quoted 8 lines
fs/gfs2/quota.c +1 -1
@@ -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]);

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

Acked-by on v1 1/6
#16 lore raw

Re: [PATCH 1/6] gfs2: Get rid of I_MUTEX_QUOTA usage

In reply to J. Bruce Fields

----- Original Message -----

| From: Jan Kara <[email protected]>
| 
| 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.

Hi,

Acked-by Bob Peterson <[email protected]>

Regards,

Bob Peterson
Red Hat File Systems

#17 lore raw

PATCH 3/6 vfs: pull ext4's double-i_mutex-locking into common code

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

We want to do this elsewhere as well.

Also, compare pointers instead of inode numbers.

Signed-off-by J. Bruce Fields <[email protected]>
---
 fs/ext4/move_extent.c |   21 ++-------------------
 fs/inode.c            |   36 ++++++++++++++++++++++++++++++++++++
 include/linux/fs.h    |    3 +++
 3 files changed, 41 insertions(+), 19 deletions(-)
fs/ext4/move_extent.c +2 -19
index c5826c6..b87d94a 100644
@@ -1086,19 +1086,7 @@ mext_inode_double_lock(struct inode *inode1, struct inode *inode2)
 	if (ret < 0)
 		goto out;
 
-	if (inode1 == inode2) {
-		mutex_lock(&inode1->i_mutex);
-		goto out;
-	}
-
-	if (inode1->i_ino < inode2->i_ino) {
-		mutex_lock_nested(&inode1->i_mutex, I_MUTEX_PARENT);
-		mutex_lock_nested(&inode2->i_mutex, I_MUTEX_CHILD);
-	} else {
-		mutex_lock_nested(&inode2->i_mutex, I_MUTEX_PARENT);
-		mutex_lock_nested(&inode1->i_mutex, I_MUTEX_CHILD);
-	}
-
+	lock_two_nondirectories(inode1, inode2);
 out:
 	return ret;
 }

@@ -1123,12 +1111,7 @@ mext_inode_double_unlock(struct inode *inode1, struct inode *inode2)
 	if (ret < 0)
 		goto out;
 
-	if (inode1)
-		mutex_unlock(&inode1->i_mutex);
-
-	if (inode2 && inode2 != inode1)
-		mutex_unlock(&inode2->i_mutex);
-
+	unlock_two_nondirectories(inode1, inode2);
 out:
 	return ret;
 }
fs/inode.c +36 -0
index 6bc8761..e48a96f 100644
@@ -969,6 +969,42 @@ void unlock_new_inode(struct inode *inode)
 EXPORT_SYMBOL(unlock_new_inode);
 
 /**
+ * lock_two_nondirectories - take two i_mutexes on non-directory objects
+ * @inode1: first inode to lock; must be non-NULL
+ * @inode2: second inode to lock; optional, may equal first or be NULL.
+ */
+void lock_two_nondirectories(struct inode *inode1, struct inode *inode2)
+{
+	if (inode1 == inode2 || inode2 == NULL)
+		mutex_lock(&inode1->i_mutex);
+	else if (inode1 < inode2) {
+		mutex_lock_nested(&inode1->i_mutex, I_MUTEX_PARENT);
+		mutex_lock_nested(&inode2->i_mutex, I_MUTEX_CHILD);
+
+	} else {
+		mutex_lock_nested(&inode2->i_mutex, I_MUTEX_PARENT);
+		mutex_lock_nested(&inode1->i_mutex, I_MUTEX_CHILD);
+	}
+}
+EXPORT_SYMBOL(lock_two_nondirectories);
+
+/**
+ * lock_two_nondirectories - release locks from lock_two_nondirectories()
+ * @inode1: first inode to unlock
+ * @inode2: second inode to lock
+ *
+ * Arguments must be same as those given to corresponding
+ * lock_two_nondirectories() call.
+ */
+void unlock_two_nondirectories(struct inode *inode1, struct inode *inode2)
+{
+	mutex_unlock(&inode1->i_mutex);
+	if (inode2 && inode2 != inode1)
+		mutex_unlock(&inode2->i_mutex);
+}
+EXPORT_SYMBOL(unlock_two_nondirectories);
+
+/**
  * iget5_locked - obtain an inode from a mounted file system
  * @sb:		super block of file system
  * @hashval:	hash value (usually inode number) to get
include/linux/fs.h +3 -0
index 5584a29..308c2a0 100644
@@ -876,6 +876,9 @@ enum inode_i_mutex_lock_class
 	I_MUTEX_QUOTA
 };
 
+void lock_two_nondirectories(struct inode *, struct inode*);
+void unlock_two_nondirectories(struct inode *, struct inode*);
+
 /*
  * NOTE: in a 32bit arch with a preemptable kernel and
  * an UP compile the i_size_read/write must be atomic
Signature · 1 line
1.7.9.5
#18 lore raw

PATCH 5/6 vfs: take i_mutex on renamed file

In reply to J. Bruce Fields

From: "J. Bruce Fields" <[email protected]>

A read delegation is used by NFSv4 as a guarantee that a client can perform local read opens without informing the server.

The open operation takes the last component of the pathname as an argument, thus is also a lookup operation, and giving the client the above guarantee means informing the client before we allow anything that would change the set of names pointing to the inode.

Therefore, we need to break delegations on rename, link, and unlink.

We also need to prevent new delegations from being acquired while one of these operations is in progress.

We could add some completely new locking for that purpose, but it's simpler to use the i_mutex, since that's already taken by all the operations we care about.

The single exception is rename. So, modify rename to take the i_mutex on the file that is being renamed.

Also fix up lockdep and Documentation/filesystems/directory-locking to reflect the change.

Signed-off-by J. Bruce Fields <[email protected]>
---
 Documentation/filesystems/directory-locking |   30 +++++++++++++++++++--------
 fs/namei.c                                  |    7 +++----
 2 files changed, 24 insertions(+), 13 deletions(-)
Documentation/filesystems/directory-locking +21 -9
index ff7b611..9e8a629 100644
@@ -2,6 +2,10 @@
 kinds of locks - per-inode (->i_mutex) and per-filesystem
 (->s_vfs_rename_mutex).
 
+	When taking the i_mutex on multiple non-directory objects, we
+always acquire the locks in order by increasing address.  We'll call
+that "inode pointer" order in the following.
+
 	For our purposes all operations fall in 5 classes:
 
 1) read access.  Locking rules: caller locks directory we are accessing.

@@ -12,8 +16,9 @@ kinds of locks - per-inode (->i_mutex) and per-filesystem
 locks victim and calls the method.
 
 4) rename() that is _not_ cross-directory.  Locking rules: caller locks
-the parent, finds source and target, if target already exists - locks it
-and then calls the method.
+the parent and finds source and target.  If source and target both
+exist, they are locked in inode pointer order.  Otherwise lock just
+source.  Then call method.
 
 5) link creation.  Locking rules:
 	* lock parent

@@ -30,7 +35,8 @@ rules:
 		fail with -ENOTEMPTY
 	* if new parent is equal to or is a descendent of source
 		fail with -ELOOP
-	* if target exists - lock it.
+	* If target exists, lock both source and target, in inode
+	  pointer order.  Otherwise lock just source.
 	* call the method.
 
 

@@ -56,9 +62,11 @@ objects - A < B iff A is an ancestor of B.
     renames will be blocked on filesystem lock and we don't start changing
     the order until we had acquired all locks).
 
-(3) any operation holds at most one lock on non-directory object and
-    that lock is acquired after all other locks.  (Proof: see descriptions
-    of operations).
+(3) locks on non-directory objects are acquired only after locks on
+    directory objects, and are acquired in inode pointer order.
+    (Proof: all operations but renames take lock on at most one
+    non-directory object, except renames, which take locks on source and
+    target in inode pointer order.)
 
 	Now consider the minimal deadlock.  Each process is blocked on
 attempt to acquire some lock and already holds at least one lock.  Let's

@@ -66,9 +74,13 @@ consider the set of contended locks.  First of all, filesystem lock is
 not contended, since any process blocked on it is not holding any locks.
 Thus all processes are blocked on ->i_mutex.
 
-	Non-directory objects are not contended due to (3).  Thus link
-creation can't be a part of deadlock - it can't be blocked on source
-and it means that it doesn't hold any locks.
+	By (3), any process holding a non-directory lock can only be
+waiting on another non-directory lock with a larger address.  Therefore
+the process holding the "largest" such lock can always make progress, and
+non-directory objects are not included in the set of contended locks.
+
+	Thus link creation can't be a part of deadlock - it can't be
+blocked on source and it means that it doesn't hold any locks.
 
 	Any contended object is either held by cross-directory rename or
 has a child that is also contended.  Indeed, suppose that it is held by
fs/namei.c +3 -4
index c651f02..421810b 100644
@@ -3162,6 +3162,7 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 			    struct inode *new_dir, struct dentry *new_dentry)
 {
 	struct inode *target = new_dentry->d_inode;
+	struct inode *source = old_dentry->d_inode;
 	int error;
 
 	error = security_inode_rename(old_dir, old_dentry, new_dir, new_dentry);

@@ -3169,8 +3170,7 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 		return error;
 
 	dget(new_dentry);
-	if (target)
-		mutex_lock(&target->i_mutex);
+	lock_two_nondirectories(source, target);
 
 	error = -EBUSY;
 	if (d_mountpoint(old_dentry)||d_mountpoint(new_dentry))

@@ -3185,8 +3185,7 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 	if (!(old_dir->i_sb->s_type->fs_flags & FS_RENAME_DOES_D_MOVE))
 		d_move(old_dentry, new_dentry);
 out:
-	if (target)
-		mutex_unlock(&target->i_mutex);
+	unlock_two_nondirectories(source, target);
 	dput(new_dentry);
 	return error;
 }
Signature · 1 line
1.7.9.5