From: Matthew Wilcox <willy@infradead.org>
To: Amir Goldstein <amir73il@gmail.com>
Cc: linux-fsdevel@vger.kernel.org
Subject: Re: RFC: asserting an inode is locked
Date: Thu, 28 Mar 2024 13:30:09 +0000 [thread overview]
Message-ID: <ZgVw4fVVLLGnG_8u@casper.infradead.org> (raw)
In-Reply-To: <CAOQ4uxjK2Dcv0oDo5K6Z6QevapViR_mPFD_+wXu1GaufXs42WA@mail.gmail.com>
On Thu, Mar 28, 2024 at 08:14:32AM +0200, Amir Goldstein wrote:
> On Thu, Mar 28, 2024 at 3:46 AM Matthew Wilcox <willy@infradead.org> wrote:
> >
> >
> > I have this patch in my tree that I'm thinking about submitting:
> >
> > +static inline void inode_assert_locked(const struct inode *inode)
> > +{
> > + rwsem_assert_held(&inode->i_rwsem);
> > +}
> > +
> > +static inline void inode_assert_locked_excl(const struct inode *inode)
> > +{
> > + rwsem_assert_held_write(&inode->i_rwsem);
> > +}
> >
> > Then we can do a whole bunch of "replace crappy existing assertions with
> > the shiny new ones".
> >
> > @@ -2746,7 +2746,7 @@ struct dentry *lookup_one_len(const char *name, struct den
> > try *base, int len)
> > struct qstr this;
> > int err;
> >
> > - WARN_ON_ONCE(!inode_is_locked(base->d_inode));
> > + inode_assert_locked(base->d_inode);
> >
> > for example.
> >
> > But the naming is confusing and I can't think of good names.
> >
> > inode_lock() takes the lock exclusively, whereas inode_assert_locked()
> > only checks that the lock is held. ie 1-3 pass and 4 fails.
> >
> > 1. inode_lock(inode); inode_assert_locked(inode);
> > 2. inode_lock_shared(inode); inode_assert_locked(inode);
> > 3. inode_lock(inode); inode_assert_locked_excl(inode);
> > 4. inode_lock_shared(inode); inode_assert_locked_excl(inode);
> >
> > I worry that this abstraction will cause people to write
> > inode_assert_locked() when they really need to check
> > inode_assert_locked_excl(). We already had/have this problem:
> > https://lore.kernel.org/all/20230831101824.qdko4daizgh7phav@f/
> >
> > So how do we make it that people write the right one?
> > Renaming inode_assert_locked() to inode_assert_locked_shared() isn't
> > the answer because it checks that the lock is _at least_ shared, it
> > might be held exclusively.
> >
> > Rename inode_assert_locked() to inode_assert_held()? That might be
> > enough of a disconnect that people would not make bad assumptions.
> > I don't have a good answer here, or I'd send a patch to do that.
> > Please suggest something ;-)
> >
>
> Damn, human engineering is hard...
>
> I think that using inode_assert_held() would help a bit, but people may
> still use it after inode_lock().
>
> How about always being explicit?
>
> static inline void inode_assert_locked(const struct inode *inode, bool excl)
> {
> if (excl)
> rwsem_assert_held_write(&inode->i_rwsem);
> else
> rwsem_assert_held(&inode->i_rwsem);
> }
>
> and change inode_is_locked() to also be explicit while at it, to nudge
> replacing all the existing weak assertion with inode_assert_locked().
I liked this idea when I first read it, but now I'm not so sure.
inode_assert_locked(base->d_inode, false);
wait, what does 'false' mean? Is that "must be write locked" or
is it "can be read locked only"? And introducing enums or defines
to replace true/false doesn't really get us anywhere because we're
still looking for a word that means "at least read locked" rather than
"exactly read locked".
We don't have this problem in MM because we have mmap_read_lock() and
mmap_write_lock(), so mmap_assert_locked() and mmap_assert_write_locked()
are natural. I totally understand the FS aversion to the read/write
terminology ("You need a read lock to do writes?"), but the real problem
is that it's not inode_lock_excl(). I don't know that we want to
replace all 337 occurrences of inode_lock() with inode_lock_excl() and
all 485 occurrences of inode_unlock() with inode_unlock_excl().
next prev parent reply other threads:[~2024-03-28 13:30 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-03-28 1:46 RFC: asserting an inode is locked Matthew Wilcox
2024-03-28 6:14 ` Amir Goldstein
2024-03-28 13:30 ` Matthew Wilcox [this message]
2024-04-01 23:51 ` Dave Chinner
2024-05-02 15:31 ` Mateusz Guzik
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZgVw4fVVLLGnG_8u@casper.infradead.org \
--to=willy@infradead.org \
--cc=amir73il@gmail.com \
--cc=linux-fsdevel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).