Re: [PATCH RFC v3 17/17] fuse: {uring} Pin the user buffer

[Bernd Schubert <bernd.schubert@fastmail.fm>]

On 9/4/24 18:16, Jens Axboe wrote:
> On 9/4/24 10:08 AM, Bernd Schubert wrote:
>> Hi Jens,
>>
>> thanks for your help.
>>
>> On 9/4/24 17:47, Jens Axboe wrote:
>>> On 9/1/24 7:37 AM, Bernd Schubert wrote:
>>>> This is to allow copying into the buffer from the application
>>>> without the need to copy in ring context (and with that,
>>>> the need that the ring task is active in kernel space).
>>>>
>>>> Also absolutely needed for now to avoid this teardown issue
>>>
>>> I'm fine using these helpers, but they are absolutely not needed to
>>> avoid that teardown issue - well they may help because it's already
>>> mapped, but it's really the fault of your handler from attempting to map
>>> in user pages from when it's teardown/fallback task_work. If invoked and
>>> the ring is dying or not in the right task (as per the patch from
>>> Pavel), then just cleanup and return -ECANCELED.
>>
>> As I had posted on Friday/Saturday, it didn't work. I had added a 
>> debug pr_info into Pavels patch, somehow it didn't trigger on PF_EXITING 
>> and I didn't further debug it yet as I was working on the pin anyway.
>> And since Monday occupied with other work...
> 
> Then there's something wrong with that patch, as it definitely should
> work. How did you reproduce the teardown crash? I'll take a look here.

Thank you! In this specific case

1) Run passthrough_hp with --debug-fuse

2) dd if=/dev/zero of=/scratch/test/testfile bs=1M count=1

Then on the console that has passthrough_hp output and runs slow with my
ASAN/etc kernel: ctrl-z and kill -9 %
I guess a pkill -9 passthrough_hp should also work


But I can investigate later on myself what is the issue with PF_EXITING,
just not today and maybe not tomorrow either.

> 
> That said, it may indeed be the better approach to pin upfront. I just
> want to make sure it's not done as a bug fix for something that should
> not be happening.
> 
>> For this series it is needed to avoid kernel crashes. If we can can fix 
>> patch 15 and 16, the better. Although we will still later on need it as
>> optimization.
> 
> Yeah exactly, didn't see this before typing the above :-)
> 
>>>> +/*
>>>> + * Copy from memmap.c, should be exported
>>>> + */
>>>> +static void io_pages_free(struct page ***pages, int npages)
>>>> +{
>>>> +	struct page **page_array = *pages;
>>>> +
>>>> +	if (!page_array)
>>>> +		return;
>>>> +
>>>> +	unpin_user_pages(page_array, npages);
>>>> +	kvfree(page_array);
>>>> +	*pages = NULL;
>>>> +}
>>>
>>> I noticed this and the mapping helper being copied before seeing the
>>> comments - just export them from memmap.c and use those rather than
>>> copying in the code. Add that as a prep patch.
>>
>> No issue to do that either. The hard part is then to get it through
>> different branches. I had removed the big optimization of 
>> __wake_up_on_current_cpu in this series, because it needs another
>> export.
> 
> It's not that hard, just split it out in the next patch and I'll be
> happy to ack/review it so it can go in with the other patches rather
> than needing to go in separately.

Great thank you very much, will do!


Thanks,
Bernd