vfs: take i_mutex on renamed file

PATCH In review J. Bruce Fields 4 participants 12 messages Mar 5, 2012 – Apr 10, 2012 2 messages not in the archive

Subsystems (5)

Documentation VFS NFS client NFS server +1 more subsystems

lore

Share & tools

lore

J. Bruce Fields

author

2012-03-05 22:38 UTC #0 lore raw

PATCH 1/1 vfs: take i_mutex on renamed file

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 |   11 ++++++-----
 fs/namei.c                                  |    3 +++
 include/linux/fs.h                          |    9 ++++++---
 3 files changed, 15 insertions(+), 8 deletions(-)

Documentation/filesystems/directory-locking +6 -5

index ff7b611..9edbcd2 100644
@@ -12,8 +12,8 @@ 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, finds source and target, locks source, also locks target if
+it already exists, and then calls the method.
 
 5) link creation.  Locking rules:
 	* lock parent

@@ -30,6 +30,7 @@ rules:
 		fail with -ENOTEMPTY
 	* if new parent is equal to or is a descendent of source
 		fail with -ELOOP
+	* lock source if it is not a directory.
 	* if target exists - lock it.
 	* call the method.

@@ -56,9 +57,9 @@ 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 taking locks
+    on their parents (which remain their parents until all locks are
+    acquired, by (1) and (2)).  (Proof: see descriptions of operations).
 
 	Now consider the minimal deadlock.  Each process is blocked on
 attempt to acquire some lock and already holds at least one lock.  Let's

fs/namei.c +3 -0

index 208c6aa..d29b3c4 100644
@@ -3073,6 +3073,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);

@@ -3080,6 +3081,7 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 		return error;
 
 	dget(new_dentry);
+	mutex_lock_nested(&source->i_mutex, I_MUTEX_RENAME_SOURCE);
 	if (target)
 		mutex_lock(&target->i_mutex);

@@ -3098,6 +3100,7 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 out:
 	if (target)
 		mutex_unlock(&target->i_mutex);
+	mutex_unlock(&source->i_mutex);
 	dput(new_dentry);
 	return error;
 }

include/linux/fs.h +6 -3

index 386da09..e537b67 100644
@@ -855,10 +855,12 @@ 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
+ * 5: the file being renamed (used only in rename of a non-directory)
  *
  * The locking order between these classes is
- * parent -> child -> normal -> xattr -> quota
+ * parent -> child -> rename_source -> normal -> xattr -> quota
  */
 enum inode_i_mutex_lock_class
 {

@@ -866,7 +868,8 @@ enum inode_i_mutex_lock_class
 	I_MUTEX_PARENT,
 	I_MUTEX_CHILD,
 	I_MUTEX_XATTR,
-	I_MUTEX_QUOTA
+	I_MUTEX_QUOTA,
+	I_MUTEX_RENAME_SOURCE
 };
 
 /*

Signature · 1 line

1.7.5.4

J. Bruce Fields

author

2012-03-05 22:43 UTC #1 lore raw

Al, do you see any reason this won't work?

After this, all I need to make nfsd happy is a patch to locks.c to implement delegations, and then a few one-liners to add break_deleg calls in the right operations. So this is the one non-trivial change I have to core vfs code.

All of those patches are also ready, and I'd like to merge them for 3.4.

--b.

Quoted 128 lines · bfields

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.

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 |   11 ++++++-----
 fs/namei.c                                  |    3 +++
 include/linux/fs.h                          |    9 ++++++---
 3 files changed, 15 insertions(+), 8 deletions(-)

Documentation/filesystems/directory-locking +6 -5

index ff7b611..9edbcd2 100644
@@ -12,8 +12,8 @@ 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, finds source and target, locks source, also locks target if
+it already exists, and then calls the method.
 
 5) link creation.  Locking rules:
 	* lock parent

@@ -30,6 +30,7 @@ rules:
 		fail with -ENOTEMPTY
 	* if new parent is equal to or is a descendent of source
 		fail with -ELOOP
+	* lock source if it is not a directory.
 	* if target exists - lock it.
 	* call the method.

@@ -56,9 +57,9 @@ 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 taking locks
+    on their parents (which remain their parents until all locks are
+    acquired, by (1) and (2)).  (Proof: see descriptions of operations).
 
 	Now consider the minimal deadlock.  Each process is blocked on
 attempt to acquire some lock and already holds at least one lock.  Let's

fs/namei.c +3 -0

index 208c6aa..d29b3c4 100644
@@ -3073,6 +3073,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);

@@ -3080,6 +3081,7 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 		return error;
 
 	dget(new_dentry);
+	mutex_lock_nested(&source->i_mutex, I_MUTEX_RENAME_SOURCE);
 	if (target)
 		mutex_lock(&target->i_mutex);

@@ -3098,6 +3100,7 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 out:
 	if (target)
 		mutex_unlock(&target->i_mutex);
+	mutex_unlock(&source->i_mutex);
 	dput(new_dentry);
 	return error;
 }

include/linux/fs.h +6 -3

index 386da09..e537b67 100644
@@ -855,10 +855,12 @@ 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
+ * 5: the file being renamed (used only in rename of a non-directory)
  *
  * The locking order between these classes is
- * parent -> child -> normal -> xattr -> quota
+ * parent -> child -> rename_source -> normal -> xattr -> quota
  */
 enum inode_i_mutex_lock_class
 {

@@ -866,7 +868,8 @@ enum inode_i_mutex_lock_class
 	I_MUTEX_PARENT,
 	I_MUTEX_CHILD,
 	I_MUTEX_XATTR,
-	I_MUTEX_QUOTA
+	I_MUTEX_QUOTA,
+	I_MUTEX_RENAME_SOURCE
 };
 
 /*

Signature · 1 line

1.7.5.4

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

J. Bruce Fields

2012-03-09 22:21 UTC #3 lore raw

Quoted 8 lines · J. Bruce Fields

Al, do you see any reason this won't work?
After this, all I need to make nfsd happy is a patch to locks.c to implement delegations, and then a few one-liners to add break_deleg calls in the right operations. So this is the one non-trivial change I have to core vfs code.
All of those patches are also ready, and I'd like to merge them for 3.4.

By the way, the following is the rest of the series.

(Compared to the last posting the deleg-breaking interface is simplified.)

--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]

J. Bruce Fields

2012-03-09 22:29 UTC #5 lore raw

PATCH 4/7 locks: break delegations on unlink

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

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/namei.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

fs/namei.c +3 -0

index d29b3c4..eb4166f 100644
@@ -2788,6 +2788,9 @@ static long do_unlinkat(int dfd, const char __user *pathname)
 		error = security_path_unlink(&nd.path, dentry);
 		if (error)
 			goto exit3;
+		error = break_deleg(inode, O_WRONLY);
+		if (error)
+			goto exit3;
 		error = vfs_unlink(nd.path.dentry->d_inode, dentry);
 exit3:
 		mnt_drop_write(nd.path.mnt);

Signature · 6 lines

1.7.5.4

--
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

J. Bruce Fields

2012-03-09 22:29 UTC #6 lore raw

PATCH 1/7 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.

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 |   11 ++++++-----
 fs/namei.c                                  |    3 +++
 include/linux/fs.h                          |    9 ++++++---
 3 files changed, 15 insertions(+), 8 deletions(-)

Documentation/filesystems/directory-locking +6 -5

index ff7b611..9edbcd2 100644
@@ -12,8 +12,8 @@ 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, finds source and target, locks source, also locks target if
+it already exists, and then calls the method.
 
 5) link creation.  Locking rules:
 	* lock parent

@@ -30,6 +30,7 @@ rules:
 		fail with -ENOTEMPTY
 	* if new parent is equal to or is a descendent of source
 		fail with -ELOOP
+	* lock source if it is not a directory.
 	* if target exists - lock it.
 	* call the method.

@@ -56,9 +57,9 @@ 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 taking locks
+    on their parents (which remain their parents until all locks are
+    acquired, by (1) and (2)).  (Proof: see descriptions of operations).
 
 	Now consider the minimal deadlock.  Each process is blocked on
 attempt to acquire some lock and already holds at least one lock.  Let's

fs/namei.c +3 -0

index 208c6aa..d29b3c4 100644
@@ -3073,6 +3073,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);

@@ -3080,6 +3081,7 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 		return error;
 
 	dget(new_dentry);
+	mutex_lock_nested(&source->i_mutex, I_MUTEX_RENAME_SOURCE);
 	if (target)
 		mutex_lock(&target->i_mutex);

@@ -3098,6 +3100,7 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 out:
 	if (target)
 		mutex_unlock(&target->i_mutex);
+	mutex_unlock(&source->i_mutex);
 	dput(new_dentry);
 	return error;
 }

include/linux/fs.h +6 -3

index 386da09..e537b67 100644
@@ -855,10 +855,12 @@ 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
+ * 5: the file being renamed (used only in rename of a non-directory)
  *
  * The locking order between these classes is
- * parent -> child -> normal -> xattr -> quota
+ * parent -> child -> rename_source -> normal -> xattr -> quota
  */
 enum inode_i_mutex_lock_class
 {

@@ -866,7 +868,8 @@ enum inode_i_mutex_lock_class
 	I_MUTEX_PARENT,
 	I_MUTEX_CHILD,
 	I_MUTEX_XATTR,
-	I_MUTEX_QUOTA
+	I_MUTEX_QUOTA,
+	I_MUTEX_RENAME_SOURCE
 };
 
 /*

Signature · 1 line

1.7.5.4

J. Bruce Fields

2012-03-09 22:29 UTC #7 lore raw

PATCH 2/7 locks: introduce new FL_DELEG lock flag

In reply to J. Bruce Fields

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

For now FL_DELEG is just a synonym for FL_LEASE. So this patch doesn't change behavior.

Next we'll modify break_lease to treat FL_DELEG leases differently, to account for the fact that NFSv4 delegations should be broken in more situations than Windows oplocks.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/locks.c          |    2 +-
 fs/nfsd/nfs4state.c |    2 +-
 include/linux/fs.h  |    1 +
 3 files changed, 3 insertions(+), 2 deletions(-)

fs/locks.c +1 -1

index 637694b..ea07a98 100644
@@ -131,7 +131,7 @@
 
 #define IS_POSIX(fl)	(fl->fl_flags & FL_POSIX)
 #define IS_FLOCK(fl)	(fl->fl_flags & FL_FLOCK)
-#define IS_LEASE(fl)	(fl->fl_flags & FL_LEASE)
+#define IS_LEASE(fl)	(fl->fl_flags & (FL_LEASE|FL_DELEG))
 
 static bool lease_breaking(struct file_lock *fl)
 {

fs/nfsd/nfs4state.c +1 -1

index 8670863..38562ee 100644
@@ -2838,7 +2838,7 @@ static struct file_lock *nfs4_alloc_init_lease(struct nfs4_delegation *dp, int f
 		return NULL;
 	locks_init_lock(fl);
 	fl->fl_lmops = &nfsd_lease_mng_ops;
-	fl->fl_flags = FL_LEASE;
+	fl->fl_flags = FL_DELEG;
 	fl->fl_type = flag == NFS4_OPEN_DELEGATE_READ? F_RDLCK: F_WRLCK;
 	fl->fl_end = OFFSET_MAX;
 	fl->fl_owner = (fl_owner_t)(dp->dl_file);

include/linux/fs.h +1 -0

index e537b67..9d7446e 100644
@@ -1085,6 +1085,7 @@ static inline int file_check_writeable(struct file *filp)
 
 #define FL_POSIX	1
 #define FL_FLOCK	2
+#define FL_DELEG	4	/* NFSv4 delegation */
 #define FL_ACCESS	8	/* not trying to lock, just looking */
 #define FL_EXISTS	16	/* when unlocking, test for existence */
 #define FL_LEASE	32	/* lease held on this file */

Signature · 1 line

1.7.5.4

J. Bruce Fields

2012-03-09 22:29 UTC #8 lore raw

PATCH 3/7 locks: implement delegations

In reply to J. Bruce Fields

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

Implement NFSv4 delegations at the vfs level using the new FL_DELEG lock type.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/locks.c         |   49 +++++++++++++++++++++++++++++++++++++++----------
 include/linux/fs.h |   18 +++++++++++++++---
 2 files changed, 54 insertions(+), 13 deletions(-)

fs/locks.c +39 ~~-10~~

index ea07a98..29f911c 100644
@@ -1185,28 +1185,40 @@ static void time_out_leases(struct inode *inode)
 	}
 }
 
+static bool leases_conflict(struct file_lock *lease, struct file_lock *breaker)
+{
+	if ((breaker->fl_flags & FL_DELEG) && (lease->fl_flags & FL_LEASE))
+		return false;
+	return locks_conflict(breaker, lease);
+}
+
 /**
  *	__break_lease	-	revoke all outstanding leases on file
  *	@inode: the inode of the file to return
- *	@mode: the open mode (read or write)
+ *	@mode: O_RDONLY: break only write leases; O_WRONLY or O_RDWR:
+ *	    break all leases
+ *	@type: FL_LEASE: break leases and delegations; FL_DELEG: break
+ *	    only delegations
  *
  *	break_lease (inlined for speed) has checked there already is at least
  *	some kind of lock (maybe a lease) on this file.  Leases are broken on
  *	a call to open() or truncate().  This function can sleep unless you
  *	specified %O_NONBLOCK to your open().
  */
-int __break_lease(struct inode *inode, unsigned int mode)
+int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
 {
 	int error = 0;
 	struct file_lock *new_fl, *flock;
 	struct file_lock *fl;
 	unsigned long break_time;
 	int i_have_this_lease = 0;
+	bool lease_conflict = false;
 	int want_write = (mode & O_ACCMODE) != O_RDONLY;
 
 	new_fl = lease_alloc(NULL, want_write ? F_WRLCK : F_RDLCK);
 	if (IS_ERR(new_fl))
 		return PTR_ERR(new_fl);
+	new_fl->fl_flags = type;
 
 	lock_flocks();

@@ -1216,13 +1228,16 @@ int __break_lease(struct inode *inode, unsigned int mode)
 	if ((flock == NULL) || !IS_LEASE(flock))
 		goto out;
 
-	if (!locks_conflict(flock, new_fl))
+	for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next) {
+		if (leases_conflict(fl, new_fl)) {
+			lease_conflict = true;
+			if (fl->fl_owner == current->files)
+				i_have_this_lease = 1;
+		}
+	}
+	if (!lease_conflict)
 		goto out;
 
-	for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next)
-		if (fl->fl_owner == current->files)
-			i_have_this_lease = 1;
-
 	break_time = 0;
 	if (lease_break_time > 0) {
 		break_time = jiffies + lease_break_time * HZ;

@@ -1231,6 +1246,8 @@ int __break_lease(struct inode *inode, unsigned int mode)
 	}
 
 	for (fl = flock; fl && IS_LEASE(fl); fl = fl->fl_next) {
+		if (!leases_conflict(fl, new_fl))
+			continue;
 		if (want_write) {
 			if (fl->fl_flags & FL_UNLOCK_PENDING)
 				continue;

@@ -1272,7 +1289,7 @@ restart:
 		 */
 		for (flock = inode->i_flock; flock && IS_LEASE(flock);
 				flock = flock->fl_next) {
-			if (locks_conflict(new_fl, flock))
+			if (leases_conflict(new_fl, flock))
 				goto restart;
 		}
 		error = 0;

@@ -1352,9 +1369,20 @@ int generic_add_lease(struct file *filp, long arg, struct file_lock **flp)
 	struct file_lock *fl, **before, **my_before = NULL, *lease;
 	struct dentry *dentry = filp->f_path.dentry;
 	struct inode *inode = dentry->d_inode;
+	bool is_deleg = (*flp)->fl_flags & FL_DELEG;
 	int error;
 
 	lease = *flp;
+	/*
+	 * In the delegation case we need mutual exclusion with
+	 * a number of operations that take the i_mutex.  We trylock
+	 * because delegations are an optional optimization, and if
+	 * there's some chance of a conflict--we'd rather not
+	 * bother, maybe that's a sign this just isn't a good file to
+	 * hand out a delegation on.
+	 */
+	if (is_deleg && !mutex_trylock(&inode->i_mutex))
+		return -EAGAIN;
 
 	error = -EAGAIN;
 	if ((arg == F_RDLCK) && (atomic_read(&inode->i_writecount) > 0))

@@ -1406,9 +1434,10 @@ int generic_add_lease(struct file *filp, long arg, struct file_lock **flp)
 		goto out;
 
 	locks_insert_lock(before, lease);
-	return 0;
-
+	error = 0;
 out:
+	if (is_deleg)
+		mutex_unlock(&inode->i_mutex);
 	return error;
 }

include/linux/fs.h +15 -3

index 9d7446e..2cc1adf 100644
@@ -1207,7 +1207,7 @@ extern int vfs_test_lock(struct file *, struct file_lock *);
 extern int vfs_lock_file(struct file *, unsigned int, struct file_lock *, struct file_lock *);
 extern int vfs_cancel_lock(struct file *filp, struct file_lock *fl);
 extern int flock_lock_file_wait(struct file *filp, struct file_lock *fl);
-extern int __break_lease(struct inode *inode, unsigned int flags);
+extern int __break_lease(struct inode *inode, unsigned int flags, unsigned int type);
 extern void lease_get_mtime(struct inode *, struct timespec *time);
 extern int generic_setlease(struct file *, long, struct file_lock **);
 extern int vfs_setlease(struct file *, long, struct file_lock **);

@@ -1319,7 +1319,7 @@ static inline int flock_lock_file_wait(struct file *filp,
 	return -ENOLCK;
 }
 
-static inline int __break_lease(struct inode *inode, unsigned int mode)
+static inline int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
 {
 	return 0;
 }

@@ -1972,9 +1972,17 @@ static inline int locks_verify_truncate(struct inode *inode,
 static inline int break_lease(struct inode *inode, unsigned int mode)
 {
 	if (inode->i_flock)
-		return __break_lease(inode, mode);
+		return __break_lease(inode, mode, FL_LEASE);
 	return 0;
 }
+
+static inline int break_deleg(struct inode *inode, unsigned int mode)
+{
+	if (inode->i_flock)
+		return __break_lease(inode, mode, FL_DELEG);
+	return 0;
+}
+
 #else /* !CONFIG_FILE_LOCKING */
 static inline int locks_mandatory_locked(struct inode *inode)
 {

@@ -2014,6 +2022,10 @@ static inline int break_lease(struct inode *inode, unsigned int mode)
 	return 0;
 }
 
+static inline int break_deleg(struct inode *inode, unsigned int mode)
+{
+	return 0;
+}
 #endif /* CONFIG_FILE_LOCKING */
 
 /* fs/open.c */

Signature · 1 line

1.7.5.4

J. Bruce Fields

2012-03-09 22:29 UTC #9 lore raw

PATCH 5/7 locks: break delegations on rename

In reply to J. Bruce Fields

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

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/namei.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

fs/namei.c +8 -0

index eb4166f..21a1d65 100644
@@ -3092,6 +3092,14 @@ static int vfs_rename_other(struct inode *old_dir, struct dentry *old_dentry,
 	if (d_mountpoint(old_dentry)||d_mountpoint(new_dentry))
 		goto out;
 
+	error = break_deleg(source, O_WRONLY);
+	if (error)
+		goto out;
+	if (target) {
+		error = break_deleg(target, O_WRONLY);
+		if (error)
+			goto out;
+	}
 	error = old_dir->i_op->rename(old_dir, old_dentry, new_dir, new_dentry);
 	if (error)
 		goto out;

Signature · 1 line

1.7.5.4

J. Bruce Fields

2012-03-09 22:29 UTC #10 lore raw

PATCH 6/7 locks: break delegations on link

In reply to J. Bruce Fields

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

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/namei.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

fs/namei.c +5 -2

index 21a1d65..6f1dbfa 100644
@@ -2920,8 +2920,11 @@ int vfs_link(struct dentry *old_dentry, struct inode *dir, struct dentry *new_de
 	/* Make sure we don't allow creating hardlink to an unlinked file */
 	if (inode->i_nlink == 0)
 		error =  -ENOENT;
-	else
-		error = dir->i_op->link(old_dentry, dir, new_dentry);
+	else {
+		error = break_deleg(inode, O_WRONLY);
+		if (!error)
+			error = dir->i_op->link(old_dentry, dir, new_dentry);
+	}
 	mutex_unlock(&inode->i_mutex);
 	if (!error)
 		fsnotify_link(dir, inode, new_dentry);

Signature · 1 line

1.7.5.4

J. Bruce Fields

2012-03-09 22:29 UTC #11 lore raw

PATCH 7/7 locks: break delegations on any attribute modification

In reply to J. Bruce Fields

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

NFSv4 uses leases to guarantee that clients can cache metadata as well as data.

This covers chmod, chown, etc.

Signed-off-by J. Bruce Fields <[email protected]>

---
 fs/attr.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

fs/attr.c +3 -0

index 95053ad..b8b5676 100644
@@ -232,6 +232,9 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
 	error = security_inode_setattr(dentry, attr);
 	if (error)
 		return error;
+	error = break_deleg(inode, O_WRONLY);
+	if (error)
+		return error;
 
 	if (inode->i_op->setattr)
 		error = inode->i_op->setattr(dentry, attr);

Signature · 1 line

1.7.5.4

J. Bruce Fields

author

2012-03-20 19:27 UTC #12 lore raw

In reply to J. Bruce Fields

Quoted 8 lines · J. Bruce Fields

Quoted 7 lines · J. Bruce Fields
Al, do you see any reason this won't work?
After this, all I need to make nfsd happy is a patch to locks.c to implement delegations, and then a few one-liners to add break_deleg calls in the right operations. So this is the one non-trivial change I have to core vfs code.

Quoted 6 lines

Quoted 1 line
All of those patches are also ready, and I'd like to merge them for 3.4.
By the way, the following is the rest of the series.
(Compared to the last posting the deleg-breaking interface is simplified.)

What are the chances of getting this reviewed for this merge cycle? Is it on somebody's list?

If it would make it any simpler: the one that I think absolutely must get wider review, and get merged through Al's tree, is "vfs: take i_mutex on renamed file".

There aren't actually any dependencies between that and the rest; and the rest is mainly in nfsd/ and locks.c (changes to which have mostly been going through my tree anyway, I think); other than that:

 fs/attr.c           |    3 ++
 fs/namei.c          |   18 +++++++++++++-

straightforward changes that tend to look like:

	+	error = break_deleg(inode, O_WRONLY);
	+	if (error)
	+		return error;

 include/linux/fs.h  |   19 +++++++++++++--

and some definitions.

--b.

J. Bruce Fields

author

2012-04-10 13:07 UTC #13 lore raw

Quoted 8 lines · J. Bruce Fields

What are the chances of getting this reviewed for this merge cycle? Is it on somebody's list?
If it would make it any simpler: the one that I think absolutely must get wider review, and get merged through Al's tree, is "vfs: take i_mutex on renamed file".
There aren't actually any dependencies between that and the rest;

Based on discussion at LSF, and absent any objections, I'm leaving "vfs:
take i_mutex on renamed file" for Al to take, and merging the rest through the nfsd tree for 3.5. (Al, assuming that's the correct thing to do, should I add an acked-by/reviewed-by for you to the patches I merge?)

--b.

Quoted 18 lines

and
the rest is mainly in nfsd/ and locks.c (changes to which have mostly been going through my tree anyway, I think); other than that:
 fs/attr.c           |    3 ++
 fs/namei.c          |   18 +++++++++++++-
straightforward changes that tend to look like:
	+	error = break_deleg(inode, O_WRONLY);
	+	if (error)
	+		return error;
 include/linux/fs.h  |   19 +++++++++++++--
and some definitions.
--b.