On Mon, Jan 13, 2025 at 04:52:34PM -0800, Dan Williams wrote:
> Alistair Popple wrote:
> > PAGE_MAPPING_DAX_SHARED is the same as PAGE_MAPPING_ANON.
>
> I think a bit a bit more detail is warranted, how about?
>
> The page ->mapping pointer can have magic values like
> PAGE_MAPPING_DAX_SHARED and PAGE_MAPPING_ANON for page owner specific
> usage. In fact, PAGE_MAPPING_DAX_SHARED and PAGE_MAPPING_ANON alias the
> same value.
Massaged it slightly but sounds good.
> > This isn't currently a problem because FS DAX pages are treated
> > specially.
>
> s/are treated specially/are never seen by the anonymous mapping code and
> vice versa/
>
> > However a future change will make FS DAX pages more like
> > normal pages, so folio_test_anon() must not return true for a FS DAX
> > page.
> >
> > We could explicitly test for a FS DAX page in folio_test_anon(),
> > etc. however the PAGE_MAPPING_DAX_SHARED flag isn't actually
> > needed. Instead we can use the page->mapping field to implicitly track
> > the first mapping of a page. If page->mapping is non-NULL it implies
> > the page is associated with a single mapping at page->index. If the
> > page is associated with a second mapping clear page->mapping and set
> > page->share to 1.
> >
> > This is possible because a shared mapping implies the file-system
> > implements dax_holder_operations which makes the ->mapping and
> > ->index, which is a union with ->share, unused.
> >
> > The page is considered shared when page->mapping == NULL and
> > page->share > 0 or page->mapping != NULL, implying it is present in at
> > least one address space. This also makes it easier for a future change
> > to detect when a page is first mapped into an address space which
> > requires special handling.
> >
> > Signed-off-by: Alistair Popple <apopple@xxxxxxxxxx>
> > ---
> > fs/dax.c | 45 +++++++++++++++++++++++++--------------
> > include/linux/page-flags.h | 6 +-----
> > 2 files changed, 29 insertions(+), 22 deletions(-)
> >
> > diff --git a/fs/dax.c b/fs/dax.c
> > index 4e49cc4..d35dbe1 100644
> > --- a/fs/dax.c
> > +++ b/fs/dax.c
> > @@ -351,38 +351,41 @@ static unsigned long dax_end_pfn(void *entry)
> > for (pfn = dax_to_pfn(entry); \
> > pfn < dax_end_pfn(entry); pfn++)
> >
> > +/*
> > + * A DAX page is considered shared if it has no mapping set and ->share (which
> > + * shares the ->index field) is non-zero. Note this may return false even if the
> > + * page is shared between multiple files but has not yet actually been mapped
> > + * into multiple address spaces.
> > + */
> > static inline bool dax_page_is_shared(struct page *page)
> > {
> > - return page->mapping == PAGE_MAPPING_DAX_SHARED;
> > + return !page->mapping && page->share;
> > }
> >
> > /*
> > - * Set the page->mapping with PAGE_MAPPING_DAX_SHARED flag, increase the
> > - * refcount.
> > + * Increase the page share refcount, warning if the page is not marked as shared.
> > */
> > static inline void dax_page_share_get(struct page *page)
> > {
> > - if (page->mapping != PAGE_MAPPING_DAX_SHARED) {
> > - /*
> > - * Reset the index if the page was already mapped
> > - * regularly before.
> > - */
> > - if (page->mapping)
> > - page->share = 1;
> > - page->mapping = PAGE_MAPPING_DAX_SHARED;
> > - }
> > + WARN_ON_ONCE(!page->share);
> > + WARN_ON_ONCE(page->mapping);
>
> Given the only caller of this function is dax_associate_entry() it seems
> like overkill to check that a function only a few lines away manipulated
> ->mapping correctly.
Good call.
> I don't see much reason for dax_page_share_get() to exist after your
> changes.
>
> Perhaps all that is needed is a dax_make_shared() helper that does the
> initial fiddling of '->mapping = NULL' and '->share = 1'?
Ok. I was going to make the argument that dax_make_shared() was overkill as
well, but as noted below it's a good place to put the comment describing how
this all works so have done that.
> > page->share++;
> > }
> >
> > static inline unsigned long dax_page_share_put(struct page *page)
> > {
> > + WARN_ON_ONCE(!page->share);
> > return --page->share;
> > }
> >
> > /*
> > - * When it is called in dax_insert_entry(), the shared flag will indicate that
> > - * whether this entry is shared by multiple files. If so, set the page->mapping
> > - * PAGE_MAPPING_DAX_SHARED, and use page->share as refcount.
> > + * When it is called in dax_insert_entry(), the shared flag will indicate
> > + * whether this entry is shared by multiple files. If the page has not
> > + * previously been associated with any mappings the ->mapping and ->index
> > + * fields will be set. If it has already been associated with a mapping
> > + * the mapping will be cleared and the share count set. It's then up to the
> > + * file-system to track which mappings contain which pages, ie. by implementing
> > + * dax_holder_operations.
>
> This feels like a good comment for a new dax_make_shared() not
> dax_associate_entry().
>
> I would also:
>
> s/up to the file-system to track which mappings contain which pages, ie. by implementing
> dax_holder_operations/up to reverse map users like memory_failure() to
> call back into the filesystem to recover ->mapping and ->index
> information/
Sounds good, although I left a reference to dax_holder_operations in the comment
because it's not immediately obvious how file-systems do this currently and I
had to relearn that more times than I'd care to admit :-)
> > */
> > static void dax_associate_entry(void *entry, struct address_space *mapping,
> > struct vm_area_struct *vma, unsigned long address, bool shared)
> > @@ -397,7 +400,17 @@ static void dax_associate_entry(void *entry, struct address_space *mapping,
> > for_each_mapped_pfn(entry, pfn) {
> > struct page *page = pfn_to_page(pfn);
> >
> > - if (shared) {
> > + if (shared && page->mapping && page->share) {
>
> How does this case happen? I don't think any page would ever enter with
> both ->mapping and ->share set, right?
Sigh. You're right - it can't. This patch series is getting a litte bit large
and unweildy with all the prerequisite bugfixes and cleanups. Obviously I fixed
this when developing the main fs dax count fixup but forgot to rebase the fix
further back in the series.
Anyway I have fixed that now, thanks.
> If the file was mapped then reflinked then ->share should be zero at the
> first mapping attempt. It might not be zero because it is aliased with
> index until it is converted to a shared page.
