| Message ID | 20230424123250.125404-1-jefflexu@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | fuse: fix return value of inode_inline_reclaim_one_dmap in error path | expand |
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;
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.
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
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.
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.
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;
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(+)