[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] Re: [PATCH 3/3] block/nbd: Make the NBD block device us
From: |
MORITA Kazutaka |
Subject: |
Re: [Qemu-devel] Re: [PATCH 3/3] block/nbd: Make the NBD block device use the AIO interface |
Date: |
Wed, 23 Feb 2011 01:06:41 +0900 |
User-agent: |
Wanderlust/2.14.0 (Africa) SEMI/1.14.6 (Maruoka) FLIM/1.14.9 (Gojō) APEL/10.7 Emacs/22.2 (x86_64-pc-linux-gnu) MULE/5.0 (SAKAKI) |
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