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@xxxxxxxxxxxxx
