[PATCH] fuse: avoid unnecessary spinlock bump

[PATCH] fuse: avoid unnecessary spinlock bump

[Jeffle Xu]

Move dmap free worker kicker inside the critical region, so that extra
spinlock lock/unlock could be avoided.

Suggested-by: Liu Jiang <gerry@linux.alibaba.com>
Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
---
 fs/fuse/dax.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
index d7d3a7f06862..b9f8795d52c4 100644
--- a/fs/fuse/dax.c
+++ b/fs/fuse/dax.c
@@ -138,9 +138,9 @@ static struct fuse_dax_mapping *alloc_dax_mapping(struct fuse_conn_dax *fcd)
 		WARN_ON(fcd->nr_free_ranges <= 0);
 		fcd->nr_free_ranges--;
 	}
+	__kick_dmap_free_worker(fcd, 0);
 	spin_unlock(&fcd->lock);
 
-	kick_dmap_free_worker(fcd, 0);
 	return dmap;
 }
 
-- 
2.27.0

Re: [PATCH] fuse: avoid unnecessary spinlock bump

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 426 bytes --]

On Sat, Apr 02, 2022 at 06:32:50PM +0800, Jeffle Xu wrote:
> Move dmap free worker kicker inside the critical region, so that extra
> spinlock lock/unlock could be avoided.
> 
> Suggested-by: Liu Jiang <gerry@linux.alibaba.com>
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> ---
>  fs/fuse/dax.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] fuse: avoid unnecessary spinlock bump

[Vivek Goyal]

On Sat, Apr 02, 2022 at 06:32:50PM +0800, Jeffle Xu wrote:
> Move dmap free worker kicker inside the critical region, so that extra
> spinlock lock/unlock could be avoided.
> 
> Suggested-by: Liu Jiang <gerry@linux.alibaba.com>
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>

Looks good to me. Have you done any testing to make sure nothing is
broken.

Reviewed-by: Vivek Goyal <vgoyal@redhat.com>

Vivek

> ---
>  fs/fuse/dax.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/fuse/dax.c b/fs/fuse/dax.c
> index d7d3a7f06862..b9f8795d52c4 100644
> --- a/fs/fuse/dax.c
> +++ b/fs/fuse/dax.c
> @@ -138,9 +138,9 @@ static struct fuse_dax_mapping *alloc_dax_mapping(struct fuse_conn_dax *fcd)
>  		WARN_ON(fcd->nr_free_ranges <= 0);
>  		fcd->nr_free_ranges--;
>  	}
> +	__kick_dmap_free_worker(fcd, 0);
>  	spin_unlock(&fcd->lock);
>  
> -	kick_dmap_free_worker(fcd, 0);
>  	return dmap;
>  }
>  
> -- 
> 2.27.0
> 

Re: [PATCH] fuse: avoid unnecessary spinlock bump

[JeffleXu]

On 4/7/22 10:10 PM, Vivek Goyal wrote:
> On Sat, Apr 02, 2022 at 06:32:50PM +0800, Jeffle Xu wrote:
>> Move dmap free worker kicker inside the critical region, so that extra
>> spinlock lock/unlock could be avoided.
>>
>> Suggested-by: Liu Jiang <gerry@linux.alibaba.com>
>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> 
> Looks good to me. Have you done any testing to make sure nothing is
> broken.

xfstests -g quick shows no regression. The tested virtiofs is mounted
with "dax=always".

-- 
Thanks,
Jeffle

Re: [PATCH] fuse: avoid unnecessary spinlock bump

[Vivek Goyal]

On Fri, Apr 08, 2022 at 10:36:40AM +0800, JeffleXu wrote:
> 
> 
> On 4/7/22 10:10 PM, Vivek Goyal wrote:
> > On Sat, Apr 02, 2022 at 06:32:50PM +0800, Jeffle Xu wrote:
> >> Move dmap free worker kicker inside the critical region, so that extra
> >> spinlock lock/unlock could be avoided.
> >>
> >> Suggested-by: Liu Jiang <gerry@linux.alibaba.com>
> >> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> > 
> > Looks good to me. Have you done any testing to make sure nothing is
> > broken.
> 
> xfstests -g quick shows no regression. The tested virtiofs is mounted
> with "dax=always".

I think xfstests might not trigger reclaim. You probably will have to
run something like blogbench with a small dax window like 1G so that
heavy reclaim happens.

For fun, I sometimes used to run it with a window of just say 16 dax
ranges so that reclaim was so heavy that if there was a bug, it will
show up.

Thanks
Vivek

Re: [PATCH] fuse: avoid unnecessary spinlock bump

[JeffleXu]

On 4/8/22 7:25 PM, Vivek Goyal wrote:
> On Fri, Apr 08, 2022 at 10:36:40AM +0800, JeffleXu wrote:
>>
>>
>> On 4/7/22 10:10 PM, Vivek Goyal wrote:
>>> On Sat, Apr 02, 2022 at 06:32:50PM +0800, Jeffle Xu wrote:
>>>> Move dmap free worker kicker inside the critical region, so that extra
>>>> spinlock lock/unlock could be avoided.
>>>>
>>>> Suggested-by: Liu Jiang <gerry@linux.alibaba.com>
>>>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
>>>
>>> Looks good to me. Have you done any testing to make sure nothing is
>>> broken.
>>
>> xfstests -g quick shows no regression. The tested virtiofs is mounted
>> with "dax=always".
> 
> I think xfstests might not trigger reclaim. You probably will have to
> run something like blogbench with a small dax window like 1G so that
> heavy reclaim happens.


Actually, I configured the DAX window to 8MB, i.e. 4 slots when running
xfstests. Thus I think the reclaim path is most likely triggered.


> 
> For fun, I sometimes used to run it with a window of just say 16 dax
> ranges so that reclaim was so heavy that if there was a bug, it will
> show up.
> 

Yeah, my colleague had ever reported that a DAX window of 4KB will cause
hang in our internal OS (which is 4.19, we back ported virtiofs to
4.19). But then I found that this issue doesn't exist in the latest
upstream. The reason seems that in the upstream kernel,
devm_memremap_pages() called in virtio_fs_setup_dax() will fail directly
since the dax window (4KB) is not aligned with the sparse memory section.

-- 
Thanks,
Jeffle

Re: [PATCH] fuse: avoid unnecessary spinlock bump

[Vivek Goyal]

On Fri, Apr 08, 2022 at 07:50:55PM +0800, JeffleXu wrote:
> 
> 
> On 4/8/22 7:25 PM, Vivek Goyal wrote:
> > On Fri, Apr 08, 2022 at 10:36:40AM +0800, JeffleXu wrote:
> >>
> >>
> >> On 4/7/22 10:10 PM, Vivek Goyal wrote:
> >>> On Sat, Apr 02, 2022 at 06:32:50PM +0800, Jeffle Xu wrote:
> >>>> Move dmap free worker kicker inside the critical region, so that extra
> >>>> spinlock lock/unlock could be avoided.
> >>>>
> >>>> Suggested-by: Liu Jiang <gerry@linux.alibaba.com>
> >>>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> >>>
> >>> Looks good to me. Have you done any testing to make sure nothing is
> >>> broken.
> >>
> >> xfstests -g quick shows no regression. The tested virtiofs is mounted
> >> with "dax=always".
> > 
> > I think xfstests might not trigger reclaim. You probably will have to
> > run something like blogbench with a small dax window like 1G so that
> > heavy reclaim happens.
> 
> 
> Actually, I configured the DAX window to 8MB, i.e. 4 slots when running
> xfstests. Thus I think the reclaim path is most likely triggered.
> 
> 
> > 
> > For fun, I sometimes used to run it with a window of just say 16 dax
> > ranges so that reclaim was so heavy that if there was a bug, it will
> > show up.
> > 
> 
> Yeah, my colleague had ever reported that a DAX window of 4KB will cause
> hang in our internal OS (which is 4.19, we back ported virtiofs to
> 4.19). But then I found that this issue doesn't exist in the latest
> upstream. The reason seems that in the upstream kernel,
> devm_memremap_pages() called in virtio_fs_setup_dax() will fail directly
> since the dax window (4KB) is not aligned with the sparse memory section.

Given our default chunk size is 2MB (FUSE_DAX_SHIFT), may be it is not
a bad idea to enforce some minimum cache window size. IIRC, even one
range is not enough. Minimum 2 are required for reclaim to not deadlock.

Hence, I guess it is not a bad idea to check for cache window size and
if it is too small, reject it and disable dax.

Thanks
Vivek

Re: [PATCH] fuse: avoid unnecessary spinlock bump

[JeffleXu]

On 4/8/22 8:06 PM, Vivek Goyal wrote:
> On Fri, Apr 08, 2022 at 07:50:55PM +0800, JeffleXu wrote:
>>
>>
>> On 4/8/22 7:25 PM, Vivek Goyal wrote:
>>> On Fri, Apr 08, 2022 at 10:36:40AM +0800, JeffleXu wrote:
>>>>
>>>>
>>>> On 4/7/22 10:10 PM, Vivek Goyal wrote:
>>>>> On Sat, Apr 02, 2022 at 06:32:50PM +0800, Jeffle Xu wrote:
>>>>>> Move dmap free worker kicker inside the critical region, so that extra
>>>>>> spinlock lock/unlock could be avoided.
>>>>>>
>>>>>> Suggested-by: Liu Jiang <gerry@linux.alibaba.com>
>>>>>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
>>>>>
>>>>> Looks good to me. Have you done any testing to make sure nothing is
>>>>> broken.
>>>>
>>>> xfstests -g quick shows no regression. The tested virtiofs is mounted
>>>> with "dax=always".
>>>
>>> I think xfstests might not trigger reclaim. You probably will have to
>>> run something like blogbench with a small dax window like 1G so that
>>> heavy reclaim happens.
>>
>>
>> Actually, I configured the DAX window to 8MB, i.e. 4 slots when running
>> xfstests. Thus I think the reclaim path is most likely triggered.
>>
>>
>>>
>>> For fun, I sometimes used to run it with a window of just say 16 dax
>>> ranges so that reclaim was so heavy that if there was a bug, it will
>>> show up.
>>>
>>
>> Yeah, my colleague had ever reported that a DAX window of 4KB will cause
>> hang in our internal OS (which is 4.19, we back ported virtiofs to
>> 4.19). But then I found that this issue doesn't exist in the latest
>> upstream. The reason seems that in the upstream kernel,
>> devm_memremap_pages() called in virtio_fs_setup_dax() will fail directly
>> since the dax window (4KB) is not aligned with the sparse memory section.
> 
> Given our default chunk size is 2MB (FUSE_DAX_SHIFT), may be it is not
> a bad idea to enforce some minimum cache window size. IIRC, even one
> range is not enough. Minimum 2 are required for reclaim to not deadlock.

Curiously, why minimum 1 range is not adequate? In which case minimum 2
are required?


-- 
Thanks,
Jeffle

Re: [PATCH] fuse: avoid unnecessary spinlock bump

[Vivek Goyal]

On Mon, Apr 11, 2022 at 10:10:23AM +0800, JeffleXu wrote:
> 
> 
> On 4/8/22 8:06 PM, Vivek Goyal wrote:
> > On Fri, Apr 08, 2022 at 07:50:55PM +0800, JeffleXu wrote:
> >>
> >>
> >> On 4/8/22 7:25 PM, Vivek Goyal wrote:
> >>> On Fri, Apr 08, 2022 at 10:36:40AM +0800, JeffleXu wrote:
> >>>>
> >>>>
> >>>> On 4/7/22 10:10 PM, Vivek Goyal wrote:
> >>>>> On Sat, Apr 02, 2022 at 06:32:50PM +0800, Jeffle Xu wrote:
> >>>>>> Move dmap free worker kicker inside the critical region, so that extra
> >>>>>> spinlock lock/unlock could be avoided.
> >>>>>>
> >>>>>> Suggested-by: Liu Jiang <gerry@linux.alibaba.com>
> >>>>>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
> >>>>>
> >>>>> Looks good to me. Have you done any testing to make sure nothing is
> >>>>> broken.
> >>>>
> >>>> xfstests -g quick shows no regression. The tested virtiofs is mounted
> >>>> with "dax=always".
> >>>
> >>> I think xfstests might not trigger reclaim. You probably will have to
> >>> run something like blogbench with a small dax window like 1G so that
> >>> heavy reclaim happens.
> >>
> >>
> >> Actually, I configured the DAX window to 8MB, i.e. 4 slots when running
> >> xfstests. Thus I think the reclaim path is most likely triggered.
> >>
> >>
> >>>
> >>> For fun, I sometimes used to run it with a window of just say 16 dax
> >>> ranges so that reclaim was so heavy that if there was a bug, it will
> >>> show up.
> >>>
> >>
> >> Yeah, my colleague had ever reported that a DAX window of 4KB will cause
> >> hang in our internal OS (which is 4.19, we back ported virtiofs to
> >> 4.19). But then I found that this issue doesn't exist in the latest
> >> upstream. The reason seems that in the upstream kernel,
> >> devm_memremap_pages() called in virtio_fs_setup_dax() will fail directly
> >> since the dax window (4KB) is not aligned with the sparse memory section.
> > 
> > Given our default chunk size is 2MB (FUSE_DAX_SHIFT), may be it is not
> > a bad idea to enforce some minimum cache window size. IIRC, even one
> > range is not enough. Minimum 2 are required for reclaim to not deadlock.
> 
> Curiously, why minimum 1 range is not adequate? In which case minimum 2
> are required?

Frankly speaking, right now I don't remember. I have vague memories
of concluding in the past that 1 range is not sufficient. But if you
like dive deeper, and try with one range and see if you can introduce
deadlock.

Thanks
Vivek

Re: [PATCH] fuse: avoid unnecessary spinlock bump

[JeffleXu]

On 4/11/22 7:52 PM, Vivek Goyal wrote:
> On Mon, Apr 11, 2022 at 10:10:23AM +0800, JeffleXu wrote:
>>
>>
>> On 4/8/22 8:06 PM, Vivek Goyal wrote:
>>> On Fri, Apr 08, 2022 at 07:50:55PM +0800, JeffleXu wrote:
>>>>
>>>>
>>>> On 4/8/22 7:25 PM, Vivek Goyal wrote:
>>>>> On Fri, Apr 08, 2022 at 10:36:40AM +0800, JeffleXu wrote:
>>>>>>
>>>>>>
>>>>>> On 4/7/22 10:10 PM, Vivek Goyal wrote:
>>>>>>> On Sat, Apr 02, 2022 at 06:32:50PM +0800, Jeffle Xu wrote:
>>>>>>>> Move dmap free worker kicker inside the critical region, so that extra
>>>>>>>> spinlock lock/unlock could be avoided.
>>>>>>>>
>>>>>>>> Suggested-by: Liu Jiang <gerry@linux.alibaba.com>
>>>>>>>> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>
>>>>>>>
>>>>>>> Looks good to me. Have you done any testing to make sure nothing is
>>>>>>> broken.
>>>>>>
>>>>>> xfstests -g quick shows no regression. The tested virtiofs is mounted
>>>>>> with "dax=always".
>>>>>
>>>>> I think xfstests might not trigger reclaim. You probably will have to
>>>>> run something like blogbench with a small dax window like 1G so that
>>>>> heavy reclaim happens.
>>>>
>>>>
>>>> Actually, I configured the DAX window to 8MB, i.e. 4 slots when running
>>>> xfstests. Thus I think the reclaim path is most likely triggered.
>>>>
>>>>
>>>>>
>>>>> For fun, I sometimes used to run it with a window of just say 16 dax
>>>>> ranges so that reclaim was so heavy that if there was a bug, it will
>>>>> show up.
>>>>>
>>>>
>>>> Yeah, my colleague had ever reported that a DAX window of 4KB will cause
>>>> hang in our internal OS (which is 4.19, we back ported virtiofs to
>>>> 4.19). But then I found that this issue doesn't exist in the latest
>>>> upstream. The reason seems that in the upstream kernel,
>>>> devm_memremap_pages() called in virtio_fs_setup_dax() will fail directly
>>>> since the dax window (4KB) is not aligned with the sparse memory section.
>>>
>>> Given our default chunk size is 2MB (FUSE_DAX_SHIFT), may be it is not
>>> a bad idea to enforce some minimum cache window size. IIRC, even one
>>> range is not enough. Minimum 2 are required for reclaim to not deadlock.
>>
>> Curiously, why minimum 1 range is not adequate? In which case minimum 2
>> are required?
> 
> Frankly speaking, right now I don't remember. I have vague memories
> of concluding in the past that 1 range is not sufficient. But if you
> like dive deeper, and try with one range and see if you can introduce
> deadlock.
> 

Alright, thanks.

-- 
Thanks,
Jeffle

Re: [PATCH] fuse: avoid unnecessary spinlock bump

[Bernd Schubert]

On 4/11/22 13:54, JeffleXu wrote:
> 
> 
> On 4/11/22 7:52 PM, Vivek Goyal wrote:
>> On Mon, Apr 11, 2022 at 10:10:23AM +0800, JeffleXu wrote:
>>>
>>>
>>> On 4/8/22 8:06 PM, Vivek Goyal wrote:
>>> Curiously, why minimum 1 range is not adequate? In which case minimum 2
>>> are required?
>>
>> Frankly speaking, right now I don't remember. I have vague memories
>> of concluding in the past that 1 range is not sufficient. But if you
>> like dive deeper, and try with one range and see if you can introduce
>> deadlock.
>>
> 
> Alright, thanks.
> 


Out of interest, how are you testing this at all? A patch from 
Dharmendra had been merged last week into libfuse to let it know about 
flags2, as we need that for our patches. But we didn't update the FLAGS 
yet to add in DAX on the libfuse side.

Is this used by virtio fs? Or is there another libfuse out there that 
should know about these flags (I think glusterfs has its own, but 
probably not using dax?).

Also, testing is always good, although I don't see how Jeffs patch would 
be able break anything here.



Thanks,
Bernd

Re: [PATCH] fuse: avoid unnecessary spinlock bump

[Vivek Goyal]

On Mon, Apr 11, 2022 at 03:20:05PM +0200, Bernd Schubert wrote:
> 
> 
> On 4/11/22 13:54, JeffleXu wrote:
> > 
> > 
> > On 4/11/22 7:52 PM, Vivek Goyal wrote:
> > > On Mon, Apr 11, 2022 at 10:10:23AM +0800, JeffleXu wrote:
> > > > 
> > > > 
> > > > On 4/8/22 8:06 PM, Vivek Goyal wrote:
> > > > Curiously, why minimum 1 range is not adequate? In which case minimum 2
> > > > are required?
> > > 
> > > Frankly speaking, right now I don't remember. I have vague memories
> > > of concluding in the past that 1 range is not sufficient. But if you
> > > like dive deeper, and try with one range and see if you can introduce
> > > deadlock.
> > > 
> > 
> > Alright, thanks.
> > 
> 
> 
> Out of interest, how are you testing this at all? A patch from Dharmendra
> had been merged last week into libfuse to let it know about flags2, as we
> need that for our patches. But we didn't update the FLAGS yet to add in DAX
> on the libfuse side.
> 
> Is this used by virtio fs?

Yes, idea is that this is used by virtiofs. Now looks like there are
multiple implementations of virtiofs daemon and they are either not
using libfuse or have forked off libfuse or created a new libfuse
equivalent in rust etc. So as fuse kernel gets updated, people are
updating their corresponding code as need be.

For example, we have C version of virtiofsd in qemu. That has taken
code from libfuse and built on top of it. BTW, C version of virtiofsd
is deprecated now and not lot of new development is expected to take
place there.

Then there is rust version of virtiofsd where new development is taking
place and which is replacement of C virtiofsd.

https://gitlab.com/virtio-fs/virtiofsd

This does not use libfuse at all.

And I think other folks (like developers from Alibaba) have probably 
written their own implementation of virtiofsd. I am not sure what
exactly are they using.

I see there is rust crate for fuse.

https://crates.io/crates/fuse

And there is one in cloud-hypervisor project.

https://github.com/cloud-hypervisor/fuse-backend-rs


> Or is there another libfuse out there that should
> know about these flags (I think glusterfs has its own, but probably not
> using dax?).
> 

So server side of fuse seem to be all fragmented to me. People have
written their own implementations based on their needs.

> Also, testing is always good, although I don't see how Jeffs patch would be
> able break anything here.

Agreed. I worry about testing constantly as well. qemu version of
virtiofsd does not have DAX support yet. Rust version DAX support is
also minimal. 

So for testing DAX, I have to rely on out of tree patches from qemu
here if any changes in virtiofs client happen.

https://gitlab.com/virtio-fs/qemu/-/tree/virtio-fs-dev

Jeffle is probably relying on their own virtiofsd implementation for DAX
testing.

Thanks
Vivek

Re: [PATCH] fuse: avoid unnecessary spinlock bump

[JeffleXu]

On 4/11/22 10:00 PM, Vivek Goyal wrote:
> On Mon, Apr 11, 2022 at 03:20:05PM +0200, Bernd Schubert wrote:
> 
> So for testing DAX, I have to rely on out of tree patches from qemu
> here if any changes in virtiofs client happen.
> 
> https://gitlab.com/virtio-fs/qemu/-/tree/virtio-fs-dev
> 
> Jeffle is probably relying on their own virtiofsd implementation for DAX
> testing.
> 

Actually I also use the C version virtiofsd in the above described
repository for testing :)

-- 
Thanks,
Jeffle

Re: [PATCH] fuse: avoid unnecessary spinlock bump

[Miklos Szeredi]

On Sat, 2 Apr 2022 at 12:32, Jeffle Xu <jefflexu@linux.alibaba.com> wrote:
>
> Move dmap free worker kicker inside the critical region, so that extra
> spinlock lock/unlock could be avoided.
>
> Suggested-by: Liu Jiang <gerry@linux.alibaba.com>
> Signed-off-by: Jeffle Xu <jefflexu@linux.alibaba.com>

Thanks, applied.

Miklos