Re: [PATCH v5 05/25] fs/dax: Create a common implementation to break DAX layouts
From: Dan Williams <hidden>
Date: 2025-01-09 00:14:30
Also in:
linux-arm-kernel, linux-cxl, linux-doc, linux-ext4, linux-fsdevel, linux-mm, linux-xfs, lkml, nvdimm
Alistair Popple wrote:
quoted hunk ↗ jump to hunk
Prior to freeing a block file systems supporting FS DAX must check that the associated pages are both unmapped from user-space and not undergoing DMA or other access from eg. get_user_pages(). This is achieved by unmapping the file range and scanning the FS DAX page-cache to see if any pages within the mapping have an elevated refcount. This is done using two functions - dax_layout_busy_page_range() which returns a page to wait for the refcount to become idle on. Rather than open-code this introduce a common implementation to both unmap and wait for the page to become idle. Signed-off-by: Alistair Popple <apopple@nvidia.com> --- Changes for v5: - Don't wait for idle pages on non-DAX mappings Changes for v4: - Fixed some build breakage due to missing symbol exports reported by John Hubbard (thanks!). --- fs/dax.c | 33 +++++++++++++++++++++++++++++++++ fs/ext4/inode.c | 10 +--------- fs/fuse/dax.c | 29 +++++------------------------ fs/xfs/xfs_inode.c | 23 +++++------------------ fs/xfs/xfs_inode.h | 2 +- include/linux/dax.h | 21 +++++++++++++++++++++ mm/madvise.c | 8 ++++---- 7 files changed, 70 insertions(+), 56 deletions(-)diff --git a/fs/dax.c b/fs/dax.c index d010c10..9c3bd07 100644 --- a/fs/dax.c +++ b/fs/dax.c@@ -845,6 +845,39 @@ int dax_delete_mapping_entry(struct address_space *mapping, pgoff_t index) return ret; } +static int wait_page_idle(struct page *page, + void (cb)(struct inode *), + struct inode *inode) +{ + return ___wait_var_event(page, page_ref_count(page) == 1, + TASK_INTERRUPTIBLE, 0, 0, cb(inode)); +} + +/* + * Unmaps the inode and waits for any DMA to complete prior to deleting the + * DAX mapping entries for the range. + */ +int dax_break_mapping(struct inode *inode, loff_t start, loff_t end, + void (cb)(struct inode *)) +{ + struct page *page; + int error; + + if (!dax_mapping(inode->i_mapping)) + return 0; + + do { + page = dax_layout_busy_page_range(inode->i_mapping, start, end); + if (!page) + break; + + error = wait_page_idle(page, cb, inode);
This implementations removes logic around @retry found in the XFS and FUSE implementations, I think that is a mistake, and EXT4 has apparently been broken in this regard. wait_page_idle() returns after @page is idle, but that does not mean @inode is DMA idle. After one found page from dax_layout_busy_page_range() is waited upon a new call to dax_break_mapping() needs to made to check if another DMA started, or if there were originally more pages active.
+ } while (error == 0); + + return error;
Surprised that the compiler does not warn about an uninitialized variable here?