[PATCH v1 0/4] Fixup NLM and kNFSD file lock callbacks

[PATCH v1 0/4] Fixup NLM and kNFSD file lock callbacks

[Benjamin Coddington]

Last year both GFS2 and OCFS2 had some work done to make their locking more
robust when exported over NFS.  Unfortunately, part of that work caused both
NLM (for NFS v3 exports) and kNFSD (for NFSv4.1+ exports) to no longer send
lock notifications to clients.

This in itself is not a huge problem because most NFS clients will still
poll the server in order to acquire a conflicted lock, but now that I've
noticed it I can't help but try to fix it because there are big advantages
for setups that might depend on timely lock notifications, and we've
supported that as a feature for a long time.

Its important for NLM and kNFSD that they do not block their kernel threads
inside filesystem's file_lock implementations because that can produce
deadlocks.  We used to make sure of this by only trusting that
posix_lock_file() can correctly handle blocking lock calls asynchronously,
so the lock managers would only setup their file_lock requests for async
callbacks if the filesystem did not define its own lock() file operation.

However, when GFS2 and OCFS2 grew the capability to correctly
handle blocking lock requests asynchronously, they started signalling this
behavior with EXPORT_OP_ASYNC_LOCK, and the check for also trusting
posix_lock_file() was inadvertently dropped, so now most filesystems no
longer produce lock notifications when exported over NFS.

I tried to fix this by simply including the old check for lock(), but the
resulting include mess and layering violations was more than I could accept.
There's a much cleaner way presented here using an fop_flag, which while
potentially flag-greedy, greatly simplifies the problem and grooms the
way for future uses by both filesystems and lock managers alike.

Criticism welcomed,
Ben

Benjamin Coddington (4):
  fs: Introduce FOP_ASYNC_LOCK
  gfs2/ocfs2: set FOP_ASYNC_LOCK
  NLM/NFSD: Fix lock notifications for async-capable filesystems
  exportfs: Remove EXPORT_OP_ASYNC_LOCK

 Documentation/filesystems/nfs/exporting.rst |  7 -------
 fs/gfs2/export.c                            |  1 -
 fs/gfs2/file.c                              |  2 ++
 fs/lockd/svclock.c                          |  5 ++---
 fs/nfsd/nfs4state.c                         | 19 ++++---------------
 fs/ocfs2/export.c                           |  1 -
 fs/ocfs2/file.c                             |  2 ++
 include/linux/exportfs.h                    | 13 -------------
 include/linux/filelock.h                    |  5 +++++
 include/linux/fs.h                          |  2 ++
 10 files changed, 17 insertions(+), 40 deletions(-)

-- 
2.44.0

[PATCH v1 1/4] fs: Introduce FOP_ASYNC_LOCK

[Benjamin Coddington]

Some lock managers (NLM, kNFSD) fastidiously avoid blocking their
kernel threads while servicing blocking locks.  If a filesystem supports
asynchronous lock requests those lock managers can use notifications to
quickly inform clients they have acquired a file lock.

Historically, only posix_lock_file() was capable of supporting asynchronous
locks so the check for support was simply file_operations->lock(), but with
recent changes in DLM, both GFS2 and OCFS2 also support asynchronous locks
and have started signalling their support with EXPORT_OP_ASYNC_LOCK.

We recently noticed that those changes dropped the checks for whether a
filesystem simply defaults to posix_lock_file(), so async lock
notifications have not been attempted for NLM and NFSv4.1+ for most
filesystems.  While trying to fix this it has become clear that testing
both the export flag combined with testing ->lock() creates quite a
layering mess.  It seems appropriate to signal support with a fop_flag.

Add FOP_ASYNC_LOCK so that filesystems with ->lock() can signal their
capability to handle lock requests asynchronously.  Add a helper for
lock managers to properly test that support.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 include/linux/filelock.h | 5 +++++
 include/linux/fs.h       | 2 ++
 2 files changed, 7 insertions(+)

diff --git a/include/linux/filelock.h b/include/linux/filelock.h
index daee999d05f3..58c1120a8253 100644
--- a/include/linux/filelock.h
+++ b/include/linux/filelock.h
@@ -180,6 +180,11 @@ static inline void locks_wake_up(struct file_lock *fl)
 	wake_up(&fl->c.flc_wait);
 }
 
+static inline bool locks_can_async_lock(const struct file_operations *fops)
+{
+	return !fops->lock || fops->fop_flags & FOP_ASYNC_LOCK;
+}
+
 /* fs/locks.c */
 void locks_free_lock_context(struct inode *inode);
 void locks_free_lock(struct file_lock *fl);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 6ca11e241a24..78221ae589d9 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -2074,6 +2074,8 @@ struct file_operations {
 #define FOP_DIO_PARALLEL_WRITE	((__force fop_flags_t)(1 << 3))
 /* Contains huge pages */
 #define FOP_HUGE_PAGES		((__force fop_flags_t)(1 << 4))
+/* Supports asynchronous lock callbacks */
+#define FOP_ASYNC_LOCK		((__force fop_flags_t)(1 << 5))
 
 /* Wrap a directory iterator that needs exclusive inode access */
 int wrap_directory_iterator(struct file *, struct dir_context *,
-- 
2.44.0

[PATCH v1 2/4] gfs2/ocfs2: set FOP_ASYNC_LOCK

[Benjamin Coddington]

Both GFS2 and OCFS2 use DLM locking, which will allow async lock requests.
Signal this support by setting FOP_ASYNC_LOCK.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/gfs2/file.c  | 2 ++
 fs/ocfs2/file.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 08982937b5df..b9ed2602287d 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -1586,6 +1586,7 @@ const struct file_operations gfs2_file_fops = {
 	.splice_write	= gfs2_file_splice_write,
 	.setlease	= simple_nosetlease,
 	.fallocate	= gfs2_fallocate,
+	.fop_flags	= FOP_ASYNC_LOCK,
 };
 
 const struct file_operations gfs2_dir_fops = {
@@ -1598,6 +1599,7 @@ const struct file_operations gfs2_dir_fops = {
 	.lock		= gfs2_lock,
 	.flock		= gfs2_flock,
 	.llseek		= default_llseek,
+	.fop_flags	= FOP_ASYNC_LOCK,
 };
 
 #endif /* CONFIG_GFS2_FS_LOCKING_DLM */
diff --git a/fs/ocfs2/file.c b/fs/ocfs2/file.c
index ccc57038a977..a642f1adee6a 100644
--- a/fs/ocfs2/file.c
+++ b/fs/ocfs2/file.c
@@ -2793,6 +2793,7 @@ const struct file_operations ocfs2_fops = {
 	.splice_write	= iter_file_splice_write,
 	.fallocate	= ocfs2_fallocate,
 	.remap_file_range = ocfs2_remap_file_range,
+	.fop_flags	= FOP_ASYNC_LOCK,
 };
 
 WRAP_DIR_ITER(ocfs2_readdir) // FIXME!
@@ -2809,6 +2810,7 @@ const struct file_operations ocfs2_dops = {
 #endif
 	.lock		= ocfs2_lock,
 	.flock		= ocfs2_flock,
+	.fop_flags	= FOP_ASYNC_LOCK,
 };
 
 /*
-- 
2.44.0

[PATCH v1 3/4] NLM/NFSD: Fix lock notifications for async-capable filesystems

[Benjamin Coddington]

Instead of checking just the exportfs flag, use the new
locks_can_async_lock() helper which allows NLM and NFSD to once again
support lock notifications for all filesystems which use posix_lock_file().

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 fs/lockd/svclock.c  |  5 ++---
 fs/nfsd/nfs4state.c | 19 ++++---------------
 2 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 1f2149db10f2..cbb87455a66d 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -30,7 +30,6 @@
 #include <linux/sunrpc/svc_xprt.h>
 #include <linux/lockd/nlm.h>
 #include <linux/lockd/lockd.h>
-#include <linux/exportfs.h>
 
 #define NLMDBG_FACILITY		NLMDBG_SVCLOCK
 
@@ -496,7 +495,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 				(long long)lock->fl.fl_end,
 				wait);
 
-	if (!exportfs_lock_op_is_async(inode->i_sb->s_export_op)) {
+	if (!locks_can_async_lock(nlmsvc_file_file(file)->f_op)) {
 		async_block = wait;
 		wait = 0;
 	}
@@ -550,7 +549,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 	 * requests on the underlaying ->lock() implementation but
 	 * only one nlm_block to being granted by lm_grant().
 	 */
-	if (exportfs_lock_op_is_async(inode->i_sb->s_export_op) &&
+	if (locks_can_async_lock(nlmsvc_file_file(file)->f_op) &&
 	    !list_empty(&block->b_list)) {
 		spin_unlock(&nlm_blocked_lock);
 		ret = nlm_lck_blocked;
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index a366fb1c1b9b..a061987abee3 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7953,9 +7953,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	fp = lock_stp->st_stid.sc_file;
 	switch (lock->lk_type) {
 		case NFS4_READW_LT:
-			if (nfsd4_has_session(cstate) ||
-			    exportfs_lock_op_is_async(sb->s_export_op))
-				flags |= FL_SLEEP;
 			fallthrough;
 		case NFS4_READ_LT:
 			spin_lock(&fp->fi_lock);
@@ -7966,9 +7963,6 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			type = F_RDLCK;
 			break;
 		case NFS4_WRITEW_LT:
-			if (nfsd4_has_session(cstate) ||
-			    exportfs_lock_op_is_async(sb->s_export_op))
-				flags |= FL_SLEEP;
 			fallthrough;
 		case NFS4_WRITE_LT:
 			spin_lock(&fp->fi_lock);
@@ -7988,15 +7982,10 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		goto out;
 	}
 
-	/*
-	 * Most filesystems with their own ->lock operations will block
-	 * the nfsd thread waiting to acquire the lock.  That leads to
-	 * deadlocks (we don't want every nfsd thread tied up waiting
-	 * for file locks), so don't attempt blocking lock notifications
-	 * on those filesystems:
-	 */
-	if (!exportfs_lock_op_is_async(sb->s_export_op))
-		flags &= ~FL_SLEEP;
+	if (lock->lk_type & (NFS4_READW_LT | NFS4_WRITEW_LT) &&
+		nfsd4_has_session(cstate) &&
+		locks_can_async_lock(nf->nf_file->f_op))
+			flags |= FL_SLEEP;
 
 	nbl = find_or_allocate_block(lock_sop, &fp->fi_fhandle, nn);
 	if (!nbl) {
-- 
2.44.0

[PATCH v1 4/4] exportfs: Remove EXPORT_OP_ASYNC_LOCK

[Benjamin Coddington]

Now that GFS2 and OCFS2 are signalling async ->lock() support with
FOP_ASYNC_LOCK and checks for support are converted, we can remove
EXPORT_OP_ASYNC_LOCK.

Signed-off-by: Benjamin Coddington <bcodding@redhat.com>
---
 Documentation/filesystems/nfs/exporting.rst |  7 -------
 fs/gfs2/export.c                            |  1 -
 fs/ocfs2/export.c                           |  1 -
 include/linux/exportfs.h                    | 13 -------------
 4 files changed, 22 deletions(-)

diff --git a/Documentation/filesystems/nfs/exporting.rst b/Documentation/filesystems/nfs/exporting.rst
index f04ce1215a03..de64d2d002a2 100644
--- a/Documentation/filesystems/nfs/exporting.rst
+++ b/Documentation/filesystems/nfs/exporting.rst
@@ -238,10 +238,3 @@ following flags are defined:
     all of an inode's dirty data on last close. Exports that behave this
     way should set EXPORT_OP_FLUSH_ON_CLOSE so that NFSD knows to skip
     waiting for writeback when closing such files.
-
-  EXPORT_OP_ASYNC_LOCK - Indicates a capable filesystem to do async lock
-    requests from lockd. Only set EXPORT_OP_ASYNC_LOCK if the filesystem has
-    it's own ->lock() functionality as core posix_lock_file() implementation
-    has no async lock request handling yet. For more information about how to
-    indicate an async lock request from a ->lock() file_operations struct, see
-    fs/locks.c and comment for the function vfs_lock_file().
diff --git a/fs/gfs2/export.c b/fs/gfs2/export.c
index d418d8b5367f..3334c394ce9c 100644
--- a/fs/gfs2/export.c
+++ b/fs/gfs2/export.c
@@ -190,6 +190,5 @@ const struct export_operations gfs2_export_ops = {
 	.fh_to_parent = gfs2_fh_to_parent,
 	.get_name = gfs2_get_name,
 	.get_parent = gfs2_get_parent,
-	.flags = EXPORT_OP_ASYNC_LOCK,
 };
 
diff --git a/fs/ocfs2/export.c b/fs/ocfs2/export.c
index 96b684763b39..b95724b767e1 100644
--- a/fs/ocfs2/export.c
+++ b/fs/ocfs2/export.c
@@ -280,5 +280,4 @@ const struct export_operations ocfs2_export_ops = {
 	.fh_to_dentry	= ocfs2_fh_to_dentry,
 	.fh_to_parent	= ocfs2_fh_to_parent,
 	.get_parent	= ocfs2_get_parent,
-	.flags		= EXPORT_OP_ASYNC_LOCK,
 };
diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index 893a1d21dc1c..1ab165c2939f 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -250,19 +250,6 @@ struct export_operations {
 	unsigned long	flags;
 };
 
-/**
- * exportfs_lock_op_is_async() - export op supports async lock operation
- * @export_ops:	the nfs export operations to check
- *
- * Returns true if the nfs export_operations structure has
- * EXPORT_OP_ASYNC_LOCK in their flags set
- */
-static inline bool
-exportfs_lock_op_is_async(const struct export_operations *export_ops)
-{
-	return export_ops->flags & EXPORT_OP_ASYNC_LOCK;
-}
-
 extern int exportfs_encode_inode_fh(struct inode *inode, struct fid *fid,
 				    int *max_len, struct inode *parent,
 				    int flags);
-- 
2.44.0

Re: [PATCH v1 0/4] Fixup NLM and kNFSD file lock callbacks

[Christian Brauner]

On Wed, Sep 11, 2024 at 03:42:56PM GMT, Benjamin Coddington wrote:
> Last year both GFS2 and OCFS2 had some work done to make their locking more
> robust when exported over NFS.  Unfortunately, part of that work caused both
> NLM (for NFS v3 exports) and kNFSD (for NFSv4.1+ exports) to no longer send
> lock notifications to clients.
> 
> This in itself is not a huge problem because most NFS clients will still
> poll the server in order to acquire a conflicted lock, but now that I've
> noticed it I can't help but try to fix it because there are big advantages
> for setups that might depend on timely lock notifications, and we've
> supported that as a feature for a long time.
> 
> Its important for NLM and kNFSD that they do not block their kernel threads
> inside filesystem's file_lock implementations because that can produce
> deadlocks.  We used to make sure of this by only trusting that
> posix_lock_file() can correctly handle blocking lock calls asynchronously,
> so the lock managers would only setup their file_lock requests for async
> callbacks if the filesystem did not define its own lock() file operation.
> 
> However, when GFS2 and OCFS2 grew the capability to correctly
> handle blocking lock requests asynchronously, they started signalling this
> behavior with EXPORT_OP_ASYNC_LOCK, and the check for also trusting
> posix_lock_file() was inadvertently dropped, so now most filesystems no
> longer produce lock notifications when exported over NFS.
> 
> I tried to fix this by simply including the old check for lock(), but the
> resulting include mess and layering violations was more than I could accept.
> There's a much cleaner way presented here using an fop_flag, which while
> potentially flag-greedy, greatly simplifies the problem and grooms the

It's fine. I've explicitly added the fop_flags so that stuff like this
we would not want to put into f->f_mode can live there.

> way for future uses by both filesystems and lock managers alike.

Re: [PATCH v1 0/4] Fixup NLM and kNFSD file lock callbacks

[Jeff Layton]

On Wed, 2024-09-11 at 15:42 -0400, Benjamin Coddington wrote:
> Last year both GFS2 and OCFS2 had some work done to make their locking more
> robust when exported over NFS.  Unfortunately, part of that work caused both
> NLM (for NFS v3 exports) and kNFSD (for NFSv4.1+ exports) to no longer send
> lock notifications to clients.
> 
> This in itself is not a huge problem because most NFS clients will still
> poll the server in order to acquire a conflicted lock, but now that I've
> noticed it I can't help but try to fix it because there are big advantages
> for setups that might depend on timely lock notifications, and we've
> supported that as a feature for a long time.
> 
> Its important for NLM and kNFSD that they do not block their kernel threads
> inside filesystem's file_lock implementations because that can produce
> deadlocks.  We used to make sure of this by only trusting that
> posix_lock_file() can correctly handle blocking lock calls asynchronously,
> so the lock managers would only setup their file_lock requests for async
> callbacks if the filesystem did not define its own lock() file operation.
> 
> However, when GFS2 and OCFS2 grew the capability to correctly
> handle blocking lock requests asynchronously, they started signalling this
> behavior with EXPORT_OP_ASYNC_LOCK, and the check for also trusting
> posix_lock_file() was inadvertently dropped, so now most filesystems no
> longer produce lock notifications when exported over NFS.
> 
> I tried to fix this by simply including the old check for lock(), but the
> resulting include mess and layering violations was more than I could accept.
> There's a much cleaner way presented here using an fop_flag, which while
> potentially flag-greedy, greatly simplifies the problem and grooms the
> way for future uses by both filesystems and lock managers alike.
> 
> Criticism welcomed,
> Ben
> 
> Benjamin Coddington (4):
>   fs: Introduce FOP_ASYNC_LOCK
>   gfs2/ocfs2: set FOP_ASYNC_LOCK
>   NLM/NFSD: Fix lock notifications for async-capable filesystems
>   exportfs: Remove EXPORT_OP_ASYNC_LOCK
> 
>  Documentation/filesystems/nfs/exporting.rst |  7 -------
>  fs/gfs2/export.c                            |  1 -
>  fs/gfs2/file.c                              |  2 ++
>  fs/lockd/svclock.c                          |  5 ++---
>  fs/nfsd/nfs4state.c                         | 19 ++++---------------
>  fs/ocfs2/export.c                           |  1 -
>  fs/ocfs2/file.c                             |  2 ++
>  include/linux/exportfs.h                    | 13 -------------
>  include/linux/filelock.h                    |  5 +++++
>  include/linux/fs.h                          |  2 ++
>  10 files changed, 17 insertions(+), 40 deletions(-)
> 

Thanks for fixing this up, Ben!

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v1 0/4] Fixup NLM and kNFSD file lock callbacks

[Christian Brauner]

On Thu, Sep 12, 2024 at 07:08:07AM GMT, Jeff Layton wrote:
> On Wed, 2024-09-11 at 15:42 -0400, Benjamin Coddington wrote:
> > Last year both GFS2 and OCFS2 had some work done to make their locking more
> > robust when exported over NFS.  Unfortunately, part of that work caused both
> > NLM (for NFS v3 exports) and kNFSD (for NFSv4.1+ exports) to no longer send
> > lock notifications to clients.
> > 
> > This in itself is not a huge problem because most NFS clients will still
> > poll the server in order to acquire a conflicted lock, but now that I've
> > noticed it I can't help but try to fix it because there are big advantages
> > for setups that might depend on timely lock notifications, and we've
> > supported that as a feature for a long time.
> > 
> > Its important for NLM and kNFSD that they do not block their kernel threads
> > inside filesystem's file_lock implementations because that can produce
> > deadlocks.  We used to make sure of this by only trusting that
> > posix_lock_file() can correctly handle blocking lock calls asynchronously,
> > so the lock managers would only setup their file_lock requests for async
> > callbacks if the filesystem did not define its own lock() file operation.
> > 
> > However, when GFS2 and OCFS2 grew the capability to correctly
> > handle blocking lock requests asynchronously, they started signalling this
> > behavior with EXPORT_OP_ASYNC_LOCK, and the check for also trusting
> > posix_lock_file() was inadvertently dropped, so now most filesystems no
> > longer produce lock notifications when exported over NFS.
> > 
> > I tried to fix this by simply including the old check for lock(), but the
> > resulting include mess and layering violations was more than I could accept.
> > There's a much cleaner way presented here using an fop_flag, which while
> > potentially flag-greedy, greatly simplifies the problem and grooms the
> > way for future uses by both filesystems and lock managers alike.
> > 
> > Criticism welcomed,
> > Ben
> > 
> > Benjamin Coddington (4):
> >   fs: Introduce FOP_ASYNC_LOCK
> >   gfs2/ocfs2: set FOP_ASYNC_LOCK
> >   NLM/NFSD: Fix lock notifications for async-capable filesystems
> >   exportfs: Remove EXPORT_OP_ASYNC_LOCK
> > 
> >  Documentation/filesystems/nfs/exporting.rst |  7 -------
> >  fs/gfs2/export.c                            |  1 -
> >  fs/gfs2/file.c                              |  2 ++
> >  fs/lockd/svclock.c                          |  5 ++---
> >  fs/nfsd/nfs4state.c                         | 19 ++++---------------
> >  fs/ocfs2/export.c                           |  1 -
> >  fs/ocfs2/file.c                             |  2 ++
> >  include/linux/exportfs.h                    | 13 -------------
> >  include/linux/filelock.h                    |  5 +++++
> >  include/linux/fs.h                          |  2 ++
> >  10 files changed, 17 insertions(+), 40 deletions(-)
> > 
> 
> Thanks for fixing this up, Ben!
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>

It might be a bit late for v6.12 so I would stuff this into a branch for
v6.13. Sound ok?

Re: [PATCH v1 0/4] Fixup NLM and kNFSD file lock callbacks

[Jeff Layton]

On Thu, 2024-09-12 at 13:32 +0200, Christian Brauner wrote:
> On Thu, Sep 12, 2024 at 07:08:07AM GMT, Jeff Layton wrote:
> > On Wed, 2024-09-11 at 15:42 -0400, Benjamin Coddington wrote:
> > > Last year both GFS2 and OCFS2 had some work done to make their locking more
> > > robust when exported over NFS.  Unfortunately, part of that work caused both
> > > NLM (for NFS v3 exports) and kNFSD (for NFSv4.1+ exports) to no longer send
> > > lock notifications to clients.
> > > 
> > > This in itself is not a huge problem because most NFS clients will still
> > > poll the server in order to acquire a conflicted lock, but now that I've
> > > noticed it I can't help but try to fix it because there are big advantages
> > > for setups that might depend on timely lock notifications, and we've
> > > supported that as a feature for a long time.
> > > 
> > > Its important for NLM and kNFSD that they do not block their kernel threads
> > > inside filesystem's file_lock implementations because that can produce
> > > deadlocks.  We used to make sure of this by only trusting that
> > > posix_lock_file() can correctly handle blocking lock calls asynchronously,
> > > so the lock managers would only setup their file_lock requests for async
> > > callbacks if the filesystem did not define its own lock() file operation.
> > > 
> > > However, when GFS2 and OCFS2 grew the capability to correctly
> > > handle blocking lock requests asynchronously, they started signalling this
> > > behavior with EXPORT_OP_ASYNC_LOCK, and the check for also trusting
> > > posix_lock_file() was inadvertently dropped, so now most filesystems no
> > > longer produce lock notifications when exported over NFS.
> > > 
> > > I tried to fix this by simply including the old check for lock(), but the
> > > resulting include mess and layering violations was more than I could accept.
> > > There's a much cleaner way presented here using an fop_flag, which while
> > > potentially flag-greedy, greatly simplifies the problem and grooms the
> > > way for future uses by both filesystems and lock managers alike.
> > > 
> > > Criticism welcomed,
> > > Ben
> > > 
> > > Benjamin Coddington (4):
> > >   fs: Introduce FOP_ASYNC_LOCK
> > >   gfs2/ocfs2: set FOP_ASYNC_LOCK
> > >   NLM/NFSD: Fix lock notifications for async-capable filesystems
> > >   exportfs: Remove EXPORT_OP_ASYNC_LOCK
> > > 
> > >  Documentation/filesystems/nfs/exporting.rst |  7 -------
> > >  fs/gfs2/export.c                            |  1 -
> > >  fs/gfs2/file.c                              |  2 ++
> > >  fs/lockd/svclock.c                          |  5 ++---
> > >  fs/nfsd/nfs4state.c                         | 19 ++++---------------
> > >  fs/ocfs2/export.c                           |  1 -
> > >  fs/ocfs2/file.c                             |  2 ++
> > >  include/linux/exportfs.h                    | 13 -------------
> > >  include/linux/filelock.h                    |  5 +++++
> > >  include/linux/fs.h                          |  2 ++
> > >  10 files changed, 17 insertions(+), 40 deletions(-)
> > > 
> > 
> > Thanks for fixing this up, Ben!
> > 
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> 
> It might be a bit late for v6.12 so I would stuff this into a branch for
> v6.13. Sound ok?

Ok. I figured Chuck would take this set, but I guess it is more VFS-y.

I think this is reasonably safe though, so if Ben needs it before then,
we could pull it in sooner.
-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH v1 0/4] Fixup NLM and kNFSD file lock callbacks

[Benjamin Coddington]

On 12 Sep 2024, at 7:51, Jeff Layton wrote:

> On Thu, 2024-09-12 at 13:32 +0200, Christian Brauner wrote:
>>
>> It might be a bit late for v6.12 so I would stuff this into a branch for
>> v6.13. Sound ok?
>
> Ok. I figured Chuck would take this set, but I guess it is more VFS-y.
>
> I think this is reasonably safe though, so if Ben needs it before then,
> we could pull it in sooner.

Absolutely no rush here, v6.13 is not a problem.

Ben

Re: [PATCH v1 0/4] Fixup NLM and kNFSD file lock callbacks

[Christian Brauner]

On Wed, 11 Sep 2024 15:42:56 -0400, Benjamin Coddington wrote:
> Last year both GFS2 and OCFS2 had some work done to make their locking more
> robust when exported over NFS.  Unfortunately, part of that work caused both
> NLM (for NFS v3 exports) and kNFSD (for NFSv4.1+ exports) to no longer send
> lock notifications to clients.
> 
> This in itself is not a huge problem because most NFS clients will still
> poll the server in order to acquire a conflicted lock, but now that I've
> noticed it I can't help but try to fix it because there are big advantages
> for setups that might depend on timely lock notifications, and we've
> supported that as a feature for a long time.
> 
> [...]

Applied to the vfs.misc.v6.13 branch of the vfs/vfs.git tree.
Patches in the vfs.misc.v6.13 branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc.v6.13

[1/4] fs: Introduce FOP_ASYNC_LOCK
      https://git.kernel.org/vfs/vfs/c/8cf9a01edc21
[2/4] gfs2/ocfs2: set FOP_ASYNC_LOCK
      https://git.kernel.org/vfs/vfs/c/2253ab99f2e9
[3/4] NLM/NFSD: Fix lock notifications for async-capable filesystems
      https://git.kernel.org/vfs/vfs/c/81be05940ccc
[4/4] exportfs: Remove EXPORT_OP_ASYNC_LOCK
      https://git.kernel.org/vfs/vfs/c/bb06326008c3

Re: [PATCH v1 0/4] Fixup NLM and kNFSD file lock callbacks

[Chuck Lever III]

> On Sep 11, 2024, at 3:42 PM, Benjamin Coddington <bcodding@redhat.com> wrote:
> 
> Last year both GFS2 and OCFS2 had some work done to make their locking more
> robust when exported over NFS.  Unfortunately, part of that work caused both
> NLM (for NFS v3 exports) and kNFSD (for NFSv4.1+ exports) to no longer send
> lock notifications to clients.
> 
> This in itself is not a huge problem because most NFS clients will still
> poll the server in order to acquire a conflicted lock, but now that I've
> noticed it I can't help but try to fix it because there are big advantages
> for setups that might depend on timely lock notifications, and we've
> supported that as a feature for a long time.
> 
> Its important for NLM and kNFSD that they do not block their kernel threads
> inside filesystem's file_lock implementations because that can produce
> deadlocks.  We used to make sure of this by only trusting that
> posix_lock_file() can correctly handle blocking lock calls asynchronously,
> so the lock managers would only setup their file_lock requests for async
> callbacks if the filesystem did not define its own lock() file operation.
> 
> However, when GFS2 and OCFS2 grew the capability to correctly
> handle blocking lock requests asynchronously, they started signalling this
> behavior with EXPORT_OP_ASYNC_LOCK, and the check for also trusting
> posix_lock_file() was inadvertently dropped, so now most filesystems no
> longer produce lock notifications when exported over NFS.
> 
> I tried to fix this by simply including the old check for lock(), but the
> resulting include mess and layering violations was more than I could accept.
> There's a much cleaner way presented here using an fop_flag, which while
> potentially flag-greedy, greatly simplifies the problem and grooms the
> way for future uses by both filesystems and lock managers alike.
> 
> Criticism welcomed,
> Ben
> 
> Benjamin Coddington (4):
>  fs: Introduce FOP_ASYNC_LOCK
>  gfs2/ocfs2: set FOP_ASYNC_LOCK
>  NLM/NFSD: Fix lock notifications for async-capable filesystems
>  exportfs: Remove EXPORT_OP_ASYNC_LOCK
> 
> Documentation/filesystems/nfs/exporting.rst |  7 -------
> fs/gfs2/export.c                            |  1 -
> fs/gfs2/file.c                              |  2 ++
> fs/lockd/svclock.c                          |  5 ++---
> fs/nfsd/nfs4state.c                         | 19 ++++---------------
> fs/ocfs2/export.c                           |  1 -
> fs/ocfs2/file.c                             |  2 ++
> include/linux/exportfs.h                    | 13 -------------
> include/linux/filelock.h                    |  5 +++++
> include/linux/fs.h                          |  2 ++
> 10 files changed, 17 insertions(+), 40 deletions(-)
> 
> -- 
> 2.44.0
> 

For the NFSD and exportfs hunks:

Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>>

"lockd: introduce safe async lock op" is in v6.10. Does this
series need to be backported to v6.10.y ? Should the series
have "Fixes: 2dd10de8e6bc ("lockd: introduce safe async lock
 op")" ?


--
Chuck Lever

Re: [PATCH v1 0/4] Fixup NLM and kNFSD file lock callbacks

[Benjamin Coddington]

On 12 Sep 2024, at 10:01, Chuck Lever III wrote:

> For the NFSD and exportfs hunks:
>
> Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>>
>
> "lockd: introduce safe async lock op" is in v6.10. Does this
> series need to be backported to v6.10.y ? Should the series
> have "Fixes: 2dd10de8e6bc ("lockd: introduce safe async lock
>  op")" ?

Thanks Chuck! Probably yes, if we want notifications fixed up there.  It
should be sufficient to add this to the signoff area for at least the first
three (and fourth for cleanup):

Cc: <stable@vger.kernel.org> # 6.10.x

No problem for me to send a v2 with these if needed.

Ben

Re: [PATCH v1 0/4] Fixup NLM and kNFSD file lock callbacks

[Chuck Lever III]

> On Sep 12, 2024, at 11:06 AM, Benjamin Coddington <bcodding@redhat.com> wrote:
> 
> On 12 Sep 2024, at 10:01, Chuck Lever III wrote:
> 
>> For the NFSD and exportfs hunks:
>> 
>> Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>>
>> 
>> "lockd: introduce safe async lock op" is in v6.10. Does this
>> series need to be backported to v6.10.y ? Should the series
>> have "Fixes: 2dd10de8e6bc ("lockd: introduce safe async lock
>> op")" ?
> 
> Thanks Chuck! Probably yes, if we want notifications fixed up there.  It
> should be sufficient to add this to the signoff area for at least the first
> three (and fourth for cleanup):
> 
> Cc: <stable@vger.kernel.org> # 6.10.x

2dd10de8e6bc landed in v6.7.

I suppose that since v6.10.y is likely to be closed by
the time this series is applied upstream, this tag might
be confusing.

Thus Fixes: 2dd10de8e6bc and a plain Cc: stable should
work best. Then whichever stable kernel is open when your
fixes are merged upstream will automatically get fixed.

None of the current LTS kernels have 2dd10de8e6bc so they
aren't relevant at this point.

--
Chuck Lever

Re: [PATCH v1 0/4] Fixup NLM and kNFSD file lock callbacks

[Benjamin Coddington]

On 12 Sep 2024, at 14:17, Chuck Lever III wrote:

>> On Sep 12, 2024, at 11:06 AM, Benjamin Coddington <bcodding@redhat.com> wrote:
>>
>> On 12 Sep 2024, at 10:01, Chuck Lever III wrote:
>>
>>> For the NFSD and exportfs hunks:
>>>
>>> Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>>
>>>
>>> "lockd: introduce safe async lock op" is in v6.10. Does this
>>> series need to be backported to v6.10.y ? Should the series
>>> have "Fixes: 2dd10de8e6bc ("lockd: introduce safe async lock
>>> op")" ?
>>
>> Thanks Chuck! Probably yes, if we want notifications fixed up there.  It
>> should be sufficient to add this to the signoff area for at least the first
>> three (and fourth for cleanup):
>>
>> Cc: <stable@vger.kernel.org> # 6.10.x
>
> 2dd10de8e6bc landed in v6.7.
>
> I suppose that since v6.10.y is likely to be closed by
> the time this series is applied upstream, this tag might
> be confusing.
>
> Thus Fixes: 2dd10de8e6bc and a plain Cc: stable should
> work best. Then whichever stable kernel is open when your
> fixes are merged upstream will automatically get fixed.

So you want "Fixes: 2dd10de8e6bc" on all these patches?  Fixing the problem
requires all of the first three patches together.  My worry is that a
"Fixes" on each implies a complete fix within that patch, so its really not
appropriate.

The stable-kernel-rules.rst documentation says for a series, the Cc: stable
tag should be suffient to request dependencies within the series, so that's
why I suggested it for the version you requested.

What exactly would you like to see?  I am happy to send a 2nd version.

Ben

Re: [PATCH v1 0/4] Fixup NLM and kNFSD file lock callbacks

[Chuck Lever III]

> On Sep 12, 2024, at 3:11 PM, Benjamin Coddington <bcodding@redhat.com> wrote:
> 
> On 12 Sep 2024, at 14:17, Chuck Lever III wrote:
> 
>>> On Sep 12, 2024, at 11:06 AM, Benjamin Coddington <bcodding@redhat.com> wrote:
>>> 
>>> On 12 Sep 2024, at 10:01, Chuck Lever III wrote:
>>> 
>>>> For the NFSD and exportfs hunks:
>>>> 
>>>> Acked-by: Chuck Lever <chuck.lever@oracle.com <mailto:chuck.lever@oracle.com>>
>>>> 
>>>> "lockd: introduce safe async lock op" is in v6.10. Does this
>>>> series need to be backported to v6.10.y ? Should the series
>>>> have "Fixes: 2dd10de8e6bc ("lockd: introduce safe async lock
>>>> op")" ?
>>> 
>>> Thanks Chuck! Probably yes, if we want notifications fixed up there.  It
>>> should be sufficient to add this to the signoff area for at least the first
>>> three (and fourth for cleanup):
>>> 
>>> Cc: <stable@vger.kernel.org> # 6.10.x
>> 
>> 2dd10de8e6bc landed in v6.7.
>> 
>> I suppose that since v6.10.y is likely to be closed by
>> the time this series is applied upstream, this tag might
>> be confusing.
>> 
>> Thus Fixes: 2dd10de8e6bc and a plain Cc: stable should
>> work best. Then whichever stable kernel is open when your
>> fixes are merged upstream will automatically get fixed.
> 
> So you want "Fixes: 2dd10de8e6bc" on all these patches?  Fixing the problem
> requires all of the first three patches together.

I didn't indicate which patches to add the tags to, sorry.
3/4 sounds like the right place.

If 4/4 is a clean-up only, no new tags apply to that.


> My worry is that a
> "Fixes" on each implies a complete fix within that patch, so its really not
> appropriate.

Fixes seems to mean different things to different people. It's
OK to drop that tag, but I prefer to see a pointer to the broken
commit. That helps downstream consumers of the commit log to
identify which patches they should be pulling in.


> The stable-kernel-rules.rst documentation says for a series, the Cc: stable
> tag should be suffient to request dependencies within the series, so that's
> why I suggested it for the version you requested.
> 
> What exactly would you like to see?  I am happy to send a 2nd version.

You don't need to send again. Christian can add tags in his repo.

My objection is to the "# 6.10.x" comment -- that doesn't make sense
because for sure, the stable tree will have moved on by the time that
v6.13-rc opens.


--
Chuck Lever