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

[Jeff Layton <jlayton@kernel.org>]

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>