Re: [PATCH v6 08/26] fs/dax: Remove PAGE_MAPPING_DAX_SHARED mapping flag

[Dan Williams <dan.j.williams@intel.com>]

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.

> 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@nvidia.com>
> ---
>  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.

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'?

>  	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/

>   */
>  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?

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.