Re: [PATCH 1/6] generic/765: fix a few issues

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, May 14, 2025 at 01:47:20PM +0100, John Garry wrote:
> On 14/05/2025 01:29, Catherine Hoang wrote:
> > From: "Darrick J. Wong" <djwong@kernel.org>
> > 
> > Fix a few bugs in the single block atomic writes test, such as requiring
> > directio, using page size for the ext4 max bsize, and making sure we check
> > the max atomic write size.
> > 
> > Cc: ritesh.list@gmail.com
> > Signed-off-by: Catherine Hoang <catherine.hoang@oracle.com>
> > Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
> > Signed-off-by: "Darrick J. Wong" <djwong@kernel.org>
> > ---
> >   common/rc         | 2 +-
> >   tests/generic/765 | 4 ++--
> >   2 files changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/common/rc b/common/rc
> > index 657772e7..bc8dabc5 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -2989,7 +2989,7 @@ _require_xfs_io_command()
> >   		fi
> >   		if [ "$param" == "-A" ]; then
> >   			opts+=" -d"
> > -			pwrite_opts+="-D -V 1 -b 4k"
> > +			pwrite_opts+="-d -V 1 -b 4k"
> 
> according to the documentation for -b, 4096 is the default (so I don't think
> that we need to set it explicitly). But is that flag even relevant to
> pwritev2?

The documentation is wrong -- on XFS the default is the fs blocksize.
Everywhere else is 4k.

> And setting -d in pwrite_opts means DIO for the input file, right? I am not
> sure if that is required.

It's not required, I mistook where that "-d" goes -- -d as an argument
to xfs_io is necessary, but -d as an argument to the pwrite subcommand
is not.  It's also benign since we don't pass -i.

Curiously the version of this patch in my tree doesn't have the extra
-d... I wonder if I made that change and forgot to send it out.

--D

> >   		fi
> >   		testio=`$XFS_IO_PROG -f $opts -c \
> >   		        "pwrite $pwrite_opts $param 0 4k" $testfile 2>&1`
> > diff --git a/tests/generic/765 b/tests/generic/765
> > index 9bab3b8a..8695a306 100755
> > --- a/tests/generic/765
> > +++ b/tests/generic/765
> > @@ -28,7 +28,7 @@ get_supported_bsize()
> >           ;;
> >       "ext4")
> >           min_bsize=1024
> > -        max_bsize=4096
> > +        max_bsize=$(_get_page_size)
> 
> looks ok
> 
> >           ;;
> >       *)
> >           _notrun "$FSTYP does not support atomic writes"
> > @@ -73,7 +73,7 @@ test_atomic_writes()
> >       # Check that atomic min/max = FS block size
> >       test $file_min_write -eq $bsize || \
> >           echo "atomic write min $file_min_write, should be fs block size $bsize"
> > -    test $file_min_write -eq $bsize || \
> > +    test $file_max_write -eq $bsize || \
> 
> looks ok
> 
> >           echo "atomic write max $file_max_write, should be fs block size $bsize"
> >       test $file_max_segments -eq 1 || \
> >           echo "atomic write max segments $file_max_segments, should be 1"
> 
> 
> Thanks,
> John
>