Re: [PATCH 3/8] shmem: factor shmem_falloc_wait() out of shmem_fault()

[Hugh Dickins]

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