[PATCH 0/5] Simple lockd clean-ups

[PATCH 0/5] Simple lockd clean-ups

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

Clean up a few nits I noticed while working on converting lockd to
use xdrgen-generated XDR functions.

Chuck Lever (5):
  lockd: Remove unused typedef
  lockd: Remove unnecessary memset()
  lockd: Remove some snippets of unfinished code
  lockd: Remove unused parameter to nlmsvc_testlock()
  lockd: Remove unneeded initialization of file_lock::c.flc_flags

 fs/lockd/svc4proc.c         | 20 +++++---------------
 fs/lockd/svclock.c          |  2 +-
 fs/lockd/svcproc.c          | 15 ++-------------
 fs/lockd/xdr4.c             |  2 --
 include/linux/lockd/lockd.h |  6 +++---
 include/linux/lockd/xdr.h   |  2 --
 6 files changed, 11 insertions(+), 36 deletions(-)

-- 
2.46.2

[PATCH 1/5] lockd: Remove unused typedef

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

Clean up: Looks like the last usage of this typedef was removed by
commit 026fec7e7c47 ("sunrpc: properly type pc_decode callbacks") in
2017.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 include/linux/lockd/xdr.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/linux/lockd/xdr.h b/include/linux/lockd/xdr.h
index 80cca9426761..17d53165d9f2 100644
--- a/include/linux/lockd/xdr.h
+++ b/include/linux/lockd/xdr.h
@@ -73,8 +73,6 @@ struct nlm_args {
 	u32			fsm_mode;
 };
 
-typedef struct nlm_args nlm_args;
-
 /*
  * Generic lockd result
  */
-- 
2.46.2

[PATCH 2/5] lockd: Remove unnecessary memset()

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

Since commit 103cc1fafee4 ("SUNRPC: Parametrize how much of argsize
should be zeroed") (and possibly long before that, even) all of the
memory underlying rqstp->rq_argp is zeroed already. There's no need
for the memset() in nlm4svc_decode_shareargs().

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/lockd/xdr4.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
index 3d28b9c3ed15..60466b8bac58 100644
--- a/fs/lockd/xdr4.c
+++ b/fs/lockd/xdr4.c
@@ -268,7 +268,6 @@ nlm4svc_decode_shareargs(struct svc_rqst *rqstp, struct xdr_stream *xdr)
 	struct nlm_args *argp = rqstp->rq_argp;
 	struct nlm_lock	*lock = &argp->lock;
 
-	memset(lock, 0, sizeof(*lock));
 	locks_init_lock(&lock->fl);
 	lock->svid = ~(u32)0;
 
-- 
2.46.2

[PATCH 3/5] lockd: Remove some snippets of unfinished code

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

Clean up.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/lockd/svc4proc.c | 12 ------------
 fs/lockd/svcproc.c  | 12 ------------
 2 files changed, 24 deletions(-)

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 8a72c418cdcc..1d0400d94b3d 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -142,18 +142,6 @@ __nlm4svc_proc_lock(struct svc_rqst *rqstp, struct nlm_res *resp)
 	if ((resp->status = nlm4svc_retrieve_args(rqstp, argp, &host, &file)))
 		return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success;
 
-#if 0
-	/* If supplied state doesn't match current state, we assume it's
-	 * an old request that time-warped somehow. Any error return would
-	 * do in this case because it's irrelevant anyway.
-	 *
-	 * NB: We don't retrieve the remote host's state yet.
-	 */
-	if (host->h_nsmstate && host->h_nsmstate != argp->state) {
-		resp->status = nlm_lck_denied_nolocks;
-	} else
-#endif
-
 	/* Now try to lock the file */
 	resp->status = nlmsvc_lock(rqstp, file, host, &argp->lock,
 					argp->block, &argp->cookie,
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index a03220e66ce0..d959a5dbe0ae 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -165,18 +165,6 @@ __nlmsvc_proc_lock(struct svc_rqst *rqstp, struct nlm_res *resp)
 	if ((resp->status = nlmsvc_retrieve_args(rqstp, argp, &host, &file)))
 		return resp->status == nlm_drop_reply ? rpc_drop_reply :rpc_success;
 
-#if 0
-	/* If supplied state doesn't match current state, we assume it's
-	 * an old request that time-warped somehow. Any error return would
-	 * do in this case because it's irrelevant anyway.
-	 *
-	 * NB: We don't retrieve the remote host's state yet.
-	 */
-	if (host->h_nsmstate && host->h_nsmstate != argp->state) {
-		resp->status = nlm_lck_denied_nolocks;
-	} else
-#endif
-
 	/* Now try to lock the file */
 	resp->status = cast_status(nlmsvc_lock(rqstp, file, host, &argp->lock,
 					       argp->block, &argp->cookie,
-- 
2.46.2

[PATCH 4/5] lockd: Remove unused parameter to nlmsvc_testlock()

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

This parameter has been unused since commit 09802fd2a8ca ("lockd:
rip out deferred lock handling from testlock codepath").

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/lockd/svc4proc.c         | 3 ++-
 fs/lockd/svclock.c          | 2 +-
 fs/lockd/svcproc.c          | 3 ++-
 include/linux/lockd/lockd.h | 6 +++---
 4 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 1d0400d94b3d..2cb603013111 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -108,7 +108,8 @@ __nlm4svc_proc_test(struct svc_rqst *rqstp, struct nlm_res *resp)
 
 	test_owner = argp->lock.fl.c.flc_owner;
 	/* Now check for conflicting locks */
-	resp->status = nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie);
+	resp->status = nlmsvc_testlock(rqstp, file, host, &argp->lock,
+				       &resp->lock);
 	if (resp->status == nlm_drop_reply)
 		rc = rpc_drop_reply;
 	else
diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
index 1f2149db10f2..33e1fd159934 100644
--- a/fs/lockd/svclock.c
+++ b/fs/lockd/svclock.c
@@ -609,7 +609,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
 __be32
 nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
 		struct nlm_host *host, struct nlm_lock *lock,
-		struct nlm_lock *conflock, struct nlm_cookie *cookie)
+		struct nlm_lock *conflock)
 {
 	int			error;
 	int			mode;
diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
index d959a5dbe0ae..f53d5177f267 100644
--- a/fs/lockd/svcproc.c
+++ b/fs/lockd/svcproc.c
@@ -130,7 +130,8 @@ __nlmsvc_proc_test(struct svc_rqst *rqstp, struct nlm_res *resp)
 	test_owner = argp->lock.fl.c.flc_owner;
 
 	/* Now check for conflicting locks */
-	resp->status = cast_status(nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie));
+	resp->status = cast_status(nlmsvc_testlock(rqstp, file, host,
+						   &argp->lock, &resp->lock));
 	if (resp->status == nlm_drop_reply)
 		rc = rpc_drop_reply;
 	else
diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
index 61c4b9c41904..c8f0f9458f2c 100644
--- a/include/linux/lockd/lockd.h
+++ b/include/linux/lockd/lockd.h
@@ -278,9 +278,9 @@ __be32		  nlmsvc_lock(struct svc_rqst *, struct nlm_file *,
 			      struct nlm_host *, struct nlm_lock *, int,
 			      struct nlm_cookie *, int);
 __be32		  nlmsvc_unlock(struct net *net, struct nlm_file *, struct nlm_lock *);
-__be32		  nlmsvc_testlock(struct svc_rqst *, struct nlm_file *,
-			struct nlm_host *, struct nlm_lock *,
-			struct nlm_lock *, struct nlm_cookie *);
+__be32		  nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
+			struct nlm_host *host, struct nlm_lock *lock,
+			struct nlm_lock *conflock);
 __be32		  nlmsvc_cancel_blocked(struct net *net, struct nlm_file *, struct nlm_lock *);
 void		  nlmsvc_retry_blocked(struct svc_rqst *rqstp);
 void		  nlmsvc_traverse_blocks(struct nlm_host *, struct nlm_file *,
-- 
2.46.2

Re: [PATCH 4/5] lockd: Remove unused parameter to nlmsvc_testlock()

[NeilBrown]

On Fri, 18 Oct 2024, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> This parameter has been unused since commit 09802fd2a8ca ("lockd:
> rip out deferred lock handling from testlock codepath").

It's a minor point, but the parameter is described as "unused" and
"this" but never as "cookie" or "nlm_cookie".  So I enter the code
section of the patch not being entirely sure what is being removed.

NeilBrown


> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/lockd/svc4proc.c         | 3 ++-
>  fs/lockd/svclock.c          | 2 +-
>  fs/lockd/svcproc.c          | 3 ++-
>  include/linux/lockd/lockd.h | 6 +++---
>  4 files changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index 1d0400d94b3d..2cb603013111 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -108,7 +108,8 @@ __nlm4svc_proc_test(struct svc_rqst *rqstp, struct nlm_res *resp)
>  
>  	test_owner = argp->lock.fl.c.flc_owner;
>  	/* Now check for conflicting locks */
> -	resp->status = nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie);
> +	resp->status = nlmsvc_testlock(rqstp, file, host, &argp->lock,
> +				       &resp->lock);
>  	if (resp->status == nlm_drop_reply)
>  		rc = rpc_drop_reply;
>  	else
> diff --git a/fs/lockd/svclock.c b/fs/lockd/svclock.c
> index 1f2149db10f2..33e1fd159934 100644
> --- a/fs/lockd/svclock.c
> +++ b/fs/lockd/svclock.c
> @@ -609,7 +609,7 @@ nlmsvc_lock(struct svc_rqst *rqstp, struct nlm_file *file,
>  __be32
>  nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
>  		struct nlm_host *host, struct nlm_lock *lock,
> -		struct nlm_lock *conflock, struct nlm_cookie *cookie)
> +		struct nlm_lock *conflock)
>  {
>  	int			error;
>  	int			mode;
> diff --git a/fs/lockd/svcproc.c b/fs/lockd/svcproc.c
> index d959a5dbe0ae..f53d5177f267 100644
> --- a/fs/lockd/svcproc.c
> +++ b/fs/lockd/svcproc.c
> @@ -130,7 +130,8 @@ __nlmsvc_proc_test(struct svc_rqst *rqstp, struct nlm_res *resp)
>  	test_owner = argp->lock.fl.c.flc_owner;
>  
>  	/* Now check for conflicting locks */
> -	resp->status = cast_status(nlmsvc_testlock(rqstp, file, host, &argp->lock, &resp->lock, &resp->cookie));
> +	resp->status = cast_status(nlmsvc_testlock(rqstp, file, host,
> +						   &argp->lock, &resp->lock));
>  	if (resp->status == nlm_drop_reply)
>  		rc = rpc_drop_reply;
>  	else
> diff --git a/include/linux/lockd/lockd.h b/include/linux/lockd/lockd.h
> index 61c4b9c41904..c8f0f9458f2c 100644
> --- a/include/linux/lockd/lockd.h
> +++ b/include/linux/lockd/lockd.h
> @@ -278,9 +278,9 @@ __be32		  nlmsvc_lock(struct svc_rqst *, struct nlm_file *,
>  			      struct nlm_host *, struct nlm_lock *, int,
>  			      struct nlm_cookie *, int);
>  __be32		  nlmsvc_unlock(struct net *net, struct nlm_file *, struct nlm_lock *);
> -__be32		  nlmsvc_testlock(struct svc_rqst *, struct nlm_file *,
> -			struct nlm_host *, struct nlm_lock *,
> -			struct nlm_lock *, struct nlm_cookie *);
> +__be32		  nlmsvc_testlock(struct svc_rqst *rqstp, struct nlm_file *file,
> +			struct nlm_host *host, struct nlm_lock *lock,
> +			struct nlm_lock *conflock);
>  __be32		  nlmsvc_cancel_blocked(struct net *net, struct nlm_file *, struct nlm_lock *);
>  void		  nlmsvc_retry_blocked(struct svc_rqst *rqstp);
>  void		  nlmsvc_traverse_blocks(struct nlm_host *, struct nlm_file *,
> -- 
> 2.46.2
> 
> 

[PATCH 5/5] lockd: Remove unneeded initialization of file_lock::c.flc_flags

[cel]

From: Chuck Lever <chuck.lever@oracle.com>

Since commit 75c7940d2a86 ("lockd: set missing fl_flags field when
retrieving args"), nlmsvc_retrieve_args() initializes the flc_flags
field. svcxdr_decode_lock() no longer needs to do this.

This clean up removes one dependency on the nlm_lock:fl field. No
behavior change is expected.

Analysis:

svcxdr_decode_lock() is called by:

nlm4svc_decode_testargs()
nlm4svc_decode_lockargs()
nlm4svc_decode_cancargs()
nlm4svc_decode_unlockargs()

nlm4svc_decode_testargs() is used by:
- NLMPROC4_TEST and NLMPROC4_TEST_MSG, which call nlmsvc_retrieve_args()
- NLMPROC4_GRANTED and NLMPROC4_GRANTED_MSG, which don't pass the
  lock's file_lock to the generic lock API

nlm4svc_decode_lockargs() is used by:
- NLMPROC4_LOCK and NLM4PROC4_LOCK_MSG, which call nlmsvc_retrieve_args()
- NLMPROC4_UNLOCK and NLM4PROC4_UNLOCK_MSG, which call nlmsvc_retrieve_args()
- NLMPROC4_NM_LOCK, which calls nlmsvc_retrieve_args()

nlm4svc_decode_cancargs() is used by:
- NLMPROC4_CANCEL and NLMPROC4_CANCEL_MSG, which call nlmsvc_retrieve_args()

nlm4svc_decode_unlockargs() is used by:
- NLMPROC4_UNLOCK and NLMPROC4_UNLOCK_MSG, which call nlmsvc_retrieve_args()

All callers except GRANTED/GRANTED_MSG eventually call
nlmsvc_retrieve_args() before using nlm_lock::fl.c.flc_flags. Thus
this change is safe.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 fs/lockd/svc4proc.c | 5 +++--
 fs/lockd/xdr4.c     | 1 -
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
index 2cb603013111..109e5caae8c7 100644
--- a/fs/lockd/svc4proc.c
+++ b/fs/lockd/svc4proc.c
@@ -46,14 +46,15 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
 	if (filp != NULL) {
 		int mode = lock_to_openmode(&lock->fl);
 
+		lock->fl.c.flc_flags = FL_POSIX;
+
 		error = nlm_lookup_file(rqstp, &file, lock);
 		if (error)
 			goto no_locks;
 		*filp = file;
 
 		/* Set up the missing parts of the file_lock structure */
-		lock->fl.c.flc_flags = FL_POSIX;
-		lock->fl.c.flc_file  = file->f_file[mode];
+		lock->fl.c.flc_file = file->f_file[mode];
 		lock->fl.c.flc_pid = current->tgid;
 		lock->fl.fl_start = (loff_t)lock->lock_start;
 		lock->fl.fl_end = lock->lock_len ?
diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
index 60466b8bac58..e343c820301f 100644
--- a/fs/lockd/xdr4.c
+++ b/fs/lockd/xdr4.c
@@ -89,7 +89,6 @@ svcxdr_decode_lock(struct xdr_stream *xdr, struct nlm_lock *lock)
 		return false;
 
 	locks_init_lock(fl);
-	fl->c.flc_flags = FL_POSIX;
 	fl->c.flc_type  = F_RDLCK;
 	nlm4svc_set_file_lock_range(fl, lock->lock_start, lock->lock_len);
 	return true;
-- 
2.46.2

Re: [PATCH 5/5] lockd: Remove unneeded initialization of file_lock::c.flc_flags

[Jeff Layton]

On Thu, 2024-10-17 at 09:36 -0400, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
> 
> Since commit 75c7940d2a86 ("lockd: set missing fl_flags field when
> retrieving args"), nlmsvc_retrieve_args() initializes the flc_flags
> field. svcxdr_decode_lock() no longer needs to do this.
> 
> This clean up removes one dependency on the nlm_lock:fl field. No
> behavior change is expected.
> 
> Analysis:
> 
> svcxdr_decode_lock() is called by:
> 
> nlm4svc_decode_testargs()
> nlm4svc_decode_lockargs()
> nlm4svc_decode_cancargs()
> nlm4svc_decode_unlockargs()
> 
> nlm4svc_decode_testargs() is used by:
> - NLMPROC4_TEST and NLMPROC4_TEST_MSG, which call nlmsvc_retrieve_args()
> - NLMPROC4_GRANTED and NLMPROC4_GRANTED_MSG, which don't pass the
>   lock's file_lock to the generic lock API
> 
> nlm4svc_decode_lockargs() is used by:
> - NLMPROC4_LOCK and NLM4PROC4_LOCK_MSG, which call nlmsvc_retrieve_args()
> - NLMPROC4_UNLOCK and NLM4PROC4_UNLOCK_MSG, which call nlmsvc_retrieve_args()
> - NLMPROC4_NM_LOCK, which calls nlmsvc_retrieve_args()
> 
> nlm4svc_decode_cancargs() is used by:
> - NLMPROC4_CANCEL and NLMPROC4_CANCEL_MSG, which call nlmsvc_retrieve_args()
> 
> nlm4svc_decode_unlockargs() is used by:
> - NLMPROC4_UNLOCK and NLMPROC4_UNLOCK_MSG, which call nlmsvc_retrieve_args()
> 
> All callers except GRANTED/GRANTED_MSG eventually call
> nlmsvc_retrieve_args() before using nlm_lock::fl.c.flc_flags. Thus
> this change is safe.
> 
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>  fs/lockd/svc4proc.c | 5 +++--
>  fs/lockd/xdr4.c     | 1 -
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> index 2cb603013111..109e5caae8c7 100644
> --- a/fs/lockd/svc4proc.c
> +++ b/fs/lockd/svc4proc.c
> @@ -46,14 +46,15 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
>  	if (filp != NULL) {
>  		int mode = lock_to_openmode(&lock->fl);
>  
> +		lock->fl.c.flc_flags = FL_POSIX;
> +
>  		error = nlm_lookup_file(rqstp, &file, lock);
>  		if (error)
>  			goto no_locks;
>  		*filp = file;
>  
>  		/* Set up the missing parts of the file_lock structure */
> -		lock->fl.c.flc_flags = FL_POSIX;
> -		lock->fl.c.flc_file  = file->f_file[mode];
> +		lock->fl.c.flc_file = file->f_file[mode];
>  		lock->fl.c.flc_pid = current->tgid;
>  		lock->fl.fl_start = (loff_t)lock->lock_start;
>  		lock->fl.fl_end = lock->lock_len ?
> diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
> index 60466b8bac58..e343c820301f 100644
> --- a/fs/lockd/xdr4.c
> +++ b/fs/lockd/xdr4.c
> @@ -89,7 +89,6 @@ svcxdr_decode_lock(struct xdr_stream *xdr, struct nlm_lock *lock)
>  		return false;
>  
>  	locks_init_lock(fl);
> -	fl->c.flc_flags = FL_POSIX;
>  	fl->c.flc_type  = F_RDLCK;
>  	nlm4svc_set_file_lock_range(fl, lock->lock_start, lock->lock_len);
>  	return true;

1-4 look fine. You can add my R-b to those.

For this one, I think I'd rather see this go the other way, and just
eliminate the setting of flc_flags in nlm4svc_retrieve_args. We only
deal with FL_POSIX locks in svc lockd, and that does it right after
locks_init_lock, so I think that means it'll be done earlier, no?

Also, I think the same duplication is in nlmsvc_retrieve_args and the
nlmv3 version of svcxdr_decode_lock.
--
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 5/5] lockd: Remove unneeded initialization of file_lock::c.flc_flags

[Chuck Lever III]

> On Oct 17, 2024, at 3:13 PM, Jeff Layton <jlayton@kernel.org> wrote:
> 
> On Thu, 2024-10-17 at 09:36 -0400, cel@kernel.org wrote:
>> From: Chuck Lever <chuck.lever@oracle.com>
>> 
>> Since commit 75c7940d2a86 ("lockd: set missing fl_flags field when
>> retrieving args"), nlmsvc_retrieve_args() initializes the flc_flags
>> field. svcxdr_decode_lock() no longer needs to do this.
>> 
>> This clean up removes one dependency on the nlm_lock:fl field. No
>> behavior change is expected.
>> 
>> Analysis:
>> 
>> svcxdr_decode_lock() is called by:
>> 
>> nlm4svc_decode_testargs()
>> nlm4svc_decode_lockargs()
>> nlm4svc_decode_cancargs()
>> nlm4svc_decode_unlockargs()
>> 
>> nlm4svc_decode_testargs() is used by:
>> - NLMPROC4_TEST and NLMPROC4_TEST_MSG, which call nlmsvc_retrieve_args()
>> - NLMPROC4_GRANTED and NLMPROC4_GRANTED_MSG, which don't pass the
>>  lock's file_lock to the generic lock API
>> 
>> nlm4svc_decode_lockargs() is used by:
>> - NLMPROC4_LOCK and NLM4PROC4_LOCK_MSG, which call nlmsvc_retrieve_args()
>> - NLMPROC4_UNLOCK and NLM4PROC4_UNLOCK_MSG, which call nlmsvc_retrieve_args()
>> - NLMPROC4_NM_LOCK, which calls nlmsvc_retrieve_args()
>> 
>> nlm4svc_decode_cancargs() is used by:
>> - NLMPROC4_CANCEL and NLMPROC4_CANCEL_MSG, which call nlmsvc_retrieve_args()
>> 
>> nlm4svc_decode_unlockargs() is used by:
>> - NLMPROC4_UNLOCK and NLMPROC4_UNLOCK_MSG, which call nlmsvc_retrieve_args()
>> 
>> All callers except GRANTED/GRANTED_MSG eventually call
>> nlmsvc_retrieve_args() before using nlm_lock::fl.c.flc_flags. Thus
>> this change is safe.
>> 
>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>> ---
>> fs/lockd/svc4proc.c | 5 +++--
>> fs/lockd/xdr4.c     | 1 -
>> 2 files changed, 3 insertions(+), 3 deletions(-)
>> 
>> diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
>> index 2cb603013111..109e5caae8c7 100644
>> --- a/fs/lockd/svc4proc.c
>> +++ b/fs/lockd/svc4proc.c
>> @@ -46,14 +46,15 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
>> if (filp != NULL) {
>> int mode = lock_to_openmode(&lock->fl);
>> 
>> + lock->fl.c.flc_flags = FL_POSIX;
>> +
>> error = nlm_lookup_file(rqstp, &file, lock);
>> if (error)
>> goto no_locks;
>> *filp = file;
>> 
>> /* Set up the missing parts of the file_lock structure */
>> - lock->fl.c.flc_flags = FL_POSIX;
>> - lock->fl.c.flc_file  = file->f_file[mode];
>> + lock->fl.c.flc_file = file->f_file[mode];
>> lock->fl.c.flc_pid = current->tgid;
>> lock->fl.fl_start = (loff_t)lock->lock_start;
>> lock->fl.fl_end = lock->lock_len ?
>> diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
>> index 60466b8bac58..e343c820301f 100644
>> --- a/fs/lockd/xdr4.c
>> +++ b/fs/lockd/xdr4.c
>> @@ -89,7 +89,6 @@ svcxdr_decode_lock(struct xdr_stream *xdr, struct nlm_lock *lock)
>> return false;
>> 
>> locks_init_lock(fl);
>> - fl->c.flc_flags = FL_POSIX;
>> fl->c.flc_type  = F_RDLCK;
>> nlm4svc_set_file_lock_range(fl, lock->lock_start, lock->lock_len);
>> return true;
> 
> 1-4 look fine. You can add my R-b to those.

Thanks!


> For this one, I think I'd rather see this go the other way, and just
> eliminate the setting of flc_flags in nlm4svc_retrieve_args. We only
> deal with FL_POSIX locks in svc lockd, and that does it right after
> locks_init_lock, so I think that means it'll be done earlier, no?

Have a look at the nlm4 branch in my kernel.org <http://kernel.org/> repo to see where
this is headed.


> Also, I think the same duplication is in nlmsvc_retrieve_args and the
> nlmv3 version of svcxdr_decode_lock.

Which is going away when NFSv2 is removed. I'm not too concerned
about that duplication.


--
Chuck Lever

Re: [PATCH 5/5] lockd: Remove unneeded initialization of file_lock::c.flc_flags

[Jeff Layton]

On Thu, 2024-10-17 at 19:16 +0000, Chuck Lever III wrote:
> 
> > On Oct 17, 2024, at 3:13 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > 
> > On Thu, 2024-10-17 at 09:36 -0400, cel@kernel.org wrote:
> > > From: Chuck Lever <chuck.lever@oracle.com>
> > > 
> > > Since commit 75c7940d2a86 ("lockd: set missing fl_flags field when
> > > retrieving args"), nlmsvc_retrieve_args() initializes the flc_flags
> > > field. svcxdr_decode_lock() no longer needs to do this.
> > > 
> > > This clean up removes one dependency on the nlm_lock:fl field. No
> > > behavior change is expected.
> > > 
> > > Analysis:
> > > 
> > > svcxdr_decode_lock() is called by:
> > > 
> > > nlm4svc_decode_testargs()
> > > nlm4svc_decode_lockargs()
> > > nlm4svc_decode_cancargs()
> > > nlm4svc_decode_unlockargs()
> > > 
> > > nlm4svc_decode_testargs() is used by:
> > > - NLMPROC4_TEST and NLMPROC4_TEST_MSG, which call nlmsvc_retrieve_args()
> > > - NLMPROC4_GRANTED and NLMPROC4_GRANTED_MSG, which don't pass the
> > >  lock's file_lock to the generic lock API
> > > 
> > > nlm4svc_decode_lockargs() is used by:
> > > - NLMPROC4_LOCK and NLM4PROC4_LOCK_MSG, which call nlmsvc_retrieve_args()
> > > - NLMPROC4_UNLOCK and NLM4PROC4_UNLOCK_MSG, which call nlmsvc_retrieve_args()
> > > - NLMPROC4_NM_LOCK, which calls nlmsvc_retrieve_args()
> > > 
> > > nlm4svc_decode_cancargs() is used by:
> > > - NLMPROC4_CANCEL and NLMPROC4_CANCEL_MSG, which call nlmsvc_retrieve_args()
> > > 
> > > nlm4svc_decode_unlockargs() is used by:
> > > - NLMPROC4_UNLOCK and NLMPROC4_UNLOCK_MSG, which call nlmsvc_retrieve_args()
> > > 
> > > All callers except GRANTED/GRANTED_MSG eventually call
> > > nlmsvc_retrieve_args() before using nlm_lock::fl.c.flc_flags. Thus
> > > this change is safe.
> > > 
> > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > ---
> > > fs/lockd/svc4proc.c | 5 +++--
> > > fs/lockd/xdr4.c     | 1 -
> > > 2 files changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> > > index 2cb603013111..109e5caae8c7 100644
> > > --- a/fs/lockd/svc4proc.c
> > > +++ b/fs/lockd/svc4proc.c
> > > @@ -46,14 +46,15 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> > > if (filp != NULL) {
> > > int mode = lock_to_openmode(&lock->fl);
> > > 
> > > + lock->fl.c.flc_flags = FL_POSIX;
> > > +
> > > error = nlm_lookup_file(rqstp, &file, lock);
> > > if (error)
> > > goto no_locks;
> > > *filp = file;
> > > 
> > > /* Set up the missing parts of the file_lock structure */
> > > - lock->fl.c.flc_flags = FL_POSIX;
> > > - lock->fl.c.flc_file  = file->f_file[mode];
> > > + lock->fl.c.flc_file = file->f_file[mode];
> > > lock->fl.c.flc_pid = current->tgid;
> > > lock->fl.fl_start = (loff_t)lock->lock_start;
> > > lock->fl.fl_end = lock->lock_len ?
> > > diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
> > > index 60466b8bac58..e343c820301f 100644
> > > --- a/fs/lockd/xdr4.c
> > > +++ b/fs/lockd/xdr4.c
> > > @@ -89,7 +89,6 @@ svcxdr_decode_lock(struct xdr_stream *xdr, struct nlm_lock *lock)
> > > return false;
> > > 
> > > locks_init_lock(fl);
> > > - fl->c.flc_flags = FL_POSIX;
> > > fl->c.flc_type  = F_RDLCK;
> > > nlm4svc_set_file_lock_range(fl, lock->lock_start, lock->lock_len);
> > > return true;
> > 
> > 1-4 look fine. You can add my R-b to those.
> 
> Thanks!
> 
> 
> > For this one, I think I'd rather see this go the other way, and just
> > eliminate the setting of flc_flags in nlm4svc_retrieve_args. We only
> > deal with FL_POSIX locks in svc lockd, and that does it right after
> > locks_init_lock, so I think that means it'll be done earlier, no?
> 
> Have a look at the nlm4 branch in my kernel.org <http://kernel.org/> repo to see where
> this is headed.
> 

(For everyone following along: It's actually in Chuck's xdrgen branch)

Oh ok, I see. This is an interim step toward moving all of the lock
initialization into nlm4svc_retrieve_args(). That probably is better. I
withdraw my objection.

> 
> > Also, I think the same duplication is in nlmsvc_retrieve_args and the
> > nlmv3 version of svcxdr_decode_lock.
> 
> Which is going away when NFSv2 is removed. I'm not too concerned
> about that duplication.
> 

Fair enough. I'm fine with leaving that to wither for now:

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

Re: [PATCH 5/5] lockd: Remove unneeded initialization of file_lock::c.flc_flags

[NeilBrown]

On Fri, 18 Oct 2024, Jeff Layton wrote:
> On Thu, 2024-10-17 at 19:16 +0000, Chuck Lever III wrote:
> > 
> > > On Oct 17, 2024, at 3:13 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > > 
> > > On Thu, 2024-10-17 at 09:36 -0400, cel@kernel.org wrote:
> > > > From: Chuck Lever <chuck.lever@oracle.com>
> > > > 
> > > > Since commit 75c7940d2a86 ("lockd: set missing fl_flags field when
> > > > retrieving args"), nlmsvc_retrieve_args() initializes the flc_flags
> > > > field. svcxdr_decode_lock() no longer needs to do this.
> > > > 
> > > > This clean up removes one dependency on the nlm_lock:fl field. No
> > > > behavior change is expected.
> > > > 
> > > > Analysis:
> > > > 
> > > > svcxdr_decode_lock() is called by:
> > > > 
> > > > nlm4svc_decode_testargs()
> > > > nlm4svc_decode_lockargs()
> > > > nlm4svc_decode_cancargs()
> > > > nlm4svc_decode_unlockargs()
> > > > 
> > > > nlm4svc_decode_testargs() is used by:
> > > > - NLMPROC4_TEST and NLMPROC4_TEST_MSG, which call nlmsvc_retrieve_args()
> > > > - NLMPROC4_GRANTED and NLMPROC4_GRANTED_MSG, which don't pass the
> > > >  lock's file_lock to the generic lock API
> > > > 
> > > > nlm4svc_decode_lockargs() is used by:
> > > > - NLMPROC4_LOCK and NLM4PROC4_LOCK_MSG, which call nlmsvc_retrieve_args()
> > > > - NLMPROC4_UNLOCK and NLM4PROC4_UNLOCK_MSG, which call nlmsvc_retrieve_args()
> > > > - NLMPROC4_NM_LOCK, which calls nlmsvc_retrieve_args()
> > > > 
> > > > nlm4svc_decode_cancargs() is used by:
> > > > - NLMPROC4_CANCEL and NLMPROC4_CANCEL_MSG, which call nlmsvc_retrieve_args()
> > > > 
> > > > nlm4svc_decode_unlockargs() is used by:
> > > > - NLMPROC4_UNLOCK and NLMPROC4_UNLOCK_MSG, which call nlmsvc_retrieve_args()
> > > > 
> > > > All callers except GRANTED/GRANTED_MSG eventually call
> > > > nlmsvc_retrieve_args() before using nlm_lock::fl.c.flc_flags. Thus
> > > > this change is safe.
> > > > 
> > > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > > ---
> > > > fs/lockd/svc4proc.c | 5 +++--
> > > > fs/lockd/xdr4.c     | 1 -
> > > > 2 files changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> > > > index 2cb603013111..109e5caae8c7 100644
> > > > --- a/fs/lockd/svc4proc.c
> > > > +++ b/fs/lockd/svc4proc.c
> > > > @@ -46,14 +46,15 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> > > > if (filp != NULL) {
> > > > int mode = lock_to_openmode(&lock->fl);
> > > > 
> > > > + lock->fl.c.flc_flags = FL_POSIX;
> > > > +
> > > > error = nlm_lookup_file(rqstp, &file, lock);
> > > > if (error)
> > > > goto no_locks;
> > > > *filp = file;
> > > > 
> > > > /* Set up the missing parts of the file_lock structure */
> > > > - lock->fl.c.flc_flags = FL_POSIX;
> > > > - lock->fl.c.flc_file  = file->f_file[mode];
> > > > + lock->fl.c.flc_file = file->f_file[mode];
> > > > lock->fl.c.flc_pid = current->tgid;
> > > > lock->fl.fl_start = (loff_t)lock->lock_start;
> > > > lock->fl.fl_end = lock->lock_len ?
> > > > diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
> > > > index 60466b8bac58..e343c820301f 100644
> > > > --- a/fs/lockd/xdr4.c
> > > > +++ b/fs/lockd/xdr4.c
> > > > @@ -89,7 +89,6 @@ svcxdr_decode_lock(struct xdr_stream *xdr, struct nlm_lock *lock)
> > > > return false;
> > > > 
> > > > locks_init_lock(fl);
> > > > - fl->c.flc_flags = FL_POSIX;
> > > > fl->c.flc_type  = F_RDLCK;
> > > > nlm4svc_set_file_lock_range(fl, lock->lock_start, lock->lock_len);
> > > > return true;
> > > 
> > > 1-4 look fine. You can add my R-b to those.
> > 
> > Thanks!
> > 
> > 
> > > For this one, I think I'd rather see this go the other way, and just
> > > eliminate the setting of flc_flags in nlm4svc_retrieve_args. We only
> > > deal with FL_POSIX locks in svc lockd, and that does it right after
> > > locks_init_lock, so I think that means it'll be done earlier, no?
> > 
> > Have a look at the nlm4 branch in my kernel.org <http://kernel.org/> repo to see where
> > this is headed.
> > 
> 
> (For everyone following along: It's actually in Chuck's xdrgen branch)
> 
> Oh ok, I see. This is an interim step toward moving all of the lock
> initialization into nlm4svc_retrieve_args(). That probably is better. I
> withdraw my objection.

Adding that observation to the commit message would help with review.
I agree that moving the initialisation to nlm4svc_retrieve_args() seems
sensible.  The half-way version in this patch looks really odd without
that explanation.

But the whole series
 Reviewed-by: NeilBrown <neilb@suse.de>

Thanks,
NeilBrown



> 
> > 
> > > Also, I think the same duplication is in nlmsvc_retrieve_args and the
> > > nlmv3 version of svcxdr_decode_lock.
> > 
> > Which is going away when NFSv2 is removed. I'm not too concerned
> > about that duplication.
> > 
> 
> Fair enough. I'm fine with leaving that to wither for now:
> 
> Reviewed-by: Jeff Layton <jlayton@kernel.org>
> 

Re: [PATCH 5/5] lockd: Remove unneeded initialization of file_lock::c.flc_flags

[Chuck Lever]

On Fri, Oct 18, 2024 at 08:22:38AM +1100, NeilBrown wrote:
> On Fri, 18 Oct 2024, Jeff Layton wrote:
> > On Thu, 2024-10-17 at 19:16 +0000, Chuck Lever III wrote:
> > > 
> > > > On Oct 17, 2024, at 3:13 PM, Jeff Layton <jlayton@kernel.org> wrote:
> > > > 
> > > > On Thu, 2024-10-17 at 09:36 -0400, cel@kernel.org wrote:
> > > > > From: Chuck Lever <chuck.lever@oracle.com>
> > > > > 
> > > > > Since commit 75c7940d2a86 ("lockd: set missing fl_flags field when
> > > > > retrieving args"), nlmsvc_retrieve_args() initializes the flc_flags
> > > > > field. svcxdr_decode_lock() no longer needs to do this.
> > > > > 
> > > > > This clean up removes one dependency on the nlm_lock:fl field. No
> > > > > behavior change is expected.
> > > > > 
> > > > > Analysis:
> > > > > 
> > > > > svcxdr_decode_lock() is called by:
> > > > > 
> > > > > nlm4svc_decode_testargs()
> > > > > nlm4svc_decode_lockargs()
> > > > > nlm4svc_decode_cancargs()
> > > > > nlm4svc_decode_unlockargs()
> > > > > 
> > > > > nlm4svc_decode_testargs() is used by:
> > > > > - NLMPROC4_TEST and NLMPROC4_TEST_MSG, which call nlmsvc_retrieve_args()
> > > > > - NLMPROC4_GRANTED and NLMPROC4_GRANTED_MSG, which don't pass the
> > > > >  lock's file_lock to the generic lock API
> > > > > 
> > > > > nlm4svc_decode_lockargs() is used by:
> > > > > - NLMPROC4_LOCK and NLM4PROC4_LOCK_MSG, which call nlmsvc_retrieve_args()
> > > > > - NLMPROC4_UNLOCK and NLM4PROC4_UNLOCK_MSG, which call nlmsvc_retrieve_args()
> > > > > - NLMPROC4_NM_LOCK, which calls nlmsvc_retrieve_args()
> > > > > 
> > > > > nlm4svc_decode_cancargs() is used by:
> > > > > - NLMPROC4_CANCEL and NLMPROC4_CANCEL_MSG, which call nlmsvc_retrieve_args()
> > > > > 
> > > > > nlm4svc_decode_unlockargs() is used by:
> > > > > - NLMPROC4_UNLOCK and NLMPROC4_UNLOCK_MSG, which call nlmsvc_retrieve_args()
> > > > > 
> > > > > All callers except GRANTED/GRANTED_MSG eventually call
> > > > > nlmsvc_retrieve_args() before using nlm_lock::fl.c.flc_flags. Thus
> > > > > this change is safe.
> > > > > 
> > > > > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > > > > ---
> > > > > fs/lockd/svc4proc.c | 5 +++--
> > > > > fs/lockd/xdr4.c     | 1 -
> > > > > 2 files changed, 3 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/fs/lockd/svc4proc.c b/fs/lockd/svc4proc.c
> > > > > index 2cb603013111..109e5caae8c7 100644
> > > > > --- a/fs/lockd/svc4proc.c
> > > > > +++ b/fs/lockd/svc4proc.c
> > > > > @@ -46,14 +46,15 @@ nlm4svc_retrieve_args(struct svc_rqst *rqstp, struct nlm_args *argp,
> > > > > if (filp != NULL) {
> > > > > int mode = lock_to_openmode(&lock->fl);
> > > > > 
> > > > > + lock->fl.c.flc_flags = FL_POSIX;
> > > > > +
> > > > > error = nlm_lookup_file(rqstp, &file, lock);
> > > > > if (error)
> > > > > goto no_locks;
> > > > > *filp = file;
> > > > > 
> > > > > /* Set up the missing parts of the file_lock structure */
> > > > > - lock->fl.c.flc_flags = FL_POSIX;
> > > > > - lock->fl.c.flc_file  = file->f_file[mode];
> > > > > + lock->fl.c.flc_file = file->f_file[mode];
> > > > > lock->fl.c.flc_pid = current->tgid;
> > > > > lock->fl.fl_start = (loff_t)lock->lock_start;
> > > > > lock->fl.fl_end = lock->lock_len ?
> > > > > diff --git a/fs/lockd/xdr4.c b/fs/lockd/xdr4.c
> > > > > index 60466b8bac58..e343c820301f 100644
> > > > > --- a/fs/lockd/xdr4.c
> > > > > +++ b/fs/lockd/xdr4.c
> > > > > @@ -89,7 +89,6 @@ svcxdr_decode_lock(struct xdr_stream *xdr, struct nlm_lock *lock)
> > > > > return false;
> > > > > 
> > > > > locks_init_lock(fl);
> > > > > - fl->c.flc_flags = FL_POSIX;
> > > > > fl->c.flc_type  = F_RDLCK;
> > > > > nlm4svc_set_file_lock_range(fl, lock->lock_start, lock->lock_len);
> > > > > return true;
> > > > 
> > > > 1-4 look fine. You can add my R-b to those.
> > > 
> > > Thanks!
> > > 
> > > 
> > > > For this one, I think I'd rather see this go the other way, and just
> > > > eliminate the setting of flc_flags in nlm4svc_retrieve_args. We only
> > > > deal with FL_POSIX locks in svc lockd, and that does it right after
> > > > locks_init_lock, so I think that means it'll be done earlier, no?
> > > 
> > > Have a look at the nlm4 branch in my kernel.org <http://kernel.org/> repo to see where
> > > this is headed.
> > > 
> > 
> > (For everyone following along: It's actually in Chuck's xdrgen branch)
> > 
> > Oh ok, I see. This is an interim step toward moving all of the lock
> > initialization into nlm4svc_retrieve_args(). That probably is better. I
> > withdraw my objection.
> 
> Adding that observation to the commit message would help with review.

I was hoping to break these up to make them easier to review, but
for 5/5 I guess it didn't help.

I can send more of these clean-up/refactors along for v6.13. But I'm
not sure the actual conversion later in the xdrgen branch is ready
for prime time.


> I agree that moving the initialisation to nlm4svc_retrieve_args() seems
> sensible.  The half-way version in this patch looks really odd without
> that explanation.
> 
> But the whole series
>  Reviewed-by: NeilBrown <neilb@suse.de>
> 
> Thanks,
> NeilBrown
> 
> 
> 
> > 
> > > 
> > > > Also, I think the same duplication is in nlmsvc_retrieve_args and the
> > > > nlmv3 version of svcxdr_decode_lock.
> > > 
> > > Which is going away when NFSv2 is removed. I'm not too concerned
> > > about that duplication.
> > > 
> > 
> > Fair enough. I'm fine with leaving that to wither for now:
> > 
> > Reviewed-by: Jeff Layton <jlayton@kernel.org>
> > 
> 

-- 
Chuck Lever

Re: [PATCH 5/5] lockd: Remove unneeded initialization of file_lock::c.flc_flags

[NeilBrown]

On Fri, 18 Oct 2024, Chuck Lever III wrote:
> 
> Which is going away when NFSv2 is removed. I'm not too concerned
> about that duplication.
> 

We have a customer with industrial hardware which stores data (logs?)
via NFSv2.  So we might need to continue to support v2 indefinitely.

Possibly we could use a user-space-nfsd solution if/when v2 service is
removed from the kernel, but I would rather not.

Reverting the nfs-utils patches which remove v2 support is fairly easy. 
Reverting kernel patches that remove v2 support wouldn't be so easy in
the long term.

So I'd vote for not removing NFSv2 from the server.

Thanks,
NeilBrown