RFC: asserting an inode is locked

RFC: asserting an inode is locked

[Matthew Wilcox]

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 ;-)

Re: RFC: asserting an inode is locked

[Amir Goldstein]

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

Thanks,
Amir.

Re: RFC: asserting an inode is locked

[Matthew Wilcox]

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

Re: RFC: asserting an inode is locked

[Dave Chinner]

On Thu, Mar 28, 2024 at 01:46:10AM +0000, Matthew Wilcox 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/

One small mistake in the middle of the major inode mutex -> rwsem
conversion that has no actual side effects other than a debug check
not being as strict as it could have been.

That's simply not something we should even be concerned about, let
alone have to jump through hand-wringing hoops of concern about how
to code specifically to avoid. It's just not going to be a
significant ongoing issue.

> 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.

I think you're getting yourself tied in knots here. The reason for
holding the lock -shared- is to ensure that the structure is
essentially in a read-only state and there are no concurrent
modifications taking place on the data that the lock protects.

If the caller holds the lock in exclusive mode, then we also have a
guarantee that there are no concurrent modifications taking place
because we own the structure entirely and hence it's still safe to
access the structure through read-only paths.

For example, there are lots of cases in XFS where the same code is
run by both read-only lookup and modification paths (e.g. in-memory
extent btrees). The only execution context difference is the lookup
runs with shared locking and the modification runs with exclusive
locking. IOWs, we can't use "assert lock is shared" or "assert lock
is excl" only in this path as both locking contexts are valid.

IOWs, what we are checking with this rwsem_assert_held() check is
there are -no concurrent modifications possible- at this point in
time and that means the lock simply needs to be held and it doesn't
matter how it is held.

Put simply: inode_assert_locked() doesn't assert that the lock
must be hold in shared mode, it is asserting that there are no
concurrent modifications taking place whilst we are running this
code.

From that perspective, the current name makes perfect sense and it
does not need changing at all. :)

> 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 ;-)

Document what it checks and the context in which it gets used. It is
a context specific check to ensure there are no concurrent
modifications taking place, not a check on the exact mode the lock
is held.

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: RFC: asserting an inode is locked

[Mateusz Guzik]

On Thu, Mar 28, 2024 at 01:46:10AM +0000, Matthew Wilcox 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);
> +}
> 

Huh, I thought this was sorted out some time last year.

> 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 ;-)

Ideally all ops would explicitly specify how they lock and what they
check, so in particular there would be inode_lock_write or similar, but
that's not worth the churn.

Second best option that I see is to patch up just the assertions to be
very explicit, to that end:
inode_assert_locked_excl
inode_assert_locked_any

No dedicated entry for shared-only, unless someone can point out
legitimate usage.

So happens I was looking at adding VFS_* debug macros (as in a config
option to have them be optionally compiled in) and this bit is related
-- namely absent the debug option *and* lockdep all these asserts should
compile to nothing. But I can elide these asserts from my initial patch
and add them after the above is settled.