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

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

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...

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.



> 
>> +/*
>> + * 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.


> 
>> @@ -417,6 +437,7 @@ static int fuse_uring_out_header_has_err(struct fuse_out_header *oh,
>>  		goto seterr;
>>  	}
>>  
>> +	/* FIXME copied from dev.c, check what 512 means  */
>>  	if (oh->error <= -512 || oh->error > 0) {
>>  		err = -EINVAL;
>>  		goto seterr;
> 
> -512 is -ERESTARTSYS
> 

Ah thank you! I'm going to add separate patch for dev.c, as I wrote, this was
just a copy-and-paste.

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 592d0d96a106..779b23fa01c2 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -2028,7 +2028,7 @@ static ssize_t fuse_dev_do_write(struct fuse_dev *fud,
        }
 
        err = -EINVAL;
-       if (oh.error <= -512 || oh.error > 0)
+       if (oh.error <= -ERESTARTSYS || oh.error > 0)
                goto copy_finish;
 
        spin_lock(&fpq->lock);


Thanks,
Bernd