Re: [RFC PATCH v2 1/8] ext4/xfs: add page refcount helper

* Alex Sierra alex.sierra@amd.com [210607 16:43]:

From: Ralph Campbell rcampbell@nvidia.com

There are several places where ZONE_DEVICE struct pages assume a reference count == 1 means the page is idle and free. Instead of open coding this, add a helper function to hide this detail.

Signed-off-by: Ralph Campbell rcampbell@nvidia.com

fs/dax.c | 4 ++-- fs/ext4/inode.c | 5 +---- fs/xfs/xfs_file.c | 4 +--- include/linux/dax.h | 10 ++++++++++ 4 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/fs/dax.c b/fs/dax.c index 26d5dcd2d69e..321f4ddc6643 100644 --- a/fs/dax.c +++ b/fs/dax.c @@ -358,7 +358,7 @@ static void dax_disassociate_entry(void *entry, struct address_space *mapping, for_each_mapped_pfn(entry, pfn) { struct page *page = pfn_to_page(pfn);

• WARN_ON_ONCE(trunc && page_ref_count(page) > 1);
  

• WARN_ON_ONCE(trunc && !dax_layout_is_idle_page(page));
  

  WARN_ON_ONCE(page->mapping && page->mapping != mapping); page->mapping = NULL; page->index = 0;

@@ -372,7 +372,7 @@ static struct page *dax_busy_page(void *entry) for_each_mapped_pfn(entry, pfn) { struct page *page = pfn_to_page(pfn);

• if (page_ref_count(page) > 1)
  

• if (!dax_layout_is_idle_page(page))
  return page;
  

  } return NULL;

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c index c173c8405856..9ee00186412f 100644 --- a/fs/ext4/inode.c +++ b/fs/ext4/inode.c @@ -3972,10 +3972,7 @@ int ext4_break_layouts(struct inode *inode) if (!page) return 0;

• error = ___wait_var_event(&page->_refcount,
  

• 		atomic_read(&page->_refcount) == 1,
  

• 		TASK_INTERRUPTIBLE, 0, 0,
  

• 		ext4_wait_dax_page(ei));
  

• error = dax_wait_page(ei, page, ext4_wait_dax_page);
  

  } while (error == 0);

  return error;

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index 5b0f93f73837..39565fe5f817 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -782,9 +782,7 @@ xfs_break_dax_layouts( return 0;

*retry = true;

• return ___wait_var_event(&page->_refcount,

• 	atomic_read(&page->_refcount) == 1, TASK_INTERRUPTIBLE,
  

• 	0, 0, xfs_wait_dax_page(inode));
  

• return dax_wait_page(inode, page, xfs_wait_dax_page);

}

int diff --git a/include/linux/dax.h b/include/linux/dax.h index b52f084aa643..8909a91cd381 100644 --- a/include/linux/dax.h +++ b/include/linux/dax.h @@ -243,6 +243,16 @@ static inline bool dax_mapping(struct address_space *mapping) return mapping->host && IS_DAX(mapping->host); }

+static inline bool dax_layout_is_idle_page(struct page *page) +{

• return page_ref_count(page) == 1;

+}

If this races with page_ref_count(page) == 0, then it will return false that a page is idle when the page is being freed. I don't know the code well enough to say if this is an issue or not so please let me know.

For example: !dax_layout_is_idle_page() will return true in dax_busy_page() above when the count is 0 and return the page.

Maybe you are sure to have at least one reference when calling this? It might be worth adding a comment.

+#define dax_wait_page(_inode, _page, _wait_cb) \

• ___wait_var_event(&(_page)->_refcount, \

• dax_layout_is_idle_page(_page),				\
  

• TASK_INTERRUPTIBLE, 0, 0, _wait_cb(_inode))
  

#ifdef CONFIG_DEV_DAX_HMEM_DEVICES void hmem_register_device(int target_nid, struct resource *r);

#else

2.17.1