Re: [PATCH v1 1/3] configure: xfs_io: Add support for preadv2

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

On Tue, Mar 04, 2025 at 05:25:35PM +0530, Ritesh Harjani (IBM) wrote:
> preadv2() was introduced in Linux 4.6. This patch adds support for
> preadv2() to xfs_io.
> 
> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
> ---
>  configure.ac          |  1 +
>  include/builddefs.in  |  1 +
>  io/Makefile           |  4 ++++
>  io/pread.c            | 45 ++++++++++++++++++++++++++++---------------
>  m4/package_libcdev.m4 | 18 +++++++++++++++++
>  5 files changed, 54 insertions(+), 15 deletions(-)
> 
> diff --git a/configure.ac b/configure.ac
> index 8c76f398..658117ad 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -153,6 +153,7 @@ AC_PACKAGE_NEED_URCU_H
>  AC_PACKAGE_NEED_RCU_INIT
>  
>  AC_HAVE_PWRITEV2
> +AC_HAVE_PREADV2

I wonder, will we ever encounter a C library that has pwritev2 and /not/
preadv2?

>  AC_HAVE_COPY_FILE_RANGE
>  AC_NEED_INTERNAL_FSXATTR
>  AC_NEED_INTERNAL_FSCRYPT_ADD_KEY_ARG
> diff --git a/include/builddefs.in b/include/builddefs.in
> index 82840ec7..a11d201c 100644
> --- a/include/builddefs.in
> +++ b/include/builddefs.in
> @@ -94,6 +94,7 @@ ENABLE_SCRUB	= @enable_scrub@
>  HAVE_ZIPPED_MANPAGES = @have_zipped_manpages@
>  
>  HAVE_PWRITEV2 = @have_pwritev2@
> +HAVE_PREADV2 = @have_preadv2@
>  HAVE_COPY_FILE_RANGE = @have_copy_file_range@
>  NEED_INTERNAL_FSXATTR = @need_internal_fsxattr@
>  NEED_INTERNAL_FSCRYPT_ADD_KEY_ARG = @need_internal_fscrypt_add_key_arg@
> diff --git a/io/Makefile b/io/Makefile
> index 8f835ec7..f8b19ac5 100644
> --- a/io/Makefile
> +++ b/io/Makefile
> @@ -69,6 +69,10 @@ ifeq ($(HAVE_PWRITEV2),yes)
>  LCFLAGS += -DHAVE_PWRITEV2
>  endif
>  
> +ifeq ($(HAVE_PREADV2),yes)
> +LCFLAGS += -DHAVE_PREADV2
> +endif
> +
>  ifeq ($(HAVE_MAP_SYNC),yes)
>  LCFLAGS += -DHAVE_MAP_SYNC
>  endif
> diff --git a/io/pread.c b/io/pread.c
> index 62c771fb..782f2a36 100644
> --- a/io/pread.c
> +++ b/io/pread.c
> @@ -162,7 +162,8 @@ static ssize_t
>  do_preadv(
>  	int		fd,
>  	off_t		offset,
> -	long long	count)
> +	long long	count,
> +	int 		preadv2_flags)

Nit:       ^ space before tab.  There's a bunch more of thense, every
time a "preadv2_flags" variable or parameter are declared.

>  {
>  	int		vecs = 0;
>  	ssize_t		oldlen = 0;
> @@ -181,8 +182,14 @@ do_preadv(
>  	} else {
>  		vecs = vectors;
>  	}
> +#ifdef HAVE_PREADV2
> +	if (preadv2_flags)
> +		bytes = preadv2(fd, iov, vectors, offset, preadv2_flags);
> +	else
> +		bytes = preadv(fd, iov, vectors, offset);
> +#else
>  	bytes = preadv(fd, iov, vectors, offset);
> -
> +#endif

Can we have the case that preadv2_flags!=0 and HAVE_PREADV2 isn't
defined?  If so, then there ought to be a warning about that.

--D

>  	/* restore trimmed iov */
>  	if (oldlen)
>  		iov[vecs - 1].iov_len = oldlen;
> @@ -195,12 +202,13 @@ do_pread(
>  	int		fd,
>  	off_t		offset,
>  	long long	count,
> -	size_t		buffer_size)
> +	size_t		buffer_size,
> +	int 		preadv2_flags)
>  {
>  	if (!vectors)
>  		return pread(fd, io_buffer, min(count, buffer_size), offset);
>  
> -	return do_preadv(fd, offset, count);
> +	return do_preadv(fd, offset, count, preadv2_flags);
>  }
>  
>  static int
> @@ -210,7 +218,8 @@ read_random(
>  	long long	count,
>  	long long	*total,
>  	unsigned int	seed,
> -	int		eof)
> +	int		eof,
> +	int 	preadv2_flags)
>  {
>  	off_t		end, off, range;
>  	ssize_t		bytes;
> @@ -234,7 +243,7 @@ read_random(
>  				io_buffersize;
>  		else
>  			off = offset;
> -		bytes = do_pread(fd, off, io_buffersize, io_buffersize);
> +		bytes = do_pread(fd, off, io_buffersize, io_buffersize, preadv2_flags);
>  		if (bytes == 0)
>  			break;
>  		if (bytes < 0) {
> @@ -256,7 +265,8 @@ read_backward(
>  	off_t		*offset,
>  	long long	*count,
>  	long long	*total,
> -	int		eof)
> +	int		eof,
> +	int 	preadv2_flags)
>  {
>  	off_t		end, off = *offset;
>  	ssize_t		bytes = 0, bytes_requested;
> @@ -276,7 +286,7 @@ read_backward(
>  	/* Do initial unaligned read if needed */
>  	if ((bytes_requested = (off % io_buffersize))) {
>  		off -= bytes_requested;
> -		bytes = do_pread(fd, off, bytes_requested, io_buffersize);
> +		bytes = do_pread(fd, off, bytes_requested, io_buffersize, preadv2_flags);
>  		if (bytes == 0)
>  			return ops;
>  		if (bytes < 0) {
> @@ -294,7 +304,7 @@ read_backward(
>  	while (cnt > end) {
>  		bytes_requested = min(cnt, io_buffersize);
>  		off -= bytes_requested;
> -		bytes = do_pread(fd, off, cnt, io_buffersize);
> +		bytes = do_pread(fd, off, cnt, io_buffersize, preadv2_flags);
>  		if (bytes == 0)
>  			break;
>  		if (bytes < 0) {
> @@ -318,14 +328,15 @@ read_forward(
>  	long long	*total,
>  	int		verbose,
>  	int		onlyone,
> -	int		eof)
> +	int		eof,
> +	int 	preadv2_flags)
>  {
>  	ssize_t		bytes;
>  	int		ops = 0;
>  
>  	*total = 0;
>  	while (count > 0 || eof) {
> -		bytes = do_pread(fd, offset, count, io_buffersize);
> +		bytes = do_pread(fd, offset, count, io_buffersize, preadv2_flags);
>  		if (bytes == 0)
>  			break;
>  		if (bytes < 0) {
> @@ -353,7 +364,7 @@ read_buffer(
>  	int		verbose,
>  	int		onlyone)
>  {
> -	return read_forward(fd, offset, count, total, verbose, onlyone, 0);
> +	return read_forward(fd, offset, count, total, verbose, onlyone, 0, 0);
>  }
>  
>  static int
> @@ -371,6 +382,7 @@ pread_f(
>  	int		Cflag, qflag, uflag, vflag;
>  	int		eof = 0, direction = IO_FORWARD;
>  	int		c;
> +	int 	preadv2_flags = 0;
>  
>  	Cflag = qflag = uflag = vflag = 0;
>  	init_cvtnum(&fsblocksize, &fssectsize);
> @@ -463,15 +475,18 @@ pread_f(
>  	case IO_RANDOM:
>  		if (!zeed)	/* srandom seed */
>  			zeed = time(NULL);
> -		c = read_random(file->fd, offset, count, &total, zeed, eof);
> +		c = read_random(file->fd, offset, count, &total, zeed, eof,
> +						preadv2_flags);
>  		break;
>  	case IO_FORWARD:
> -		c = read_forward(file->fd, offset, count, &total, vflag, 0, eof);
> +		c = read_forward(file->fd, offset, count, &total, vflag, 0, eof,
> +						 preadv2_flags);
>  		if (eof)
>  			count = total;
>  		break;
>  	case IO_BACKWARD:
> -		c = read_backward(file->fd, &offset, &count, &total, eof);
> +		c = read_backward(file->fd, &offset, &count, &total, eof,
> +						  preadv2_flags);
>  		break;
>  	default:
>  		ASSERT(0);
> diff --git a/m4/package_libcdev.m4 b/m4/package_libcdev.m4
> index 4ef7e8f6..5a1f748a 100644
> --- a/m4/package_libcdev.m4
> +++ b/m4/package_libcdev.m4
> @@ -16,6 +16,24 @@ pwritev2(0, 0, 0, 0, 0);
>      AC_SUBST(have_pwritev2)
>    ])
>  
> +#
> +# Check if we have a preadv2 libc call (Linux)
> +#
> +AC_DEFUN([AC_HAVE_PREADV2],
> +  [ AC_MSG_CHECKING([for preadv2])
> +    AC_LINK_IFELSE(
> +    [	AC_LANG_PROGRAM([[
> +#define _GNU_SOURCE
> +#include <sys/uio.h>
> +	]], [[
> +preadv2(0, 0, 0, 0, 0);
> +	]])
> +    ], have_preadv2=yes
> +       AC_MSG_RESULT(yes),
> +       AC_MSG_RESULT(no))
> +    AC_SUBST(have_preadv2)
> +  ])
> +
>  #
>  # Check if we have a copy_file_range system call (Linux)
>  #
> -- 
> 2.48.1
> 
>