Re: [PATCH 01/20] filelock: split common fields into struct file_lock_core

From: "NeilBrown" <neilb@suse.de>
To: "Jeff Layton" <jlayton@kernel.org>
Cc: "Christian Brauner" <brauner@kernel.org>,
	"Alexander Viro" <viro@zeniv.linux.org.uk>,
	"Eric Van Hensbergen" <ericvh@kernel.org>,
	"Latchesar Ionkov" <lucho@ionkov.net>,
	"Dominique Martinet" <asmadeus@codewreck.org>,
	"Christian Schoenebeck" <linux_oss@crudebyte.com>,
	"David Howells" <dhowells@redhat.com>,
	"Marc Dionne" <marc.dionne@auristor.com>,
	"Xiubo Li" <xiubli@redhat.com>,
	"Ilya Dryomov" <idryomov@gmail.com>,
	"Alexander Aring" <aahringo@redhat.com>,
	"David Teigland" <teigland@redhat.com>,
	"Miklos Szeredi" <miklos@szeredi.hu>,
	"Andreas Gruenbacher" <agruenba@redhat.com>,
	"Trond Myklebust" <trond.myklebust@hammerspace.com>,
	"Anna Schumaker" <anna@kernel.org>,
	"Chuck Lever" <chuck.lever@oracle.com>,
	"Olga Kornievskaia" <kolga@netapp.com>,
	"Dai Ngo" <Dai.Ngo@oracle.com>, "Tom Talpey" <tom@talpey.com>,
	"Jan Kara" <jack@suse.cz>, "Mark Fasheh" <mark@fasheh.com>,
	"Joel Becker" <jlbec@evilplan.org>,
	"Joseph Qi" <joseph.qi@linux.alibaba.com>,
	"Steve French" <sfrench@samba.org>,
	"Paulo Alcantara" <pc@manguebit.com>,
	"Ronnie Sahlberg" <lsahlber@redhat.com>,
	"Shyam Prasad N" <sprasad@microsoft.com>,
	"Namjae Jeon" <linkinjeon@kernel.org>,
	"Sergey Senozhatsky" <senozhatsky@chromium.org>,
	"Steven Rostedt" <rostedt@goodmis.org>,
	"Masami Hiramatsu" <mhiramat@kernel.org>,
	"Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>,
	linux-kernel@vger.kernel.org, v9fs@lists.linux.dev,
	linux-afs@lists.infradead.org, ceph-devel@vger.kernel.org,
	gfs2@lists.linux.dev, linux-fsdevel@vger.kernel.org,
	linux-nfs@vger.kernel.org, ocfs2-devel@lists.linux.dev,
	linux-cifs@vger.kernel.org, samba-technical@lists.samba.org,
	linux-trace-kernel@vger.kernel.org,
	"Jeff Layton" <jlayton@kernel.org>
Subject: Re: [PATCH 01/20] filelock: split common fields into struct file_lock_core
Date: Wed, 17 Jan 2024 09:07:02 +1100	[thread overview]
Message-ID: <170544282220.23031.10628392788633554158@noble.neil.brown.name> (raw)
In-Reply-To: <20240116-flsplit-v1-1-c9d0f4370a5d@kernel.org>

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