Message-id: <173933773664.22054.1727909798811618895@noble.neil.brown.name>
Date: Wed, 12 Feb 2025 16:22:16 +1100
From: "NeilBrown" <neilb@...e.de>
To: "Al Viro" <viro@...iv.linux.org.uk>
Cc: "Christian Brauner" <brauner@...nel.org>, "Jan Kara" <jack@...e.cz>,
 "Linus Torvalds" <torvalds@...ux-foundation.org>,
 "Jeff Layton" <jlayton@...nel.org>, "Dave Chinner" <david@...morbit.com>,
 linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 08/19] VFS: introduce lookup_and_lock() and friends

On Sun, 09 Feb 2025, Al Viro wrote:
> On Fri, Feb 07, 2025 at 08:22:35PM +0000, Al Viro wrote:
> > On Thu, Feb 06, 2025 at 04:42:45PM +1100, NeilBrown wrote:
> > > lookup_and_lock() combines locking the directory and performing a lookup
> > > prior to a change to the directory.
> > > Abstracting this prepares for changing the locking requirements.
> > > 
> > > done_lookup_and_lock() provides the inverse of putting the dentry and
> > > unlocking.
> > > 
> > > For "silly_rename" we will need to lookup_and_lock() in a directory that
> > > is already locked.  For this purpose we add LOOKUP_PARENT_LOCKED.
> > 
> > Ewww...  I do realize that such things might appear in intermediate
> > stages of locking massage, but they'd better be _GONE_ by the end of it.
> > Conditional locking of that sort is really asking for trouble.
> > 
> > If nothing else, better split the function in two variants and document
> > the differences; that kind of stuff really does not belong in arguments.
> > If you need it to exist through the series, that is - if not, you should
> > just leave lookup_one_qstr() for the "locked" case from the very beginning.
> 
> The same, BTW, applies to more than LOOKUP_PARENT_LOCKED part.
> 
> One general observation: if the locking behaviour of a function depends
> upon the flags passed to it, it's going to cause massive headache afterwards.
> 
> If you need to bother with data flow analysis to tell what given call will
> do, expect trouble.
> 
> If anything, I would rather have separate lookup_for_removal(), etc., each
> with its locking effects explicitly spelled out.  Incidentally, looking

lookup_for_removal() etc would only be temporarily needed.  Eventually
(I hope) we would get to a place where all filesystems support all
operations with only a shared lock.  When we get there,
lookup_for_remove() and lookup_for_create() would be identical again.

And the difference wouldn't be that one takes a shared lock and the
other takes an exclusive lock.  It would be that one takes a shared or
exclusive lock based on flag X stored somewhere (inode, inode_operations,
...) while the other takes a shared or exclusive lock based on flag Y.

It would be nice to be able to accelerate that and push the locking down
into the filesystems call at once as Linus suggested last time:

https://lore.kernel.org/all/CAHk-=whz69y=98udgGB5ujH6bapYuapwfHS2esWaFrKEoi9-Ow@mail.gmail.com/

That would require either adding a new rwsem to each inode, possibly in
the filesystem-private part of the inode, or changing VFS to not lock
the inode at all.  The first would be unwelcome by fs developers I
expect, the second would be a serious challenge.  I started thinking
about and quickly decided I had enough challenges already.

So I think we need some way for the VFS to determine and select the lock
type requires by the filesystem.  Christian suggested a flag in
inode_operations and think that is a good idea.  I originally suggested
a flag in the superblock, but Linus suggested different operations might
want different locking (same email linked above).

But I don't think we can get away without having conditional locking
(like we already do in open_last_lookup() depending on O_CREAT).

Thanks,
NeilBrown

Powered by blists - more mailing lists