[Qemu-devel] Re: [PATCH 3/3] block/nbd: Make the NBD block device use the AIO interface

[Stefan Hajnoczi]

On Fri, Feb 18, 2011 at 12:55:29PM +0000, Nick Thomas wrote:
> +static inline AIOReq *alloc_aio_req(BDRVNBDState *s, NBDAIOCB *acb,
> +                                    size_t data_len,
> +                                    off_t offset,
> +                                    off_t iov_offset)
> +{
> +    AIOReq *aio_req;
>
> -    if (reply.handle != request.handle)
> +    aio_req = qemu_malloc(sizeof(*aio_req));
> +    aio_req->aiocb = acb;
> +    aio_req->iov_offset = iov_offset;
> +    aio_req->offset = offset;
> +    aio_req->data_len = data_len;
> +    aio_req->handle = s->aioreq_seq_num++; /* FIXME: Trivially guessable */

Does it matter at all that this number is guessable?  I think we can
drop the FIXME comment.

> +/*
> + * Send I/O requests to the server.
> + *
> + * This function sends requests to the server, links the requests to
> + * the outstanding_list in BDRVNBDState, and exits without waiting for
> + * the response.  The responses are received in the `aio_read_response'
> + * function which is called from the main loop as a fd handler.
> + * If this is a write request and it's >1MB, split it into multiple AIOReqs
> + */
> +static void nbd_readv_writev_bh_cb(void *p)
>  {
> -    BDRVNBDState *s = bs->opaque;
> -    struct nbd_request request;
> +    NBDAIOCB *acb = p;
> +    int ret = 0;
>
> -    request.type = NBD_CMD_DISC;
> -    request.handle = (uint64_t)(intptr_t)bs;
> -    request.from = 0;
> -    request.len = 0;
> -    nbd_send_request(s->sock, &request);
> +    size_t len, done = 0;
> +    size_t total = acb->nb_sectors * SECTOR_SIZE;
> +
> +    /* Where the read/write starts from */
> +    size_t offset = acb->sector_num * SECTOR_SIZE;

On 32-bit hosts size_t is only 32 bits wide.  It's not suitable for
storing the byte offset into the device.  Please use off_t.

> +    BDRVNBDState *s = acb->common.bs->opaque;

I don't follow this.  You have no guarantee that acb->common.bs->opaque
is the BDRVNBDState *.

> +
> +    AIOReq *aio_req;
>
> -    close(s->sock);
> +    qemu_bh_delete(acb->bh);
> +    acb->bh = NULL;
> +
> +    while (done != total) {
> +        len = (total - done);
> +
> +        /* Split write requests into 1MiB segments */
> +        if(acb->aiocb_type == AIOCB_WRITE_UDATA && len > MAX_NBD_WRITE) {
> +          len = MAX_NBD_WRITE;
> +        }

We need to be more conservative than 1 MB because qemu-nbd.c checks the
following:

    if (request.len + NBD_REPLY_SIZE > data_size) {
        LOG("len (%u) is larger than max len (%u)",
            request.len + NBD_REPLY_SIZE, data_size);
        errno = EINVAL;
        return -1;
    }

and NBD_REPLY_SIZE is 16.

Stefan