Re: [PATCH v10 9/9] fsdax: set a CoW flag when associate reflink mappings

Re: [PATCH v10 9/9] fsdax: set a CoW flag when associate reflink mappings

[kernel test robot]

Hi Shiyang,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on linux/master]
[also build test WARNING on linus/master v5.17-rc1]
[cannot apply to xfs-linux/for-next hnaz-mm/master next-20220128]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Shiyang-Ruan/fsdax-introduce-fs-query-to-support-reflink/20220127-204239
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 2c271fe77d52a0555161926c232cd5bc07178b39
config: x86_64-randconfig-c007-20220124 (https://download.01.org/0day-ci/archive/20220129/202201290206.TU7AHhWg-lkp@intel.com/config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project f32dccb9a43b02ce4e540d6ba5dbbdb188f2dc7d)
reproduce (this is a W=1 build):
         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
         chmod +x ~/bin/make.cross
         # https://github.com/0day-ci/linux/commit/26822296a70d211de3c5f4ef903d4c4cf29179bd
         git remote add linux-review https://github.com/0day-ci/linux
         git fetch --no-tags linux-review Shiyang-Ruan/fsdax-introduce-fs-query-to-support-reflink/20220127-204239
         git checkout 26822296a70d211de3c5f4ef903d4c4cf29179bd
         # save the config file to linux build tree
         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64 clang-analyzer

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


clang-analyzer warnings: (new ones prefixed by >>)

 >> fs/dax.c:339:2: warning: Value stored to 'mapping' is never read [clang-analyzer-deadcode.DeadStores]
            mapping = (struct address_space *)PAGE_MAPPING_DAX_COW;
            ^         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

vim +/mapping +339 fs/dax.c

d2c997c0f14535 Dan Williams   2017-12-22  328
d2c997c0f14535 Dan Williams   2017-12-22  329  /*
d2c997c0f14535 Dan Williams   2017-12-22  330   * Iterate through all mapped pfns represented by an entry, i.e. skip
d2c997c0f14535 Dan Williams   2017-12-22  331   * 'empty' and 'zero' entries.
d2c997c0f14535 Dan Williams   2017-12-22  332   */
d2c997c0f14535 Dan Williams   2017-12-22  333  #define for_each_mapped_pfn(entry, pfn) \
a77d19f46a37c0 Matthew Wilcox 2018-03-27  334  	for (pfn = dax_to_pfn(entry); \
a77d19f46a37c0 Matthew Wilcox 2018-03-27  335  			pfn < dax_end_pfn(entry); pfn++)
d2c997c0f14535 Dan Williams   2017-12-22  336
26822296a70d21 Shiyang Ruan   2022-01-27  337  static inline void dax_mapping_set_cow_flag(struct address_space *mapping)
26822296a70d21 Shiyang Ruan   2022-01-27  338  {
26822296a70d21 Shiyang Ruan   2022-01-27 @339  	mapping = (struct address_space *)PAGE_MAPPING_DAX_COW;
26822296a70d21 Shiyang Ruan   2022-01-27  340  }
26822296a70d21 Shiyang Ruan   2022-01-27  341

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[PATCH v10 0/9] fsdax: introduce fs query to support reflink

[Shiyang Ruan]

This patchset is aimed to support shared pages tracking for fsdax.

Changes since V9:
  - Remove dax write/read lock patch
  - Remove dax_lock_entry() in dax_lock_mapping_entry() and rename it to
      dax_load_page()
  - Wrap dax code in memory-failure.c with #if IS_ENABLED(CONFIG_FS_DAX)
  - Wrap xfs_dax_failure_fn() with #if IS_ENABLED(CONFIG_MEMORY_FAILURE)
  - Move PAGE_MAPPING_DAX_COW into page-flags.h

This patchset moves owner tracking from dax_assocaite_entry() to pmem
device driver, by introducing an interface ->memory_failure() for struct
pagemap.  This interface is called by memory_failure() in mm, and
implemented by pmem device.

Then call holder operations to find the filesystem which the corrupted
data located in, and call filesystem handler to track files or metadata
associated with this page.

Finally we are able to try to fix the corrupted data in filesystem and
do other necessary processing, such as killing processes who are using
the files affected.

The call trace is like this:
memory_failure()
|* fsdax case
|------------
|pgmap->ops->memory_failure()      => pmem_pgmap_memory_failure()
| dax_holder_notify_failure()      =>
|  dax_device->holder_ops->notify_failure() =>
|                                     - xfs_dax_notify_failure()
|  |* xfs_dax_notify_failure()
|  |--------------------------
|  |   xfs_rmap_query_range()
|  |    xfs_dax_failure_fn()
|  |    * corrupted on metadata
|  |       try to recover data, call xfs_force_shutdown()
|  |    * corrupted on file data
|  |       try to recover data, call mf_dax_kill_procs()
|* normal case
|-------------
|mf_generic_kill_procs()

==
Shiyang Ruan (9):
  dax: Introduce holder for dax_device
  mm: factor helpers for memory_failure_dev_pagemap
  pagemap,pmem: Introduce ->memory_failure()
  fsdax: fix function description
  fsdax: Introduce dax_load_page()
  mm: move pgoff_address() to vma_pgoff_address()
  mm: Introduce mf_dax_kill_procs() for fsdax case
  xfs: Implement ->notify_failure() for XFS
  fsdax: set a CoW flag when associate reflink mappings

 drivers/dax/super.c         |  62 ++++++++++
 drivers/nvdimm/pmem.c       |  16 +++
 fs/dax.c                    | 123 +++++++++++++++----
 fs/xfs/Makefile             |   1 +
 fs/xfs/xfs_buf.c            |  12 ++
 fs/xfs/xfs_fsops.c          |   3 +
 fs/xfs/xfs_mount.h          |   1 +
 fs/xfs/xfs_notify_failure.c | 222 ++++++++++++++++++++++++++++++++++
 fs/xfs/xfs_notify_failure.h |  10 ++
 include/linux/dax.h         |  37 ++++++
 include/linux/memremap.h    |  12 ++
 include/linux/mm.h          |  17 +++
 include/linux/page-flags.h  |   6 +
 mm/memory-failure.c         | 234 +++++++++++++++++++++++++-----------
 14 files changed, 666 insertions(+), 90 deletions(-)
 create mode 100644 fs/xfs/xfs_notify_failure.c
 create mode 100644 fs/xfs/xfs_notify_failure.h

-- 
2.34.1

[PATCH v10 9/9] fsdax: set a CoW flag when associate reflink mappings

[Shiyang Ruan]

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);
 		/*
 		 * 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

Re: [PATCH v10 9/9] fsdax: set a CoW flag when associate reflink mappings

[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.

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?

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

Re: [PATCH v10 9/9] fsdax: set a CoW flag when associate reflink mappings

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

Re: [PATCH v10 9/9] fsdax: set a CoW flag when associate reflink mappings

[Dan Williams]

On Tue, Feb 15, 2022 at 6:55 PM Shiyang Ruan <ruansy.fnst@fujitsu.com> wrote:
>
>
>
> 在 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.

Ok, so should this patch wait and be a part of that series? It's
otherwise confusing to introduce a new capability in a patch set and
not take advantage of it until a separate / later patch set.

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

That could help, or move it to the COW support series.

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