lkml.org 
[lkml]   [2006]   [May]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/4] Vectorize aio_read/aio_write methods


Andrew Morton wrote:

>Badari Pulavarty <pbadari@us.ibm.com> wrote:
>
>>On Thu, 2006-05-11 at 11:47 -0700, Andrew Morton wrote:
>>
>>>Badari Pulavarty <pbadari@us.ibm.com> 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@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-05-11 22:47    [W:0.079 / U:0.140 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site