linux-nfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCHv2 nfsd/master 0/7] lockd: dlm: async lock request changes
@ 2023-09-12 21:53 Alexander Aring
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 1/7] lockd: introduce safe async lock op Alexander Aring
                   ` (7 more replies)
  0 siblings, 8 replies; 10+ messages in thread
From: Alexander Aring @ 2023-09-12 21:53 UTC (permalink / raw)
  To: linux-nfs
  Cc: gfs2, ocfs2-devel, linux-fsdevel, teigland, rpeterso, agruenba,
	trond.myklebust, anna, chuck.lever, jlayton, aahringo

Hi,

I sent this as a PATCH now and drop the RFC. I got some review back from
Jeff Layton and hope I was successful to follow it. There are still
issues with lockd and asynchronous lock request but it will at least not
directly crash when somebody is using nfs on top of GFS2. The issues are
related to cancellation of locks/lockd decides to drop nlm_block for
some reasons while pending request is happening.

I did not change more documentation about the asynchronous lock request
functionality to not confuse users. In my opinion there should be more
documentation about what you SHOULD NOT do with this API. Otherwise I
think the documentation is still correct.

I will send a follow up patch to fsdevel to change all users using
IS_SETLKW() to use FL_SLEEP to determine if it's blocking or
non-blocking and send it to fsdevel as it was recommended. This will
just be a grep and replace patch and look what happens.

- Alex

changes since v2:
 - remove B_PENDING_CALLBACK paragraph from commit msg. Was a leftover
   and I forgot to update the commit message.
 - change function name from export_op_support_safe_async_lock()
   to exportfs_lock_op_is_async()
 - change flag name from EXPORT_OP_SAFE_ASYNC_LOCK to
   EXPORT_OP_ASYNC_LOCK
 - add documentation for EXPORT_OP_ASYNC_LOCK to
   Documentation/filesystems/nfs/exporting.rst
 - add newline between function name and return type of
   exportfs_lock_op_is_async()
 - remove f_op->lock() check and mention it in
   Documentation/filesystems/nfs/exporting.rst to only use it with
   filesystems implementing their own ->lock()
 - add kdoc for exportfs_lock_op_is_async()

changes since RFC:

- drop the pending callback flag but introduce "lockd: don't call
  vfs_lock_file() for pending requests", see patch why I need it.
- drop per nlm_block callback mutex
- change async lock request documentation
- use always FL_SLEEP to determine if it's blocking vs non-blocking in
  DLM

Alexander Aring (7):
  lockd: introduce safe async lock op
  lockd: don't call vfs_lock_file() for pending requests
  lockd: fix race in async lock request handling
  lockd: add doc to enable EXPORT_OP_ASYNC_LOCK
  dlm: use fl_owner from lockd
  dlm: use FL_SLEEP to determine blocking vs non-blocking
  dlm: implement EXPORT_OP_ASYNC_LOCK

 Documentation/filesystems/nfs/exporting.rst |  7 ++++
 fs/dlm/plock.c                              | 20 +++--------
 fs/gfs2/export.c                            |  1 +
 fs/lockd/svclock.c                          | 38 ++++++++++++++-------
 fs/locks.c                                  | 12 ++++---
 fs/nfsd/nfs4state.c                         | 10 ++++--
 fs/ocfs2/export.c                           |  1 +
 include/linux/exportfs.h                    | 14 ++++++++
 8 files changed, 67 insertions(+), 36 deletions(-)

-- 
2.31.1


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCHv2 nfsd/master 1/7] lockd: introduce safe async lock op
  2023-09-12 21:53 [PATCHv2 nfsd/master 0/7] lockd: dlm: async lock request changes Alexander Aring
@ 2023-09-12 21:53 ` Alexander Aring
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 2/7] lockd: don't call vfs_lock_file() for pending requests Alexander Aring
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Alexander Aring @ 2023-09-12 21:53 UTC (permalink / raw)
  To: linux-nfs
  Cc: gfs2, ocfs2-devel, linux-fsdevel, teigland, rpeterso, agruenba,
	trond.myklebust, anna, chuck.lever, jlayton, aahringo

This patch reverts mostly commit 40595cdc93ed ("nfs: block notification
on fs with its own ->lock") and introduces an EXPORT_OP_ASYNC_LOCK
export flag to signal that the "own ->lock" implementation supports
async lock requests. The only main user is DLM that is used by GFS2 and
OCFS2 filesystem. Those implement their own lock() implementation and
return FILE_LOCK_DEFERRED as return value. Since commit 40595cdc93ed
("nfs: block notification on fs with its own ->lock") the DLM
implementation were never updated. This patch should prepare for DLM
to set the EXPORT_OP_ASYNC_LOCK export flag and update the DLM
plock implementation regarding to it.

Acked-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Alexander Aring <aahringo@redhat.com>
---
 Documentation/filesystems/nfs/exporting.rst |  7 +++++++
 fs/lockd/svclock.c                          |  4 +---
 fs/nfsd/nfs4state.c                         | 10 +++++++---
 include/linux/exportfs.h                    | 14 ++++++++++++++
 4 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/Documentation/filesystems/nfs/exporting.rst b/Documentation/filesystems/nfs/exporting.rst
index 4b30daee399a..198d805d611c 100644
--- a/Documentation/filesystems/nfs/exporting.rst
+++ b/Documentation/filesystems/nfs/exporting.rst
@@ -241,3 +241,10 @@ 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/lockd/svclock.c b/fs/lockd/svclock.c
index 43aeba9de55c..d500e32ebb18 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -481,9 +481,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 	    struct nlm_host *host, struct nlm_lock *lock, int wait,
 	    struct nlm_cookie *cookie, int reclaim)
 {
-#if IS_ENABLED(CONFIG_SUNRPC_DEBUG)
 	struct inode		*inode = nlmsvc_file_inode(file);
-#endif
 	struct nlm_block	*block = NULL;
 	int			error;
 	int			mode;
@@ -497,7 +495,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 				(long long)lock->fl.fl_end,
 				wait);
 
-	if (nlmsvc_file_file(file)->f_op->lock) {
+	if (!exportfs_lock_op_is_async(inode->i_sb->s_export_op)) {
 		async_block = wait;
 		wait = 0;
 	}
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 8534693eb6a4..7cabe882724e 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -7487,6 +7487,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	struct nfsd4_blocked_lock *nbl = NULL;
 	struct file_lock *file_lock = NULL;
 	struct file_lock *conflock = NULL;
+	struct super_block *sb;
 	__be32 status = 0;
 	int lkflg;
 	int err;
@@ -7508,6 +7509,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		dprintk("NFSD: nfsd4_lock: permission denied!\n");
 		return status;
 	}
+	sb = cstate->current_fh.fh_dentry->d_sb;
 
 	if (lock->lk_is_new) {
 		if (nfsd4_has_session(cstate))
@@ -7559,7 +7561,8 @@ 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))
+			if (nfsd4_has_session(cstate) ||
+			    exportfs_lock_op_is_async(sb->s_export_op))
 				fl_flags |= FL_SLEEP;
 			fallthrough;
 		case NFS4_READ_LT:
@@ -7571,7 +7574,8 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			fl_type = F_RDLCK;
 			break;
 		case NFS4_WRITEW_LT:
-			if (nfsd4_has_session(cstate))
+			if (nfsd4_has_session(cstate) ||
+			    exportfs_lock_op_is_async(sb->s_export_op))
 				fl_flags |= FL_SLEEP;
 			fallthrough;
 		case NFS4_WRITE_LT:
@@ -7599,7 +7603,7 @@ nfsd4_lock(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	 * for file locks), so don't attempt blocking lock notifications
 	 * on those filesystems:
 	 */
-	if (nf->nf_file->f_op->lock)
+	if (!exportfs_lock_op_is_async(sb->s_export_op))
 		fl_flags &= ~FL_SLEEP;
 
 	nbl = find_or_allocate_block(lock_sop, &fp->fi_fhandle, nn);
diff --git a/include/linux/exportfs.h b/include/linux/exportfs.h
index 11fbd0ee1370..6dd993240fcc 100644
--- a/include/linux/exportfs.h
+++ b/include/linux/exportfs.h
@@ -224,9 +224,23 @@ struct export_operations {
 						  atomic attribute updates
 						*/
 #define EXPORT_OP_FLUSH_ON_CLOSE	(0x20) /* fs flushes file data on close */
+#define EXPORT_OP_ASYNC_LOCK		(0x40) /* fs can do async lock request */
 	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.31.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv2 nfsd/master 2/7] lockd: don't call vfs_lock_file() for pending requests
  2023-09-12 21:53 [PATCHv2 nfsd/master 0/7] lockd: dlm: async lock request changes Alexander Aring
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 1/7] lockd: introduce safe async lock op Alexander Aring
@ 2023-09-12 21:53 ` Alexander Aring
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 3/7] lockd: fix race in async lock request handling Alexander Aring
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Alexander Aring @ 2023-09-12 21:53 UTC (permalink / raw)
  To: linux-nfs
  Cc: gfs2, ocfs2-devel, linux-fsdevel, teigland, rpeterso, agruenba,
	trond.myklebust, anna, chuck.lever, jlayton, aahringo

This patch returns nlm_lck_blocked in nlmsvc_lock() when an asynchronous
lock request is pending. During testing I ran into the case with the
side-effects that lockd is waiting for only one lm_grant() callback
because it's already part of the nlm_blocked list. If another
asynchronous for the same nlm_block is triggered two lm_grant()
callbacks will occur but lockd was only waiting for one.

To avoid any change of existing users this handling will only being made
when export_op_support_safe_async_lock() returns true.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Alexander Aring <aahringo@redhat.com>
---
 fs/lockd/svclock.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index d500e32ebb18..c313622a9578 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -541,6 +541,22 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 		goto out;
 	}
 
+	spin_lock(&nlm_blocked_lock);
+	/*
+	 * If this is a lock request for an already pending
+	 * lock request we return nlm_lck_blocked without calling
+	 * vfs_lock_file() again. Otherwise we have two pending
+	 * 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) &&
+	    !list_empty(&block->b_list)) {
+		spin_unlock(&nlm_blocked_lock);
+		ret = nlm_lck_blocked;
+		goto out;
+	}
+	spin_unlock(&nlm_blocked_lock);
+
 	if (!wait)
 		lock->fl.fl_flags &= ~FL_SLEEP;
 	mode = lock_to_openmode(&lock->fl);
@@ -553,13 +569,6 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 			ret = nlm_granted;
 			goto out;
 		case -EAGAIN:
-			/*
-			 * If this is a blocking request for an
-			 * already pending lock request then we need
-			 * to put it back on lockd's block list
-			 */
-			if (wait)
-				break;
 			ret = async_block ? nlm_lck_blocked : nlm_lck_denied;
 			goto out;
 		case FILE_LOCK_DEFERRED:
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv2 nfsd/master 3/7] lockd: fix race in async lock request handling
  2023-09-12 21:53 [PATCHv2 nfsd/master 0/7] lockd: dlm: async lock request changes Alexander Aring
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 1/7] lockd: introduce safe async lock op Alexander Aring
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 2/7] lockd: don't call vfs_lock_file() for pending requests Alexander Aring
@ 2023-09-12 21:53 ` Alexander Aring
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 4/7] lockd: add doc to enable EXPORT_OP_ASYNC_LOCK Alexander Aring
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Alexander Aring @ 2023-09-12 21:53 UTC (permalink / raw)
  To: linux-nfs
  Cc: gfs2, ocfs2-devel, linux-fsdevel, teigland, rpeterso, agruenba,
	trond.myklebust, anna, chuck.lever, jlayton, aahringo

This patch fixes a race in async lock request handling between adding
the relevant struct nlm_block to nlm_blocked list after the request was
sent by vfs_lock_file() and nlmsvc_grant_deferred() does a lookup of the
nlm_block in the nlm_blocked list. It could be that the async request is
completed before the nlm_block was added to the list. This would end
in a -ENOENT and a kernel log message of "lockd: grant for unknown
block".

To solve this issue we add the nlm_block before the vfs_lock_file() call
to be sure it has been added when a possible nlmsvc_grant_deferred() is
called. If the vfs_lock_file() results in an case when it wouldn't be
added to nlm_blocked list, the nlm_block struct will be removed from
this list again.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Alexander Aring <aahringo@redhat.com>
---
 fs/lockd/svclock.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index c313622a9578..993999297e31 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -555,6 +555,9 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 		ret = nlm_lck_blocked;
 		goto out;
 	}
+
+	/* Append to list of blocked */
+	nlmsvc_insert_block_locked(block, NLM_NEVER);
 	spin_unlock(&nlm_blocked_lock);
 
 	if (!wait)
@@ -566,9 +569,12 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 	dprintk("lockd: vfs_lock_file returned %d\n", error);
 	switch (error) {
 		case 0:
+			nlmsvc_remove_block(block);
 			ret = nlm_granted;
 			goto out;
 		case -EAGAIN:
+			if (!wait)
+				nlmsvc_remove_block(block);
 			ret = async_block ? nlm_lck_blocked : nlm_lck_denied;
 			goto out;
 		case FILE_LOCK_DEFERRED:
@@ -579,17 +585,16 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 			ret = nlmsvc_defer_lock_rqst(rqstp, block);
 			goto out;
 		case -EDEADLK:
+			nlmsvc_remove_block(block);
 			ret = nlm_deadlock;
 			goto out;
 		default:			/* includes ENOLCK */
+			nlmsvc_remove_block(block);
 			ret = nlm_lck_denied_nolocks;
 			goto out;
 	}
 
 	ret = nlm_lck_blocked;
-
-	/* Append to list of blocked */
-	nlmsvc_insert_block(block, NLM_NEVER);
 out:
 	mutex_unlock(&file->f_mutex);
 	nlmsvc_release_block(block);
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv2 nfsd/master 4/7] lockd: add doc to enable EXPORT_OP_ASYNC_LOCK
  2023-09-12 21:53 [PATCHv2 nfsd/master 0/7] lockd: dlm: async lock request changes Alexander Aring
                   ` (2 preceding siblings ...)
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 3/7] lockd: fix race in async lock request handling Alexander Aring
@ 2023-09-12 21:53 ` Alexander Aring
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 5/7] dlm: use fl_owner from lockd Alexander Aring
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Alexander Aring @ 2023-09-12 21:53 UTC (permalink / raw)
  To: linux-nfs
  Cc: gfs2, ocfs2-devel, linux-fsdevel, teigland, rpeterso, agruenba,
	trond.myklebust, anna, chuck.lever, jlayton, aahringo

This patch adds a note to enable EXPORT_OP_ASYNC_LOCK for
asynchronous lock request handling.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Alexander Aring <aahringo@redhat.com>
---
 fs/locks.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index 76ad05f8070a..d4e49a990a8d 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -2264,11 +2264,13 @@ int fcntl_getlk(struct file *filp, unsigned int cmd, struct flock *flock)
  * To avoid blocking kernel daemons, such as lockd, that need to acquire POSIX
  * locks, the ->lock() interface may return asynchronously, before the lock has
  * been granted or denied by the underlying filesystem, if (and only if)
- * lm_grant is set. Callers expecting ->lock() to return asynchronously
- * will only use F_SETLK, not F_SETLKW; they will set FL_SLEEP if (and only if)
- * the request is for a blocking lock. When ->lock() does return asynchronously,
- * it must return FILE_LOCK_DEFERRED, and call ->lm_grant() when the lock
- * request completes.
+ * lm_grant is set. Additionally EXPORT_OP_ASYNC_LOCK in export_operations
+ * flags need to be set.
+ *
+ * Callers expecting ->lock() to return asynchronously will only use F_SETLK,
+ * not F_SETLKW; they will set FL_SLEEP if (and only if) the request is for a
+ * blocking lock. When ->lock() does return asynchronously, it must return
+ * FILE_LOCK_DEFERRED, and call ->lm_grant() when the lock request completes.
  * If the request is for non-blocking lock the file system should return
  * FILE_LOCK_DEFERRED then try to get the lock and call the callback routine
  * with the result. If the request timed out the callback routine will return a
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv2 nfsd/master 5/7] dlm: use fl_owner from lockd
  2023-09-12 21:53 [PATCHv2 nfsd/master 0/7] lockd: dlm: async lock request changes Alexander Aring
                   ` (3 preceding siblings ...)
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 4/7] lockd: add doc to enable EXPORT_OP_ASYNC_LOCK Alexander Aring
@ 2023-09-12 21:53 ` Alexander Aring
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 6/7] dlm: use FL_SLEEP to determine blocking vs non-blocking Alexander Aring
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 10+ messages in thread
From: Alexander Aring @ 2023-09-12 21:53 UTC (permalink / raw)
  To: linux-nfs
  Cc: gfs2, ocfs2-devel, linux-fsdevel, teigland, rpeterso, agruenba,
	trond.myklebust, anna, chuck.lever, jlayton, aahringo

This patch is changing the fl_owner value in case of an nfs lock request
to not be the pid of lockd. Instead this patch changes it to be the
owner value that nfs is giving us.

Currently there exists proved problems with this behaviour. One nfsd
server was created to export a gfs2 filesystem mount. Two nfs clients
doing a nfs mount of this export. Those two clients should conflict each
other operating on the same nfs file.

A small test program was written:

int main(int argc, const char *argv[])
{
	struct flock fl = {
		.l_type = F_WRLCK,
		.l_whence = SEEK_SET,
		.l_start = 1L,
		.l_len = 1L,
	};
	int fd;

	fd = open("filename", O_RDWR | O_CREAT, 0700);
	printf("try to lock...\n");
	fcntl(fd, F_SETLKW, &fl);
	printf("locked!\n");
	getc(stdin);

	return 0;
}

Running on both clients at the same time and don't interrupting by
pressing any key. It will show that both clients are able to acquire the
lock which shouldn't be the case. The issue is here that the fl_owner
value is the same and the lock context of both clients should be
separated.

This patch lets lockd define how to deal with lock contexts and chose
hopefully the right fl_owner value. A test after this patch was made and
the locks conflicts each other which should be the case.

Acked-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Alexander Aring <aahringo@redhat.com>
---
 fs/dlm/plock.c | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
index e6b4c1a21446..ee6e0236d4f8 100644
--- a/fs/dlm/plock.c
+++ b/fs/dlm/plock.c
@@ -145,6 +145,7 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file,
 	op->info.number		= number;
 	op->info.start		= fl->fl_start;
 	op->info.end		= fl->fl_end;
+	op->info.owner = (__u64)(long)fl->fl_owner;
 	/* async handling */
 	if (fl->fl_lmops && fl->fl_lmops->lm_grant) {
 		op_data = kzalloc(sizeof(*op_data), GFP_NOFS);
@@ -154,9 +155,6 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file,
 			goto out;
 		}
 
-		/* fl_owner is lockd which doesn't distinguish
-		   processes on the nfs client */
-		op->info.owner	= (__u64) fl->fl_pid;
 		op_data->callback = fl->fl_lmops->lm_grant;
 		locks_init_lock(&op_data->flc);
 		locks_copy_lock(&op_data->flc, fl);
@@ -168,8 +166,6 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file,
 		send_op(op);
 		rv = FILE_LOCK_DEFERRED;
 		goto out;
-	} else {
-		op->info.owner	= (__u64)(long) fl->fl_owner;
 	}
 
 	send_op(op);
@@ -326,10 +322,7 @@ int dlm_posix_unlock(dlm_lockspace_t *lockspace, u64 number, struct file *file,
 	op->info.number		= number;
 	op->info.start		= fl->fl_start;
 	op->info.end		= fl->fl_end;
-	if (fl->fl_lmops && fl->fl_lmops->lm_grant)
-		op->info.owner	= (__u64) fl->fl_pid;
-	else
-		op->info.owner	= (__u64)(long) fl->fl_owner;
+	op->info.owner = (__u64)(long)fl->fl_owner;
 
 	if (fl->fl_flags & FL_CLOSE) {
 		op->info.flags |= DLM_PLOCK_FL_CLOSE;
@@ -389,7 +382,7 @@ int dlm_posix_cancel(dlm_lockspace_t *lockspace, u64 number, struct file *file,
 	info.number = number;
 	info.start = fl->fl_start;
 	info.end = fl->fl_end;
-	info.owner = (__u64)fl->fl_pid;
+	info.owner = (__u64)(long)fl->fl_owner;
 
 	rv = do_lock_cancel(&info);
 	switch (rv) {
@@ -450,10 +443,7 @@ int dlm_posix_get(dlm_lockspace_t *lockspace, u64 number, struct file *file,
 	op->info.number		= number;
 	op->info.start		= fl->fl_start;
 	op->info.end		= fl->fl_end;
-	if (fl->fl_lmops && fl->fl_lmops->lm_grant)
-		op->info.owner	= (__u64) fl->fl_pid;
-	else
-		op->info.owner	= (__u64)(long) fl->fl_owner;
+	op->info.owner = (__u64)(long)fl->fl_owner;
 
 	send_op(op);
 	wait_event(recv_wq, (op->done != 0));
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv2 nfsd/master 6/7] dlm: use FL_SLEEP to determine blocking vs non-blocking
  2023-09-12 21:53 [PATCHv2 nfsd/master 0/7] lockd: dlm: async lock request changes Alexander Aring
                   ` (4 preceding siblings ...)
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 5/7] dlm: use fl_owner from lockd Alexander Aring
@ 2023-09-12 21:53 ` Alexander Aring
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 7/7] dlm: implement EXPORT_OP_ASYNC_LOCK Alexander Aring
  2023-09-13 13:24 ` [PATCHv2 nfsd/master 0/7] lockd: dlm: async lock request changes Chuck Lever III
  7 siblings, 0 replies; 10+ messages in thread
From: Alexander Aring @ 2023-09-12 21:53 UTC (permalink / raw)
  To: linux-nfs
  Cc: gfs2, ocfs2-devel, linux-fsdevel, teigland, rpeterso, agruenba,
	trond.myklebust, anna, chuck.lever, jlayton, aahringo

This patch uses the FL_SLEEP flag in struct file_lock to determine if
the lock request is a blocking or non-blocking request. Before dlm was
using IS_SETLKW() was being used which is not usable for lock requests
coming from lockd when EXPORT_OP_ASYNC_LOCK inside the export flags
is set.

Reviewed-by: Jeff Layton <jlayton@kernel.org>
Signed-off-by: Alexander Aring <aahringo@redhat.com>
---
 fs/dlm/plock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/dlm/plock.c b/fs/dlm/plock.c
index ee6e0236d4f8..d814c5121367 100644
--- a/fs/dlm/plock.c
+++ b/fs/dlm/plock.c
@@ -140,7 +140,7 @@ int dlm_posix_lock(dlm_lockspace_t *lockspace, u64 number, struct file *file,
 	op->info.optype		= DLM_PLOCK_OP_LOCK;
 	op->info.pid		= fl->fl_pid;
 	op->info.ex		= (fl->fl_type == F_WRLCK);
-	op->info.wait		= IS_SETLKW(cmd);
+	op->info.wait		= !!(fl->fl_flags & FL_SLEEP);
 	op->info.fsid		= ls->ls_global_id;
 	op->info.number		= number;
 	op->info.start		= fl->fl_start;
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCHv2 nfsd/master 7/7] dlm: implement EXPORT_OP_ASYNC_LOCK
  2023-09-12 21:53 [PATCHv2 nfsd/master 0/7] lockd: dlm: async lock request changes Alexander Aring
                   ` (5 preceding siblings ...)
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 6/7] dlm: use FL_SLEEP to determine blocking vs non-blocking Alexander Aring
@ 2023-09-12 21:53 ` Alexander Aring
  2023-09-14 10:29   ` Jeff Layton
  2023-09-13 13:24 ` [PATCHv2 nfsd/master 0/7] lockd: dlm: async lock request changes Chuck Lever III
  7 siblings, 1 reply; 10+ messages in thread
From: Alexander Aring @ 2023-09-12 21:53 UTC (permalink / raw)
  To: linux-nfs
  Cc: gfs2, ocfs2-devel, linux-fsdevel, teigland, rpeterso, agruenba,
	trond.myklebust, anna, chuck.lever, jlayton, aahringo

This patch is activating the EXPORT_OP_ASYNC_LOCK export flag to
signal lockd that both filesystems are able to handle async lock
requests. The cluster filesystems gfs2 and ocfs2 will redirect their
lock requests to DLMs plock implementation that can handle async lock
requests.

Signed-off-by: Alexander Aring <aahringo@redhat.com>
---
 fs/gfs2/export.c  | 1 +
 fs/ocfs2/export.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/fs/gfs2/export.c b/fs/gfs2/export.c
index cf40895233f5..ef1013eff936 100644
--- a/fs/gfs2/export.c
+++ b/fs/gfs2/export.c
@@ -192,5 +192,6 @@ 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 eaa8c80ace3c..b8b6a191b5cb 100644
--- a/fs/ocfs2/export.c
+++ b/fs/ocfs2/export.c
@@ -280,4 +280,5 @@ 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,
 };
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCHv2 nfsd/master 0/7] lockd: dlm: async lock request changes
  2023-09-12 21:53 [PATCHv2 nfsd/master 0/7] lockd: dlm: async lock request changes Alexander Aring
                   ` (6 preceding siblings ...)
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 7/7] dlm: implement EXPORT_OP_ASYNC_LOCK Alexander Aring
@ 2023-09-13 13:24 ` Chuck Lever III
  7 siblings, 0 replies; 10+ messages in thread
From: Chuck Lever III @ 2023-09-13 13:24 UTC (permalink / raw)
  To: Alexander Aring
  Cc: Linux NFS Mailing List, gfs2, ocfs2-devel, linux-fsdevel,
	teigland, rpeterso, agruenba, Trond Myklebust, Anna Schumaker,
	jlayton



> On Sep 12, 2023, at 5:53 PM, Alexander Aring <aahringo@redhat.com> wrote:
> 
> Hi,
> 
> I sent this as a PATCH now and drop the RFC. I got some review back from
> Jeff Layton and hope I was successful to follow it. There are still
> issues with lockd and asynchronous lock request but it will at least not
> directly crash when somebody is using nfs on top of GFS2. The issues are
> related to cancellation of locks/lockd decides to drop nlm_block for
> some reasons while pending request is happening.
> 
> I did not change more documentation about the asynchronous lock request
> functionality to not confuse users. In my opinion there should be more
> documentation about what you SHOULD NOT do with this API. Otherwise I
> think the documentation is still correct.
> 
> I will send a follow up patch to fsdevel to change all users using
> IS_SETLKW() to use FL_SLEEP to determine if it's blocking or
> non-blocking and send it to fsdevel as it was recommended. This will
> just be a grep and replace patch and look what happens.
> 
> - Alex
> 
> changes since v2:
> - remove B_PENDING_CALLBACK paragraph from commit msg. Was a leftover
>   and I forgot to update the commit message.
> - change function name from export_op_support_safe_async_lock()
>   to exportfs_lock_op_is_async()
> - change flag name from EXPORT_OP_SAFE_ASYNC_LOCK to
>   EXPORT_OP_ASYNC_LOCK
> - add documentation for EXPORT_OP_ASYNC_LOCK to
>   Documentation/filesystems/nfs/exporting.rst
> - add newline between function name and return type of
>   exportfs_lock_op_is_async()
> - remove f_op->lock() check and mention it in
>   Documentation/filesystems/nfs/exporting.rst to only use it with
>   filesystems implementing their own ->lock()
> - add kdoc for exportfs_lock_op_is_async()
> 
> changes since RFC:
> 
> - drop the pending callback flag but introduce "lockd: don't call
>  vfs_lock_file() for pending requests", see patch why I need it.
> - drop per nlm_block callback mutex
> - change async lock request documentation
> - use always FL_SLEEP to determine if it's blocking vs non-blocking in
>  DLM
> 
> Alexander Aring (7):
>  lockd: introduce safe async lock op
>  lockd: don't call vfs_lock_file() for pending requests
>  lockd: fix race in async lock request handling
>  lockd: add doc to enable EXPORT_OP_ASYNC_LOCK

I've applied these four to the nfsd/lockd tree for v6.7.

https://git.kernel.org/pub/scm/linux/kernel/git/cel/linux.git/


>  dlm: use fl_owner from lockd
>  dlm: use FL_SLEEP to determine blocking vs non-blocking
>  dlm: implement EXPORT_OP_ASYNC_LOCK
> 
> Documentation/filesystems/nfs/exporting.rst |  7 ++++
> fs/dlm/plock.c                              | 20 +++--------
> fs/gfs2/export.c                            |  1 +
> fs/lockd/svclock.c                          | 38 ++++++++++++++-------
> fs/locks.c                                  | 12 ++++---
> fs/nfsd/nfs4state.c                         | 10 ++++--
> fs/ocfs2/export.c                           |  1 +
> include/linux/exportfs.h                    | 14 ++++++++
> 8 files changed, 67 insertions(+), 36 deletions(-)
> 
> -- 
> 2.31.1
> 

--
Chuck Lever



^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCHv2 nfsd/master 7/7] dlm: implement EXPORT_OP_ASYNC_LOCK
  2023-09-12 21:53 ` [PATCHv2 nfsd/master 7/7] dlm: implement EXPORT_OP_ASYNC_LOCK Alexander Aring
@ 2023-09-14 10:29   ` Jeff Layton
  0 siblings, 0 replies; 10+ messages in thread
From: Jeff Layton @ 2023-09-14 10:29 UTC (permalink / raw)
  To: Alexander Aring, linux-nfs
  Cc: gfs2, ocfs2-devel, linux-fsdevel, teigland, rpeterso, agruenba,
	trond.myklebust, anna, chuck.lever

On Tue, 2023-09-12 at 17:53 -0400, Alexander Aring wrote:
> This patch is activating the EXPORT_OP_ASYNC_LOCK export flag to
> signal lockd that both filesystems are able to handle async lock
> requests. The cluster filesystems gfs2 and ocfs2 will redirect their
> lock requests to DLMs plock implementation that can handle async lock
> requests.
> 
> Signed-off-by: Alexander Aring <aahringo@redhat.com>
> ---
>  fs/gfs2/export.c  | 1 +
>  fs/ocfs2/export.c | 1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/fs/gfs2/export.c b/fs/gfs2/export.c
> index cf40895233f5..ef1013eff936 100644
> --- a/fs/gfs2/export.c
> +++ b/fs/gfs2/export.c
> @@ -192,5 +192,6 @@ 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 eaa8c80ace3c..b8b6a191b5cb 100644
> --- a/fs/ocfs2/export.c
> +++ b/fs/ocfs2/export.c
> @@ -280,4 +280,5 @@ 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,
>  };

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2023-09-14 10:29 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-09-12 21:53 [PATCHv2 nfsd/master 0/7] lockd: dlm: async lock request changes Alexander Aring
2023-09-12 21:53 ` [PATCHv2 nfsd/master 1/7] lockd: introduce safe async lock op Alexander Aring
2023-09-12 21:53 ` [PATCHv2 nfsd/master 2/7] lockd: don't call vfs_lock_file() for pending requests Alexander Aring
2023-09-12 21:53 ` [PATCHv2 nfsd/master 3/7] lockd: fix race in async lock request handling Alexander Aring
2023-09-12 21:53 ` [PATCHv2 nfsd/master 4/7] lockd: add doc to enable EXPORT_OP_ASYNC_LOCK Alexander Aring
2023-09-12 21:53 ` [PATCHv2 nfsd/master 5/7] dlm: use fl_owner from lockd Alexander Aring
2023-09-12 21:53 ` [PATCHv2 nfsd/master 6/7] dlm: use FL_SLEEP to determine blocking vs non-blocking Alexander Aring
2023-09-12 21:53 ` [PATCHv2 nfsd/master 7/7] dlm: implement EXPORT_OP_ASYNC_LOCK Alexander Aring
2023-09-14 10:29   ` Jeff Layton
2023-09-13 13:24 ` [PATCHv2 nfsd/master 0/7] lockd: dlm: async lock request changes Chuck Lever III

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).