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

[Ritesh Harjani (IBM) <ritesh.list@gmail.com>]

"Darrick J. Wong" <djwong@kernel.org> writes:

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

Sure make sense. I will use Makefile to detect if we have support of
HAVE_PWRITEV2 to also define HAVE_PREADV2 (instead of using autoconf).


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

Ohk, sorry about that. Let me fix these in v2.

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

That won't happen. Since case 'U' to take input flags is defined under
#ifdef HAVE_PREADV2. So if someone tries to set preadv2_flags using -U
when HAVE_PREADV2 is not true, it will go into default case where we
will use exitcode 1 and print command_usage()


> --D
>

Thanks for the quick review. 

-ritesh


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