[PATCH] fuse: fix return value of inode_inline_reclaim_one_dmap in error path

[PATCH] fuse: fix return value of inode_inline_reclaim_one_dmap in error path

[Jingbo Xu]

When range already got reclaimed by somebody else, return NULL so that
the caller could retry to allocate or reclaim another range, instead of
mistakenly returning the range already got reclaimed and reused by
others.

Reported-by: Liu Jiang <gerry@linux.alibaba.com>
Fixes: 9a752d18c85a ("virtiofs: add logic to free up a memory range")
Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
---
 fs/fuse/dax.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index 8e74f278a3f6..59aadfd89ee5 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -985,6 +985,7 @@ inode_inline_reclaim_one_dmap(struct fuse_conn_dax *fcd, struct inode *inode,
 	node = interval_tree_iter_first(&fi->dax->tree, start_idx, start_idx);
 	/* Range already got reclaimed by somebody else */
 	if (!node) {
+		dmap = NULL;
 		if (retry)
 			*retry = true;
 		goto out_write_dmap_sem;
-- 
2.19.1.6.gb485710b

Re: [PATCH] fuse: fix return value of inode_inline_reclaim_one_dmap in error path

[Jingbo Xu]

ping...

On 4/24/23 8:32 PM, Jingbo Xu wrote:
> When range already got reclaimed by somebody else, return NULL so that
> the caller could retry to allocate or reclaim another range, instead of
> mistakenly returning the range already got reclaimed and reused by
> others.
> 
> Reported-by: Liu Jiang <gerry@linux.alibaba.com>
> Fixes: 9a752d18c85a ("virtiofs: add logic to free up a memory range")
> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> ---
>  fs/fuse/dax.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
> index 8e74f278a3f6..59aadfd89ee5 100644
> --- a/fs/fuse/dax.c
> +++ b/fs/fuse/dax.c
> @@ -985,6 +985,7 @@ inode_inline_reclaim_one_dmap(struct fuse_conn_dax *fcd, struct inode *inode,
>  	node = interval_tree_iter_first(&fi->dax->tree, start_idx, start_idx);
>  	/* Range already got reclaimed by somebody else */
>  	if (!node) {
> +		dmap = NULL;
>  		if (retry)
>  			*retry = true;
>  		goto out_write_dmap_sem;

-- 
Thanks,
Jingbo

Re: [PATCH] fuse: fix return value of inode_inline_reclaim_one_dmap in error path

[Jingbo Xu]

On 6/1/23 4:03 AM, Vivek Goyal wrote:
> On Mon, Apr 24, 2023 at 08:32:50PM +0800, Jingbo Xu wrote:
>> When range already got reclaimed by somebody else, return NULL so that
>> the caller could retry to allocate or reclaim another range, instead of
>> mistakenly returning the range already got reclaimed and reused by
>> others.
>>
>> Reported-by: Liu Jiang <gerry@linux.alibaba.com>
>> Fixes: 9a752d18c85a ("virtiofs: add logic to free up a memory range")
>> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> 
> Hi Jingbo,
> 
> This patch looks correct to me.
> 
> Are you able to reproduce the problem? Or you are fixing it based on
> code inspection?

It's spotted by Liu Jiang during code review.  Not tested yet.

> 
> How are you testing this? We don't have virtiofsd DAX implementation yet
> in rust virtiofsd yet. 
> 
> I am not sure how to test this chagne now. We had out of tree patches
> in qemu and now qemu has gotten rid of C version of virtiofsd so these
> patches might not even work now.

Yeah this exception path may not be so easy to be tested as it is only
triggered in the race condition.  I have the old branch (of qemu) with
support for DAX, and maybe I could try to reproduce the exception path
by configuring limited DAX window and heavy IO workload.


-- 
Thanks,
Jingbo

Re: [PATCH] fuse: fix return value of inode_inline_reclaim_one_dmap in error path

[Vivek Goyal]

On Thu, Jun 01, 2023 at 09:45:52AM +0800, Jingbo Xu wrote:
> 
> 
> On 6/1/23 4:03 AM, Vivek Goyal wrote:
> > On Mon, Apr 24, 2023 at 08:32:50PM +0800, Jingbo Xu wrote:
> >> When range already got reclaimed by somebody else, return NULL so that
> >> the caller could retry to allocate or reclaim another range, instead of
> >> mistakenly returning the range already got reclaimed and reused by
> >> others.
> >>
> >> Reported-by: Liu Jiang <gerry@linux.alibaba.com>
> >> Fixes: 9a752d18c85a ("virtiofs: add logic to free up a memory range")
> >> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
> > 
> > Hi Jingbo,
> > 
> > This patch looks correct to me.
> > 
> > Are you able to reproduce the problem? Or you are fixing it based on
> > code inspection?
> 
> It's spotted by Liu Jiang during code review.  Not tested yet.
> 
> > 
> > How are you testing this? We don't have virtiofsd DAX implementation yet
> > in rust virtiofsd yet. 
> > 
> > I am not sure how to test this chagne now. We had out of tree patches
> > in qemu and now qemu has gotten rid of C version of virtiofsd so these
> > patches might not even work now.
> 
> Yeah this exception path may not be so easy to be tested as it is only
> triggered in the race condition.  I have the old branch (of qemu) with
> support for DAX, and maybe I could try to reproduce the exception path
> by configuring limited DAX window and heavy IO workload.

That would be great. Please test it with really small DAX window size.
Also put some pr_debug() statements to make sure you are hitting this
particular path during testing.

Thanks
Vivek

Re: [PATCH] fuse: fix return value of inode_inline_reclaim_one_dmap in error path

[Jingbo Xu]

On 6/1/23 7:45 PM, Vivek Goyal wrote:
> On Thu, Jun 01, 2023 at 09:45:52AM +0800, Jingbo Xu wrote:
>>
>>
>> On 6/1/23 4:03 AM, Vivek Goyal wrote:
>>> On Mon, Apr 24, 2023 at 08:32:50PM +0800, Jingbo Xu wrote:
>>>> When range already got reclaimed by somebody else, return NULL so that
>>>> the caller could retry to allocate or reclaim another range, instead of
>>>> mistakenly returning the range already got reclaimed and reused by
>>>> others.
>>>>
>>>> Reported-by: Liu Jiang <gerry@linux.alibaba.com>
>>>> Fixes: 9a752d18c85a ("virtiofs: add logic to free up a memory range")
>>>> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
>>>
>>> Hi Jingbo,
>>>
>>> This patch looks correct to me.
>>>
>>> Are you able to reproduce the problem? Or you are fixing it based on
>>> code inspection?
>>
>> It's spotted by Liu Jiang during code review.  Not tested yet.
>>
>>>
>>> How are you testing this? We don't have virtiofsd DAX implementation yet
>>> in rust virtiofsd yet. 
>>>
>>> I am not sure how to test this chagne now. We had out of tree patches
>>> in qemu and now qemu has gotten rid of C version of virtiofsd so these
>>> patches might not even work now.
>>
>> Yeah this exception path may not be so easy to be tested as it is only
>> triggered in the race condition.  I have the old branch (of qemu) with
>> support for DAX, and maybe I could try to reproduce the exception path
>> by configuring limited DAX window and heavy IO workload.
> 
> That would be great. Please test it with really small DAX window size.
> Also put some pr_debug() statements to make sure you are hitting this
> particular path during testing.

Got it. Thanks.

-- 
Thanks,
Jingbo

Re: [PATCH] fuse: fix return value of inode_inline_reclaim_one_dmap in error path

[Jingbo Xu]

On 6/1/23 7:45 PM, Vivek Goyal wrote:
> On Thu, Jun 01, 2023 at 09:45:52AM +0800, Jingbo Xu wrote:
>>
>>
>> On 6/1/23 4:03 AM, Vivek Goyal wrote:
>>> On Mon, Apr 24, 2023 at 08:32:50PM +0800, Jingbo Xu wrote:
>>>> When range already got reclaimed by somebody else, return NULL so that
>>>> the caller could retry to allocate or reclaim another range, instead of
>>>> mistakenly returning the range already got reclaimed and reused by
>>>> others.
>>>>
>>>> Reported-by: Liu Jiang <gerry@linux.alibaba.com>
>>>> Fixes: 9a752d18c85a ("virtiofs: add logic to free up a memory range")
>>>> Signed-off-by: Jingbo Xu <jefflexu@linux.alibaba.com>
>>>
>>> Hi Jingbo,
>>>
>>> This patch looks correct to me.
>>>
>>> Are you able to reproduce the problem? Or you are fixing it based on
>>> code inspection?
>>
>> It's spotted by Liu Jiang during code review.  Not tested yet.
>>
>>>
>>> How are you testing this? We don't have virtiofsd DAX implementation yet
>>> in rust virtiofsd yet. 
>>>
>>> I am not sure how to test this chagne now. We had out of tree patches
>>> in qemu and now qemu has gotten rid of C version of virtiofsd so these
>>> patches might not even work now.
>>
>> Yeah this exception path may not be so easy to be tested as it is only
>> triggered in the race condition.  I have the old branch (of qemu) with
>> support for DAX, and maybe I could try to reproduce the exception path
>> by configuring limited DAX window and heavy IO workload.
> 
> That would be great. Please test it with really small DAX window size.
> Also put some pr_debug() statements to make sure you are hitting this
> particular path during testing.

I tried to reproduce it but failed.  It seems the race is impossible
theoretically.

In theory, the race occurs when a freeable dmap is found in inode's
interval tree but found it is removed from the interval tree in the
second query.

However the above procedure is protected with
filemap_invalidate_lock(inode->i_mapping) held in
inode_inline_reclaim_one_dmap().  Given the dmap deletion operations
from inode's interval tree are all protected with
filemap_invalidate_lock(inode->i_mapping) held, e.g. inside
inode_inline_reclaim_one_dmap() and lookup_and_reclaim_dmap(), the above
race seems impossible then.

-- 
Thanks,
Jingbo