On Thu, 2024-10-17 at 19:16 +0000, Chuck Lever III wrote: > > > On Oct 17, 2024, at 3:13 PM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > > > On Thu, 2024-10-17 at 09:36 -0400, cel@xxxxxxxxxx wrote: > > > From: Chuck Lever <chuck.lever@xxxxxxxxxx> > > > > > > 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@xxxxxxxxxx> > > > --- > > > 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@xxxxxxxxxx>