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

[Kevin Wolf]

Am 21.02.2011 17:31, schrieb Nicholas Thomas:
> Hi again,
> 
> Thanks for looking through the patches. I'm just going through and
> making the suggested changes now. I've also got qemu-nbd and block/nbd.c
> working over IPv6 :) - hopefully I'll be able to provide patches in a
> couple of days. Just a few questions about some of the changes...
> 
> Canceled requests: 
>>> +
>>> +
>>> +static void nbd_aio_cancel(BlockDriverAIOCB *blockacb)
>>> +{
>>> +    NBDAIOCB *acb = (NBDAIOCB *)blockacb;
>>> +
>>> +    /*
>>> +     * We cannot cancel the requests which are already sent to
>>> +     * the servers, so we just complete the request with -EIO here.
>>> +     */
>>> +    acb->common.cb(acb->common.opaque, -EIO);
>>> +    acb->canceled = 1;
>>> +}
>>
>> I think you need to check for acb->canceled before you write to the
>> associated buffer when receiving the reply for a read request. The
>> buffer might not exist any more after the request is cancelled.
> 
> I "borrowed" this code from block/sheepdog.c (along with a fair few
> other bits ;) ) - which doesn't seem to do any special checking for
> cancelled write requests. So if there is a potential SIGSEGV here, I
> guess sheepdog is also vulnerable.

Right, now that you mention it, I seem to remember this from Sheepdog. I
think I had a discussion with Stefan and he convinced me that we could
get away with it in Sheepdog because of some condition that Sheepdog
meets. Not sure any more what that condition was and if it applies to NBD.

Was it that Sheepdog has a bounce buffer for all requests?

> Presumably, in aio_read_response, I'd need to allocate a buffer of the
> appropriate size, read the data from the socket, then deallocate the
> buffer? Or would fsetpos(fd, ftell(fd)+data_len) be sufficient?

I don't think that fseek works with sockets, so you'd probably need to
read into a temporary buffer, yes.

> qemu-io doesn't seem to have any provisions for testing this particular
> code path - can you think of any tests I could run to ensure
> correctness? I've no idea under what situations an IO request might be
> cancelled.

aio_cancel is hard to implement and hard to test, and I would be
surprised if any format more complex than raw got it completely right...

>>> +static BlockDriverAIOCB *nbd_aio_readv(BlockDriverState *bs,
>>> +        int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
>>> +        BlockDriverCompletionFunc *cb, void *opaque)
>>> +{
>>> [...]
>>> +    for (i = 0; i < qiov->niov; i++) {
>>> +        memset(qiov->iov[i].iov_base, 0, qiov->iov[i].iov_len);
>>> +    }
>>
>> qemu_iovec_memset?
>>
>> What is this even for? Aren't these zeros overwritten anyway?
> 
> Again, present in sheepdog - but it does seem to work fine without it.
> I'll remove it from NBD.

Maybe Sheepdog reads only partially from the server if blocks are
unallocated or something.

Kevin