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 <apop...@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.
