Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
MIME-Version: 1.0
In-Reply-To: <ZPiaYjcTMyuM0JL5@casper.infradead.org>
References: <20230831151414.2714750-1-mjguzik@gmail.com> <ZPiYp+t6JTUscc81@casper.infradead.org>
 <b0434328-01f9-dc5c-fe25-4a249130a81d@fastmail.fm> <ZPiaYjcTMyuM0JL5@casper.infradead.org>
From:   Mateusz Guzik <mjguzik@gmail.com>
Date:   Thu, 14 Sep 2023 15:16:50 +0200
Message-ID: <CAGudoHGU0=BhHzzGrkzt22J0iC5YCcujY-koL+Eq7Uiga6Lh9g@mail.gmail.com>
Subject: Re: [RFC PATCH] vfs: add inode lockdep assertions
To:     Matthew Wilcox <willy@infradead.org>
Cc:     Bernd Schubert <bernd.schubert@fastmail.fm>, brauner@kernel.org,
        viro@zeniv.linux.org.uk, linux-kernel@vger.kernel.org,
        linux-fsdevel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On 9/6/23, Matthew Wilcox <willy@infradead.org> wrote:
> On Wed, Sep 06, 2023 at 05:23:42PM +0200, Bernd Schubert wrote:
>>
>>
>> On 9/6/23 17:20, Matthew Wilcox wrote:
>> > On Thu, Aug 31, 2023 at 05:14:14PM +0200, Mateusz Guzik wrote:
>> > > +++ b/include/linux/fs.h
>> > > @@ -842,6 +842,16 @@ static inline void
>> > > inode_lock_shared_nested(struct inode *inode, unsigned subcla
>> > >   	down_read_nested(&inode->i_rwsem, subclass);
>> > >   }
>> > > +static inline void inode_assert_locked(struct inode *inode)
>> > > +{
>> > > +	lockdep_assert_held(&inode->i_rwsem);
>> > > +}
>> > > +
>> > > +static inline void inode_assert_write_locked(struct inode *inode)
>> > > +{
>> > > +	lockdep_assert_held_write(&inode->i_rwsem);
>> > > +}
>> >
>> > This mirrors what we have in mm, but it's only going to trigger on
>> > builds that have lockdep enabled.  Lockdep is very expensive; it
>> > easily doubles the time it takes to run xfstests on my laptop, so
>> > I don't generally enable it.  So what we also have in MM is:
>> >
>> > static inline void mmap_assert_write_locked(struct mm_struct *mm)
>> > {
>> >          lockdep_assert_held_write(&mm->mmap_lock);
>> >          VM_BUG_ON_MM(!rwsem_is_locked(&mm->mmap_lock), mm);
>> > }
>> >
>> > Now if you have lockdep enabled, you get the lockdep check which
>> > gives you all the lovely lockdep information, but if you don't, you
>> > at least get the cheap check that someone is holding the lock at all.
>> >
>> > ie I would make this:
>> >
>> > +static inline void inode_assert_write_locked(struct inode *inode)
>> > +{
>> > +	lockdep_assert_held_write(&inode->i_rwsem);
>> > +	WARN_ON_ONCE(!inode_is_locked(inode));
>> > +}
>> >
>> > Maybe the locking people could give us a rwsem_is_write_locked()
>> > predicate, but until then, this is the best solution we came up with.
>>
>>
>> Which is exactly what I had suggested in the other thread :)
>
> Yes, but apparently comments in that thread don't count :eyeroll:
>

Pretty weird reaction mate, they very much *do* count which is why I'm
confused why you resent an e-mail with the bogus is_locked check
(which I explicitly pointed out).

Since you posted a separate patch to add write-locking check to rwsem
I'm going to wait for that bit to get sorted out (unless it stalls for
a long time).

fwiw I'm confused what's up with people making kernel changes without
running lockdep. If it adds too much overhead for use in normal
development someone(tm) should have fixed that aspect long time ago.

-- 
Mateusz Guzik <mjguzik gmail.com>