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

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

Hi Joanne,

Thanks for keeping pushing this forward.

On 11/1/24 5:52 AM, Joanne Koong wrote:
> On Thu, Oct 31, 2024 at 1:06 PM Shakeel Butt <shakeel.butt@linux.dev> wrote:
>>
>> On Thu, Oct 31, 2024 at 12:06:49PM GMT, Joanne Koong wrote:
>>> On Wed, Oct 30, 2024 at 5:30 PM Shakeel Butt <shakeel.butt@linux.dev> wrote:
>> [...]
>>>>
>>>> Memory pool is a bit confusing term here. Most probably you are asking
>>>> about the migrate type of the page block from which tmp page is
>>>> allocated from. In a normal system, tmp page would be allocated from page
>>>> block with MIGRATE_UNMOVABLE migrate type while the page cache page, it
>>>> depends on what gfp flag was used for its allocation. What does fuse fs
>>>> use? GFP_HIGHUSER_MOVABLE or something else? Under low memory situation
>>>> allocations can get mixed up with different migrate types.
>>>>
>>>
>>> I believe it's GFP_HIGHUSER_MOVABLE for the page cache pages since
>>> fuse doesn't set any additional gfp masks on the inode mapping.
>>>
>>> Could we just allocate the fuse writeback pages with GFP_HIGHUSER
>>> instead of GFP_HIGHUSER_MOVABLE? That would be in fuse_write_begin()
>>> where we pass in the gfp mask to __filemap_get_folio(). I think this
>>> would give us the same behavior memory-wise as what the tmp pages
>>> currently do,
>>
>> I don't think it would be the same behavior. From what I understand the
>> liftime of the tmp page is from the start of the writeback till the ack
>> from the fuse server that writeback is done. While the lifetime of the
>> page of the page cache can be arbitrarily large. We should just make it
>> unmovable for its lifetime. I think it is fine to make the page
>> unmovable during the writeback. We should not try to optimize for the
>> bad or buggy behavior of fuse server.
>>
>> Regarding the avoidance of wait on writeback for fuse folios, I think we
>> can handle the migration similar to how you are handling reclaim and in
>> addition we can add a WARN() in folio_wait_writeback() if the kernel ever
>> sees a fuse folio in that function.
> 
> Awesome, this is what I'm planning to do in v3 to address migration then:
> 
> 1) in migrate_folio_unmap(), only call "folio_wait_writeback(src);" if
> src->mapping does not have the AS_NO_WRITEBACK_WAIT bit set on it (eg
> fuse folios will have that AS_NO_WRITEBACK_WAIT bit set)

I think it's generally okay to skip FUSE pages under writeback when the
sync migrate_pages() is called in low memory context, which only tries
to migrate as many pages as possible (i.e. best effort).

While more caution may be needed when the sync migrate_pages() is called
with an implicit hint that the migration can not fail.  For example,

```
offline_pages
	while {
		scan_movable_pages
		do_migrate_range
	}
```

If the malicious server never completes the writeback IO, no progress
will be made in the above while loop, and I'm afraid it will be a dead
loop then.


> 
> 2) in the fuse filesystem's implementation of the
> mapping->a_ops->migrate_folio callback, return -EAGAIN if the folio is
> under writeback.

Is there any possibility that a_ops->migrate_folio() may be called with
the folio under writeback?

- for most pages without AS_NO_WRITEBACK_WAIT, a_ops->migrate_folio()
will be called only when Page_writeback is cleared;
- for AS_NO_WRITEBACK_WAIT pages, they are skipped if they are under
writeback

-- 
Thanks,
Jingbo