Re: [PATCH] btrfs: fix lockdep warnings on io_uring encoded reads

[Mark Harmstone <maharmstone@meta.com>]

On 14/11/24 15:06, David Sterba wrote:
> > 
> On Tue, Nov 12, 2024 at 04:00:49PM +0000, Mark Harmstone wrote:
>> Lockdep doesn't like the fact that btrfs_uring_read_extent() returns to
>> userspace still holding the inode lock, even though we release it once
>> the I/O finishes. Add calls to rwsem_release() and rwsem_acquire_read() to
>> work round this.
>>
>> Signed-off-by: Mark Harmstone <maharmstone@fb.com>
>> Reported-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>> ---
>>   fs/btrfs/ioctl.c | 14 ++++++++++++++
>>   1 file changed, 14 insertions(+)
>>
>> diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c
>> index 1fdeb216bf6c..6ea01e4f940e 100644
>> --- a/fs/btrfs/ioctl.c
>> +++ b/fs/btrfs/ioctl.c
>> @@ -4752,6 +4752,11 @@ static void btrfs_uring_read_finished(struct io_uring_cmd *cmd, unsigned int iss
>>   	size_t page_offset;
>>   	ssize_t ret;
>>   
>> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
>> +	/* The inode lock has already been acquired in btrfs_uring_read_extent.  */
>> +	rwsem_acquire_read(&inode->vfs_inode.i_rwsem.dep_map, 0, 0, _THIS_IP_);
>> +#endif
> 
> Please put that to a wrapper, we want to avoid #ifdefs in the middle of
> functions (there are exceptions), especially when the macro is for lock
> debugging.
> 
> The wrapper name can follow naming and syntax of the other
> btrfs_lockde_* wrappers, so like btrfs_lockdep_inode_acquire(owner,
> lock). There is only one rwsem in inode, but for clarity and consistency
> I think it makes sense.
> 
> btrfs_lockdep_inode_acquire(inode, i_rwsem);

Thanks David. The CONFIG_DEBUG_LOCK_ALLOC #ifdefs turned out not to be 
necessary, lockdep handles this, but I'll create the wrappers to match 
everything else.

Mark