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

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

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

> On Wed, Mar 05, 2025 at 03:57:46PM +0530, Ritesh Harjani (IBM) wrote:
>> This patch adds support for preadv2() to xfs_io.
>> 
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>
>> ---
>>  io/Makefile |  2 +-
>>  io/pread.c  | 45 ++++++++++++++++++++++++++++++---------------
>>  2 files changed, 31 insertions(+), 16 deletions(-)
>> 
>> diff --git a/io/Makefile b/io/Makefile
>> index 8f835ec7..14a3fe20 100644
>> --- a/io/Makefile
>> +++ b/io/Makefile
>> @@ -66,7 +66,7 @@ LLDLIBS += $(LIBEDITLINE) $(LIBTERMCAP)
>>  endif
>>  
>>  ifeq ($(HAVE_PWRITEV2),yes)
>> -LCFLAGS += -DHAVE_PWRITEV2
>> +LCFLAGS += -DHAVE_PWRITEV2 -DHAVE_PREADV2
>>  endif
>>  
>>  ifeq ($(HAVE_MAP_SYNC),yes)
>> diff --git a/io/pread.c b/io/pread.c
>> index 62c771fb..b314fbc7 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)
>>  {
>>  	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
>>  	/* 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)
>
> Too much indenting here ^^ I think?
>

This is how I think git patch is showing. But the indentation is proper
when we apply the patch. In fact the "int fd" param is not properly
aligned to the rest of the params (lacking exactly 1 tab). Otherwise it
would have been easier to compare with "int fd".

> With that fixed,

So I don't think this needs any fixing. 

> Reviewed-by: "Darrick J. Wong" <djwong@kernel.org>
>

Thanks for the review!

-ritesh

> --D
>
>>  {
>>  	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);
>> -- 
>> 2.48.1
>> 
>>