| Message ID | 20241017133631.213274-6-cel@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Simple lockd clean-ups | expand |
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>
> 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
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>
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
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> >
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> > > >
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;