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@xxxxxxxxxxxxx> 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().
