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 18:02, Joanne Koong wrote:
> On Wed, Oct 30, 2024 at 9:21 AM Bernd Schubert
> <bernd.schubert@fastmail.fm> wrote:
>>
>> 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).
>>
> 
> Thanks for your comments/reviews, Bernd! I always appreciate them.
> 
>>>
>>> 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?
> 
> My understanding (and Shakeel please jump in here if this isn't right)
> is that tmp pages can be migrated/compacted. I think it's only pages
> marked as under writeback that are considered to be non-movable.
> 
>>
>> 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?
>>
7> 
> For the page migration / compaction paths, I don't think we can do the
> workaround we could do for sync where we skip waiting on writeback for
> fuse folios and continue on with the operation, because the migration
> / compaction paths operate on the pages. For the splice case, we
> assign the page to the pipebuffer (fuse_ref_page()), so if the
> migration/compaction happens on the page before the server has read
> this page from the pipebuffer, it'll be incorrect data or maybe crash
> the kernel.
> 
>>
>>
>>>
>>>>>>
>>>>>
>>>>> 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.

Hmm, if tmp pages can be compacted, isn't that a problem for splice?
I.e. I don't understand what the difference between tmp page and
write-back page for migration.


>>
> 
> Ah, for this the " /proc/sys/fs/fuse/writeback_optimization_timeout"
> would be a separate thing from the
> "/proc/sys/fs/fuse/max_request_timeout". The
> "/proc/sys/fs/fuse/writeback_optimization_timeout" would only apply
> for writeback requests. I was thinking implementation-wise, for
> writebacks we could just have a timer associated with each request
> (instead of having to grab locks with the fuse lists), since they
> won't be super common.

Ah, thank you! I had missed that this is another variable. Issue
with too short timeouts would probably be network hick-up that
would immediately kill fuse-server. I.e. if it just the missing
page compaction/migration, maybe larger time outs would be
acceptable.


Thanks,
Bernd