Date:   Wed, 9 Mar 2022 19:00:46 +0100
From:   David Hildenbrand <david@...hat.com>
To:     Matthew Wilcox <willy@...radead.org>
Cc:     linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Hugh Dickins <hughd@...gle.com>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        David Rientjes <rientjes@...gle.com>,
        Shakeel Butt <shakeelb@...gle.com>,
        John Hubbard <jhubbard@...dia.com>,
        Jason Gunthorpe <jgg@...dia.com>,
        Mike Kravetz <mike.kravetz@...cle.com>,
        Mike Rapoport <rppt@...ux.ibm.com>,
        Yang Shi <shy828301@...il.com>,
        "Kirill A . Shutemov" <kirill.shutemov@...ux.intel.com>,
        Vlastimil Babka <vbabka@...e.cz>, Jann Horn <jannh@...gle.com>,
        Michal Hocko <mhocko@...nel.org>,
        Nadav Amit <namit@...are.com>, Rik van Riel <riel@...riel.com>,
        Roman Gushchin <guro@...com>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Peter Xu <peterx@...hat.com>,
        Donald Dutile <ddutile@...hat.com>,
        Christoph Hellwig <hch@....de>,
        Oleg Nesterov <oleg@...hat.com>, Jan Kara <jack@...e.cz>,
        Liang Zhang <zhangliang5@...wei.com>,
        Pedro Gomes <pedrodemargomes@...il.com>,
        Oded Gabbay <oded.gabbay@...il.com>, linux-mm@...ck.org,
        Alexander Potapenko <glider@...gle.com>
Subject: Re: [PATCH v1 10/15] mm/page-flags: reuse PG_slab as
 PG_anon_exclusive for PageAnon() pages

>> It's making the semantics of PG_slab depend on another bit in the head
>> page. I agree, it's not perfect, but it's not *too* crazy. As raised in
>> the cover letter, not proud of this, but I didn't really find an
>> alternative for the time being.
>>
>>> requirement that SL[AOU]B doesn't use the bottom two bits of
>>
>> I think only the last bit (0x1)
> 
> Yeah, OK, they can use three of the four possible combinations of the
> bottom two bits ;-)  Still, it's yet more constraints on use of struct
> page, which aren't obvious (and are even upside down for the casual
> observer).

I don't disagree that such constraints are nasty.

Having that said, I'd really like to avoid overloading PG_slab
(especially, such that I don't have to mess with
scripts/crash/makedumpfile). So if we can reuse MappedToDisk, that would
be very nice.

> 
>>> I have plans to get rid of PageError and PagePrivate, but those are going
>>> to be too late for you.  I don't think mappedtodisk has meaning for anon
>>> pages, even if they're in the swapcache.  It would need PG_has_hwpoisoned
>>
>> Are you sure it's not used if the page is in the swapcache? I have no
>> detailed knowledge how file-back swap targets are handled in that
>> regard. So fs experience would be highly appreciated :)
> 
> I have two arguments here.  One is based on grep:
> 
> $ git grep mappedtodisk
> fs/proc/page.c: u |= kpf_copy_bit(k, KPF_MAPPEDTODISK,  PG_mappedtodisk);
> include/linux/page-flags.h:     PG_mappedtodisk,        /* Has blocks allocated on-disk */
> include/linux/page-flags.h:     PG_has_hwpoisoned = PG_mappedtodisk,
> include/linux/page-flags.h:PAGEFLAG(MappedToDisk, mappedtodisk, PF_NO_TAIL)
> include/trace/events/mmflags.h: {1UL << PG_mappedtodisk,        "mappedtodisk"  },              \
> include/trace/events/pagemap.h: (folio_test_mappedtodisk(folio) ? PAGEMAP_MAPPEDDISK : 0) | \
> mm/migrate.c:   if (folio_test_mappedtodisk(folio))
> mm/migrate.c:           folio_set_mappedtodisk(newfolio);
> mm/truncate.c:  folio_clear_mappedtodisk(folio);
> tools/vm/page-types.c:  [KPF_MAPPEDTODISK]      = "d:mappedtodisk",
> 
> $ git grep MappedToDisk
> fs/buffer.c:            SetPageMappedToDisk(page);
> fs/buffer.c:    if (PageMappedToDisk(page))
> fs/buffer.c:            SetPageMappedToDisk(page);
> fs/ext4/readpage.c:                     SetPageMappedToDisk(page);
> fs/f2fs/data.c:         SetPageMappedToDisk(page);
> fs/f2fs/file.c: if (PageMappedToDisk(page))
> fs/fuse/dev.c:  ClearPageMappedToDisk(newpage);
> fs/mpage.c:             SetPageMappedToDisk(page);
> fs/nilfs2/file.c:       if (PageMappedToDisk(page))
> fs/nilfs2/file.c:                       SetPageMappedToDisk(page);
> fs/nilfs2/page.c:       ClearPageMappedToDisk(page);
> fs/nilfs2/page.c:               SetPageMappedToDisk(dpage);
> fs/nilfs2/page.c:               ClearPageMappedToDisk(dpage);
> fs/nilfs2/page.c:       if (PageMappedToDisk(src) && !PageMappedToDisk(dst))
> fs/nilfs2/page.c:               SetPageMappedToDisk(dst);
> fs/nilfs2/page.c:       else if (!PageMappedToDisk(src) && PageMappedToDisk(dst))
> fs/nilfs2/page.c:               ClearPageMappedToDisk(dst);
> fs/nilfs2/page.c:       ClearPageMappedToDisk(page);
> include/linux/page-flags.h:PAGEFLAG(MappedToDisk, mappedtodisk, PF_NO_TAIL)
> 
> so you can see it's _rarely_ used, and only by specific filesystems.

Right, but I spot ext4 and fs/buffer.c core functionality. That
naturally makes me nervous :)

> 
> The swap code actually bypasses the filesystem (except for network
> filesystems) and submits BIOs directly (see __swap_writepage() and
> swap_readpage()).  There's no check for MappedToDisk, and nowhere to
> set it/clear it.
> 
> The second argument is that MappedToDisk is used for delayed allocation
> on writes for filesystems.  But swap is required to be allocated at
> swapfile setup (so that the swap code can bypass the filesystem ...)
> and so this flag is useless.

I have some faint memory that there are corner cases, but maybe
(hopefully) my memory is wrong.

> 
> Of course, I may have missed something.  This is complex.
> 

Yeah, that's why I was very careful. If any FS would end up setting that
flag we'd be in trouble and would have to blacklist that fs for swapping
or rework it (well, if we're even able to identify such a file system).

I think one sanity check I could add is making sure that
PageAnonExclusive() is never set when getting a page from the swapcache
in do_swap_page(). I think that would take care of the obvious bugs.

-- 
Thanks,

David / dhildenb

Powered by blists - more mailing lists