Re: [PATCH next 3/3] shmem: Fix "Unused swap" messages

[Hugh Dickins <hughd@google.com>]

On Mon, 3 Jan 2022, Matthew Wilcox wrote:
> On Sun, Jan 02, 2022 at 05:35:50PM -0800, Hugh Dickins wrote:
> > shmem_swapin_page()'s swap_free() has occasionally been generating
> > "_swap_info_get: Unused swap offset entry" messages.  Usually that's
> > no worse than noise; but perhaps it indicates a worse case, when we
> > might there be freeing swap already reused by others.
> > 
> > The multi-index xas_find_conflict() loop in shmem_add_to_page_cache()
> > did not allow for entry found NULL when expected to be non-NULL, so did
> > not catch that race when the swap has already been freed.
> > 
> > The loop would not actually catch a realistic conflict which the single
> > check does not catch, so revert it back to the single check.
> 
> I think what led to the loop was concern for the xa_state if trying
> to find a swap entry that's smaller than the size of the folio.
> So yes, the loop was expected to execute twice, but I didn't consider
> the case where we were looking for something non-NULL and actually found
> NULL.
> 
> So should we actually call xas_find_conflict() twice (if we're looking
> for something non-NULL), and check that we get @expected, followed by
> NULL?

Sorry, I've no idea.

You say "twice", and that does not fit the imaginary model I had when I
said "The loop would not actually catch a realistic conflict which the
single check does not catch".

I was imagining it either looking at a single entry, or looking at an
array of (perhaps sometimes in shmem's case 512) entries, looking for
conflict with the supplied pointer/value expected there.

The loop technique was already unable to report on unexpected NULLs,
and the single test would catch a non-NULL entry different from an
expected non-NULL entry.  Its only relative weakness appeared to be
if that array contained (perhaps some NULLs then) a "narrow" instance
of the same pointer/value that was expected to fill the array; and I
didn't see any possibility for shmem to be inserting small and large
folios sharing the same address at the same time.

That "explanation" may make no sense to you, don't worry about it;
just as "twice" makes no immediate sense to me - I'd have to go off
and study multi-index XArray to make sense of it, which I'm not
about to do.

I've seen no problems with the proposed patch, but if you see a real
case that it's failing to cover, yes, please do improve it of course.

Though now I'm wondering if the "loop" totally misled me; and your
"twice" just means that we need to test first this and then that and
we're done - yeah, maybe.

Hugh