On Mon, Jan 03, 2022 at 08:43:13PM -0800, Hugh Dickins wrote:
> On Tue, 4 Jan 2022, Matthew Wilcox wrote:
> > So let me try again. My concern was that we might be trying to store
> > a 2MB entry which had a non-NULL 'expected' entry which was found in a
> > 4k (ie single-index) slot within the 512 entries (as the first non-NULL
> > entry in that range), and we'd then store the 2MB entry into a
> > single-entry slot.
>
> Thanks, that sounds much more like how I was imagining it. And the
> two separate tests much more understandable than twice round a loop.
>
> > Now, maybe that can't happen for higher-level reasons, and I don't need
> > to worry about it. But I feel like we should check for that? Anyway,
> > I think the right fix is this:
>
> I don't object to (cheaply) excluding a possibility at the low level,
> even if there happen to be high level reasons why it cannot happen at
> present.
>
> But I don't think your second xas_find_conflict() gives quite as much
> assurance as you're expecting of it. Since xas_find_conflict() skips
> NULLs, the conflict test would pass if there were 511 NULLs and one
> 4k entry matching the expected entry, but a 2MB entry to be inserted
> (the "small and large folios" case in my earlier ramblings).
>
> I think xas_find_conflict() is not really up to giving that assurance;
> and maybe better than a second call to xas_find_conflict(), might be
> a VM_BUG_ON earlier, to say that if 'expected' is non-NULL, then the
> range is PAGE_SIZE - or something more folio-friendly like that?
> That would give you the extra assurance you're looking for,
> wouldn't it?
I actually don't need that assurance. The wretch who wrote the
documentation for xas_find_conflict() needs to be put on a diet of
bread and water, but the promise that it should make is that once it
returns NULL, the xa_state is restored to where it was before the first
call to xas_find_conflict(). So if you're trying to store a 2MB entry
and the only swap entry in the 2MB range is a 4KB entry, the xa_state
gets walked back up to point at the original 512-aligned entry and the
subsequent xas_store() will free the node containing the swap entry.
> For now and the known future, shmem only swaps PAGE_SIZE, out and in;
> maybe someone will want to change that one day, then xas_find_conflict()
> could be enhanced to know more of what's expected.
Good to know.
> >
> > +++ b/mm/shmem.c
> > @@ -733,11 +733,12 @@ static int shmem_add_to_page_cache(struct page *page,
> > cgroup_throttle_swaprate(page, gfp);
> >
> > do {
> > - void *entry;
> > xas_lock_irq(&xas);
> > - while ((entry = xas_find_conflict(&xas)) != NULL) {
> > - if (entry == expected)
> > - continue;
> > + if (expected != xas_find_conflict(&xas)) {
> > + xas_set_err(&xas, -EEXIST);
> > + goto unlock;
> > + }
> > + if (expected && xas_find_conflict(&xas)) {
> > xas_set_err(&xas, -EEXIST);
> > goto unlock;
> > }
>
> That also worried me because, if the second xas_find_conflict()
> is to make any sense, the first must have had a side-effect on xas:
> are those side-effects okay for the subsequent xas_store(&xas, page)?
> You'll know that they are, but it's not obvious to the reader.
>
> >
> > which says what I mean. I certainly didn't intend to imply that I
> > was expecting to see 512 consecutive entries which were all identical,
> > which would be the idiomatic way to read the code that was there before.
> > I shouldn't've tried to be so concise.
> >
> > (If you'd rather I write any of this differently, I'm more than happy
> > to change it)
>
> No, I'm happy with the style of it, just discontented that the second
> xas_find_conflict() pretends to more than it provides (I think).
>
> Hugh
