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@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?
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
>
