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

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

[Mark Harmstone]

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
+
 	if (priv->err) {
 		ret = priv->err;
 		goto out;
@@ -4860,6 +4865,15 @@ static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
 	 * and inode and freeing the allocations.
 	 */
 
+#ifdef CONFIG_DEBUG_LOCK_ALLOC
+	/*
+	 * We're returning to userspace with the inode lock held, and that's
+	 * okay - it'll get unlocked in a kthread.  Call rwsem_release to
+	 * avoid confusing lockdep.
+	 */
+	rwsem_release(&inode->vfs_inode.i_rwsem.dep_map, _THIS_IP_);
+#endif
+
 	return -EIOCBQUEUED;
 
 out_fail:
-- 
2.45.2

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

[Johannes Thumshirn]

On 12.11.24 17:01, 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
> +
>   	if (priv->err) {
>   		ret = priv->err;
>   		goto out;
> @@ -4860,6 +4865,15 @@ static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
>   	 * and inode and freeing the allocations.
>   	 */
>   
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
> +	/*
> +	 * We're returning to userspace with the inode lock held, and that's
> +	 * okay - it'll get unlocked in a kthread.  Call rwsem_release to
> +	 * avoid confusing lockdep.
> +	 */
> +	rwsem_release(&inode->vfs_inode.i_rwsem.dep_map, _THIS_IP_);
> +#endif
> +
>   	return -EIOCBQUEUED;
>   
>   out_fail:

Can't say anything about the correctness (as I have no clue), but we 
have wrappers around rwsem_release (btrfs_lockdep_release()) and 
rwsem_acquire_read (btrfs_lockdep_acquire()) that I think serve the 
documentation purpose.

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

[Mark Harmstone]

On 13/11/24 08:21, Johannes Thumshirn wrote:
> > 
> On 12.11.24 17:01, 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
>> +
>>    	if (priv->err) {
>>    		ret = priv->err;
>>    		goto out;
>> @@ -4860,6 +4865,15 @@ static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
>>    	 * and inode and freeing the allocations.
>>    	 */
>>    
>> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
>> +	/*
>> +	 * We're returning to userspace with the inode lock held, and that's
>> +	 * okay - it'll get unlocked in a kthread.  Call rwsem_release to
>> +	 * avoid confusing lockdep.
>> +	 */
>> +	rwsem_release(&inode->vfs_inode.i_rwsem.dep_map, _THIS_IP_);
>> +#endif
>> +
>>    	return -EIOCBQUEUED;
>>    
>>    out_fail:
> 
> Can't say anything about the correctness (as I have no clue), but we
> have wrappers around rwsem_release (btrfs_lockdep_release()) and
> rwsem_acquire_read (btrfs_lockdep_acquire()) that I think serve the
> documentation purpose.

btrfs_lockdep_acquire(a,b) calls rwsem_acquire_read on &a->b_map, so 
can't be used for something located at &inode->vfs_inode.i_rwsem.dep_map.

Mark

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

[Johannes Thumshirn]

On 13.11.24 11:24, Mark Harmstone wrote:
> On 13/11/24 08:21, Johannes Thumshirn wrote:
>>>
>> On 12.11.24 17:01, 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
>>> +
>>>     	if (priv->err) {
>>>     		ret = priv->err;
>>>     		goto out;
>>> @@ -4860,6 +4865,15 @@ static int btrfs_uring_read_extent(struct kiocb *iocb, struct iov_iter *iter,
>>>     	 * and inode and freeing the allocations.
>>>     	 */
>>>     
>>> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
>>> +	/*
>>> +	 * We're returning to userspace with the inode lock held, and that's
>>> +	 * okay - it'll get unlocked in a kthread.  Call rwsem_release to
>>> +	 * avoid confusing lockdep.
>>> +	 */
>>> +	rwsem_release(&inode->vfs_inode.i_rwsem.dep_map, _THIS_IP_);
>>> +#endif
>>> +
>>>     	return -EIOCBQUEUED;
>>>     
>>>     out_fail:
>>
>> Can't say anything about the correctness (as I have no clue), but we
>> have wrappers around rwsem_release (btrfs_lockdep_release()) and
>> rwsem_acquire_read (btrfs_lockdep_acquire()) that I think serve the
>> documentation purpose.
> 
> btrfs_lockdep_acquire(a,b) calls rwsem_acquire_read on &a->b_map, so
> can't be used for something located at &inode->vfs_inode.i_rwsem.dep_map.

Ah my bad, sorry.

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

[David Sterba]

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

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

[Mark Harmstone]

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