All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH - RFC] VFS: disable new delegations during delegation-breaking operations
@ 2024-09-16  4:34 NeilBrown
  2024-09-18 20:40 ` kernel test robot
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: NeilBrown @ 2024-09-16  4:34 UTC (permalink / raw)
  To: Jeff Layton, Alexander Viro, Christian Brauner
  Cc: Jan Kara, linux-fsdevel, Chuck Lever, Olga Kornievskaia,
	linux-nfs


Various operations such as rename and unlink must break any delegations
before they proceed.
do_dentry_open() and vfs_truncate(), which use break_lease(), increment
i_writecount and/or i_readcount which blocks delegations until the
counter is decremented, but the various callers of try_break_deleg() do
not impose any such barrier.  They hold the inode lock while performing
the operation which blocks delegations, but must drop it while waiting
for a delegation to be broken, which leaves an opportunity for a new
delegation to be added.

nfsd - the only current user of delegations - records any files on which
it is called to break a delegation in a manner which blocks further
delegations for 30-60 seconds.  This is normally sufficient.  However
there is talk of reducing the timeout and it would be best if operations
that needed delegations to be blocked used something more definitive
than a timer.

This patch adds that definitive blocking by adding a counter to struct
file_lock_context of the number of concurrent operations which require
delegations to be blocked.  check_conflicting_open() checks that counter
when a delegation is requested and denies the delegation if the counter
is elevated.

try_break_deleg() now increments that counter when it records the inode
as a 'delegated_inode'.

break_deleg_wait() now leaves the inode pointer in *delegated_inode when
it signals that the operation should be retried, and then clears it -
decrementing the new counter - when the operation has completed, whether
successfully or not.  To achieve this we now pass the current error
status in to break_deleg_wait().

vfs_rename() now uses two delegated_inode pointers, one for the
source and one for the destination in the case of replacement.  This is
needed as it may be necessary to block further delegations to both
inodes while the rename completes.

The net result is that we no longer depend on the delay that nfsd
imposes on new delegation in order for various functions that break
delegations to be sure that new delegations won't be added while they wait
with the inode unlocked.  This gives more freedom to nfsd to make more
subtle choices about when and for how long to block delegations when
there is no active contention.

try_break_deleg() is possibly now large enough that it shouldn't be
inline.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 fs/locks.c               | 12 ++++++++++--
 fs/namei.c               | 32 ++++++++++++++++++++------------
 fs/open.c                |  8 ++++----
 fs/posix_acl.c           |  8 ++++----
 fs/utimes.c              |  4 ++--
 fs/xattr.c               |  8 ++++----
 include/linux/filelock.h | 31 ++++++++++++++++++++++++-------
 include/linux/fs.h       |  3 ++-
 8 files changed, 70 insertions(+), 36 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index e45cad40f8b6..171628094daa 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -191,6 +191,7 @@ locks_get_lock_context(struct inode *inode, int type)
 	INIT_LIST_HEAD(&ctx->flc_flock);
 	INIT_LIST_HEAD(&ctx->flc_posix);
 	INIT_LIST_HEAD(&ctx->flc_lease);
+	atomic_set(&ctx->flc_deleg_blockers, 0);
 
 	/*
 	 * Assign the pointer if it's not already assigned. If it is, then
@@ -255,6 +256,7 @@ locks_free_lock_context(struct inode *inode)
 	struct file_lock_context *ctx = locks_inode_context(inode);
 
 	if (unlikely(ctx)) {
+		WARN_ON(atomic_read(&ctx->flc_deleg_blockers) != 0);
 		locks_check_ctx_lists(inode);
 		kmem_cache_free(flctx_cache, ctx);
 	}
@@ -1743,9 +1745,15 @@ check_conflicting_open(struct file *filp, const int arg, int flags)
 
 	if (flags & FL_LAYOUT)
 		return 0;
-	if (flags & FL_DELEG)
-		/* We leave these checks to the caller */
+	if (flags & FL_DELEG) {
+		struct file_lock_context *ctx = locks_inode_context(inode);
+
+		if (ctx && atomic_read(&ctx->flc_deleg_blockers) > 0)
+			return -EAGAIN;
+
+		/* We leave the remaining checks to the caller */
 		return 0;
+	}
 
 	if (arg == F_RDLCK)
 		return inode_is_open_for_write(inode) ? -EAGAIN : 0;
diff --git a/fs/namei.c b/fs/namei.c
index 5512cb10fa89..3054da90276b 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -4493,8 +4493,8 @@ int do_unlinkat(int dfd, struct filename *name)
 		iput(inode);	/* truncate the inode here */
 	inode = NULL;
 	if (delegated_inode) {
-		error = break_deleg_wait(&delegated_inode);
-		if (!error)
+		error = break_deleg_wait(&delegated_inode, error);
+		if (error == -EWOULDBLOCK)
 			goto retry_deleg;
 	}
 	mnt_drop_write(path.mnt);
@@ -4764,8 +4764,8 @@ int do_linkat(int olddfd, struct filename *old, int newdfd,
 out_dput:
 	done_path_create(&new_path, new_dentry);
 	if (delegated_inode) {
-		error = break_deleg_wait(&delegated_inode);
-		if (!error) {
+		error = break_deleg_wait(&delegated_inode, error);
+		if (error == -EWOULDBLOCK) {
 			path_put(&old_path);
 			goto retry;
 		}
@@ -4848,7 +4848,8 @@ int vfs_rename(struct renamedata *rd)
 	struct inode *old_dir = rd->old_dir, *new_dir = rd->new_dir;
 	struct dentry *old_dentry = rd->old_dentry;
 	struct dentry *new_dentry = rd->new_dentry;
-	struct inode **delegated_inode = rd->delegated_inode;
+	struct inode **delegated_inode_old = rd->delegated_inode_old;
+	struct inode **delegated_inode_new = rd->delegated_inode_new;
 	unsigned int flags = rd->flags;
 	bool is_dir = d_is_dir(old_dentry);
 	struct inode *source = old_dentry->d_inode;
@@ -4954,12 +4955,12 @@ int vfs_rename(struct renamedata *rd)
 			goto out;
 	}
 	if (!is_dir) {
-		error = try_break_deleg(source, delegated_inode);
+		error = try_break_deleg(source, delegated_inode_old);
 		if (error)
 			goto out;
 	}
 	if (target && !new_is_dir) {
-		error = try_break_deleg(target, delegated_inode);
+		error = try_break_deleg(target, delegated_inode_new);
 		if (error)
 			goto out;
 	}
@@ -5011,7 +5012,8 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
 	struct path old_path, new_path;
 	struct qstr old_last, new_last;
 	int old_type, new_type;
-	struct inode *delegated_inode = NULL;
+	struct inode *delegated_inode_old = NULL;
+	struct inode *delegated_inode_new = NULL;
 	unsigned int lookup_flags = 0, target_flags = LOOKUP_RENAME_TARGET;
 	bool should_retry = false;
 	int error = -EINVAL;
@@ -5118,7 +5120,8 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
 	rd.new_dir	   = new_path.dentry->d_inode;
 	rd.new_dentry	   = new_dentry;
 	rd.new_mnt_idmap   = mnt_idmap(new_path.mnt);
-	rd.delegated_inode = &delegated_inode;
+	rd.delegated_inode_old = &delegated_inode_old;
+	rd.delegated_inode_new = &delegated_inode_new;
 	rd.flags	   = flags;
 	error = vfs_rename(&rd);
 exit5:
@@ -5128,9 +5131,14 @@ int do_renameat2(int olddfd, struct filename *from, int newdfd,
 exit3:
 	unlock_rename(new_path.dentry, old_path.dentry);
 exit_lock_rename:
-	if (delegated_inode) {
-		error = break_deleg_wait(&delegated_inode);
-		if (!error)
+	if (delegated_inode_old) {
+		error = break_deleg_wait(&delegated_inode_old, error);
+		if (error == -EWOULDBLOCK)
+			goto retry_deleg;
+	}
+	if (delegated_inode_new) {
+		error = break_deleg_wait(&delegated_inode_new, error);
+		if (error == -EWOULDBLOCK)
 			goto retry_deleg;
 	}
 	mnt_drop_write(old_path.mnt);
diff --git a/fs/open.c b/fs/open.c
index 22adbef7ecc2..6b6d20a68dd8 100644
--- a/fs/open.c
+++ b/fs/open.c
@@ -656,8 +656,8 @@ int chmod_common(const struct path *path, umode_t mode)
 out_unlock:
 	inode_unlock(inode);
 	if (delegated_inode) {
-		error = break_deleg_wait(&delegated_inode);
-		if (!error)
+		error = break_deleg_wait(&delegated_inode, error);
+		if (error == -EWOULDBLOCK)
 			goto retry_deleg;
 	}
 	mnt_drop_write(path->mnt);
@@ -795,8 +795,8 @@ int chown_common(const struct path *path, uid_t user, gid_t group)
 				      &delegated_inode);
 	inode_unlock(inode);
 	if (delegated_inode) {
-		error = break_deleg_wait(&delegated_inode);
-		if (!error)
+		error = break_deleg_wait(&delegated_inode, error);
+		if (error == -EWOULDBLOCK)
 			goto retry_deleg;
 	}
 	return error;
diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 3f87297dbfdb..5eb3635d1067 100644
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -1143,8 +1143,8 @@ int vfs_set_acl(struct mnt_idmap *idmap, struct dentry *dentry,
 	inode_unlock(inode);
 
 	if (delegated_inode) {
-		error = break_deleg_wait(&delegated_inode);
-		if (!error)
+		error = break_deleg_wait(&delegated_inode, error);
+		if (error == -EWOULDBLOCK)
 			goto retry_deleg;
 	}
 
@@ -1251,8 +1251,8 @@ int vfs_remove_acl(struct mnt_idmap *idmap, struct dentry *dentry,
 	inode_unlock(inode);
 
 	if (delegated_inode) {
-		error = break_deleg_wait(&delegated_inode);
-		if (!error)
+		error = break_deleg_wait(&delegated_inode, error);
+		if (error == -EWOULDBLOCK)
 			goto retry_deleg;
 	}
 
diff --git a/fs/utimes.c b/fs/utimes.c
index 3701b3946f88..21b7605551dc 100644
--- a/fs/utimes.c
+++ b/fs/utimes.c
@@ -67,8 +67,8 @@ int vfs_utimes(const struct path *path, struct timespec64 *times)
 			      &delegated_inode);
 	inode_unlock(inode);
 	if (delegated_inode) {
-		error = break_deleg_wait(&delegated_inode);
-		if (!error)
+		error = break_deleg_wait(&delegated_inode, error);
+		if (error == -EWOULDBLOCK)
 			goto retry_deleg;
 	}
 
diff --git a/fs/xattr.c b/fs/xattr.c
index 7672ce5486c5..63e0b067dab9 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -323,8 +323,8 @@ vfs_setxattr(struct mnt_idmap *idmap, struct dentry *dentry,
 	inode_unlock(inode);
 
 	if (delegated_inode) {
-		error = break_deleg_wait(&delegated_inode);
-		if (!error)
+		error = break_deleg_wait(&delegated_inode, error);
+		if (error == -EWOULDBLOCK)
 			goto retry_deleg;
 	}
 	if (value != orig_value)
@@ -577,8 +577,8 @@ vfs_removexattr(struct mnt_idmap *idmap, struct dentry *dentry,
 	inode_unlock(inode);
 
 	if (delegated_inode) {
-		error = break_deleg_wait(&delegated_inode);
-		if (!error)
+		error = break_deleg_wait(&delegated_inode, error);
+		if (error == -EWOULDBLOCK)
 			goto retry_deleg;
 	}
 
diff --git a/include/linux/filelock.h b/include/linux/filelock.h
index daee999d05f3..66470ba9658c 100644
--- a/include/linux/filelock.h
+++ b/include/linux/filelock.h
@@ -144,6 +144,7 @@ struct file_lock_context {
 	struct list_head	flc_flock;
 	struct list_head	flc_posix;
 	struct list_head	flc_lease;
+	atomic_t		flc_deleg_blockers;
 };
 
 #ifdef CONFIG_FILE_LOCKING
@@ -450,21 +451,37 @@ static inline int try_break_deleg(struct inode *inode, struct inode **delegated_
 {
 	int ret;
 
+	if (delegated_inode && *delegated_inode) {
+		if (*delegated_inode == inode)
+			/* Don't need to count this */
+			return break_deleg(inode, O_WRONLY|O_NONBLOCK);
+
+		/* inode changed, forget the old one */
+		atomic_dec(&(*delegated_inode)->i_flctx->flc_deleg_blockers);
+		iput(*delegated_inode);
+		*delegated_inode = NULL;
+	}
 	ret = break_deleg(inode, O_WRONLY|O_NONBLOCK);
 	if (ret == -EWOULDBLOCK && delegated_inode) {
 		*delegated_inode = inode;
+		atomic_inc(&(*delegated_inode)->i_flctx->flc_deleg_blockers);
 		ihold(inode);
 	}
 	return ret;
 }
 
-static inline int break_deleg_wait(struct inode **delegated_inode)
+static inline int break_deleg_wait(struct inode **delegated_inode, int ret)
 {
-	int ret;
-
-	ret = break_deleg(*delegated_inode, O_WRONLY);
-	iput(*delegated_inode);
-	*delegated_inode = NULL;
+	if (ret == -EWOULDBLOCK) {
+		ret = break_deleg(*delegated_inode, O_WRONLY);
+		if (ret == 0)
+			ret = -EWOULDBLOCK;
+	}
+	if (ret != -EWOULDBLOCK) {
+		atomic_dec(&(*delegated_inode)->i_flctx->flc_deleg_blockers);
+		iput(*delegated_inode);
+		*delegated_inode = NULL;
+	}
 	return ret;
 }
 
@@ -494,7 +511,7 @@ static inline int try_break_deleg(struct inode *inode, struct inode **delegated_
 	return 0;
 }
 
-static inline int break_deleg_wait(struct inode **delegated_inode)
+static inline int break_deleg_wait(struct inode **delegated_inode, int ret)
 {
 	BUG();
 	return 0;
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6ca11e241a24..50957d9e1c2b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1902,7 +1902,8 @@ struct renamedata {
 	struct mnt_idmap *new_mnt_idmap;
 	struct inode *new_dir;
 	struct dentry *new_dentry;
-	struct inode **delegated_inode;
+	struct inode **delegated_inode_old;
+	struct inode **delegated_inode_new;
 	unsigned int flags;
 } __randomize_layout;
 

base-commit: 98f7e32f20d28ec452afb208f9cffc08448a2652
-- 
2.46.0


^ permalink raw reply related	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2024-09-30 14:15 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-16  4:34 [PATCH - RFC] VFS: disable new delegations during delegation-breaking operations NeilBrown
2024-09-18 20:40 ` kernel test robot
2024-09-18 22:24 ` kernel test robot
2024-09-25  8:56 ` Christian Brauner
2024-09-25 22:19 ` Al Viro
2024-09-25 22:42   ` NeilBrown
2024-09-25 23:06     ` Al Viro
2024-09-25 23:46       ` NeilBrown
2024-09-30 14:04 ` Jeff Layton
2024-09-30 14:15   ` Chuck Lever III

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.