[permalink] [raw]

Subject: Re: [PATCH v2 00/41] filelock: split struct file_lock into file_lock and file_lease structs

On Thu, 2024-01-25 at 09:57 -0500, Chuck Lever wrote:
> On Thu, Jan 25, 2024 at 05:42:41AM -0500, Jeff Layton wrote:
> > Long ago, file locks used to hang off of a singly-linked list in struct
> > inode. Because of this, when leases were added, they were added to the
> > same list and so they had to be tracked using the same sort of
> > structure.
> >
> > Several years ago, we added struct file_lock_context, which allowed us
> > to use separate lists to track different types of file locks. Given
> > that, leases no longer need to be tracked using struct file_lock.
> >
> > That said, a lot of the underlying infrastructure _is_ the same between
> > file leases and locks, so we can't completely separate everything.
> >
> > This patchset first splits a group of fields used by both file locks and
> > leases into a new struct file_lock_core, that is then embedded in struct
> > file_lock. Coccinelle was then used to convert a lot of the callers to
> > deal with the move, with the remaining 25% or so converted by hand.
> >
> > It then converts several internal functions in fs/locks.c to work
> > with struct file_lock_core. Lastly, struct file_lock is split into
> > struct file_lock and file_lease, and the lease-related APIs converted to
> > take struct file_lease.
> >
> > After the first few patches (which I left split up for easier review),
> > the set should be bisectable. I'll plan to squash the first few
> > together to make sure the resulting set is bisectable before merge.
> >
> > Finally, I left the coccinelle scripts I used in tree. I had heard it
> > was preferable to merge those along with the patches that they
> > generate, but I wasn't sure where they go. I can either move those to a
> > more appropriate location or we can just drop that commit if it's not
> > needed.
> >
> > Signed-off-by: Jeff Layton <[email protected]>
>
> v2 looks nicer.
>
> I would add a few list handling primitives, as I see enough
> instances of list_for_each_entry, list_for_each_entry_safe,
> list_first_entry, and list_first_entry_or_null on fl_core.flc_list
> to make it worth having those.
>
> Also, there doesn't seem to be benefit for API consumers to have to
> understand the internal structure of struct file_lock/lease to reach
> into fl_core. Having accessor functions for common fields like
> fl_type and fl_flags could be cleaner.
>

That is a good suggestion. I had considered it before and figured "why
bother", but I think that would make things simpler.

I'll plan to do a v3 that has more helpers. Possibly we can just convert
some of the subsystems ahead of time and avoid some churn. Stay tuned...

> For the series:
>
> Reviewed-by: Chuck Lever <[email protected]>
>
> For the nfsd and lockd parts:
>
> Acked-by: Chuck Lever <[email protected]>
>
>
> > ---
> > Changes in v2:
> > - renamed file_lock_core fields to have "flc_" prefix
> > - used macros to more easily do the change piecemeal
> > - broke up patches into per-subsystem ones
> > - Link to v1: https://lore.kernel.org/r/[email protected]
> >
> > ---
> > Jeff Layton (41):
> > filelock: rename some fields in tracepoints
> > filelock: rename fl_pid variable in lock_get_status
> > dlm: rename fl_flags variable in dlm_posix_unlock
> > nfs: rename fl_flags variable in nfs4_proc_unlck
> > nfsd: rename fl_type and fl_flags variables in nfsd4_lock
> > lockd: rename fl_flags and fl_type variables in nlmclnt_lock
> > 9p: rename fl_type variable in v9fs_file_do_lock
> > afs: rename fl_type variable in afs_next_locker
> > filelock: drop the IS_* macros
> > filelock: split common fields into struct file_lock_core
> > filelock: add coccinelle scripts to move fields to struct file_lock_core
> > filelock: have fs/locks.c deal with file_lock_core directly
> > filelock: convert some internal functions to use file_lock_core instead
> > filelock: convert more internal functions to use file_lock_core
> > filelock: make posix_same_owner take file_lock_core pointers
> > filelock: convert posix_owner_key to take file_lock_core arg
> > filelock: make locks_{insert,delete}_global_locks take file_lock_core arg
> > filelock: convert locks_{insert,delete}_global_blocked
> > filelock: make __locks_delete_block and __locks_wake_up_blocks take file_lock_core
> > filelock: convert __locks_insert_block, conflict and deadlock checks to use file_lock_core
> > filelock: convert fl_blocker to file_lock_core
> > filelock: clean up locks_delete_block internals
> > filelock: reorganize locks_delete_block and __locks_insert_block
> > filelock: make assign_type helper take a file_lock_core pointer
> > filelock: convert locks_wake_up_blocks to take a file_lock_core pointer
> > filelock: convert locks_insert_lock_ctx and locks_delete_lock_ctx
> > filelock: convert locks_translate_pid to take file_lock_core
> > filelock: convert seqfile handling to use file_lock_core
> > 9p: adapt to breakup of struct file_lock
> > afs: adapt to breakup of struct file_lock
> > ceph: adapt to breakup of struct file_lock
> > dlm: adapt to breakup of struct file_lock
> > gfs2: adapt to breakup of struct file_lock
> > lockd: adapt to breakup of struct file_lock
> > nfs: adapt to breakup of struct file_lock
> > nfsd: adapt to breakup of struct file_lock
> > ocfs2: adapt to breakup of struct file_lock
> > smb/client: adapt to breakup of struct file_lock
> > smb/server: adapt to breakup of struct file_lock
> > filelock: remove temporary compatability macros
> > filelock: split leases out of struct file_lock
> >
> > cocci/filelock.cocci | 88 +++++
> > cocci/nlm.cocci | 81 ++++
> > fs/9p/vfs_file.c | 40 +-
> > fs/afs/flock.c | 59 +--
> > fs/ceph/locks.c | 74 ++--
> > fs/dlm/plock.c | 44 +--
> > fs/gfs2/file.c | 16 +-
> > fs/libfs.c | 2 +-
> > fs/lockd/clnt4xdr.c | 14 +-
> > fs/lockd/clntlock.c | 2 +-
> > fs/lockd/clntproc.c | 65 +--
> > fs/lockd/clntxdr.c | 14 +-
> > fs/lockd/svc4proc.c | 10 +-
> > fs/lockd/svclock.c | 64 +--
> > fs/lockd/svcproc.c | 10 +-
> > fs/lockd/svcsubs.c | 24 +-
> > fs/lockd/xdr.c | 14 +-
> > fs/lockd/xdr4.c | 14 +-
> > fs/locks.c | 848 ++++++++++++++++++++++------------------
> > fs/nfs/delegation.c | 4 +-
> > fs/nfs/file.c | 22 +-
> > fs/nfs/nfs3proc.c | 2 +-
> > fs/nfs/nfs4_fs.h | 2 +-
> > fs/nfs/nfs4file.c | 2 +-
> > fs/nfs/nfs4proc.c | 39 +-
> > fs/nfs/nfs4state.c | 22 +-
> > fs/nfs/nfs4trace.h | 4 +-
> > fs/nfs/nfs4xdr.c | 8 +-
> > fs/nfs/write.c | 8 +-
> > fs/nfsd/filecache.c | 4 +-
> > fs/nfsd/nfs4callback.c | 2 +-
> > fs/nfsd/nfs4layouts.c | 34 +-
> > fs/nfsd/nfs4state.c | 118 +++---
> > fs/ocfs2/locks.c | 12 +-
> > fs/ocfs2/stack_user.c | 2 +-
> > fs/open.c | 2 +-
> > fs/posix_acl.c | 4 +-
> > fs/smb/client/cifsfs.c | 2 +-
> > fs/smb/client/cifssmb.c | 8 +-
> > fs/smb/client/file.c | 76 ++--
> > fs/smb/client/smb2file.c | 2 +-
> > fs/smb/server/smb2pdu.c | 44 +--
> > fs/smb/server/vfs.c | 14 +-
> > include/linux/filelock.h | 80 ++--
> > include/linux/fs.h | 5 +-
> > include/linux/lockd/lockd.h | 8 +-
> > include/linux/lockd/xdr.h | 2 +-
> > include/trace/events/afs.h | 4 +-
> > include/trace/events/filelock.h | 102 ++---
> > 49 files changed, 1198 insertions(+), 923 deletions(-)
> > ---
> > base-commit: 615d300648869c774bd1fe54b4627bb0c20faed4
> > change-id: 20240116-flsplit-bdb46824db68
> >
> > Best regards,
> > --
> > Jeff Layton <[email protected]>
> >
>

--
Jeff Layton <[email protected]>

2024-01-25 22:35:34

by NeilBrown

[permalink] [raw]

Subject: Re: [PATCH v2 00/41] filelock: split struct file_lock into file_lock and file_lease structs

On Fri, 26 Jan 2024, Chuck Lever wrote:
> On Thu, Jan 25, 2024 at 05:42:41AM -0500, Jeff Layton wrote:
> > Long ago, file locks used to hang off of a singly-linked list in struct
> > inode. Because of this, when leases were added, they were added to the
> > same list and so they had to be tracked using the same sort of
> > structure.
> >
> > Several years ago, we added struct file_lock_context, which allowed us
> > to use separate lists to track different types of file locks. Given
> > that, leases no longer need to be tracked using struct file_lock.
> >
> > That said, a lot of the underlying infrastructure _is_ the same between
> > file leases and locks, so we can't completely separate everything.
> >
> > This patchset first splits a group of fields used by both file locks and
> > leases into a new struct file_lock_core, that is then embedded in struct
> > file_lock. Coccinelle was then used to convert a lot of the callers to
> > deal with the move, with the remaining 25% or so converted by hand.
> >
> > It then converts several internal functions in fs/locks.c to work
> > with struct file_lock_core. Lastly, struct file_lock is split into
> > struct file_lock and file_lease, and the lease-related APIs converted to
> > take struct file_lease.
> >
> > After the first few patches (which I left split up for easier review),
> > the set should be bisectable. I'll plan to squash the first few
> > together to make sure the resulting set is bisectable before merge.
> >
> > Finally, I left the coccinelle scripts I used in tree. I had heard it
> > was preferable to merge those along with the patches that they
> > generate, but I wasn't sure where they go. I can either move those to a
> > more appropriate location or we can just drop that commit if it's not
> > needed.
> >
> > Signed-off-by: Jeff Layton <[email protected]>
>
> v2 looks nicer.
>
> I would add a few list handling primitives, as I see enough
> instances of list_for_each_entry, list_for_each_entry_safe,
> list_first_entry, and list_first_entry_or_null on fl_core.flc_list
> to make it worth having those.
>
> Also, there doesn't seem to be benefit for API consumers to have to
> understand the internal structure of struct file_lock/lease to reach
> into fl_core. Having accessor functions for common fields like
> fl_type and fl_flags could be cleaner.

I'm not a big fan of accessor functions. They don't *look* like normal
field access, so a casual reader has to go find out what the function
does, just to find the it doesn't really do anything.

But neither am I a fan have requiring filesystems to use
"fl_core.flc_foo". As you say, reaching into fl_core isn't ideal.

It would be nice if we could make fl_core and anonymous structure, but
that really requires -fplan9-extensions which Linus is on-record as not
liking.
Unless...

How horrible would it be to use

union {
struct file_lock_core flc_core;
struct file_lock_core;
};

I think that only requires -fms-extensions, which Linus was less
negative towards. That would allow access to the members of
file_lock_core without the "flc_core." prefix, but would still allow
getting the address of 'flc_core'.
Maybe it's too ugly.

While fl_type and fl_flags are most common, fl_pid, fl_owner, fl_file
and even fl_wait are also used. Having accessor functions for all of those
would be too much I think.

Maybe higher-level functions which meet the real need of the filesystem
might be a useful approach:

locks_wakeup(lock)
locks_wait_interruptible(lock, condition)
locks_posix_init(lock, type, pid, ...) ??
locks_is_unlock() - fl_type is compared with F_UNLCK 22 times.

While those are probably a good idea, through don't really help much
with reducing the need for accessor functions.

I don't suppose we could just leave the #defines in place? Probably not
a good idea.

Maybe spell "fl_core" as "c"? lk->c.flc_flags ???

And I wonder if we could have a new fl_flag for 'FOREIGN' locks rather
than encoding that flag in the sign of the pid. That seems a bit ...
clunky?

NeilBrown