Re: [PATCH v2 00/41] filelock: split struct file_lock into file_lock and file_lease structs
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
> >
> > 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 h
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
>
> 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
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
>
> 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
>
> For the nfsd and lockd parts:
>
> Acked-by: Chuck Lever
>
>
> > ---
> > 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-c9d0f4370...@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: conv
Re: [PATCH v2 00/41] filelock: split struct file_lock into file_lock and file_lease structs
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
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
For the nfsd and lockd parts:
Acked-by: Chuck Lever
> ---
> 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-c9d0f4370...@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
[PATCH v2 00/41] filelock: split struct file_lock into file_lock and file_lease structs
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
---
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-c9d0f4370...@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
