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

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

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/

Changes since RFC:
- Use roundup() and rounddown() (Darrick)
- Change xfs_prepare_shift() comment (Darrick)
- Add Christoph's RB tags - I think ok, even though code has changed
  since RFC

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 | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

-- 
2.31.1

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

[John Garry]

Currently xfs_flush_unmap_range() does unmap for a full RT extent range,
which we also want to ensure is clean and idle.

This code change is originally from Dave Chinner.

Reviewed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 fs/xfs/xfs_bmap_util.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
index ac2e77ebb54c..5d4aac50cbf5 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -794,14 +794,18 @@ 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);
-	start = round_down(offset, rounding);
-	end = round_up(offset + len, rounding) - 1;
+	/*
+	 * 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 = rounddown(offset, rounding);
+	end = roundup(offset + len, rounding) - 1;
 
 	error = filemap_write_and_wait_range(inode->i_mapping, start, end);
 	if (error)
-- 
2.31.1

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

Reviewed-by: Christoph Hellwig <hch@lst.de>
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 5d4aac50cbf5..52d7ee5bbb72 100644
--- a/fs/xfs/xfs_bmap_util.c
+++ b/fs/xfs/xfs_bmap_util.c
@@ -888,7 +888,7 @@ xfs_prepare_shift(
 	struct xfs_inode	*ip,
 	loff_t			offset)
 {
-	struct xfs_mount	*mp = ip->i_mount;
+	unsigned int		rounding;
 	int			error;
 
 	/*
@@ -906,11 +906,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 allocation unit 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 = rounddown(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 0/2] xfs: fallocate RT flush unmap range fixes

[Darrick J. Wong]

On Thu, May 09, 2024 at 10:40:55AM +0000, John Garry wrote:
> 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.
> 
> 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/

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

--D

> 
> Changes since RFC:
> - Use roundup() and rounddown() (Darrick)
> - Change xfs_prepare_shift() comment (Darrick)
> - Add Christoph's RB tags - I think ok, even though code has changed
>   since RFC
> 
> 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 | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> -- 
> 2.31.1
> 
> 

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

[kernel test robot]

Hi John,

kernel test robot noticed the following build errors:

[auto build test ERROR on xfs-linux/for-next]
[also build test ERROR on next-20240509]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/John-Garry/xfs-Fix-xfs_flush_unmap_range-range-for-RT/20240509-184217
base:   https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git for-next
patch link:    https://lore.kernel.org/r/20240509104057.1197846-2-john.g.garry%40oracle.com
patch subject: [PATCH 1/2] xfs: Fix xfs_flush_unmap_range() range for RT
config: mips-randconfig-r053-20240510 (https://download.01.org/0day-ci/archive/20240510/202405100640.0Xf0XLtT-lkp@intel.com/config)
compiler: mips-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240510/202405100640.0Xf0XLtT-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405100640.0Xf0XLtT-lkp@intel.com/

All errors (new ones prefixed by >>):

   mips-linux-ld: fs/xfs/xfs_bmap_util.o: in function `xfs_flush_unmap_range':
>> xfs_bmap_util.c:(.text+0x2934): undefined reference to `__moddi3'
>> mips-linux-ld: xfs_bmap_util.c:(.text+0x2978): undefined reference to `__moddi3'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Dave Chinner]

On Thu, May 09, 2024 at 10:40:56AM +0000, John Garry wrote:
> Currently xfs_flush_unmap_range() does unmap for a full RT extent range,
> which we also want to ensure is clean and idle.
> 
> This code change is originally from Dave Chinner.
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: John Garry <john.g.garry@oracle.com>
> ---
>  fs/xfs/xfs_bmap_util.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_bmap_util.c b/fs/xfs/xfs_bmap_util.c
> index ac2e77ebb54c..5d4aac50cbf5 100644
> --- a/fs/xfs/xfs_bmap_util.c
> +++ b/fs/xfs/xfs_bmap_util.c
> @@ -794,14 +794,18 @@ 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);
> -	start = round_down(offset, rounding);
> -	end = round_up(offset + len, rounding) - 1;
> +	/*
> +	 * 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 = rounddown(offset, rounding);
> +	end = roundup(offset + len, rounding) - 1;

These are 64 bit values, so roundup_64() and rounddown_64().

-Dave.
-- 
Dave Chinner
david@fromorbit.com

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

[kernel test robot]

Hi John,

kernel test robot noticed the following build errors:

[auto build test ERROR on xfs-linux/for-next]
[also build test ERROR on next-20240509]
[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#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/John-Garry/xfs-Fix-xfs_flush_unmap_range-range-for-RT/20240509-184217
base:   https://git.kernel.org/pub/scm/fs/xfs/xfs-linux.git for-next
patch link:    https://lore.kernel.org/r/20240509104057.1197846-3-john.g.garry%40oracle.com
patch subject: [PATCH 2/2] xfs: Fix xfs_prepare_shift() range for RT
config: xtensa-randconfig-001-20240510 (https://download.01.org/0day-ci/archive/20240510/202405101325.ODONtoFD-lkp@intel.com/config)
compiler: xtensa-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240510/202405101325.ODONtoFD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202405101325.ODONtoFD-lkp@intel.com/

All errors (new ones prefixed by >>):

   `.exit.text' referenced in section `__jump_table' of fs/fuse/inode.o: defined in discarded section `.exit.text' of fs/fuse/inode.o
   `.exit.text' referenced in section `__jump_table' of fs/fuse/inode.o: defined in discarded section `.exit.text' of fs/fuse/inode.o
   xtensa-linux-ld: fs/xfs/xfs_bmap_util.o: in function `xfs_alloc_file_space':
   xfs_bmap_util.c:(.text+0x1dbc): undefined reference to `__moddi3'
   xtensa-linux-ld: fs/xfs/xfs_bmap_util.o: in function `xfs_flush_unmap_range':
   xfs_bmap_util.c:(.text+0x1deb): undefined reference to `__moddi3'
   xtensa-linux-ld: fs/xfs/xfs_bmap_util.o: in function `xfs_alloc_file_space':
   xfs_bmap_util.c:(.text+0x1dc0): undefined reference to `__moddi3'
   xtensa-linux-ld: fs/xfs/xfs_bmap_util.o: in function `xfs_flush_unmap_range':
   xfs_bmap_util.c:(.text+0x1e36): undefined reference to `__moddi3'
>> xtensa-linux-ld: xfs_bmap_util.c:(.text+0x1ea4): undefined reference to `__moddi3'
   xtensa-linux-ld: fs/xfs/xfs_bmap_util.o:xfs_bmap_util.c:(.text+0x1efa): more undefined references to `__moddi3' follow
   `.exit.text' referenced in section `__jump_table' of drivers/misc/phantom.o: defined in discarded section `.exit.text' of drivers/misc/phantom.o
   `.exit.text' referenced in section `__jump_table' of drivers/misc/phantom.o: defined in discarded section `.exit.text' of drivers/misc/phantom.o
   `.exit.text' referenced in section `__jump_table' of drivers/mtd/maps/pcmciamtd.o: defined in discarded section `.exit.text' of drivers/mtd/maps/pcmciamtd.o
   `.exit.text' referenced in section `__jump_table' of drivers/mtd/maps/pcmciamtd.o: defined in discarded section `.exit.text' of drivers/mtd/maps/pcmciamtd.o

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[John Garry]

On 10/05/2024 00:40, Dave Chinner wrote:
>>   
>> -	rounding = max_t(xfs_off_t, mp->m_sb.sb_blocksize, PAGE_SIZE);
>> -	start = round_down(offset, rounding);
>> -	end = round_up(offset + len, rounding) - 1;
>> +	/*
>> +	 * 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 = rounddown(offset, rounding);
>> +	end = roundup(offset + len, rounding) - 1;

I have to admit that I am not the biggest fan of this rounding API.

So round_{down, up}() happens to handle 64b, but round{down, up} doesn't :(

And that is not to mention the vague naming.

> These are 64 bit values, so roundup_64() and rounddown_64().

yeah, thanks.

I can't help but think such a thing should be part of core kernel API.

Thanks,
John