On Tue, 3 Oct 2023, Jan Kara wrote: > On Fri 29-09-23 20:27:53, Hugh Dickins wrote: > > That Trinity livelock shmem_falloc avoidance block is unlikely, and a > > distraction from the proper business of shmem_fault(): separate it out. > > (This used to help compilers save stack on the fault path too, but both > > gcc and clang nowadays seem to make better choices anyway.) > > > > Signed-off-by: Hugh Dickins <hughd@xxxxxxxxxx> > > Looks good. Feel free to add: > > Reviewed-by: Jan Kara <jack@xxxxxxx> Thanks a lot for all these reviews, Jan. (And I particularly enjoyed your "Autumn cleaning" remark: sweeping up the leaves, I've been glad to have "Autumn Almanac" running through my head since reading that.) > > Looking at the code I'm just wondering whether the livelock with > shmem_undo_range() couldn't be more easy to avoid by making > shmem_undo_range() always advance the index by 1 after evicting a page and > thus guaranteeing a forward progress... Because the forward progress within > find_get_entries() is guaranteed these days, it should be enough. I'm not sure that I understand your "advance the index by 1" comment. Since the "/* If all gone or hole-punch or unfalloc, we're done */" change went in, I believe shmem_undo_range() does make guaranteed forward progress; but its forward progress is not enough. I would love to delete all that shmem_falloc_wait() strangeness; and your comment excited me to look, hey, can we just delete all that stuff now, instead of shifting it aside? That would be much nicer. And if I'd replied to you yesterday, I'd have been saying yes we can. But that's because I hadn't got far enough through re-reading the various July 2014 3.16-rc mail threads. I had been excited to find myself posting a revert of the patch; before reaching Sasha's later livelock which ended up with "belt and braces" retaining the shmem_falloc wait while adding the "If all gone or hole-punch" mod. https://marc.info/?l=linux-kernel&m=140487864819409&w=2 though that thread did not resolve, and morphed into lockdep moans. So I've reverted to my usual position: that it's conceivable that something has changed meanwhile, to make that Trinity livelock no longer an issue (in particular, i_mmap_mutex changed to i_mmap_rwsem, and much later unmap_mapping_range() changed to only take it for read: but though that change gives hope, I suspect it would turn out to be ineffectual against the livelock); but that would have to be proved. If there's someone who can re-demonstrate Sasha's Trinity livelock on 3.16-with-shmem-falloc-wait-disabled, or re-demonstrate it on any later release-with-shmem-falloc-wait-disabled, but demonstrate that the livelock does not occur on 6.6-rc-with-shmem-falloc-wait-disabled (or that plus some simple adjustment of hacker's choosing): that would be great news, and we could delete all this - though probably not without bisecting to satisfy ourselves on what was the crucial change. But I never got around to running up Trinity to work on it originally, nor in the years since, nor do I expect to soon. (Vlastimil had a good simple technique for demonstrating a part of the problem, but fixing that part turned out not fix the whole issue with Trinity.) Hugh
