All of lore.kernel.org
 help / color / mirror / Atom feed
From: Matthew Wilcox <willy@infradead.org>
To: linux-fsdevel@vger.kernel.org
Subject: RFC: asserting an inode is locked
Date: Thu, 28 Mar 2024 01:46:10 +0000	[thread overview]
Message-ID: <ZgTL4jrUqIgCItx3@casper.infradead.org> (raw)


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

             reply	other threads:[~2024-03-28  1:46 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-28  1:46 Matthew Wilcox [this message]
2024-03-28  6:14 ` RFC: asserting an inode is locked Amir Goldstein
2024-03-28 13:30   ` Matthew Wilcox
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=ZgTL4jrUqIgCItx3@casper.infradead.org \
    --to=willy@infradead.org \
    --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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.