Re: [PATCH 4/4] aio: use iovec array rather than the single one

Re: [PATCH 4/4] aio: use iovec array rather than the single one

[Jeff Moyer]

Gu Zheng <guz.fnst@cn.fujitsu.com> writes:

> use an iovec array rather than the single one, so that we can avoid
> to alloc more iovecs buffer in small(< 8) PREADV/PWRITEV cases.

It would be helpful to know what motivated this change and how you
tested it.

Thanks,
Jeff

Re: [PATCH 4/4] aio: use iovec array rather than the single one

[Jeff Moyer]

Gu Zheng <guz.fnst@cn.fujitsu.com> writes:

> use an iovec array rather than the single one, so that we can avoid
> to alloc more iovecs buffer in small(< 8) PREADV/PWRITEV cases.

I did some basic functional testing of this change and the change in
patch 1/4.  That testing included using aio-stress to drive queue depths
of 7, 8 and 9, and verify that it didn't fall over.  I also ran xfstests
'./check -g aio', and libaio's 'make partcheck'.

The change looks good to me, and passed testing, so:

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

However, I still would like some comment on the reasoning behind it, and
whether there is some measurable performance advantage for some
workload.  Additionally, it would be nice if that comment made its way
into the commit message.

Cheers,
Jeff

>
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> ---
>  fs/aio.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/fs/aio.c b/fs/aio.c
> index 0cd0479..ef21efe 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1260,12 +1260,12 @@ static ssize_t aio_setup_vectored_rw(struct kiocb *kiocb,
>  	if (compat)
>  		ret = compat_rw_copy_check_uvector(rw,
>  				(struct compat_iovec __user *)buf,
> -				*nr_segs, 1, *iovec, iovec);
> +				*nr_segs, UIO_FASTIOV, *iovec, iovec);
>  	else
>  #endif
>  		ret = rw_copy_check_uvector(rw,
>  				(struct iovec __user *)buf,
> -				*nr_segs, 1, *iovec, iovec);
> +				*nr_segs, UIO_FASTIOV, *iovec, iovec);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -1302,7 +1302,7 @@ static ssize_t aio_run_iocb(struct kiocb *req, unsigned opcode,
>  	fmode_t mode;
>  	aio_rw_op *rw_op;
>  	rw_iter_op *iter_op;
> -	struct iovec inline_vec, *iovec = &inline_vec;
> +	struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;
>  	struct iov_iter iter;
>  
>  	switch (opcode) {
> @@ -1337,7 +1337,7 @@ rw_common:
>  		if (!ret)
>  			ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);
>  		if (ret < 0) {
> -			if (iovec != &inline_vec)
> +			if (iovec != inline_vecs)
>  				kfree(iovec);
>  			return ret;
>  		}
> @@ -1384,7 +1384,7 @@ rw_common:
>  		return -EINVAL;
>  	}
>  
> -	if (iovec != &inline_vec)
> +	if (iovec != inline_vecs)
>  		kfree(iovec);
>  
>  	if (ret != -EIOCBQUEUED) {

Re: [PATCH 4/4] aio: use iovec array rather than the single one

[Gu Zheng]

Hi Jeff,
On 07/22/2014 11:20 PM, Jeff Moyer wrote:

> Gu Zheng <guz.fnst@cn.fujitsu.com> writes:
> 
>> use an iovec array rather than the single one, so that we can avoid
>> to alloc more iovecs buffer in small(< 8) PREADV/PWRITEV cases.
> 
> I did some basic functional testing of this change and the change in
> patch 1/4.  That testing included using aio-stress to drive queue depths
> of 7, 8 and 9, and verify that it didn't fall over.  I also ran xfstests
> './check -g aio', and libaio's 'make partcheck'.

> 
> The change looks good to me, and passed testing, so:
> 
> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

Thanks for your review and test.

> 
> However, I still would like some comment on the reasoning behind it, and
> whether there is some measurable performance advantage for some
> workload.  Additionally, it would be nice if that comment made its way
> into the commit message.

I'll add more useful info, and send it out later.

Thanks,
Gu

> 
> Cheers,
> Jeff
> 
>>
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>> ---
>>  fs/aio.c |   10 +++++-----
>>  1 files changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/aio.c b/fs/aio.c
>> index 0cd0479..ef21efe 100644
>> --- a/fs/aio.c
>> +++ b/fs/aio.c
>> @@ -1260,12 +1260,12 @@ static ssize_t aio_setup_vectored_rw(struct kiocb *kiocb,
>>  	if (compat)
>>  		ret = compat_rw_copy_check_uvector(rw,
>>  				(struct compat_iovec __user *)buf,
>> -				*nr_segs, 1, *iovec, iovec);
>> +				*nr_segs, UIO_FASTIOV, *iovec, iovec);
>>  	else
>>  #endif
>>  		ret = rw_copy_check_uvector(rw,
>>  				(struct iovec __user *)buf,
>> -				*nr_segs, 1, *iovec, iovec);
>> +				*nr_segs, UIO_FASTIOV, *iovec, iovec);
>>  	if (ret < 0)
>>  		return ret;
>>  
>> @@ -1302,7 +1302,7 @@ static ssize_t aio_run_iocb(struct kiocb *req, unsigned opcode,
>>  	fmode_t mode;
>>  	aio_rw_op *rw_op;
>>  	rw_iter_op *iter_op;
>> -	struct iovec inline_vec, *iovec = &inline_vec;
>> +	struct iovec inline_vecs[UIO_FASTIOV], *iovec = inline_vecs;
>>  	struct iov_iter iter;
>>  
>>  	switch (opcode) {
>> @@ -1337,7 +1337,7 @@ rw_common:
>>  		if (!ret)
>>  			ret = rw_verify_area(rw, file, &req->ki_pos, req->ki_nbytes);
>>  		if (ret < 0) {
>> -			if (iovec != &inline_vec)
>> +			if (iovec != inline_vecs)
>>  				kfree(iovec);
>>  			return ret;
>>  		}
>> @@ -1384,7 +1384,7 @@ rw_common:
>>  		return -EINVAL;
>>  	}
>>  
>> -	if (iovec != &inline_vec)
>> +	if (iovec != inline_vecs)
>>  		kfree(iovec);
>>  
>>  	if (ret != -EIOCBQUEUED) {
> .
> 

Re: [PATCH 1/4] aio: remove the needless registration of ring file's private_data

[Jeff Moyer]

Gu Zheng <guz.fnst@cn.fujitsu.com> writes:

> Remove the registration of ring file's private_data, we do not use
> it.
>
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

> ---
>  fs/aio.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
>
> diff --git a/fs/aio.c b/fs/aio.c
> index 955947e..ad35876 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -192,7 +192,6 @@ static struct file *aio_private_file(struct kioctx *ctx, loff_t nr_pages)
>  	}
>  
>  	file->f_flags = O_RDWR;
> -	file->private_data = ctx;
>  	return file;
>  }

Re: [PATCH 2/4] aio: use the macro rather than the inline magic number

[Jeff Moyer]

Gu Zheng <guz.fnst@cn.fujitsu.com> writes:

> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

> ---
>  fs/aio.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/fs/aio.c b/fs/aio.c
> index ad35876..1dc6158 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -201,7 +201,7 @@ static struct dentry *aio_mount(struct file_system_type *fs_type,
>  	static const struct dentry_operations ops = {
>  		.d_dname	= simple_dname,
>  	};
> -	return mount_pseudo(fs_type, "aio:", NULL, &ops, 0xa10a10a1);
> +	return mount_pseudo(fs_type, "aio:", NULL, &ops, AIO_RING_MAGIC);
>  }
>  
>  /* aio_setup

Re: [PATCH 2/4] aio: use the macro rather than the inline magic number

[Ming Lei]

On Tue, Jul 22, 2014 at 9:54 PM, Jeff Moyer <jmoyer@redhat.com> wrote:
> Gu Zheng <guz.fnst@cn.fujitsu.com> writes:
>
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>
> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
>
>> ---
>>  fs/aio.c |    2 +-
>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/fs/aio.c b/fs/aio.c
>> index ad35876..1dc6158 100644
>> --- a/fs/aio.c
>> +++ b/fs/aio.c
>> @@ -201,7 +201,7 @@ static struct dentry *aio_mount(struct file_system_type *fs_type,
>>       static const struct dentry_operations ops = {
>>               .d_dname        = simple_dname,
>>       };
>> -     return mount_pseudo(fs_type, "aio:", NULL, &ops, 0xa10a10a1);
>> +     return mount_pseudo(fs_type, "aio:", NULL, &ops, AIO_RING_MAGIC);

IMO, it might be better to use a new macro like AIO_FS_MAGIC
with same number here because the same magic number
0xa10a10a1 serves for two purposes.

Thanks,

Re: [PATCH 2/4] aio: use the macro rather than the inline magic number

[Gu Zheng]

Hi Lei,
On 07/25/2014 11:13 AM, Ming Lei wrote:

> On Tue, Jul 22, 2014 at 9:54 PM, Jeff Moyer <jmoyer@redhat.com> wrote:
>> Gu Zheng <guz.fnst@cn.fujitsu.com> writes:
>>
>>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
>>
>> Reviewed-by: Jeff Moyer <jmoyer@redhat.com>
>>
>>> ---
>>>  fs/aio.c |    2 +-
>>>  1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/fs/aio.c b/fs/aio.c
>>> index ad35876..1dc6158 100644
>>> --- a/fs/aio.c
>>> +++ b/fs/aio.c
>>> @@ -201,7 +201,7 @@ static struct dentry *aio_mount(struct file_system_type *fs_type,
>>>       static const struct dentry_operations ops = {
>>>               .d_dname        = simple_dname,
>>>       };
>>> -     return mount_pseudo(fs_type, "aio:", NULL, &ops, 0xa10a10a1);
>>> +     return mount_pseudo(fs_type, "aio:", NULL, &ops, AIO_RING_MAGIC);
> 
> IMO, it might be better to use a new macro like AIO_FS_MAGIC
> with same number here because the same magic number
> 0xa10a10a1 serves for two purposes.

Sounds reasonable, but I prefer a more common name like
AIO_MAGIC to serve two if we need.:)

Thanks,
Gu

> 
> Thanks,
> .
> 

Re: [PATCH 2/4] aio: use the macro rather than the inline magic number

[Benjamin LaHaise]

On Tue, Jul 22, 2014 at 10:40:02AM +0800, Gu Zheng wrote:
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>

You're missing a commit message here.  The patch is otherwise fine.

		-ben

> ---
>  fs/aio.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index ad35876..1dc6158 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -201,7 +201,7 @@ static struct dentry *aio_mount(struct file_system_type *fs_type,
>  	static const struct dentry_operations ops = {
>  		.d_dname	= simple_dname,
>  	};
> -	return mount_pseudo(fs_type, "aio:", NULL, &ops, 0xa10a10a1);
> +	return mount_pseudo(fs_type, "aio:", NULL, &ops, AIO_RING_MAGIC);
>  }
>  
>  /* aio_setup
> -- 
> 1.7.7

-- 
"Thought is the essence of where you are now."

Re: [PATCH 3/4] aio: fix some comments

[Jeff Moyer]

Gu Zheng <guz.fnst@cn.fujitsu.com> writes:

> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>

Reviewed-by: Jeff Moyer <jmoyer@redhat.com>

> ---
>  fs/aio.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/fs/aio.c b/fs/aio.c
> index 1dc6158..0cd0479 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1037,7 +1037,7 @@ void aio_complete(struct kiocb *iocb, long res, long res2)
>  }
>  EXPORT_SYMBOL(aio_complete);
>  
> -/* aio_read_events
> +/* aio_read_events_ring
>   *	Pull an event off of the ioctx's event ring.  Returns the number of
>   *	events fetched
>   */
> @@ -1289,9 +1289,8 @@ static ssize_t aio_setup_single_vector(struct kiocb *kiocb,
>  }
>  
>  /*
> - * aio_setup_iocb:
> - *	Performs the initial checks and aio retry method
> - *	setup for the kiocb at the time of io submission.
> + * aio_run_iocb:
> + *	Performs the initial checks and io submission.
>   */
>  static ssize_t aio_run_iocb(struct kiocb *req, unsigned opcode,
>  			    char __user *buf, bool compat)

Re: [PATCH 3/4] aio: fix some comments

[Benjamin LaHaise]

On Tue, Jul 22, 2014 at 10:40:03AM +0800, Gu Zheng wrote:
> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>

Again, you're missing a commit message here.  Please resubmit with a commit 
message.

		-ben

> ---
>  fs/aio.c |    7 +++----
>  1 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/aio.c b/fs/aio.c
> index 1dc6158..0cd0479 100644
> --- a/fs/aio.c
> +++ b/fs/aio.c
> @@ -1037,7 +1037,7 @@ void aio_complete(struct kiocb *iocb, long res, long res2)
>  }
>  EXPORT_SYMBOL(aio_complete);
>  
> -/* aio_read_events
> +/* aio_read_events_ring
>   *	Pull an event off of the ioctx's event ring.  Returns the number of
>   *	events fetched
>   */
> @@ -1289,9 +1289,8 @@ static ssize_t aio_setup_single_vector(struct kiocb *kiocb,
>  }
>  
>  /*
> - * aio_setup_iocb:
> - *	Performs the initial checks and aio retry method
> - *	setup for the kiocb at the time of io submission.
> + * aio_run_iocb:
> + *	Performs the initial checks and io submission.
>   */
>  static ssize_t aio_run_iocb(struct kiocb *req, unsigned opcode,
>  			    char __user *buf, bool compat)
> -- 
> 1.7.7

-- 
"Thought is the essence of where you are now."

Re: [PATCH 3/4] aio: fix some comments

[Gu Zheng]

On 07/22/2014 10:12 PM, Benjamin LaHaise wrote:

> On Tue, Jul 22, 2014 at 10:40:03AM +0800, Gu Zheng wrote:
>> Signed-off-by: Gu Zheng <guz.fnst@cn.fujitsu.com>
> 
> Again, you're missing a commit message here.  Please resubmit with a commit 
> message.

Got it, I'll resend it later.

Thanks,
Gu

> 
> 		-ben
> 
>> ---
>>  fs/aio.c |    7 +++----
>>  1 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/aio.c b/fs/aio.c
>> index 1dc6158..0cd0479 100644
>> --- a/fs/aio.c
>> +++ b/fs/aio.c
>> @@ -1037,7 +1037,7 @@ void aio_complete(struct kiocb *iocb, long res, long res2)
>>  }
>>  EXPORT_SYMBOL(aio_complete);
>>  
>> -/* aio_read_events
>> +/* aio_read_events_ring
>>   *	Pull an event off of the ioctx's event ring.  Returns the number of
>>   *	events fetched
>>   */
>> @@ -1289,9 +1289,8 @@ static ssize_t aio_setup_single_vector(struct kiocb *kiocb,
>>  }
>>  
>>  /*
>> - * aio_setup_iocb:
>> - *	Performs the initial checks and aio retry method
>> - *	setup for the kiocb at the time of io submission.
>> + * aio_run_iocb:
>> + *	Performs the initial checks and io submission.
>>   */
>>  static ssize_t aio_run_iocb(struct kiocb *req, unsigned opcode,
>>  			    char __user *buf, bool compat)
>> -- 
>> 1.7.7
>