Re: [Qemu-devel] [PATCH] NBD: Convert the NBD driver to use the AIO interface.

[Nicholas Thomas]

Hi again Kevin, all,

Thanks for applying the first four patches, and apologies for taking so
long to get back to you. I've found the time to take your comments
on-board and re-do the last patch, + the string-leak patch; I'll send
them on shortly, I just wanted to make a few notes on yours, first. 

> > +  * If there'an unrecoverable error reading from the socket, [...]
> 
> Something's missing here? ;-)

Documentation on how reconnections happen ;) - big fat TODO.
[...]
> 
> > +            free_aio_req(s, a);
> > +        }
> > +        nbd_finish_aiocb(acb);
> > +    }
> > +
> > +    nbd_teardown_connection(s);
> 
> And now there's no way to get the disk back to life without a reboot? Do
> I understand correctly that now trying to access the disk will always
> return -EBADF?

That's right - this is the case with the current code too. I want to put
the reconnection logic into a separate patch, but haven't actually
written it yet. 


> > +static void nbd_aio_read_response(void *opaque)
> > +{
> > +    BDRVNBDState *s = opaque;
> > +    AIOReq *aio_req = NULL;
> > +    NBDAIOCB *acb;
> > +    NBDReply rsp;
> > +
> > +    size_t total;
> > +    ssize_t ret;
> > +    int rest;
> > +
> > +    if (s->current_req == NULL && QTAILQ_EMPTY(&s->reqs_for_reply_head)) {
> > +        return;
> > +    }
> > +
> > +    /* Build our nbd_reply object if we've got it */
> > +    if (s->current_req && (s->nbd_rsp_offset == sizeof(NBDReply))) {
> > +        nbd_buf_to_reply((uint8_t *)&s->nbd_rsp_buf, &rsp);
> > +    }
> > +
> > +    if (s->current_req == NULL) {
> > +        /* Try to read a header */
> 
> Factor this whole block out in its own function?

I wasn't sure here if you meant the preceding, or following, code - I
guessed the latter, anyway, as that made the most sense. Done in the
following patch.

> > +        QEMUIOVector hdr;
> > +        qemu_iovec_init(&hdr, 1);
> > +        qemu_iovec_add(&hdr, ((&s->nbd_rsp_buf) + s->nbd_rsp_offset),
> > +                              (sizeof(NBDReply) - s->nbd_rsp_offset));
> > +        ret = readv(s->sock, hdr.iov, hdr.niov);
> > +        qemu_iovec_destroy(&hdr);
> > +
> > +        if (ret == -1) {
> > +          nbd_handle_io_err(s, aio_req, socket_error());
> > +          return;
> > +        }
[...]

> > +ssize_t nbd_wr_aio(int fd, QEMUIOVector *qiov, size_t len, off_t offset,
> > +                   bool do_read)
> 
> Isn't this name misleading? The function is completely synchronous. It
> just happens not to block because it's only called when the socket is ready.

As far as I understand it, a read/write may still block (or return
EAGAIN, EWOULDBLOCK, EINTR, etc) even if select has marked the socket as
ready for reads or writes. So it's aio, because we don't loop on those
errors. Anyway, I've renamed it to nbd_qiov_wr, since that's just as
relevant.