mbox series

[v2,00/41] filelock: split struct file_lock into file_lock and file_lease structs

Message ID 20240125-flsplit-v2-0-7485322b62c7@kernel.org (mailing list archive)
Headers show
Series filelock: split struct file_lock into file_lock and file_lease structs | expand

Message

Jeff Layton Jan. 25, 2024, 10:42 a.m. UTC
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 <jlayton@kernel.org>
---
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/20240116-flsplit-v1-0-c9d0f4370a5d@kernel.org

---
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,

Comments

Chuck Lever Jan. 25, 2024, 2:57 p.m. UTC | #1
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 <jlayton@kernel.org>

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.

For the series:

Reviewed-by: Chuck Lever <chuck.lever@oracle.com>

For the nfsd and lockd parts:

Acked-by: Chuck Lever <chuck.lever@oracle.com>


> ---
> 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/20240116-flsplit-v1-0-c9d0f4370a5d@kernel.org
> 
> ---
> 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 <jlayton@kernel.org>
>
Jeff Layton Jan. 25, 2024, 5 p.m. UTC | #2
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 <jlayton@kernel.org>
> 
> 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 <chuck.lever@oracle.com>
> 
> For the nfsd and lockd parts:
> 
> Acked-by: Chuck Lever <chuck.lever@oracle.com>
> 
> 
> > ---
> > 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/20240116-flsplit-v1-0-c9d0f4370a5d@kernel.org
> > 
> > ---
> > 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 <jlayton@kernel.org>
> > 
>
NeilBrown Jan. 25, 2024, 10:34 p.m. UTC | #3
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 <jlayton@kernel.org>
> 
> 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
Jeff Layton Jan. 25, 2024, 11:58 p.m. UTC | #4
On Fri, 2024-01-26 at 09:34 +1100, NeilBrown wrote:
> 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 <jlayton@kernel.org>
> > 
> > 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.

I might have been a bit too hasty with the idea. I took a look earlier
today and it gets pretty ugly trying to handle these fields with
accessors. flc_flags, for instance will need both a get and a set
method, which gets wordy after a while.

Some of the flc_list accesses don't involve list walks either so I don't
think we'll ever be able to make this "neat" without a ton of one-off
accessors.

> 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.
> 

I too think it's ugly.

> 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.
> 

I'd rather not rely on special compiler flags.

> 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.
> 

Some of them need setters too, and some like fl_flags like to be able to
do this:

    fl->fl_flags |= FL_SLEEP;

That's hard to deal with in an accessor unless you want to do it with
macros or something.

> 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 can take a look at some of those. Reducing the number of instances can
only help.

> 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 ???
> 

It's at least a little shorter. I can make that change if it's
preferred.

> 
> 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?
> 

The kernel just treats the fl_pid as an opaque value that gets reported
to various consumers. Having it encoded in the sign is actually more
convenient, since reporting "foreign" lock holders as negative pid
values has some precedent in Unix.

flock and posix locks conflict on BSD, and the POSIX lock API reports
fl_pid as '-1' when there is a conflicting flock lock. I think solaris
may also report remote NFS locks as negative numbers too? (not certain
there).

So it works in our favor in this case, but it is a hack.

Now that I look too, I'm not sure why fl_pid is unsigned given that
pid_t is signed. I'll have to look into that as well.