Re: [PATCH 2/3] tcm_vhost: Optimize gup in vhost_scsi_map_to_sgl

[Marcelo Tosatti <mtosatti@redhat.com>]

On Mon, Jan 21, 2013 at 04:05:27PM +0800, Asias He wrote:
> We can get all the pages in one time instead of calling
> gup N times.
> 
> Signed-off-by: Asias He <asias@redhat.com>
> ---
>  drivers/vhost/tcm_vhost.c | 33 ++++++++++++++++++++-------------
>  1 file changed, 20 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/vhost/tcm_vhost.c b/drivers/vhost/tcm_vhost.c
> index ca35c16..59be442 100644
> --- a/drivers/vhost/tcm_vhost.c
> +++ b/drivers/vhost/tcm_vhost.c
> @@ -430,37 +430,45 @@ static struct tcm_vhost_cmd *vhost_scsi_allocate_cmd(
>   * Returns the number of scatterlist entries used or -errno on error.
>   */
>  static int vhost_scsi_map_to_sgl(struct scatterlist *sgl,
> -	unsigned int sgl_count, void __user *ptr, size_t len, int write)
> +	unsigned int sgl_count, struct iovec *iov, int write)
>  {
>  	struct scatterlist *sg = sgl;
>  	unsigned int npages = 0;
> +	void __user *ptr = iov->iov_base;
> +	size_t len = iov->iov_len;
>  	int ret;
> +	unsigned int pages_nr, offset, nbytes;
> +	struct page **pages;
> +
> +	pages_nr = iov_num_pages(iov);
> +	pages = kmalloc(pages_nr * sizeof(struct page *), GFP_ATOMIC);
> +	if (!pages)
> +		return -ENOMEM;
> +
> +	ret = get_user_pages_fast((unsigned long)ptr, pages_nr, write, pages);
> +	if (ret != pages_nr)
> +		goto err;

1. Why GFP_ATOMIC? get_user_pages_fast can sleep, so this path must not
be atomic (if it is, should use __get_user_pages_fast).

GFP_ATOMIC should be avoided.

2. Should drop reference to pages whose refcount has been increased,
if ret > 0 && ret != pages_nr (see last phrase of get_user_pages_fast
commentary).