| Message ID | 20240116-flsplit-v1-1-c9d0f4370a5d@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | filelock: split struct file_lock into file_lock and file_lease structs | expand |
On Wed, 17 Jan 2024, Jeff Layton wrote: > In a future patch, we're going to split file leases into their own > structure. Since a lot of the underlying machinery uses the same fields > move those into a new file_lock_core, and embed that inside struct > file_lock. > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > --- > include/linux/filelock.h | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/include/linux/filelock.h b/include/linux/filelock.h > index 95e868e09e29..7825511c1c11 100644 > --- a/include/linux/filelock.h > +++ b/include/linux/filelock.h > @@ -85,8 +85,9 @@ bool opens_in_grace(struct net *); > * > * Obviously, the last two criteria only matter for POSIX locks. > */ > -struct file_lock { > - struct file_lock *fl_blocker; /* The lock, that is blocking us */ > + > +struct file_lock_core { > + struct file_lock *fl_blocker; /* The lock that is blocking us */ > struct list_head fl_list; /* link into file_lock_context */ > struct hlist_node fl_link; /* node in global lists */ > struct list_head fl_blocked_requests; /* list of requests with > @@ -102,6 +103,10 @@ struct file_lock { > int fl_link_cpu; /* what cpu's list is this on? */ > wait_queue_head_t fl_wait; > struct file *fl_file; > +}; > + > +struct file_lock { > + struct file_lock_core fl_core; > loff_t fl_start; > loff_t fl_end; > If I we doing this, I would rename all the fields in file_lock_core to have an "flc_" prefix, and add some #defines like #define fl_list fl_core.flc_list so there would be no need to squash this with later patches to achieve bisectability. The #defines would be removed after the coccinelle scripts etc are applied. I would also do the "convert some internal functions" patches *before* the bulk conversion of fl_foo to fl_code.flc_foo so that those functions don't get patched twice. But this is all personal preference. If you prefer your approach, please leave it that way. The only clear benefit of my approach is that you don't need to squash patches together, and that is probably not a big deal. Thanks, NeilBrown
On Wed, 2024-01-17 at 09:07 +1100, NeilBrown wrote: > On Wed, 17 Jan 2024, Jeff Layton wrote: > > In a future patch, we're going to split file leases into their own > > structure. Since a lot of the underlying machinery uses the same fields > > move those into a new file_lock_core, and embed that inside struct > > file_lock. > > > > Signed-off-by: Jeff Layton <jlayton@kernel.org> > > --- > > include/linux/filelock.h | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/include/linux/filelock.h b/include/linux/filelock.h > > index 95e868e09e29..7825511c1c11 100644 > > --- a/include/linux/filelock.h > > +++ b/include/linux/filelock.h > > @@ -85,8 +85,9 @@ bool opens_in_grace(struct net *); > > * > > * Obviously, the last two criteria only matter for POSIX locks. > > */ > > -struct file_lock { > > - struct file_lock *fl_blocker; /* The lock, that is blocking us */ > > + > > +struct file_lock_core { > > + struct file_lock *fl_blocker; /* The lock that is blocking us */ > > struct list_head fl_list; /* link into file_lock_context */ > > struct hlist_node fl_link; /* node in global lists */ > > struct list_head fl_blocked_requests; /* list of requests with > > @@ -102,6 +103,10 @@ struct file_lock { > > int fl_link_cpu; /* what cpu's list is this on? */ > > wait_queue_head_t fl_wait; > > struct file *fl_file; > > +}; > > + > > +struct file_lock { > > + struct file_lock_core fl_core; > > loff_t fl_start; > > loff_t fl_end; > > > > If I we doing this, I would rename all the fields in file_lock_core to > have an "flc_" prefix, and add some #defines like > > #define fl_list fl_core.flc_list > > so there would be no need to squash this with later patches to achieve > bisectability. > > The #defines would be removed after the coccinelle scripts etc are > applied. > > I would also do the "convert some internal functions" patches *before* > the bulk conversion of fl_foo to fl_code.flc_foo so that those functions > don't get patched twice. > > But this is all personal preference. If you prefer your approach, > please leave it that way. The only clear benefit of my approach is that > you don't need to squash patches together, and that is probably not a > big deal. > I considered going back and doing that. It would allow me to break this up into smaller patches, but I think that basically means doing all of this work over again. I'll probably stick with this approach, unless I end up needing to respin for other reasons.
diff --git a/include/linux/filelock.h b/include/linux/filelock.h index 95e868e09e29..7825511c1c11 100644 --- a/include/linux/filelock.h +++ b/include/linux/filelock.h @@ -85,8 +85,9 @@ bool opens_in_grace(struct net *); * * Obviously, the last two criteria only matter for POSIX locks. */ -struct file_lock { - struct file_lock *fl_blocker; /* The lock, that is blocking us */ + +struct file_lock_core { + struct file_lock *fl_blocker; /* The lock that is blocking us */ struct list_head fl_list; /* link into file_lock_context */ struct hlist_node fl_link; /* node in global lists */ struct list_head fl_blocked_requests; /* list of requests with @@ -102,6 +103,10 @@ struct file_lock { int fl_link_cpu; /* what cpu's list is this on? */ wait_queue_head_t fl_wait; struct file *fl_file; +}; + +struct file_lock { + struct file_lock_core fl_core; loff_t fl_start; loff_t fl_end;
In a future patch, we're going to split file leases into their own structure. Since a lot of the underlying machinery uses the same fields move those into a new file_lock_core, and embed that inside struct file_lock. Signed-off-by: Jeff Layton <jlayton@kernel.org> --- include/linux/filelock.h | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)