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

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

On Wed, May 14, 2025 at 11:42:40PM +0000, Catherine Hoang wrote:
> > On May 14, 2025, at 8:38 AM, Darrick J. Wong <djwong@kernel.org> wrote:
> > 
> > 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.
> 
> Hmm, it might have been from an old patch on my branch
> that I forgot to update when I sent this out. Just to clarify,
> this should just be 
> 
> pwrite_opts+="-V 1 -b 4k”
> 
> right?

Yep.

--D

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