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

[MORITA Kazutaka]

At Mon, 21 Feb 2011 17:48:49 +0100,
Kevin Wolf wrote:
> 
> 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?

Sheepdog doesn't use a bounce buffer for any requests, and to me, it
seems that Sheepdog also needs to check acb->canceled before reading
the response of a read request...


> >>> +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.

Yes, exactly.


Thanks,

Kazutaka