Re: [PATCH 1/4] Vectorize aio_read/aio_write methods

[Badari Pulavarty]

Andrew Morton wrote:

> Badari Pulavarty <pbadari@xxxxxxxxxx> wrote:
>
> > On Thu, 2006-05-11 at 11:47 -0700, Andrew Morton wrote:
> >
> > > Badari Pulavarty <pbadari@xxxxxxxxxx> wrote:
> > >
> > > > static ssize_t ep_aio_read_retry(struct kiocb *iocb)
> > > > {
> > > > 	struct kiocb_priv	*priv = iocb->private;
> > > > -	ssize_t			status = priv->actual;
> > > > +	ssize_t			len, total;
> > > >
> > > > 	/* we "retry" to get the right mm context for this: */
> > > > -	status = copy_to_user(priv->ubuf, priv->buf, priv->actual);
> > > > -	if (unlikely(0 != status))
> > > > -		status = -EFAULT;
> > > > -	else
> > > > -		status = priv->actual;
> > > > +
> > > > +	/* copy stuff into user buffers */
> > > > +	total = priv->actual;
> > > > +	len = 0;
> > > > +	for (i=0; i < priv->count; i++) {
> > > > +		ssize_t this = min(priv->iv[i].iov_len, total);
> > > > +
> > > > +		if (copy_to_user(priv->iv[i].iov_buf, priv->buf, this))
> > > > +			break;
> > > > +
> > > > +		total -= this;
> > > > +		len += this;
> > > > +		if (total <= 0)
> > > > +			break;
> > > > +	}
> > > > +
> > > > +	if (unlikely(len == 0))
> > > > +		len = -EFAULT;
> > > This is still wrong, isn't it?  Or am I looking at the same patch?
> > >
> > > There's no way in which `total' can go negative, so it'd be nicer to just
> > > test it for equality with zero.  Because if it goes unexpectedly negative,
> > > we _want_ the kernel to malfunction, rather than mysteriously covering
> > > things up.
> > >
> > > The final test there should be
> > >
> > > 	if (unlikely(total != 0))
> > >
> > > yes?
> > No. The original check is correct - we want to return EFAULT if
> > copy_to_user() failed and we haven't copied anything so far.
> > If we copied anything so far, we should return, that many bytes.
> > (like short-io).
>
> oic.  And we're sure that we cannot call into this code if someone's trying
> a zero-sized read?
>
> Either way, the below (which is faster!) will fix, yes?
>
> --- 25/drivers/usb/gadget/inode.c~vectorize-aio_read-aio_write-methods-fix	Thu May 11 11:53:41 2006
> +++ 25-akpm/drivers/usb/gadget/inode.c	Thu May 11 13:19:45 2006
> @@ -567,18 +567,18 @@ static ssize_t ep_aio_read_retry(struct 
> 	for (i = 0; i < priv->count; i++) {
> 		ssize_t this = min(priv->iv[i].iov_len, total);
>
> -		if (copy_to_user(priv->iv[i].iov_buf, priv->buf, this))
> +		if (copy_to_user(priv->iv[i].iov_buf, priv->buf, this)) {
> +			if (len == 0)
> +				len = -EFAULT;
> 			break;
> +		}
>
> 		total -= this;
> 		len += this;
> -		if (total <= 0)
> +		if (total == 0)
> 			break;
> 	}
>
> -	if (unlikely(len == 0))
> -		len = -EFAULT;
> -
> 	kfree(priv->buf);
> 	kfree(priv);
> 	aio_put_req(iocb);
> _
Yes, this is good.

No one should call into this code with size == 0, since we should have 
returned
success without doing any IO in the first place.

Thanks,
Badari


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/