| Message ID | 4412cece5c3f2175fa076a3b29fe6d0bb4c43a6e.1732126365.git.nirjhar@linux.ibm.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | Addition of new tests for extsize hints | expand |
Nirjhar Roy <nirjhar@linux.ibm.com> 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@linux.ibm.com> > Signed-off-by: Nirjhar Roy <nirjhar@linux.ibm.com> > --- > 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? > + _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? > +} > + > + ^^ Extra newline. > # Write a byte into a range of a file > _pwrite_byte() { > local pattern="$1" > -- > 2.43.5
On 11/21/24 13:23, Ritesh Harjani (IBM) wrote: > Nirjhar Roy <nirjhar@linux.ibm.com> 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@linux.ibm.com> >> Signed-off-by: Nirjhar Roy <nirjhar@linux.ibm.com> >> --- >> 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. > >> +} >> + >> + > ^^ Extra newline. Noted. I will fix this. --NR > >> # Write a byte into a range of a file >> _pwrite_byte() { >> local pattern="$1" >> -- >> 2.43.5
Nirjhar Roy <nirjhar@linux.ibm.com> writes: > On 11/21/24 13:23, Ritesh Harjani (IBM) wrote: >> Nirjhar Roy <nirjhar@linux.ibm.com> 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@linux.ibm.com> >>> Signed-off-by: Nirjhar Roy <nirjhar@linux.ibm.com> >>> --- >>> 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. -ritesh
On Fri, Nov 22, 2024 at 12:22:41AM +0530, Ritesh Harjani wrote: > Nirjhar Roy <nirjhar@linux.ibm.com> writes: > > > On 11/21/24 13:23, Ritesh Harjani (IBM) wrote: > >> Nirjhar Roy <nirjhar@linux.ibm.com> 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@linux.ibm.com> > >>> Signed-off-by: Nirjhar Roy <nirjhar@linux.ibm.com> > >>> --- > >>> 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 > -ritesh >
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@linux.ibm.com> writes: >> >>> On 11/21/24 13:23, Ritesh Harjani (IBM) wrote: >>>> Nirjhar Roy <nirjhar@linux.ibm.com> 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@linux.ibm.com> >>>>> Signed-off-by: Nirjhar Roy <nirjhar@linux.ibm.com> >>>>> --- >>>>> 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 >>
On Fri, Nov 22, 2024 at 11:37:17PM +0530, Nirjhar Roy wrote: > > 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@linux.ibm.com> writes: > > > > > > > On 11/21/24 13:23, Ritesh Harjani (IBM) wrote: > > > > > Nirjhar Roy <nirjhar@linux.ibm.com> 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@linux.ibm.com> > > > > > > Signed-off-by: Nirjhar Roy <nirjhar@linux.ibm.com> > > > > > > --- > > > > > > 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? You don't have to check fsx_flags if you don't use fileattr_fill_xflags. ext4 doesn't use that. --D > --NR > > > > > > > > -ritesh > > > > -- > --- > Nirjhar Roy > Linux Kernel Developer > IBM, Bangalore >
On 11/23/24 00:14, Darrick J. Wong wrote: > On Fri, Nov 22, 2024 at 11:37:17PM +0530, Nirjhar Roy wrote: >> 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@linux.ibm.com> writes: >>>> >>>>> On 11/21/24 13:23, Ritesh Harjani (IBM) wrote: >>>>>> Nirjhar Roy <nirjhar@linux.ibm.com> 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@linux.ibm.com> >>>>>>> Signed-off-by: Nirjhar Roy <nirjhar@linux.ibm.com> >>>>>>> --- >>>>>>> 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? > You don't have to check fsx_flags if you don't use fileattr_fill_xflags. > ext4 doesn't use that. > > --D Okay got it. Thank you. How does the overall logic of the _require_scratch_extsize() look? > >> --NR >> >> >> >>>> -ritesh >>>> >> -- >> --- >> Nirjhar Roy >> Linux Kernel Developer >> IBM, Bangalore >>
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 + _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" +} + + # Write a byte into a range of a file _pwrite_byte() { local pattern="$1"