[PATCH] xfs: allow changing extsize on file

[PATCH] xfs: allow changing extsize on file

[Wengang Wang]

Hi Dave, this is more a question than a patch.

We are current disallowing the change of extsize on files/dirs if the file/dir
have blocks allocated. That's not that friendly to users. Say somehow the
extsize was set very huge (1GiB), in the following cases, it's not that
convenient:
case 1: the file now extends very little. -- 1GiB extsize leads a waste of
        almost 1GiB.
case 2: when CoW happens, 1GiB is preallocated. 1GiB is now too big for the
        IO pattern, so the huge preallocting and then reclaiming is not necessary
        and that cost extra time especially when the system if fragmented.

In above cases, changing extsize smaller is needed.

In theory, the exthint is a hint for future allocation, I can't connect it
to the blocks which are already allocated to the file/dir.
So the only reason why we disallow that is that there might be some problems if
we allow it.  Well, can we fix the real problem(s) rather than disallowing
extsize changing?

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
---
 fs/xfs/xfs_ioctl.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/xfs/xfs_ioctl.c b/fs/xfs/xfs_ioctl.c
index d0e2cec6210d..b34992d9932f 100644
--- a/fs/xfs/xfs_ioctl.c
+++ b/fs/xfs/xfs_ioctl.c
@@ -1221,8 +1221,7 @@ xfs_ioctl_setattr_get_trans(
 }
 
 /*
- * Validate a proposed extent size hint.  For regular files, the hint can only
- * be changed if no extents are allocated.
+ * Validate a proposed extent size hint.
  */
 static int
 xfs_ioctl_setattr_check_extsize(
@@ -1236,10 +1235,6 @@ xfs_ioctl_setattr_check_extsize(
 	if (!fa->fsx_valid)
 		return 0;
 
-	if (S_ISREG(VFS_I(ip)->i_mode) && ip->i_df.if_nextents &&
-	    XFS_FSB_TO_B(mp, ip->i_extsize) != fa->fsx_extsize)
-		return -EINVAL;
-
 	if (fa->fsx_extsize & mp->m_blockmask)
 		return -EINVAL;
 
-- 
2.39.3 (Apple Git-146)

Re: [PATCH] xfs: allow changing extsize on file

[Dave Chinner]

On Wed, May 08, 2024 at 10:03:43AM -0700, Wengang Wang wrote:
> Hi Dave, this is more a question than a patch.
> 
> We are current disallowing the change of extsize on files/dirs if the file/dir
> have blocks allocated. That's not that friendly to users. Say somehow the
> extsize was set very huge (1GiB), in the following cases, it's not that

The first problem is ensuring that "say somehow extsize was set very
huge" doesn't happen in the first place. Then all the other problems
just don't happen.

> convenient:
> case 1: the file now extends very little. -- 1GiB extsize leads a waste of
>         almost 1GiB.
> case 2: when CoW happens, 1GiB is preallocated. 1GiB is now too big for the
>         IO pattern, so the huge preallocting and then reclaiming is not necessary
>         and that cost extra time especially when the system if fragmented.
> 
> In above cases, changing extsize smaller is needed.
> 
> In theory, the exthint is a hint for future allocation,

It's not that simple because future allocation is influenced by past
allocation. e.g. What happens if the new extent size hint is not
aligned with the old one and we now have two different extent
alignments in the file?

What happens if an admin sees this when trying to triage some
other problem and doesn't know that the extent size hint has been
changed? They'll think there is a bug in the filesystem allocator
and report it.

What do we do with that report now? Do we waste hours trying to
reproduce it and fail, maybe never learning that the an extent
size hint change caused the issue? i.e. how do we determine that the
issue is a real allocation alignment bug versus it simply being a
result of "application did something whacky with extent size hints"?

Hence allowing extent size hints to change dynamically basically
makes it impossible to trust that the current extent size hint
defines the alignment for all the extents in the file. And at that
point, we completely lose the ability to triage allocation alignment
issues without an exact reproducer from the reporter...

Now, just disabling extent size hints avoids this issue (i.e. allow
return to zero if extents already exist) because there's now no
alignment restriction at all and nobody is going to care. However,
this creates new issues.

e.g it opens up the possibility that applications will scan existing
files for extent size hints set on them and be able to -override the
admin set alignment hints- used to create the data set.

The admin may have set inheritable extent size hints to ensure
allocation alignment to underlying storage because the applications
don't know about optimal storage alignments (e.g. for PMD alignment
on DAX storage). We don't want applications to be able to disable
these hints because the precise reason they are set is to optimise
storage alignment for better application performance....

IOWs, there are good reasons for not allowing extent size hints to
be overrridden by applications just by clearing/changing the inode
extent size field...

> I can't connect it
> to the blocks which are already allocated to the file/dir.
> So the only reason why we disallow that is that there might be some problems if
> we allow it.  Well, can we fix the real problem(s) rather than disallowing
> extsize changing?

The only reliable way to change extent size hints so allocation
alignment always matches the new extent size hint is to physically
realign the data in the file to the new extent size hint. i.e. do it
through xfs_fsr to "defrag" the file according to the new extent
size hint. Then when we swap the old and new data extents, we also
set the new extent size hint that matches the new data extents.

This extent size hint change is then enabled through a completely
different interface which is not one applications will use in
general operation. Hence it becomes an explicit admin operation,
enabling users to rectify the rare problems you document above
without compromising the existing behaviour of extent size hints for
everyone else.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH] xfs: allow changing extsize on file

[Wengang Wang]

Thanks Dave for replying. I will think about it.

Wengang

> On May 10, 2024, at 6:56 PM, Dave Chinner <david@fromorbit.com> wrote:
> 
> On Wed, May 08, 2024 at 10:03:43AM -0700, Wengang Wang wrote:
>> Hi Dave, this is more a question than a patch.
>> 
>> We are current disallowing the change of extsize on files/dirs if the file/dir
>> have blocks allocated. That's not that friendly to users. Say somehow the
>> extsize was set very huge (1GiB), in the following cases, it's not that
> 
> The first problem is ensuring that "say somehow extsize was set very
> huge" doesn't happen in the first place. Then all the other problems
> just don't happen.
> 
>> convenient:
>> case 1: the file now extends very little. -- 1GiB extsize leads a waste of
>>        almost 1GiB.
>> case 2: when CoW happens, 1GiB is preallocated. 1GiB is now too big for the
>>        IO pattern, so the huge preallocting and then reclaiming is not necessary
>>        and that cost extra time especially when the system if fragmented.
>> 
>> In above cases, changing extsize smaller is needed.
>> 
>> In theory, the exthint is a hint for future allocation,
> 
> It's not that simple because future allocation is influenced by past
> allocation. e.g. What happens if the new extent size hint is not
> aligned with the old one and we now have two different extent
> alignments in the file?
> 
> What happens if an admin sees this when trying to triage some
> other problem and doesn't know that the extent size hint has been
> changed? They'll think there is a bug in the filesystem allocator
> and report it.
> 
> What do we do with that report now? Do we waste hours trying to
> reproduce it and fail, maybe never learning that the an extent
> size hint change caused the issue? i.e. how do we determine that the
> issue is a real allocation alignment bug versus it simply being a
> result of "application did something whacky with extent size hints"?
> 
> Hence allowing extent size hints to change dynamically basically
> makes it impossible to trust that the current extent size hint
> defines the alignment for all the extents in the file. And at that
> point, we completely lose the ability to triage allocation alignment
> issues without an exact reproducer from the reporter...
> 
> Now, just disabling extent size hints avoids this issue (i.e. allow
> return to zero if extents already exist) because there's now no
> alignment restriction at all and nobody is going to care. However,
> this creates new issues.
> 
> e.g it opens up the possibility that applications will scan existing
> files for extent size hints set on them and be able to -override the
> admin set alignment hints- used to create the data set.
> 
> The admin may have set inheritable extent size hints to ensure
> allocation alignment to underlying storage because the applications
> don't know about optimal storage alignments (e.g. for PMD alignment
> on DAX storage). We don't want applications to be able to disable
> these hints because the precise reason they are set is to optimise
> storage alignment for better application performance....
> 
> IOWs, there are good reasons for not allowing extent size hints to
> be overrridden by applications just by clearing/changing the inode
> extent size field...
> 
>> I can't connect it
>> to the blocks which are already allocated to the file/dir.
>> So the only reason why we disallow that is that there might be some problems if
>> we allow it.  Well, can we fix the real problem(s) rather than disallowing
>> extsize changing?
> 
> The only reliable way to change extent size hints so allocation
> alignment always matches the new extent size hint is to physically
> realign the data in the file to the new extent size hint. i.e. do it
> through xfs_fsr to "defrag" the file according to the new extent
> size hint. Then when we swap the old and new data extents, we also
> set the new extent size hint that matches the new data extents.
> 
> This extent size hint change is then enabled through a completely
> different interface which is not one applications will use in
> general operation. Hence it becomes an explicit admin operation,
> enabling users to rectify the rare problems you document above
> without compromising the existing behaviour of extent size hints for
> everyone else.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH] xfs: allow changing extsize on file

[kernel test robot]

hi, Wengang Wang,

we noticed "this is more a question than a patch."
but not sure if below report could supply any useful information to you, so
still send FYI what we observed in our tests.


Hello,

kernel test robot noticed "xfstests.xfs.207.fail" on:

commit: ab25dcd364e632586fe0bd3a2cdb194e03f5b484 ("[PATCH] xfs: allow changing extsize on file")
url: https://github.com/intel-lab-lkp/linux/commits/Wengang-Wang/xfs-allow-changing-extsize-on-file/20240509-010818
base: https://git.kernel.org/cgit/fs/xfs/xfs-linux.git for-next
patch link: https://lore.kernel.org/all/20240508170343.2774-1-wen.gang.wang@oracle.com/
patch subject: [PATCH] xfs: allow changing extsize on file

in testcase: xfstests
version: xfstests-x86_64-b26d68da-1_20240517
with following parameters:

	disk: 4HDD
	fs: xfs
	test: xfs-207



compiler: gcc-13
test machine: 4 threads Intel(R) Xeon(R) CPU E3-1225 v5 @ 3.30GHz (Skylake) with 16G memory

(please refer to attached dmesg/kmsg for entire log/backtrace)




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 <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202405201028.ca3b53bf-oliver.sang@intel.com

2024-05-17 16:19:24 export TEST_DIR=/fs/sda1
2024-05-17 16:19:24 export TEST_DEV=/dev/sda1
2024-05-17 16:19:24 export FSTYP=xfs
2024-05-17 16:19:24 export SCRATCH_MNT=/fs/scratch
2024-05-17 16:19:24 mkdir /fs/scratch -p
2024-05-17 16:19:24 export SCRATCH_DEV=/dev/sda4
2024-05-17 16:19:24 export SCRATCH_LOGDEV=/dev/sda2
2024-05-17 16:19:24 export SCRATCH_XFS_LIST_METADATA_FIELDS=u3.sfdir3.hdr.parent.i4
2024-05-17 16:19:24 export SCRATCH_XFS_LIST_FUZZ_VERBS=random
2024-05-17 16:19:24 export MKFS_OPTIONS=-mreflink=1
2024-05-17 16:19:24 echo xfs/207
2024-05-17 16:19:24 ./check xfs/207
FSTYP         -- xfs (debug)
PLATFORM      -- Linux/x86_64 lkp-skl-d06 6.9.0-rc4-00248-gab25dcd364e6 #1 SMP PREEMPT_DYNAMIC Fri May 17 14:27:24 CST 2024
MKFS_OPTIONS  -- -f -mreflink=1 /dev/sda4
MOUNT_OPTIONS -- /dev/sda4 /fs/scratch

xfs/207       - output mismatch (see /lkp/benchmarks/xfstests/results//xfs/207.out.bad)
    --- tests/xfs/207.out	2024-05-17 14:38:40.000000000 +0000
    +++ /lkp/benchmarks/xfstests/results//xfs/207.out.bad	2024-05-17 16:20:09.902112701 +0000
    @@ -3,12 +3,11 @@
     Create the original files
     Set extsz and cowextsz on zero byte file
     Set extsz and cowextsz on 1Mbyte file
    -xfs_io: FS_IOC_FSSETXATTR SCRATCH_MNT/test-207/file2: Invalid argument
     Check extsz and cowextsz settings on zero byte file
     [1048576] SCRATCH_MNT/test-207/file1
     [1048576] SCRATCH_MNT/test-207/file1
    ...
    (Run 'diff -u /lkp/benchmarks/xfstests/tests/xfs/207.out /lkp/benchmarks/xfstests/results//xfs/207.out.bad'  to see the entire diff)
Ran: xfs/207
Failures: xfs/207
Failed 1 of 1 tests




The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20240520/202405201028.ca3b53bf-oliver.sang@intel.com



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