On Tue, May 02, 2023 at 07:13:49PM +0200, David Hildenbrand wrote:
> [...]
>
> > +{
> > + struct address_space *mapping;
> > +
> > + /*
> > + * GUP-fast disables IRQs - this prevents IPIs from causing page tables
> > + * to disappear from under us, as well as preventing RCU grace periods
> > + * from making progress (i.e. implying rcu_read_lock()).
> > + *
> > + * This means we can rely on the folio remaining stable for all
> > + * architectures, both those that set CONFIG_MMU_GATHER_RCU_TABLE_FREE
> > + * and those that do not.
> > + *
> > + * We get the added benefit that given inodes, and thus address_space,
> > + * objects are RCU freed, we can rely on the mapping remaining stable
> > + * here with no risk of a truncation or similar race.
> > + */
> > + lockdep_assert_irqs_disabled();
> > +
> > + /*
> > + * If no mapping can be found, this implies an anonymous or otherwise
> > + * non-file backed folio so in this instance we permit the pin.
> > + *
> > + * shmem and hugetlb mappings do not require dirty-tracking so we
> > + * explicitly whitelist these.
> > + *
> > + * Other non dirty-tracked folios will be picked up on the slow path.
> > + */
> > + mapping = folio_mapping(folio);
> > + return !mapping || shmem_mapping(mapping) || folio_test_hugetlb(folio);
>
> "Folios in the swap cache return the swap mapping" -- you might disallow
> pinning anonymous pages that are in the swap cache.
>
> I recall that there are corner cases where we can end up with an anon page
> that's mapped writable but still in the swap cache ... so you'd fallback to
> the GUP slow path (acceptable for these corner cases, I guess), however
> especially the comment is a bit misleading then.
>
> So I'd suggest not dropping the folio_test_anon() check, or open-coding it
> ... which will make this piece of code most certainly easier to get when
> staring at folio_mapping(). Or to spell it out in the comment (usually I
> prefer code over comments).
So how stable is folio->mapping at this point? Can two subsequent reads
get different values? (eg. an actual mapping and NULL)
If so, folio_mapping() itself seems to be missing a READ_ONCE() to avoid
the compiler from emitting the load multiple times.
