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

[Joanne Koong <joannelkoong@gmail.com>]

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

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

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.


Thanks,
Joanne
>
> Thanks,
> Bernd