On Fri, Oct 18, 2024 at 08:22:38AM +1100, NeilBrown wrote:

On Fri, 18 Oct 2024, Jeff Layton wrote:

quoted

On Thu, 2024-10-17 at 19:16 +0000, Chuck Lever III wrote:

quoted

quoted

On Oct 17, 2024, at 3:13 PM, Jeff Layton [off-list ref] wrote:

On Thu, 2024-10-17 at 09:36 -0400, cel@kernel.org wrote:

quoted

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!

quoted

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 [off-list ref]

Thanks,
NeilBrown

quoted

quoted

quoted

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