Use the new rwsem_assert_held functions to implement these new
assertions. Convert the inode_is_locked() callers in the VFS to
use them.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
fs/attr.c | 2 +-
fs/namei.c | 6 +++---
include/linux/fs.h | 10 ++++++++++
3 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/fs/attr.c b/fs/attr.c
index a8ae5f6d9b16..5e32b0a4f8c2 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -387,7 +387,7 @@ int notify_change(struct mnt_idmap *idmap, struct dentry *dentry,
struct timespec64 now;
unsigned int ia_valid = attr->ia_valid;
- WARN_ON_ONCE(!inode_is_locked(inode));
+ inode_assert_locked_excl(inode);
error = may_setattr(idmap, inode, ia_valid);
if (error)
diff --git a/fs/namei.c b/fs/namei.c
index 567ee547492b..6b595ad4318d 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -2708,7 +2708,7 @@ struct dentry *try_lookup_one_len(const char *name, struct dentry *base, int len
struct qstr this;
int err;
- WARN_ON_ONCE(!inode_is_locked(base->d_inode));
+ inode_assert_locked(base->d_inode);
err = lookup_one_common(&nop_mnt_idmap, name, base, len, &this);
if (err)
@@ -2735,7 +2735,7 @@ struct dentry *lookup_one_len(const char *name, struct dentry *base, int len)
struct qstr this;
int err;
- WARN_ON_ONCE(!inode_is_locked(base->d_inode));
+ inode_assert_locked(base->d_inode);
err = lookup_one_common(&nop_mnt_idmap, name, base, len, &this);
if (err)
@@ -2765,7 +2765,7 @@ struct dentry *lookup_one(struct mnt_idmap *idmap, const char *name,
struct qstr this;
int err;
- WARN_ON_ONCE(!inode_is_locked(base->d_inode));
+ inode_assert_locked(base->d_inode);
err = lookup_one_common(idmap, name, base, len, &this);
if (err)
diff --git a/include/linux/fs.h b/include/linux/fs.h
index b528f063e8ff..e01e041c102b 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -832,6 +832,16 @@ static inline int inode_is_locked(struct inode *inode)
return rwsem_is_locked(&inode->i_rwsem);
}
+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);
+}
+
static inline void inode_lock_nested(struct inode *inode, unsigned subclass)
{
down_write_nested(&inode->i_rwsem, subclass);
--
2.40.1
On 10/7/23, Matthew Wilcox (Oracle) <[email protected]> wrote:
> +static inline void inode_assert_locked_excl(const struct inode *inode)
> +{
> + rwsem_assert_held_write(&inode->i_rwsem);
> +}
> +
> static inline void inode_lock_nested(struct inode *inode, unsigned
> subclass)
> {
> down_write_nested(&inode->i_rwsem, subclass);
Why "excl" instead of "write"? Apart from looking weird, it is
inconsistent with "prior art" in the file: i_mmap_assert_write_locked.
--
Mateusz Guzik <mjguzik gmail.com>
On Sun, Oct 08, 2023 at 10:26:40PM +0200, Mateusz Guzik wrote:
> On 10/7/23, Matthew Wilcox (Oracle) <[email protected]> wrote:
> > +static inline void inode_assert_locked_excl(const struct inode *inode)
> > +{
> > + rwsem_assert_held_write(&inode->i_rwsem);
> > +}
> > +
> > static inline void inode_lock_nested(struct inode *inode, unsigned
> > subclass)
> > {
> > down_write_nested(&inode->i_rwsem, subclass);
>
> Why "excl" instead of "write"? Apart from looking weird, it is
> inconsistent with "prior art" in the file: i_mmap_assert_write_locked.
Yes, but that pairs with i_mmap_lock_write() / i_mmap_lock_read().
The problem is that we have inode_lock() / inode_lock_shared()
inode_assert_locked_read/write doesn't make sense with them. But
inode_assert_locked() doesn't make sense as the assertion for
inode_lock() because you'd expect it to assert whether the inode lock
is held at all. So I went with inode_assert_locked_excl().
I wouldn't mind if we converted all the inode_lock()/shared to
inode_lock_read() / inode_lock_write(), and then added
inode_assert_read_locked() / inode_assert_write_locked(). That's
a bit of a bigger job than I want to take on today.
On 10/8/23, Matthew Wilcox <[email protected]> wrote:
> On Sun, Oct 08, 2023 at 10:26:40PM +0200, Mateusz Guzik wrote:
>> On 10/7/23, Matthew Wilcox (Oracle) <[email protected]> wrote:
>> > +static inline void inode_assert_locked_excl(const struct inode *inode)
>> > +{
>> > + rwsem_assert_held_write(&inode->i_rwsem);
>> > +}
>> > +
>> > static inline void inode_lock_nested(struct inode *inode, unsigned
>> > subclass)
>> > {
>> > down_write_nested(&inode->i_rwsem, subclass);
>>
>> Why "excl" instead of "write"? Apart from looking weird, it is
>> inconsistent with "prior art" in the file: i_mmap_assert_write_locked.
>
> Yes, but that pairs with i_mmap_lock_write() / i_mmap_lock_read().
>
> The problem is that we have inode_lock() / inode_lock_shared()
> inode_assert_locked_read/write doesn't make sense with them. But
> inode_assert_locked() doesn't make sense as the assertion for
> inode_lock() because you'd expect it to assert whether the inode lock
> is held at all. So I went with inode_assert_locked_excl().
>
> I wouldn't mind if we converted all the inode_lock()/shared to
> inode_lock_read() / inode_lock_write(), and then added
> inode_assert_read_locked() / inode_assert_write_locked(). That's
> a bit of a bigger job than I want to take on today.
>
I agree it is rather messy and I'm not going to spend time arguing as
it is not my call anyway.
Speaking of that, I just noticed the vfs folk are not CC'ed, which I'm
rectifying with this e-mail.
--
Mateusz Guzik <mjguzik gmail.com>