On Wed, Oct 30, 2024 at 10:27 AM Bernd Schubert <bernd.schubert@xxxxxxxxxxx> wrote: > > > > On 10/30/24 18:02, Joanne Koong wrote: > > On Wed, Oct 30, 2024 at 9:21 AM Bernd Schubert > > <bernd.schubert@xxxxxxxxxxx> wrote: > >> > >> On 10/30/24 17:04, Joanne Koong wrote: > >>> On Wed, Oct 30, 2024 at 2:32 AM Bernd Schubert > >>> <bernd.schubert@xxxxxxxxxxx> wrote: > >>>> > >>>> On 10/28/24 22:58, Joanne Koong wrote: > >>>>> On Fri, Oct 25, 2024 at 3:40 PM Joanne Koong <joannelkoong@xxxxxxxxx> 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. > That's a great question! I have no idea how compaction works for pages being used in splice. Shakeel, do you know the answer to this? Thanks, Joanne > > >> > > > > 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
