On Mon, Oct 03, 2016 at 10:07:39AM -0700, Linus Torvalds wrote:
> On Sun, Oct 2, 2016 at 8:34 PM, Al Viro <viro@xxxxxxxxxxxxxxxxxx> wrote:
> >
> > Linus, do you have any objections against such behaviour change? AFAICS,
> > all it takes is this:
> >
> > diff --git a/fs/direct-io.c b/fs/direct-io.c
> > index 7c3ce73..3a8ebda 100644
> > --- a/fs/direct-io.c
> > +++ b/fs/direct-io.c
> > @@ -246,6 +246,8 @@ static ssize_t dio_complete(struct dio *dio, ssize_t ret, bool is_async)
> > if ((dio->op == REQ_OP_READ) &&
> > ((offset + transferred) > dio->i_size))
> > transferred = dio->i_size - offset;
> > + if (ret == -EFAULT)
> > + ret = 0;
>
> I don't think that's right. To me it looks like the short read case
> might have changed "transferred" back to zero, in which case we do
> *not* want to skip the EFAULT.
There's this in do_blockdev_direct_IO():
/* Once we sampled i_size check for reads beyond EOF */
dio->i_size = i_size_read(inode);
if (iov_iter_rw(iter) == READ && offset >= dio->i_size) {
if (dio->flags & DIO_LOCKING)
mutex_unlock(&inode->i_mutex);
kmem_cache_free(dio_cache, dio);
retval = 0;
goto out;
}
so that shouldn't happen. Said that,
> But if there's some reason that can't happen (ie "dio->i_size" is
> guaranteed to be larger than "offset"), then with a comment to that
> effect it's ok.
>
> Otherwise I think it would need to be something like
>
> /* If we were partially successful, ignore later EFAULT */
> if (transferred && ret == -EFAULT)
> ret = 0;
... it's certainly less brittle that way. I'd probably still put it under
the same if (dio->result) and write it as
if (unlikely(ret == -EFAULT) && transferred)
though.
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
