Re: [PATCH 03/33] gadget: remove only user of aio retry

[Felipe Balbi]

Hi,

On Thu, Mar 21, 2013 at 09:35:24AM -0700, Kent Overstreet wrote:
> From: Zach Brown <zab@xxxxxxxxxx>
> 
> This removes the only in-tree user of aio retry.  This will let us remove
> the retry code from the aio core.
> 
> Removing retry is relatively easy as the USB gadget wasn't using it to
> retry IOs at all.  It always fully submitted the IO in the context of the
> initial io_submit() call.  It only used the AIO retry facility to get the
> submitter's mm context for copying the result of a read back to user
> space.  This is easy to implement with use_mm() and a work struct, much
> like kvm does with async_pf_execute() for get_user_pages().
> 
> Signed-off-by: Zach Brown <zab@xxxxxxxxxx>
> Signed-off-by: Kent Overstreet <koverstreet@xxxxxxxxxx>
> Cc: Felipe Balbi <balbi@xxxxxx>
> Cc: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> Cc: Mark Fasheh <mfasheh@xxxxxxxx>
> Cc: Joel Becker <jlbec@xxxxxxxxxxxx>
> Cc: Rusty Russell <rusty@xxxxxxxxxxxxxxx>
> Cc: Jens Axboe <axboe@xxxxxxxxx>
> Cc: Asai Thambi S P <asamymuthupa@xxxxxxxxxx>
> Cc: Selvan Mani <smani@xxxxxxxxxx>
> Cc: Sam Bradshaw <sbradshaw@xxxxxxxxxx>
> Cc: Jeff Moyer <jmoyer@xxxxxxxxxx>
> Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
> Cc: Benjamin LaHaise <bcrl@xxxxxxxxx>
> Cc: Theodore Ts'o <tytso@xxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>

I don't have any objections with the approach. Let's see if anyone from
linux-usb has anything to say, though

>  drivers/usb/gadget/inode.c | 38 +++++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/usb/gadget/inode.c b/drivers/usb/gadget/inode.c
> index e2b2e9c..a1aad43 100644
> --- a/drivers/usb/gadget/inode.c
> +++ b/drivers/usb/gadget/inode.c
> @@ -24,6 +24,7 @@
>  #include <linux/sched.h>
>  #include <linux/slab.h>
>  #include <linux/poll.h>
> +#include <linux/mmu_context.h>
>  
>  #include <linux/device.h>
>  #include <linux/moduleparam.h>
> @@ -513,6 +514,9 @@ static long ep_ioctl(struct file *fd, unsigned code, unsigned long value)
>  struct kiocb_priv {
>  	struct usb_request	*req;
>  	struct ep_data		*epdata;
> +	struct kiocb 		*iocb;
> +	struct mm_struct 	*mm;
> +	struct work_struct	work;
>  	void			*buf;
>  	const struct iovec	*iv;
>  	unsigned long		nr_segs;
> @@ -540,15 +544,12 @@ static int ep_aio_cancel(struct kiocb *iocb, struct io_event *e)
>  	return value;
>  }
>  
> -static ssize_t ep_aio_read_retry(struct kiocb *iocb)
> +static ssize_t ep_copy_to_user(struct kiocb_priv *priv)
>  {
> -	struct kiocb_priv	*priv = iocb->private;
>  	ssize_t			len, total;
>  	void			*to_copy;
>  	int			i;
>  
> -	/* we "retry" to get the right mm context for this: */
> -
>  	/* copy stuff into user buffers */
>  	total = priv->actual;
>  	len = 0;
> @@ -568,9 +569,26 @@ static ssize_t ep_aio_read_retry(struct kiocb *iocb)
>  		if (total == 0)
>  			break;
>  	}
> +
> +	return len;
> +}
> +
> +static void ep_user_copy_worker(struct work_struct *work)
> +{
> +	struct kiocb_priv *priv = container_of(work, struct kiocb_priv, work);
> +	struct mm_struct *mm = priv->mm;
> +	struct kiocb *iocb = priv->iocb;
> +	size_t ret;
> +
> +	use_mm(mm);
> +	ret = ep_copy_to_user(priv);
> +	unuse_mm(mm);
> +
> +	/* completing the iocb can drop the ctx and mm, don't touch mm after */
> +	aio_complete(iocb, ret, ret);
> +
>  	kfree(priv->buf);
>  	kfree(priv);
> -	return len;
>  }
>  
>  static void ep_aio_complete(struct usb_ep *ep, struct usb_request *req)
> @@ -596,14 +614,14 @@ static void ep_aio_complete(struct usb_ep *ep, struct usb_request *req)
>  		aio_complete(iocb, req->actual ? req->actual : req->status,
>  				req->status);
>  	} else {
> -		/* retry() won't report both; so we hide some faults */
> +		/* ep_copy_to_user() won't report both; we hide some faults */
>  		if (unlikely(0 != req->status))
>  			DBG(epdata->dev, "%s fault %d len %d\n",
>  				ep->name, req->status, req->actual);
>  
>  		priv->buf = req->buf;
>  		priv->actual = req->actual;
> -		kick_iocb(iocb);
> +		schedule_work(&priv->work);
>  	}
>  	spin_unlock(&epdata->dev->lock);
>  
> @@ -633,8 +651,10 @@ fail:
>  		return value;
>  	}
>  	iocb->private = priv;
> +	priv->iocb = iocb;
>  	priv->iv = iv;
>  	priv->nr_segs = nr_segs;
> +	INIT_WORK(&priv->work, ep_user_copy_worker);
>  
>  	value = get_ready_ep(iocb->ki_filp->f_flags, epdata);
>  	if (unlikely(value < 0)) {
> @@ -646,6 +666,7 @@ fail:
>  	get_ep(epdata);
>  	priv->epdata = epdata;
>  	priv->actual = 0;
> +	priv->mm = current->mm; /* mm teardown waits for iocbs in exit_aio() */
>  
>  	/* each kiocb is coupled to one usb_request, but we can't
>  	 * allocate or submit those if the host disconnected.
> @@ -674,7 +695,7 @@ fail:
>  		kfree(priv);
>  		put_ep(epdata);
>  	} else
> -		value = (iv ? -EIOCBRETRY : -EIOCBQUEUED);
> +		value = -EIOCBQUEUED;
>  	return value;
>  }
>  
> @@ -692,7 +713,6 @@ ep_aio_read(struct kiocb *iocb, const struct iovec *iov,
>  	if (unlikely(!buf))
>  		return -ENOMEM;
>  
> -	iocb->ki_retry = ep_aio_read_retry;
>  	return ep_aio_rwtail(iocb, buf, iocb->ki_left, epdata, iov, nr_segs);
>  }
>  
> -- 
> 1.8.1.3
> 

-- 
balbi

Attachment: signature.asc
Description: Digital signature