misc page fault handler cleanups

misc page fault handler cleanups

[Christoph Hellwig]

Hi all,

I recently started looking at the page fault handlers, and noticed
that our code is still a convoluted mess even after my previous
round of cleanups.

No bug fixes or speedups in this series, just pure cleanups.

[PATCH 1/4] xfs: split the page fault trace event

[Christoph Hellwig]

Split the xfs_filemap_fault trace event into separate ones for read and
write faults and move them into the applicable locations.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c  |  8 ++++++--
 fs/xfs/xfs_trace.h | 20 ++++++++++++--------
 2 files changed, 18 insertions(+), 10 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index b19916b11fd5..20f7f92b8867 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1425,6 +1425,8 @@ xfs_dax_read_fault(
 	struct xfs_inode	*ip = XFS_I(file_inode(vmf->vma->vm_file));
 	vm_fault_t		ret;
 
+	trace_xfs_read_fault(ip, order);
+
 	xfs_ilock(ip, XFS_MMAPLOCK_SHARED);
 	ret = xfs_dax_fault_locked(vmf, order, false);
 	xfs_iunlock(ip, XFS_MMAPLOCK_SHARED);
@@ -1442,6 +1444,8 @@ xfs_write_fault(
 	unsigned int		lock_mode = XFS_MMAPLOCK_SHARED;
 	vm_fault_t		ret;
 
+	trace_xfs_write_fault(ip, order);
+
 	sb_start_pagefault(inode->i_sb);
 	file_update_time(vmf->vma->vm_file);
 
@@ -1485,12 +1489,12 @@ __xfs_filemap_fault(
 {
 	struct inode		*inode = file_inode(vmf->vma->vm_file);
 
-	trace_xfs_filemap_fault(XFS_I(inode), order, write_fault);
-
 	if (write_fault)
 		return xfs_write_fault(vmf, order);
 	if (IS_DAX(inode))
 		return xfs_dax_read_fault(vmf, order);
+
+	trace_xfs_read_fault(XFS_I(inode), order);
 	return filemap_fault(vmf);
 }
 
diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
index ee9f0b1f548d..efc4aae295aa 100644
--- a/fs/xfs/xfs_trace.h
+++ b/fs/xfs/xfs_trace.h
@@ -827,28 +827,32 @@ DEFINE_INODE_EVENT(xfs_inode_inactivating);
 TRACE_DEFINE_ENUM(XFS_REFC_DOMAIN_SHARED);
 TRACE_DEFINE_ENUM(XFS_REFC_DOMAIN_COW);
 
-TRACE_EVENT(xfs_filemap_fault,
-	TP_PROTO(struct xfs_inode *ip, unsigned int order, bool write_fault),
-	TP_ARGS(ip, order, write_fault),
+DECLARE_EVENT_CLASS(xfs_fault_class,
+	TP_PROTO(struct xfs_inode *ip, unsigned int order),
+	TP_ARGS(ip, order),
 	TP_STRUCT__entry(
 		__field(dev_t, dev)
 		__field(xfs_ino_t, ino)
 		__field(unsigned int, order)
-		__field(bool, write_fault)
 	),
 	TP_fast_assign(
 		__entry->dev = VFS_I(ip)->i_sb->s_dev;
 		__entry->ino = ip->i_ino;
 		__entry->order = order;
-		__entry->write_fault = write_fault;
 	),
-	TP_printk("dev %d:%d ino 0x%llx order %u write_fault %d",
+	TP_printk("dev %d:%d ino 0x%llx order %u",
 		  MAJOR(__entry->dev), MINOR(__entry->dev),
 		  __entry->ino,
-		  __entry->order,
-		  __entry->write_fault)
+		  __entry->order)
 )
 
+#define DEFINE_FAULT_EVENT(name) \
+DEFINE_EVENT(xfs_fault_class, name, \
+	TP_PROTO(struct xfs_inode *ip, unsigned int order), \
+	TP_ARGS(ip, order))
+DEFINE_FAULT_EVENT(xfs_read_fault);
+DEFINE_FAULT_EVENT(xfs_write_fault);
+
 DECLARE_EVENT_CLASS(xfs_iref_class,
 	TP_PROTO(struct xfs_inode *ip, unsigned long caller_ip),
 	TP_ARGS(ip, caller_ip),
-- 
2.45.2

Re: [PATCH 1/4] xfs: split the page fault trace event

[Darrick J. Wong]

On Tue, Oct 29, 2024 at 04:11:57PM +0100, Christoph Hellwig wrote:
> Split the xfs_filemap_fault trace event into separate ones for read and
> write faults and move them into the applicable locations.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Yay nice split!
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_file.c  |  8 ++++++--
>  fs/xfs/xfs_trace.h | 20 ++++++++++++--------
>  2 files changed, 18 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index b19916b11fd5..20f7f92b8867 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1425,6 +1425,8 @@ xfs_dax_read_fault(
>  	struct xfs_inode	*ip = XFS_I(file_inode(vmf->vma->vm_file));
>  	vm_fault_t		ret;
>  
> +	trace_xfs_read_fault(ip, order);
> +
>  	xfs_ilock(ip, XFS_MMAPLOCK_SHARED);
>  	ret = xfs_dax_fault_locked(vmf, order, false);
>  	xfs_iunlock(ip, XFS_MMAPLOCK_SHARED);
> @@ -1442,6 +1444,8 @@ xfs_write_fault(
>  	unsigned int		lock_mode = XFS_MMAPLOCK_SHARED;
>  	vm_fault_t		ret;
>  
> +	trace_xfs_write_fault(ip, order);
> +
>  	sb_start_pagefault(inode->i_sb);
>  	file_update_time(vmf->vma->vm_file);
>  
> @@ -1485,12 +1489,12 @@ __xfs_filemap_fault(
>  {
>  	struct inode		*inode = file_inode(vmf->vma->vm_file);
>  
> -	trace_xfs_filemap_fault(XFS_I(inode), order, write_fault);
> -
>  	if (write_fault)
>  		return xfs_write_fault(vmf, order);
>  	if (IS_DAX(inode))
>  		return xfs_dax_read_fault(vmf, order);
> +
> +	trace_xfs_read_fault(XFS_I(inode), order);
>  	return filemap_fault(vmf);
>  }
>  
> diff --git a/fs/xfs/xfs_trace.h b/fs/xfs/xfs_trace.h
> index ee9f0b1f548d..efc4aae295aa 100644
> --- a/fs/xfs/xfs_trace.h
> +++ b/fs/xfs/xfs_trace.h
> @@ -827,28 +827,32 @@ DEFINE_INODE_EVENT(xfs_inode_inactivating);
>  TRACE_DEFINE_ENUM(XFS_REFC_DOMAIN_SHARED);
>  TRACE_DEFINE_ENUM(XFS_REFC_DOMAIN_COW);
>  
> -TRACE_EVENT(xfs_filemap_fault,
> -	TP_PROTO(struct xfs_inode *ip, unsigned int order, bool write_fault),
> -	TP_ARGS(ip, order, write_fault),
> +DECLARE_EVENT_CLASS(xfs_fault_class,
> +	TP_PROTO(struct xfs_inode *ip, unsigned int order),
> +	TP_ARGS(ip, order),
>  	TP_STRUCT__entry(
>  		__field(dev_t, dev)
>  		__field(xfs_ino_t, ino)
>  		__field(unsigned int, order)
> -		__field(bool, write_fault)
>  	),
>  	TP_fast_assign(
>  		__entry->dev = VFS_I(ip)->i_sb->s_dev;
>  		__entry->ino = ip->i_ino;
>  		__entry->order = order;
> -		__entry->write_fault = write_fault;
>  	),
> -	TP_printk("dev %d:%d ino 0x%llx order %u write_fault %d",
> +	TP_printk("dev %d:%d ino 0x%llx order %u",
>  		  MAJOR(__entry->dev), MINOR(__entry->dev),
>  		  __entry->ino,
> -		  __entry->order,
> -		  __entry->write_fault)
> +		  __entry->order)
>  )
>  
> +#define DEFINE_FAULT_EVENT(name) \
> +DEFINE_EVENT(xfs_fault_class, name, \
> +	TP_PROTO(struct xfs_inode *ip, unsigned int order), \
> +	TP_ARGS(ip, order))
> +DEFINE_FAULT_EVENT(xfs_read_fault);
> +DEFINE_FAULT_EVENT(xfs_write_fault);
> +
>  DECLARE_EVENT_CLASS(xfs_iref_class,
>  	TP_PROTO(struct xfs_inode *ip, unsigned long caller_ip),
>  	TP_ARGS(ip, caller_ip),
> -- 
> 2.45.2
> 
> 

[PATCH 2/4] xfs: split write fault handling out of __xfs_filemap_fault

[Christoph Hellwig]

Only two of the callers of __xfs_filemap_fault every handle read faults.
Split the write_fault handling out of __xfs_filemap_fault so that all
callers call that directly either conditionally or unconditionally and
only leave the read fault handling in __xfs_filemap_fault.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c | 41 +++++++++++++++++++----------------------
 1 file changed, 19 insertions(+), 22 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 20f7f92b8867..0b8e36f8703c 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1434,6 +1434,16 @@ xfs_dax_read_fault(
 	return ret;
 }
 
+/*
+ * Locking for serialisation of IO during page faults. This results in a lock
+ * ordering of:
+ *
+ * mmap_lock (MM)
+ *   sb_start_pagefault(vfs, freeze)
+ *     invalidate_lock (vfs/XFS_MMAPLOCK - truncate serialisation)
+ *       page_lock (MM)
+ *         i_lock (XFS - extent map serialisation)
+ */
 static vm_fault_t
 xfs_write_fault(
 	struct vm_fault		*vmf,
@@ -1471,26 +1481,13 @@ xfs_write_fault(
 	return ret;
 }
 
-/*
- * Locking for serialisation of IO during page faults. This results in a lock
- * ordering of:
- *
- * mmap_lock (MM)
- *   sb_start_pagefault(vfs, freeze)
- *     invalidate_lock (vfs/XFS_MMAPLOCK - truncate serialisation)
- *       page_lock (MM)
- *         i_lock (XFS - extent map serialisation)
- */
 static vm_fault_t
 __xfs_filemap_fault(
 	struct vm_fault		*vmf,
-	unsigned int		order,
-	bool			write_fault)
+	unsigned int		order)
 {
 	struct inode		*inode = file_inode(vmf->vma->vm_file);
 
-	if (write_fault)
-		return xfs_write_fault(vmf, order);
 	if (IS_DAX(inode))
 		return xfs_dax_read_fault(vmf, order);
 
@@ -1511,9 +1508,9 @@ xfs_filemap_fault(
 	struct vm_fault		*vmf)
 {
 	/* DAX can shortcut the normal fault path on write faults! */
-	return __xfs_filemap_fault(vmf, 0,
-			IS_DAX(file_inode(vmf->vma->vm_file)) &&
-			xfs_is_write_fault(vmf));
+	if (IS_DAX(file_inode(vmf->vma->vm_file)) && xfs_is_write_fault(vmf))
+		return xfs_write_fault(vmf, 0);
+	return __xfs_filemap_fault(vmf, 0);
 }
 
 static vm_fault_t
@@ -1525,15 +1522,16 @@ xfs_filemap_huge_fault(
 		return VM_FAULT_FALLBACK;
 
 	/* DAX can shortcut the normal fault path on write faults! */
-	return __xfs_filemap_fault(vmf, order,
-			xfs_is_write_fault(vmf));
+	if (xfs_is_write_fault(vmf))
+		return xfs_write_fault(vmf, order);
+	return __xfs_filemap_fault(vmf, order);
 }
 
 static vm_fault_t
 xfs_filemap_page_mkwrite(
 	struct vm_fault		*vmf)
 {
-	return __xfs_filemap_fault(vmf, 0, true);
+	return xfs_write_fault(vmf, 0);
 }
 
 /*
@@ -1545,8 +1543,7 @@ static vm_fault_t
 xfs_filemap_pfn_mkwrite(
 	struct vm_fault		*vmf)
 {
-
-	return __xfs_filemap_fault(vmf, 0, true);
+	return xfs_write_fault(vmf, 0);
 }
 
 static const struct vm_operations_struct xfs_file_vm_ops = {
-- 
2.45.2

Re: [PATCH 2/4] xfs: split write fault handling out of __xfs_filemap_fault

[Darrick J. Wong]

On Tue, Oct 29, 2024 at 04:11:58PM +0100, Christoph Hellwig wrote:
> Only two of the callers of __xfs_filemap_fault every handle read faults.
> Split the write_fault handling out of __xfs_filemap_fault so that all
> callers call that directly either conditionally or unconditionally and
> only leave the read fault handling in __xfs_filemap_fault.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

This seems pretty straightforward so
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_file.c | 41 +++++++++++++++++++----------------------
>  1 file changed, 19 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 20f7f92b8867..0b8e36f8703c 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1434,6 +1434,16 @@ xfs_dax_read_fault(
>  	return ret;
>  }
>  
> +/*
> + * Locking for serialisation of IO during page faults. This results in a lock
> + * ordering of:
> + *
> + * mmap_lock (MM)
> + *   sb_start_pagefault(vfs, freeze)
> + *     invalidate_lock (vfs/XFS_MMAPLOCK - truncate serialisation)
> + *       page_lock (MM)
> + *         i_lock (XFS - extent map serialisation)
> + */
>  static vm_fault_t
>  xfs_write_fault(
>  	struct vm_fault		*vmf,
> @@ -1471,26 +1481,13 @@ xfs_write_fault(
>  	return ret;
>  }
>  
> -/*
> - * Locking for serialisation of IO during page faults. This results in a lock
> - * ordering of:
> - *
> - * mmap_lock (MM)
> - *   sb_start_pagefault(vfs, freeze)
> - *     invalidate_lock (vfs/XFS_MMAPLOCK - truncate serialisation)
> - *       page_lock (MM)
> - *         i_lock (XFS - extent map serialisation)
> - */
>  static vm_fault_t
>  __xfs_filemap_fault(
>  	struct vm_fault		*vmf,
> -	unsigned int		order,
> -	bool			write_fault)
> +	unsigned int		order)
>  {
>  	struct inode		*inode = file_inode(vmf->vma->vm_file);
>  
> -	if (write_fault)
> -		return xfs_write_fault(vmf, order);
>  	if (IS_DAX(inode))
>  		return xfs_dax_read_fault(vmf, order);
>  
> @@ -1511,9 +1508,9 @@ xfs_filemap_fault(
>  	struct vm_fault		*vmf)
>  {
>  	/* DAX can shortcut the normal fault path on write faults! */
> -	return __xfs_filemap_fault(vmf, 0,
> -			IS_DAX(file_inode(vmf->vma->vm_file)) &&
> -			xfs_is_write_fault(vmf));
> +	if (IS_DAX(file_inode(vmf->vma->vm_file)) && xfs_is_write_fault(vmf))
> +		return xfs_write_fault(vmf, 0);
> +	return __xfs_filemap_fault(vmf, 0);
>  }
>  
>  static vm_fault_t
> @@ -1525,15 +1522,16 @@ xfs_filemap_huge_fault(
>  		return VM_FAULT_FALLBACK;
>  
>  	/* DAX can shortcut the normal fault path on write faults! */
> -	return __xfs_filemap_fault(vmf, order,
> -			xfs_is_write_fault(vmf));
> +	if (xfs_is_write_fault(vmf))
> +		return xfs_write_fault(vmf, order);
> +	return __xfs_filemap_fault(vmf, order);
>  }
>  
>  static vm_fault_t
>  xfs_filemap_page_mkwrite(
>  	struct vm_fault		*vmf)
>  {
> -	return __xfs_filemap_fault(vmf, 0, true);
> +	return xfs_write_fault(vmf, 0);
>  }
>  
>  /*
> @@ -1545,8 +1543,7 @@ static vm_fault_t
>  xfs_filemap_pfn_mkwrite(
>  	struct vm_fault		*vmf)
>  {
> -
> -	return __xfs_filemap_fault(vmf, 0, true);
> +	return xfs_write_fault(vmf, 0);
>  }
>  
>  static const struct vm_operations_struct xfs_file_vm_ops = {
> -- 
> 2.45.2
> 
> 

[PATCH 3/4] xfs: remove __xfs_filemap_fault

[Christoph Hellwig]

xfs_filemap_huge_fault only ever serves DAX faults, so hard code the
call to xfs_dax_read_fault and open code __xfs_filemap_fault in the
only remaining caller.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c | 29 +++++++++++------------------
 1 file changed, 11 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 0b8e36f8703c..7464d874e766 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1481,20 +1481,6 @@ xfs_write_fault(
 	return ret;
 }
 
-static vm_fault_t
-__xfs_filemap_fault(
-	struct vm_fault		*vmf,
-	unsigned int		order)
-{
-	struct inode		*inode = file_inode(vmf->vma->vm_file);
-
-	if (IS_DAX(inode))
-		return xfs_dax_read_fault(vmf, order);
-
-	trace_xfs_read_fault(XFS_I(inode), order);
-	return filemap_fault(vmf);
-}
-
 static inline bool
 xfs_is_write_fault(
 	struct vm_fault		*vmf)
@@ -1507,10 +1493,17 @@ static vm_fault_t
 xfs_filemap_fault(
 	struct vm_fault		*vmf)
 {
+	struct inode		*inode = file_inode(vmf->vma->vm_file);
+
 	/* DAX can shortcut the normal fault path on write faults! */
-	if (IS_DAX(file_inode(vmf->vma->vm_file)) && xfs_is_write_fault(vmf))
-		return xfs_write_fault(vmf, 0);
-	return __xfs_filemap_fault(vmf, 0);
+	if (IS_DAX(inode)) {
+		if (xfs_is_write_fault(vmf))
+			return xfs_write_fault(vmf, 0);
+		return xfs_dax_read_fault(vmf, 0);
+	}
+
+	trace_xfs_read_fault(XFS_I(inode), 0);
+	return filemap_fault(vmf);
 }
 
 static vm_fault_t
@@ -1524,7 +1517,7 @@ xfs_filemap_huge_fault(
 	/* DAX can shortcut the normal fault path on write faults! */
 	if (xfs_is_write_fault(vmf))
 		return xfs_write_fault(vmf, order);
-	return __xfs_filemap_fault(vmf, order);
+	return xfs_dax_read_fault(vmf, order);
 }
 
 static vm_fault_t
-- 
2.45.2

Re: [PATCH 3/4] xfs: remove __xfs_filemap_fault

[Darrick J. Wong]

On Tue, Oct 29, 2024 at 04:11:59PM +0100, Christoph Hellwig wrote:
> xfs_filemap_huge_fault only ever serves DAX faults, so hard code the
> call to xfs_dax_read_fault and open code __xfs_filemap_fault in the
> only remaining caller.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>

Looks good,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_file.c | 29 +++++++++++------------------
>  1 file changed, 11 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 0b8e36f8703c..7464d874e766 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1481,20 +1481,6 @@ xfs_write_fault(
>  	return ret;
>  }
>  
> -static vm_fault_t
> -__xfs_filemap_fault(
> -	struct vm_fault		*vmf,
> -	unsigned int		order)
> -{
> -	struct inode		*inode = file_inode(vmf->vma->vm_file);
> -
> -	if (IS_DAX(inode))
> -		return xfs_dax_read_fault(vmf, order);
> -
> -	trace_xfs_read_fault(XFS_I(inode), order);
> -	return filemap_fault(vmf);
> -}
> -
>  static inline bool
>  xfs_is_write_fault(
>  	struct vm_fault		*vmf)
> @@ -1507,10 +1493,17 @@ static vm_fault_t
>  xfs_filemap_fault(
>  	struct vm_fault		*vmf)
>  {
> +	struct inode		*inode = file_inode(vmf->vma->vm_file);
> +
>  	/* DAX can shortcut the normal fault path on write faults! */
> -	if (IS_DAX(file_inode(vmf->vma->vm_file)) && xfs_is_write_fault(vmf))
> -		return xfs_write_fault(vmf, 0);
> -	return __xfs_filemap_fault(vmf, 0);
> +	if (IS_DAX(inode)) {
> +		if (xfs_is_write_fault(vmf))
> +			return xfs_write_fault(vmf, 0);
> +		return xfs_dax_read_fault(vmf, 0);
> +	}
> +
> +	trace_xfs_read_fault(XFS_I(inode), 0);
> +	return filemap_fault(vmf);
>  }
>  
>  static vm_fault_t
> @@ -1524,7 +1517,7 @@ xfs_filemap_huge_fault(
>  	/* DAX can shortcut the normal fault path on write faults! */
>  	if (xfs_is_write_fault(vmf))
>  		return xfs_write_fault(vmf, order);
> -	return __xfs_filemap_fault(vmf, order);
> +	return xfs_dax_read_fault(vmf, order);
>  }
>  
>  static vm_fault_t
> -- 
> 2.45.2
> 
> 

[PATCH 4/4] xfs: remove xfs_page_mkwrite_iomap_ops

[Christoph Hellwig]

Shared the regular buffered write iomap_ops with the page fault path
and just check for the IOMAP_FAULT flag to skip delalloc punching.

This keeps the delalloc punching checks in one place, and will make it
easier to convert iomap to an iter model where the begin and end
handlers are merged into a single callback.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_file.c  |  2 +-
 fs/xfs/xfs_iomap.c | 17 ++++++++---------
 fs/xfs/xfs_iomap.h |  1 -
 3 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index 7464d874e766..c6de6b865ef1 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -1474,7 +1474,7 @@ xfs_write_fault(
 	if (IS_DAX(inode))
 		ret = xfs_dax_fault_locked(vmf, order, true);
 	else
-		ret = iomap_page_mkwrite(vmf, &xfs_page_mkwrite_iomap_ops);
+		ret = iomap_page_mkwrite(vmf, &xfs_buffered_write_iomap_ops);
 	xfs_iunlock(ip, lock_mode);
 
 	sb_end_pagefault(inode->i_sb);
diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 916531d9f83c..bfc5b0a4d633 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -1234,6 +1234,14 @@ xfs_buffered_write_iomap_end(
 	if (iomap->type != IOMAP_DELALLOC || !(iomap->flags & IOMAP_F_NEW))
 		return 0;
 
+	/*
+	 * iomap_page_mkwrite() will never fail in a way that requires delalloc
+	 * extents that it allocated to be revoked.  Hence never try to release
+	 * them here.
+	 */
+	if (flags & IOMAP_FAULT)
+		return 0;
+
 	/* Nothing to do if we've written the entire delalloc extent */
 	start_byte = iomap_last_written_block(inode, offset, written);
 	end_byte = round_up(offset + length, i_blocksize(inode));
@@ -1260,15 +1268,6 @@ const struct iomap_ops xfs_buffered_write_iomap_ops = {
 	.iomap_end		= xfs_buffered_write_iomap_end,
 };
 
-/*
- * iomap_page_mkwrite() will never fail in a way that requires delalloc extents
- * that it allocated to be revoked. Hence we do not need an .iomap_end method
- * for this operation.
- */
-const struct iomap_ops xfs_page_mkwrite_iomap_ops = {
-	.iomap_begin		= xfs_buffered_write_iomap_begin,
-};
-
 static int
 xfs_read_iomap_begin(
 	struct inode		*inode,
diff --git a/fs/xfs/xfs_iomap.h b/fs/xfs/xfs_iomap.h
index 4da13440bae9..8347268af727 100644
--- a/fs/xfs/xfs_iomap.h
+++ b/fs/xfs/xfs_iomap.h
@@ -48,7 +48,6 @@ xfs_aligned_fsb_count(
 }
 
 extern const struct iomap_ops xfs_buffered_write_iomap_ops;
-extern const struct iomap_ops xfs_page_mkwrite_iomap_ops;
 extern const struct iomap_ops xfs_direct_write_iomap_ops;
 extern const struct iomap_ops xfs_read_iomap_ops;
 extern const struct iomap_ops xfs_seek_iomap_ops;
-- 
2.45.2

Re: [PATCH 4/4] xfs: remove xfs_page_mkwrite_iomap_ops

[Darrick J. Wong]

On Tue, Oct 29, 2024 at 04:12:00PM +0100, Christoph Hellwig wrote:
> Shared the regular buffered write iomap_ops with the page fault path
> and just check for the IOMAP_FAULT flag to skip delalloc punching.
> 
> This keeps the delalloc punching checks in one place, and will make it
> easier to convert iomap to an iter model where the begin and end
> handlers are merged into a single callback.

"merged into a single callback"?  What plans are these? :)

> Signed-off-by: Christoph Hellwig <hch@lst.de>

Code changes here look ok to me, so
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_file.c  |  2 +-
>  fs/xfs/xfs_iomap.c | 17 ++++++++---------
>  fs/xfs/xfs_iomap.h |  1 -
>  3 files changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index 7464d874e766..c6de6b865ef1 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -1474,7 +1474,7 @@ xfs_write_fault(
>  	if (IS_DAX(inode))
>  		ret = xfs_dax_fault_locked(vmf, order, true);
>  	else
> -		ret = iomap_page_mkwrite(vmf, &xfs_page_mkwrite_iomap_ops);
> +		ret = iomap_page_mkwrite(vmf, &xfs_buffered_write_iomap_ops);
>  	xfs_iunlock(ip, lock_mode);
>  
>  	sb_end_pagefault(inode->i_sb);
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 916531d9f83c..bfc5b0a4d633 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -1234,6 +1234,14 @@ xfs_buffered_write_iomap_end(
>  	if (iomap->type != IOMAP_DELALLOC || !(iomap->flags & IOMAP_F_NEW))
>  		return 0;
>  
> +	/*
> +	 * iomap_page_mkwrite() will never fail in a way that requires delalloc
> +	 * extents that it allocated to be revoked.  Hence never try to release
> +	 * them here.
> +	 */
> +	if (flags & IOMAP_FAULT)
> +		return 0;
> +
>  	/* Nothing to do if we've written the entire delalloc extent */
>  	start_byte = iomap_last_written_block(inode, offset, written);
>  	end_byte = round_up(offset + length, i_blocksize(inode));
> @@ -1260,15 +1268,6 @@ const struct iomap_ops xfs_buffered_write_iomap_ops = {
>  	.iomap_end		= xfs_buffered_write_iomap_end,
>  };
>  
> -/*
> - * iomap_page_mkwrite() will never fail in a way that requires delalloc extents
> - * that it allocated to be revoked. Hence we do not need an .iomap_end method
> - * for this operation.
> - */
> -const struct iomap_ops xfs_page_mkwrite_iomap_ops = {
> -	.iomap_begin		= xfs_buffered_write_iomap_begin,
> -};
> -
>  static int
>  xfs_read_iomap_begin(
>  	struct inode		*inode,
> diff --git a/fs/xfs/xfs_iomap.h b/fs/xfs/xfs_iomap.h
> index 4da13440bae9..8347268af727 100644
> --- a/fs/xfs/xfs_iomap.h
> +++ b/fs/xfs/xfs_iomap.h
> @@ -48,7 +48,6 @@ xfs_aligned_fsb_count(
>  }
>  
>  extern const struct iomap_ops xfs_buffered_write_iomap_ops;
> -extern const struct iomap_ops xfs_page_mkwrite_iomap_ops;
>  extern const struct iomap_ops xfs_direct_write_iomap_ops;
>  extern const struct iomap_ops xfs_read_iomap_ops;
>  extern const struct iomap_ops xfs_seek_iomap_ops;
> -- 
> 2.45.2
> 
> 

Re: [PATCH 4/4] xfs: remove xfs_page_mkwrite_iomap_ops

[Christoph Hellwig]

On Tue, Oct 29, 2024 at 08:56:49AM -0700, Darrick J. Wong wrote:
> On Tue, Oct 29, 2024 at 04:12:00PM +0100, Christoph Hellwig wrote:
> > Shared the regular buffered write iomap_ops with the page fault path
> > and just check for the IOMAP_FAULT flag to skip delalloc punching.
> > 
> > This keeps the delalloc punching checks in one place, and will make it
> > easier to convert iomap to an iter model where the begin and end
> > handlers are merged into a single callback.
> 
> "merged into a single callback"?  What plans are these? :)

Well, the good old iterator conversion originally from willy.  His
intent I think was to avoid the indirect call entirely for performance
critical calls.  I'm more interested in allowing the caller to keep
more state, and to allow nested iterations to e.g. make buffered
out of place overwrites suck less, and to maybe not do synchronous
reads one block at a time in unshare.

This was my last spin on the idea:

https://www.spinics.net/lists/linux-btrfs/msg122508.html

I can't find willy's two older takes right now.

Re: misc page fault handler cleanups

[Dave Chinner]

On Tue, Oct 29, 2024 at 04:11:56PM +0100, Christoph Hellwig wrote:
> Hi all,
> 
> I recently started looking at the page fault handlers, and noticed
> that our code is still a convoluted mess even after my previous
> round of cleanups.
> 
> No bug fixes or speedups in this series, just pure cleanups.

Whole series looks good to me.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com