[Virtio-fs] [PATCH 1/4] virtiofsd: send reply correctly on read failure
Dr. David Alan Gilbert
dgilbert at redhat.com
Thu Apr 18 19:05:54 UTC 2019
* Liu Bo (bo.liu at linux.alibaba.com) wrote:
> On Thu, Apr 18, 2019 at 01:25:27PM +0100, Dr. David Alan Gilbert wrote:
> > * Dr. David Alan Gilbert (dgilbert at redhat.com) wrote:
> > > * Liu Bo (bo.liu at linux.alibaba.com) wrote:
> > > > From: Eryu Guan <eguan at linux.alibaba.com>
> > > >
> > > > Currently when a lo_read() operation fails, we don't send the failure
> > > > back to fuse client, and read(2) operation from guest kernel would hang
> > > > on waiting for the reply.
> > > >
> > > > This is easily triggered by a direct read with non-aligned length.
> > > >
> > > > Fix it by detecting preadv(2) error in virtio_send_data_iov(), and
> > > > teaching fuse_reply_data() to reply error on error case.
> > >
> > > Thank you for spotting this.
> > >
> > > > Reviewed-by: Liu Bo <bo.liu at linux.alibaba.com>
> > > > Signed-off-by: Eryu Guan <eguan at linux.alibaba.com>
> > > > ---
> > > > contrib/virtiofsd/fuse_lowlevel.c | 4 ++--
> > > > contrib/virtiofsd/fuse_virtio.c | 12 +++++++++---
> > > > 2 files changed, 11 insertions(+), 5 deletions(-)
> > > >
> > > > diff --git a/contrib/virtiofsd/fuse_lowlevel.c b/contrib/virtiofsd/fuse_lowlevel.c
> > > > index 111c6e1..aeb5fe2 100644
> > > > --- a/contrib/virtiofsd/fuse_lowlevel.c
> > > > +++ b/contrib/virtiofsd/fuse_lowlevel.c
> > > > @@ -524,11 +524,11 @@ int fuse_reply_data(fuse_req_t req, struct fuse_bufvec *bufv,
> > > > out.error = 0;
> > > >
> > > > res = fuse_send_data_iov(req->se, req->ch, iov, 1, bufv, flags);
> > > > - if (res <= 0) {
> > > > + if (res >= 0) {
> > >
> > > I need to go and ask the upstream fuse list about this; it's not clear
> > > to me what fuse_send_data_iov is supposed to return; let me clarify
> > > that and get back to you.
> >
> > Miklos replied to me on the upstream fuse list:
> > https://sourceforge.net/p/fuse/mailman/fuse-devel/thread/CAOssrKdVViWewmh3nrfKkq6eaWNJ629AR7w90KG3XT2hV9_D1w%40mail.gmail.com/#msg36642807
> >
> > so this comparison is actually already correct because a negative value
> > means that something is wrong with the fuse connection (or in our case
> > virtio) so we can't send an error reply anyway.
> >
>
> I see, that makes sense.
>
> > > Dave
> > >
> > > > fuse_free_req(req);
> > > > return res;
> > > > } else {
> > > > - return fuse_reply_err(req, res);
> > > > + return fuse_reply_err(req, -res);
> > > > }
> > > > }
> > > >
> > > > diff --git a/contrib/virtiofsd/fuse_virtio.c b/contrib/virtiofsd/fuse_virtio.c
> > > > index ca988aa..0b5736d 100644
> > > > --- a/contrib/virtiofsd/fuse_virtio.c
> > > > +++ b/contrib/virtiofsd/fuse_virtio.c
> > > > @@ -333,8 +333,13 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
> > > > ret = preadv(buf->buf[0].fd, in_sg_ptr, in_sg_cpy_count, buf->buf[0].pos);
> > > >
> > > > if (se->debug)
> > > > - fprintf(stderr, "%s: preadv_res=%d len=%zd\n",
> > > > - __func__, ret, len);
> > > > + fprintf(stderr, "%s: preadv_res=%d(%s) len=%zd\n",
> > > > + __func__, ret, strerror(errno), len);
> > > > + if (ret == -1) {
> > > > + ret = -errno;
> >
> > So this will need to be ret = errno to give a postive error
> > response to mean that the error was on the file side on the fuse side.
> > (There's also another case below that where I set ret = EIO where it
> > should be EIO I think)
> >
>
> OK, do you want me to fix both in the same patch?
Yes please.
> > > > + free(in_sg_cpy);
> > > > + goto err;
> > > > + }
> > > > if (ret < len && ret) {
> > > > if (se->debug)
> > > > fprintf(stderr, "%s: ret < len\n", __func__);
> > > > @@ -379,7 +384,8 @@ int virtio_send_data_iov(struct fuse_session *se, struct fuse_chan *ch,
> > > > vu_queue_notify(&se->virtio_dev->dev, q);
> > > >
> > > > err:
> > > > - ch->qi->reply_sent = true;
> > > > + if (!ret)
> > > > + ch->qi->reply_sent = true;
> >
> > Yes, I think that's OK.
>
> Correct, this is the one causing fuse kernel side's hang.
>
Thanks!
Dave (Note, I'm out for a week)
> thanks,
> -liubo
>
> >
> > Dave
> >
> > > >
> > > > return ret;
> > > > }
> > > > --
> > > > 1.8.3.1
> > > >
> > > > _______________________________________________
> > > > Virtio-fs mailing list
> > > > Virtio-fs at redhat.com
> > > > https://www.redhat.com/mailman/listinfo/virtio-fs
> > > --
> > > Dr. David Alan Gilbert / dgilbert at redhat.com / Manchester, UK
> > >
> > > _______________________________________________
> > > Virtio-fs mailing list
> > > Virtio-fs at redhat.com
> > > https://www.redhat.com/mailman/listinfo/virtio-fs
> > --
> > Dr. David Alan Gilbert / dgilbert at redhat.com / Manchester, UK
--
Dr. David Alan Gilbert / dgilbert at redhat.com / Manchester, UK
More information about the Virtio-fs
mailing list