Re: [PATCH v4 6/6] fuse: remove tmp folio for writebacks and internal rb tree

[Jingbo Xu <jefflexu@linux.alibaba.com>]

On 11/12/24 5:30 AM, Joanne Koong wrote:
> On Mon, Nov 11, 2024 at 12:32 AM Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>>
>> Hi, Joanne and Miklos,
>>
>> On 11/8/24 7:56 AM, Joanne Koong wrote:
>>> Currently, we allocate and copy data to a temporary folio when
>>> handling writeback in order to mitigate the following deadlock scenario
>>> that may arise if reclaim waits on writeback to complete:
>>> * single-threaded FUSE server is in the middle of handling a request
>>>   that needs a memory allocation
>>> * memory allocation triggers direct reclaim
>>> * direct reclaim waits on a folio under writeback
>>> * the FUSE server can't write back the folio since it's stuck in
>>>   direct reclaim
>>>
>>> To work around this, we allocate a temporary folio and copy over the
>>> original folio to the temporary folio so that writeback can be
>>> immediately cleared on the original folio. This additionally requires us
>>> to maintain an internal rb tree to keep track of writeback state on the
>>> temporary folios.
>>>
>>> A recent change prevents reclaim logic from waiting on writeback for
>>> folios whose mappings have the AS_WRITEBACK_MAY_BLOCK flag set in it.
>>> This commit sets AS_WRITEBACK_MAY_BLOCK on FUSE inode mappings (which
>>> will prevent FUSE folios from running into the reclaim deadlock described
>>> above) and removes the temporary folio + extra copying and the internal
>>> rb tree.
>>>
>>> fio benchmarks --
>>> (using averages observed from 10 runs, throwing away outliers)
>>>
>>> Setup:
>>> sudo mount -t tmpfs -o size=30G tmpfs ~/tmp_mount
>>>  ./libfuse/build/example/passthrough_ll -o writeback -o max_threads=4 -o source=~/tmp_mount ~/fuse_mount
>>>
>>> fio --name=writeback --ioengine=sync --rw=write --bs={1k,4k,1M} --size=2G
>>> --numjobs=2 --ramp_time=30 --group_reporting=1 --directory=/root/fuse_mount
>>>
>>>         bs =  1k          4k            1M
>>> Before  351 MiB/s     1818 MiB/s     1851 MiB/s
>>> After   341 MiB/s     2246 MiB/s     2685 MiB/s
>>> % diff        -3%          23%         45%
>>>
>>> Signed-off-by: Joanne Koong <joannelkoong@gmail.com>
>>
>>
>> IIUC this patch seems to break commit
>> 8b284dc47291daf72fe300e1138a2e7ed56f38ab ("fuse: writepages: handle same
>> page rewrites").
>>
> 
> Interesting!  My understanding was that we only needed that commit
> because we were clearing writeback on the original folio before
> writeback had actually finished.
> 
> Now that folio writeback state is accounted for normally (eg through
> writeback being set/cleared on the original folio), does the
> folio_wait_writeback() call we do in fuse_page_mkwrite() not mitigate
> this?

Yes, after inspecting the writeback logic more, it seems that the second
writeback won't be initiated if the first one has not completed yet, see

```
a_ops->writepages
  write_cache_pages
    writeback_iter
      writeback_get_folio
	folio_prepare_writeback
	  if folio_test_writeback(folio):
	    folio_wait_writeback(folio)
```

and thus it won't be an issue to remove the auxiliary list ;)

> 
>>> -     /*
>>> -      * Being under writeback is unlikely but possible.  For example direct
>>> -      * read to an mmaped fuse file will set the page dirty twice; once when
>>> -      * the pages are faulted with get_user_pages(), and then after the read
>>> -      * completed.
>>> -      */
>>
>> In short, the target scenario is like:
>>
>> ```
>> # open a fuse file and mmap
>> fd1 = open("fuse-file-path", ...)
>> uaddr = mmap(fd1, ...)
>>
>> # DIRECT read to the mmaped fuse file
>> fd2 = open("ext4-file-path", O_DIRECT, ...)
>> read(fd2, uaddr, ...)
>>     # get_user_pages() of uaddr, and triggers faultin
>>     # a_ops->dirty_folio() <--- mark PG_dirty
>>
>>     # when DIRECT IO completed:
>>     # a_ops->dirty_folio() <--- mark PG_dirty
> 
> If you have the direct io function call stack at hand, could you point
> me to the function where the direct io completion marks this folio as
> dirty?


FYI The full call stack is like:

```
# DIRECT read(2) to the mmaped fuse file
read(fd2, uaddr1, ...)
  f_ops->read_iter()
    (iomap-based ) iomap_dio_rw
      # for READ && user_backed_iter(iter):
        dio->flags |= IOMAP_DIO_DIRTY
      iomap_dio_iter
        iomap_dio_bio_iter
          # add user or kernel pages to a bio
          bio_iov_iter_get_pages
            ...
            pin_user_pages_fast(..., FOLL_WRITE, ...)
              # find corresponding vma of dest buffer (fuse page cache)
              # search page table (pet) to find corresponding page
              # if not fault yet, trigger explicit faultin:
                faultin_page(..., FOLL_WRITE, ...)
                  handle_mm_fault(..., FAULT_FLAG_WRITE)
                    handle_pte_fault
                      do_wp_page
                        (vma->vm_flags & VM_SHARED) wp_page_shared
			  ...
			  fault_dirty_shared_page
                            folio_mark_dirty
                              a_ops->dirty_folio(), i.e.,
filemap_dirty_folio()
				# set PG_dirty
				folio_test_set_dirty(folio)
				# set PAGECACHE_TAG_DIRTY
				__folio_mark_dirty


          # if dio->flags & IOMAP_DIO_DIRTY:
          bio_set_pages_dirty
            (for each dest page) folio_mark_dirty
               a_ops->dirty_folio(), i.e., filemap_dirty_folio()
                 # set PG_dirty
		 folio_test_set_dirty(folio)
                 # set PAGECACHE_TAG_DIRTY
		 __folio_mark_dirty
```


> 
>> ```
>>
>> The auxiliary write request list was introduced to fix this.
>>
>> I'm not sure if there's an alternative other than the auxiliary list to
>> fix it, e.g. calling folio_wait_writeback() in a_ops->dirty_folio() so
>> that the same folio won't get dirtied when the writeback has not
>> completed yet?
>>
> 
> I'm curious how other filesystems solve for this - this seems like a
> generic situation other filesystems would run into as well.
> 

As mentioned above, the writeback path will prevent the duplicate
writeback request on the same page when the first writeback IO has not
completed yet.

Sorry for the noise...

-- 
Thanks,
Jingbo