[PATCH RFC 0/2] xfs: fallocate RT flush unmap range fixes

[PATCH RFC 0/2] xfs: fallocate RT flush unmap range fixes

[John Garry]

As mentioned by Dave Chinner at [0], xfs_flush_unmap_range() and
xfs_prepare_shift() should consider RT extents in the flush unmap range,
and need to be fixed.

I don't want to add such changes to that series, so I am sending
separately.

I am marking as an RFC as I am not intimately familiar enough with the
code for me to say that the changes are technically correct.

About the change in xfs_prepare_shift(), that function is only called
from xfs_insert_file_space() and xfs_collapse_file_space(). Those
functions only permit RT extent-aligned calls in xfs_is_falloc_aligned(),
so in practice I don't think that this change would affect
xfs_prepare_shift(). And xfs_prepare_shift() calls
xfs_flush_unmap_range(), which is being fixed up anyway.

[0] https://lore.kernel.org/linux-xfs/ZjGSiOt21g5JCOhf@dread.disaster.area/

John Garry (2):
  xfs: Fix xfs_flush_unmap_range() range for RT
  xfs: Fix xfs_prepare_shift() range for RT

 fs/xfs/xfs_bmap_util.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

-- 
2.31.1

[PATCH RFC 1/2] xfs: Fix xfs_flush_unmap_range() range for RT

[John Garry]

Currently xfs_flush_unmap_range() does a flush for full FS blocks. Extend
this to cover full RT extents so that any range overlap with start/end of
the modification are clean and idle. 

This code change is originally from Dave Chinner.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 fs/xfs/xfs_bmap_util.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index 2e6f08198c07..da67c52d5f94 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -802,12 +802,16 @@ xfs_flush_unmap_range(
 	xfs_off_t		offset,
 	xfs_off_t		len)
 {
-	struct xfs_mount	*mp = ip->i_mount;
 	struct inode		*inode = VFS_I(ip);
 	xfs_off_t		rounding, start, end;
 	int			error;
 
-	rounding = max_t(xfs_off_t, mp->m_sb.sb_blocksize, PAGE_SIZE);
+	/*
+	 * Make sure we extend the flush out to extent alignment
+	 * boundaries so any extent range overlapping the start/end
+	 * of the modification we are about to do is clean and idle.
+	 */
+	rounding = max_t(xfs_off_t, xfs_inode_alloc_unitsize(ip), PAGE_SIZE);
 	start = round_down(offset, rounding);
 	end = round_up(offset + len, rounding) - 1;
 
-- 
2.31.1

Re: [PATCH RFC 1/2] xfs: Fix xfs_flush_unmap_range() range for RT

[Christoph Hellwig]

Looks good:

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

Re: [PATCH RFC 1/2] xfs: Fix xfs_flush_unmap_range() range for RT

[Darrick J. Wong]

On Fri, May 03, 2024 at 02:03:36PM +0000, John Garry wrote:
> Currently xfs_flush_unmap_range() does a flush for full FS blocks. Extend
> this to cover full RT extents so that any range overlap with start/end of
> the modification are clean and idle. 
> 
> This code change is originally from Dave Chinner.
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
>  fs/xfs/xfs_bmap_util.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index 2e6f08198c07..da67c52d5f94 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -802,12 +802,16 @@ xfs_flush_unmap_range(
>  	xfs_off_t		offset,
>  	xfs_off_t		len)
>  {
> -	struct xfs_mount	*mp = ip->i_mount;
>  	struct inode		*inode = VFS_I(ip);
>  	xfs_off_t		rounding, start, end;
>  	int			error;
>  
> -	rounding = max_t(xfs_off_t, mp->m_sb.sb_blocksize, PAGE_SIZE);
> +	/*
> +	 * Make sure we extend the flush out to extent alignment
> +	 * boundaries so any extent range overlapping the start/end
> +	 * of the modification we are about to do is clean and idle.
> +	 */
> +	rounding = max_t(xfs_off_t, xfs_inode_alloc_unitsize(ip), PAGE_SIZE);
>  	start = round_down(offset, rounding);

round_down requires the divisor to be a power of two.

--D

>  	end = round_up(offset + len, rounding) - 1;
>  
> -- 
> 2.31.1
> 
> 

Re: [PATCH RFC 1/2] xfs: Fix xfs_flush_unmap_range() range for RT

[John Garry]

On 07/05/2024 21:58, Darrick J. Wong wrote:
>> -	rounding = max_t(xfs_off_t, mp->m_sb.sb_blocksize, PAGE_SIZE);
>> +	/*
>> +	 * Make sure we extend the flush out to extent alignment
>> +	 * boundaries so any extent range overlapping the start/end
>> +	 * of the modification we are about to do is clean and idle.
>> +	 */
>> +	rounding = max_t(xfs_off_t, xfs_inode_alloc_unitsize(ip), PAGE_SIZE);
>>   	start = round_down(offset, rounding);
> round_down requires the divisor to be a power of two.

well spotted, and so the round_up() call, below, also needs fixing.

> 
> --D
> 
>>   	end = round_up(offset + len, rounding) - 1;

Re: [PATCH RFC 1/2] xfs: Fix xfs_flush_unmap_range() range for RT

[John Garry]

On 07/05/2024 21:58, Darrick J. Wong wrote:
>> -	rounding = max_t(xfs_off_t, mp->m_sb.sb_blocksize, PAGE_SIZE);
>> +	/*
>> +	 * Make sure we extend the flush out to extent alignment
>> +	 * boundaries so any extent range overlapping the start/end
>> +	 * of the modification we are about to do is clean and idle.
>> +	 */
>> +	rounding = max_t(xfs_off_t, xfs_inode_alloc_unitsize(ip), PAGE_SIZE);
>>   	start = round_down(offset, rounding);
> round_down requires the divisor to be a power of two.

well spotted, and so the round_up() call, below, also needs fixing.

> 
> --D
> 
>>   	end = round_up(offset + len, rounding) - 1;

[PATCH RFC 2/2] xfs: Fix xfs_prepare_shift() range for RT

[John Garry]

The RT extent range must be considered in the xfs_flush_unmap_range() call
to stabilize the boundary.

This code change is originally from Dave Chinner.

Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 fs/xfs/xfs_bmap_util.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index da67c52d5f94..2775bb32489e 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -896,8 +896,8 @@ xfs_prepare_shift(
 	struct xfs_inode	*ip,
 	loff_t			offset)
 {
-	struct xfs_mount	*mp = ip->i_mount;
 	int			error;
+	unsigned int		rounding;
 
 	/*
 	 * Trim eofblocks to avoid shifting uninitialized post-eof preallocation
@@ -914,11 +914,13 @@ xfs_prepare_shift(
 	 * with the full range of the operation. If we don't, a COW writeback
 	 * completion could race with an insert, front merge with the start
 	 * extent (after split) during the shift and corrupt the file. Start
-	 * with the block just prior to the start to stabilize the boundary.
+	 * with the aligned block just prior to the start to stabilize the
+	 * boundary.
 	 */
-	offset = round_down(offset, mp->m_sb.sb_blocksize);
+	rounding = xfs_inode_alloc_unitsize(ip);
+	offset = round_down(offset, rounding);
 	if (offset)
-		offset -= mp->m_sb.sb_blocksize;
+		offset -= rounding;
 
 	/*
 	 * Writeback and invalidate cache for the remainder of the file as we're
-- 
2.31.1

Re: [PATCH RFC 2/2] xfs: Fix xfs_prepare_shift() range for RT

[Christoph Hellwig]

Looks good:

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

Re: [PATCH RFC 2/2] xfs: Fix xfs_prepare_shift() range for RT

[Darrick J. Wong]

On Fri, May 03, 2024 at 02:03:37PM +0000, John Garry wrote:
> The RT extent range must be considered in the xfs_flush_unmap_range() call
> to stabilize the boundary.
> 
> This code change is originally from Dave Chinner.
> 
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
>  fs/xfs/xfs_bmap_util.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index da67c52d5f94..2775bb32489e 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -896,8 +896,8 @@ xfs_prepare_shift(
>  	struct xfs_inode	*ip,
>  	loff_t			offset)
>  {
> -	struct xfs_mount	*mp = ip->i_mount;
>  	int			error;
> +	unsigned int		rounding;
>  
>  	/*
>  	 * Trim eofblocks to avoid shifting uninitialized post-eof preallocation
> @@ -914,11 +914,13 @@ xfs_prepare_shift(
>  	 * with the full range of the operation. If we don't, a COW writeback
>  	 * completion could race with an insert, front merge with the start
>  	 * extent (after split) during the shift and corrupt the file. Start
> -	 * with the block just prior to the start to stabilize the boundary.
> +	 * with the aligned block just prior to the start to stabilize the

"...with the allocation unit just prior to the start..."

> +	 * boundary.
>  	 */
> -	offset = round_down(offset, mp->m_sb.sb_blocksize);
> +	rounding = xfs_inode_alloc_unitsize(ip);
> +	offset = round_down(offset, rounding);

Again, round_down requires the divisor to be a power of two.

--D

>  	if (offset)
> -		offset -= mp->m_sb.sb_blocksize;
> +		offset -= rounding;
>  
>  	/*
>  	 * Writeback and invalidate cache for the remainder of the file as we're
> -- 
> 2.31.1
> 
>