Messages in this thread | | | Date | Wed, 16 Feb 2022 10:55:06 +0800 | Subject | Re: [PATCH v10 9/9] fsdax: set a CoW flag when associate reflink mappings | From | Shiyang Ruan <> |
| |
在 2022/2/16 10:09, Dan Williams 写道: > On Thu, Jan 27, 2022 at 4:41 AM Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote: >> >> Introduce a PAGE_MAPPING_DAX_COW flag to support association with CoW file >> mappings. In this case, the dax-RMAP already takes the responsibility >> to look up for shared files by given dax page. The page->mapping is no >> longer to used for rmap but for marking that this dax page is shared. >> And to make sure disassociation works fine, we use page->index as >> refcount, and clear page->mapping to the initial state when page->index >> is decreased to 0. >> >> With the help of this new flag, it is able to distinguish normal case >> and CoW case, and keep the warning in normal case. >> >> Signed-off-by: Shiyang Ruan <ruansy.fnst@fujitsu.com> >> --- >> fs/dax.c | 65 ++++++++++++++++++++++++++++++++------ >> include/linux/page-flags.h | 6 ++++ >> 2 files changed, 62 insertions(+), 9 deletions(-) >> >> diff --git a/fs/dax.c b/fs/dax.c >> index 250794a5b789..88879c579c1f 100644 >> --- a/fs/dax.c >> +++ b/fs/dax.c >> @@ -334,13 +334,46 @@ static unsigned long dax_end_pfn(void *entry) >> for (pfn = dax_to_pfn(entry); \ >> pfn < dax_end_pfn(entry); pfn++) >> >> +static inline void dax_mapping_set_cow_flag(struct address_space *mapping) >> +{ >> + mapping = (struct address_space *)PAGE_MAPPING_DAX_COW; >> +} >> + >> +static inline bool dax_mapping_is_cow(struct address_space *mapping) >> +{ >> + return (unsigned long)mapping == PAGE_MAPPING_DAX_COW; >> +} >> + >> /* >> - * TODO: for reflink+dax we need a way to associate a single page with >> - * multiple address_space instances at different linear_page_index() >> - * offsets. >> + * Set or Update the page->mapping with FS_DAX_MAPPING_COW flag. >> + * Return true if it is an Update. >> + */ >> +static inline bool dax_mapping_set_cow(struct page *page) >> +{ >> + if (page->mapping) { >> + /* flag already set */ >> + if (dax_mapping_is_cow(page->mapping)) >> + return false; >> + >> + /* >> + * This page has been mapped even before it is shared, just >> + * need to set this FS_DAX_MAPPING_COW flag. >> + */ >> + dax_mapping_set_cow_flag(page->mapping); >> + return true; >> + } >> + /* Newly associate CoW mapping */ >> + dax_mapping_set_cow_flag(page->mapping); >> + return false; >> +} >> + >> +/* >> + * When it is called in dax_insert_entry(), the cow flag will indicate that >> + * whether this entry is shared by multiple files. If so, set the page->mapping >> + * to be FS_DAX_MAPPING_COW, and use page->index as refcount. >> */ >> static void dax_associate_entry(void *entry, struct address_space *mapping, >> - struct vm_area_struct *vma, unsigned long address) >> + struct vm_area_struct *vma, unsigned long address, bool cow) >> { >> unsigned long size = dax_entry_size(entry), pfn, index; >> int i = 0; >> @@ -352,9 +385,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); >> >> - WARN_ON_ONCE(page->mapping); >> - page->mapping = mapping; >> - page->index = index + i++; >> + if (cow) { >> + if (dax_mapping_set_cow(page)) { >> + /* Was normal, now updated to CoW */ >> + page->index = 2; >> + } else >> + page->index++; >> + } else { >> + WARN_ON_ONCE(page->mapping); >> + page->mapping = mapping; >> + page->index = index + i++; >> + } >> } >> } >> >> @@ -370,7 +411,12 @@ static void dax_disassociate_entry(void *entry, struct address_space *mapping, >> struct page *page = pfn_to_page(pfn); >> >> WARN_ON_ONCE(trunc && page_ref_count(page) > 1); >> - WARN_ON_ONCE(page->mapping && page->mapping != mapping); >> + if (!dax_mapping_is_cow(page->mapping)) { >> + /* keep the CoW flag if this page is still shared */ >> + if (page->index-- > 0) >> + continue; >> + } else >> + WARN_ON_ONCE(page->mapping && page->mapping != mapping); >> page->mapping = NULL; >> page->index = 0; >> } >> @@ -810,7 +856,8 @@ static void *dax_insert_entry(struct xa_state *xas, >> void *old; >> >> dax_disassociate_entry(entry, mapping, false); >> - dax_associate_entry(new_entry, mapping, vmf->vma, vmf->address); >> + dax_associate_entry(new_entry, mapping, vmf->vma, vmf->address, >> + false); > > Where is the caller that passes 'true'? Also when that caller arrives > introduce a separate dax_associate_cow_entry() as that's easier to > read than dax_associate_entry(..., true) in case someone does not > remember what that boolean flag means.
This flag is supposed to be used when CoW support is introduced. When it is a CoW operation, which is decided by iomap & srcmap's flag, this flag will be set true.
I think I should describe it in detail in the commit message.
> > However, it's not clear to me that this approach is a good idea given > that the filesystem is the source of truth for how many address_spaces > this page mapping might be duplicated. What about a iomap_page_ops for > fsdax to ask the filesystem when it is ok to clear the mapping > association for a page?
I'll think how to implement it in this way.
-- Thanks, Ruan.
> >> /* >> * Only swap our new entry into the page cache if the current >> * entry is a zero page or an empty entry. If a normal PTE or >> diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h >> index 1c3b6e5c8bfd..6370d279795a 100644 >> --- a/include/linux/page-flags.h >> +++ b/include/linux/page-flags.h >> @@ -572,6 +572,12 @@ __PAGEFLAG(Reported, reported, PF_NO_COMPOUND) >> #define PAGE_MAPPING_KSM (PAGE_MAPPING_ANON | PAGE_MAPPING_MOVABLE) >> #define PAGE_MAPPING_FLAGS (PAGE_MAPPING_ANON | PAGE_MAPPING_MOVABLE) >> >> +/* >> + * Different with flags above, this flag is used only for fsdax mode. It >> + * indicates that this page->mapping is now under reflink case. >> + */ >> +#define PAGE_MAPPING_DAX_COW 0x1 >> + >> static __always_inline int PageMappingFlags(struct page *page) >> { >> return ((unsigned long)page->mapping & PAGE_MAPPING_FLAGS) != 0; >> -- >> 2.34.1 >> >> >>
| |