Received-SPF: pass (google.com: domain of linux-nfs+bounces-1444-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
From: "NeilBrown" <neilb@suse.de>
To: "Chuck Lever" <chuck.lever@oracle.com>
Cc: "Jeff Layton" <jlayton@kernel.org>,
 "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>, "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>,
 "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>,
 "Ronnie Sahlberg" <ronniesahlberg@gmail.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,
 linux-trace-kernel@vger.kernel.org
Subject: Re: [PATCH v2 00/41] filelock: split struct file_lock into file_lock
 and file_lease structs
In-reply-to: <ZbJ2zc3I3uBwF/RE@tissot.1015granger.net>
References: <20240125-flsplit-v2-0-7485322b62c7@kernel.org>,
 <ZbJ2zc3I3uBwF/RE@tissot.1015granger.net>
Date: Fri, 26 Jan 2024 09:34:43 +1100
Message-id: <170622208395.21664.2510213291504081000@noble.neil.brown.name>

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