On 11/22/24 21:34, Darrick J. Wong wrote:
On Fri, Nov 22, 2024 at 12:22:41AM +0530, Ritesh Harjani wrote:
Nirjhar Roy <nirjhar@xxxxxxxxxxxxx> writes:
On 11/21/24 13:23, Ritesh Harjani (IBM) wrote:
Nirjhar Roy <nirjhar@xxxxxxxxxxxxx> writes:
_require_scratch_extsize helper function will be used in the
the next patch to make the test run only on filesystems with
extsize support.
Reviewed-by: Ojaswin Mujoo <ojaswin@xxxxxxxxxxxxx>
Signed-off-by: Nirjhar Roy <nirjhar@xxxxxxxxxxxxx>
---
common/rc | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/common/rc b/common/rc
index cccc98f5..995979e9 100644
--- a/common/rc
+++ b/common/rc
@@ -48,6 +48,23 @@ _test_fsxattr_xflag()
grep -q "fsxattr.xflags.*\[.*$2.*\]" <($XFS_IO_PROG -c "stat -v" "$1")
}
+# This test requires extsize support on the filesystem
+_require_scratch_extsize()
+{
+ _require_scratch
_require_xfs_io_command "extsize"
^^^ Don't we need this too?
Yes, good point. I will add this in the next revision.
+ _scratch_mkfs > /dev/null
+ _scratch_mount
+ local filename=$SCRATCH_MNT/$RANDOM
+ local blksz=$(_get_block_size $SCRATCH_MNT)
+ local extsz=$(( blksz*2 ))
+ local res=$($XFS_IO_PROG -c "open -f $filename" -c "extsize $extsz" \
+ -c "extsize")
+ _scratch_unmount
+ grep -q "\[$extsz\] $filename" <(echo $res) || \
+ _notrun "this test requires extsize support on the filesystem"
Why grep when we can simply just check the return value of previous xfs_io command?
No, I don't think we can rely on the return value of xfs_io. For ex,
let's look at the following set of commands which are ran on an ext4 system:
root@AMARPC: /mnt1/test$ xfs_io -V
xfs_io version 5.13.0
root@AMARPC: /mnt1/test$ touch new
root@AMARPC: /mnt1/test$ xfs_io -c "extsize 8k" new
foreign file active, extsize command is for XFS filesystems only
root@AMARPC: /mnt1/test$ echo "$?"
0
This incorrect return value might have been fixed in some later versions
of xfs_io but there are still versions where we can't solely rely on the
return value.
Ok. That's bad, we then have to rely on grep.
Sure, thanks for checking and confirming that.
You all should add CMD_FOREIGN_OK to the extsize command in xfs_io,
assuming that you've not already done that in your dev workspace.
--D
Yes, I have tested with that as well. I have applied the following patch
to xfsprogs and tested with the modified xfs_io.
diff --git a/io/open.c b/io/open.c
index 15850b55..6407b7e8 100644
--- a/io/open.c
+++ b/io/open.c
@@ -980,7 +980,7 @@ open_init(void)
extsize_cmd.args = _("[-D | -R] [extsize]");
extsize_cmd.argmin = 0;
extsize_cmd.argmax = -1;
- extsize_cmd.flags = CMD_NOMAP_OK;
+ extsize_cmd.flags = CMD_NOMAP_OK | CMD_FOREIGN_OK;
extsize_cmd.oneline =
_("get/set preferred extent size (in bytes) for the
open file");
extsize_cmd.help = extsize_help;
The return values are similar.
root@AMARPC: /mnt1/scratch$ touch new
root@AMARPC: /mnt1/scratch$ /home/ubuntu/xfsprogs-dev/io/xfs_io -c
"extsize 8k" new
root@AMARPC: /mnt1/scratch$ echo "$?"
0
root@AMARPC: /mnt1/scratch$ /home/ubuntu/xfsprogs-dev/io/xfs_io -c
"extsize" new
[0] new
This is the reason I am not relying on the return value, instead I am
checking if only the extsize gets changed, we will assume that the
extsize support is there, else the test will _notrun.
Also,
root@AMARPC: /mnt1/scratch$ strace -f /mnt1/scratch$
/home/ubuntu/xfsprogs-dev/io/xfs_io -c "extsize 16k" new
...
...
ioctl(3, FS_IOC_FSGETXATTR, {fsx_xflags=0, fsx_extsize=0,
fsx_nextents=0, fsx_projid=0, fsx_cowextsize=0}) = 0
ioctl(3, FS_IOC_FSSETXATTR, {fsx_xflags=FS_XFLAG_EXTSIZE,
fsx_extsize=16384, fsx_projid=0, fsx_cowextsize=0}) = 0
exit_group(0)
Looking at the existing code for ext4_fileattr_set(), We validate the
flags but I think we silently don't validate(and ignore) the xflags.
Like, we have
int err = -EOPNOTSUPP;
if (flags & ~EXT4_FL_USER_VISIBLE)
goto out;
BUT we do NOT have something like
int err = -EOPNOTSUPP;
if (fa->fsx_flags & ~EXT4_VALID_XFLAGS) // where EXT4_VALID_XFLAGS
should be an || of all the supported xflags in ext4.
goto out;
I am not sure what other filesystems do, but if we check whether the
extsize got changed, then we can correctly determine extsize support.
Does that make sense?
--NR
-ritesh
--
---
Nirjhar Roy
Linux Kernel Developer
IBM, Bangalore
