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

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

On 10/30/24 17:04, Joanne Koong wrote:
> On Wed, Oct 30, 2024 at 2:32 AM Bernd Schubert
> <bernd.schubert@fastmail.fm> wrote:
>>
>> On 10/28/24 22:58, Joanne Koong wrote:
>>> On Fri, Oct 25, 2024 at 3:40 PM Joanne Koong <joannelkoong@gmail.com> wrote:
>>>>
>>>>> Same here, I need to look some more into the compaction / page
>>>>> migration paths. I'm planning to do this early next week and will
>>>>> report back with what I find.
>>>>>
>>>>
>>>> These are my notes so far:
>>>>
>>>> * We hit the folio_wait_writeback() path when callers call
>>>> migrate_pages() with mode MIGRATE_SYNC
>>>>    ... -> migrate_pages() -> migrate_pages_sync() ->
>>>> migrate_pages_batch() -> migrate_folio_unmap() ->
>>>> folio_wait_writeback()
>>>>
>>>> * These are the places where we call migrate_pages():
>>>> 1) demote_folio_list()
>>>> Can ignore this. It calls migrate_pages() in MIGRATE_ASYNC mode
>>>>
>>>> 2) __damon_pa_migrate_folio_list()
>>>> Can ignore this. It calls migrate_pages() in MIGRATE_ASYNC mode
>>>>
>>>> 3) migrate_misplaced_folio()
>>>> Can ignore this. It calls migrate_pages() in MIGRATE_ASYNC mode
>>>>
>>>> 4) do_move_pages_to_node()
>>>> Can ignore this. This calls migrate_pages() in MIGRATE_SYNC mode but
>>>> this path is only invoked by the move_pages() syscall. It's fine to
>>>> wait on writeback for the move_pages() syscall since the user would
>>>> have to deliberately invoke this on the fuse server for this to apply
>>>> to the server's fuse folios
>>>>
>>>> 5)  migrate_to_node()
>>>> Can ignore this for the same reason as in 4. This path is only invoked
>>>> by the migrate_pages() syscall.
>>>>
>>>> 6) do_mbind()
>>>> Can ignore this for the same reason as 4 and 5. This path is only
>>>> invoked by the mbind() syscall.
>>>>
>>>> 7) soft_offline_in_use_page()
>>>> Can skip soft offlining fuse folios (eg folios with the
>>>> AS_NO_WRITEBACK_WAIT mapping flag set).
>>>> The path for this is soft_offline_page() -> soft_offline_in_use_page()
>>>> -> migrate_pages(). soft_offline_page() only invokes this for in-use
>>>> pages in a well-defined state (see ret value of get_hwpoison_page()).
>>>> My understanding of soft offlining pages is that it's a mitigation
>>>> strategy for handling pages that are experiencing errors but are not
>>>> yet completely unusable, and its main purpose is to prevent future
>>>> issues. It seems fine to skip this for fuse folios.
>>>>
>>>> 8) do_migrate_range()
>>>> 9) compact_zone()
>>>> 10) migrate_longterm_unpinnable_folios()
>>>> 11) __alloc_contig_migrate_range()
>>>>
>>>> 8 to 11 needs more investigation / thinking about. I don't see a good
>>>> way around these tbh. I think we have to operate under the assumption
>>>> that the fuse server running is malicious or benevolently but
>>>> incorrectly written and could possibly never complete writeback. So we
>>>> definitely can't wait on these but it also doesn't seem like we can
>>>> skip waiting on these, especially for the case where the server uses
>>>> spliced pages, nor does it seem like we can just fail these with
>>>> -EBUSY or something.
>>
>> I see some code paths with -EAGAIN in migration. Could you explain why
>> we can't just fail migration for fuse write-back pages?
>>

Hi Joanne,

thanks a lot for your quick reply (especially as my reviews come in very 
late).

> 
> My understanding (and please correct me here Shakeel if I'm wrong) is
> that this could block system optimizations, especially since if an
> unprivileged malicious fuse server never replies to the writeback
> request, then this completely stalls progress. In the best case
> scenario, -EAGAIN could be used because the server might just be slow
> in serving the writeback, but I think we need to also account for
> servers that never complete the writeback. For
> __alloc_contig_migrate_range() for example, my understanding is that
> this is used to migrate pages so that there are more physically
> contiguous ranges of memory freed up. If fuse writeback blocks that,
> then that hurts system health overall.

Hmm, I wonder what is worse - tmp page copies or missing compaction.
Especially if we expect a low range of in-writeback pages/folios. 
One could argue that an evil user might spawn many fuse server
processes to work around the default low fuse write-back limits, but
does that make any difference with tmp pages? And these cannot be
compacted either?

And with timeouts that would be so far totally uncritical, I
think.


You also mentioned 

> especially for the case where the server uses spliced pages

could you provide more details for that? 



> 
>>>>
>>>
>>> I'm still not seeing a good way around this.
>>>
>>> What about this then? We add a new fuse sysctl called something like
>>> "/proc/sys/fs/fuse/writeback_optimization_timeout" where if the sys
>>> admin sets this, then it opts into optimizing writeback to be as fast
>>> as possible (eg skipping the page copies) and if the server doesn't
>>> fulfill the writeback by the set timeout value, then the connection is
>>> aborted.
>>>
>>> Alternatively, we could also repurpose
>>> /proc/sys/fs/fuse/max_request_timeout from the request timeout
>>> patchset [1] but I like the additional flexibility and explicitness
>>> having the "writeback_optimization_timeout" sysctl gives.
>>>
>>> Any thoughts on this?
>>
>>
>> I'm a bit worried that we might lock up the system until time out is
>> reached - not ideal. Especially as timeouts are in minutes now. But
>> even a slightly stuttering video system not be great. I think we
>> should give users/admin the choice then, if they prefer slow page
>> copies or fast, but possibly shortly unresponsive system.
>>
> I was thinking the /proc/sys/fs/fuse/writeback_optimization_timeout
> would be in seconds, where the sys admin would probably set something
> more reasonable like 5 seconds or so.
> If this syctl value is set, then servers who want writebacks to be
> fast can opt into it at mount time (and by doing so agree that they
> will service writeback requests by the timeout or their connection
> will be aborted).


I think your current patch set has it in minutes? (Should be easy
enough to change that.) Though I'm more worried about the impact
of _frequent_ timeout scanning through the different fuse lists 
on performance, than about missing compaction for folios that are
currently in write-back.


Thanks,
Bernd