| Message ID | 1458137817-15383-12-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
Am 16.03.2016 um 15:16 hat Paolo Bonzini geschrieben: > sheepdog has some calls to aio_poll that are hard to eliminate, for > example in sd_sheepdog_goto's call to do_req. Since I don't have > means to test sheepdog well, disable dataplane altogether for this > driver. > > Reviewed-by: Fam Zheng <famz@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > block/sheepdog.c | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/block/sheepdog.c b/block/sheepdog.c > index a6e98a5..8ced3e5 100644 > --- a/block/sheepdog.c > +++ b/block/sheepdog.c > @@ -364,6 +364,7 @@ struct SheepdogAIOCB { > typedef struct BDRVSheepdogState { > BlockDriverState *bs; > AioContext *aio_context; > + Error *blocker; > > SheepdogInode inode; > > @@ -1422,6 +1423,21 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, > Error *local_err = NULL; > const char *filename; > > + /* sd_snapshot_goto does blocking operations that call aio_poll > + * (through do_req). This can cause races with iothread: > + * > + * main thread I/O thread > + * ----------------- ------------------ > + * while(srco.finished == false) > + * aio_poll(..., true) > + * srco.finished = true > + * aio_poll(..., true) > + * > + * Now aio_poll potentially blocks forever. > + */ > + error_setg(&s->blocker, "sheepdog does not support iothreads"); > + bdrv_op_block(bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker); Our current op blockers are weak, so this doesn't completely rule out having a sheepdog BDS under a dataplane device. Actually, did you check that even the most obvious case works? If we have a format layer on top (which we do by default), I think attaching to dataplane would still work because only the blockers of the top level would be considered. We have to be sure about this one. But there are other, less common cases that could still result in a bad setup. Essentially, this just blocks enabling dataplane on a device that has a sheepdog BDS (on top), but it doesn't block opening a sheepdog backend for a device that has dataplane already enabled. This includes scenarios with removable media like virtio-scsi CD-ROMs, but also live snapshots or block jobs with a target image on sheepdog. I wouldn't feel comfortable about ignoring this second part, but maybe we could get away with it if we have a plan how to fix it in the long run. The new op blockers should be able to do that, but I guess it will be well into the 2.7 development cycle, if not later, before we have them. Kevin
On 23/03/2016 11:45, Kevin Wolf wrote: > I wouldn't feel comfortable about ignoring this second part, but maybe > we could get away with it if we have a plan how to fix it in the long > run. The new op blockers should be able to do that, but I guess it will > be well into the 2.7 development cycle, if not later, before we have > them. I have agreed with Stefan to only merge the first four patches of this series (and even that seems unlikely right now). I'll keep this in mind, and probably just try and fix sheepdog. Paolo
diff --git a/block/sheepdog.c b/block/sheepdog.c index a6e98a5..8ced3e5 100644 --- a/block/sheepdog.c +++ b/block/sheepdog.c @@ -364,6 +364,7 @@ struct SheepdogAIOCB { typedef struct BDRVSheepdogState { BlockDriverState *bs; AioContext *aio_context; + Error *blocker; SheepdogInode inode; @@ -1422,6 +1423,21 @@ static int sd_open(BlockDriverState *bs, QDict *options, int flags, Error *local_err = NULL; const char *filename; + /* sd_snapshot_goto does blocking operations that call aio_poll + * (through do_req). This can cause races with iothread: + * + * main thread I/O thread + * ----------------- ------------------ + * while(srco.finished == false) + * aio_poll(..., true) + * srco.finished = true + * aio_poll(..., true) + * + * Now aio_poll potentially blocks forever. + */ + error_setg(&s->blocker, "sheepdog does not support iothreads"); + bdrv_op_block(bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker); + s->bs = bs; s->aio_context = bdrv_get_aio_context(bs); @@ -1962,6 +1978,9 @@ static void sd_close(BlockDriverState *bs) false, NULL, NULL, NULL); closesocket(s->fd); g_free(s->host_spec); + + bdrv_op_unblock(bs, BLOCK_OP_TYPE_DATAPLANE, s->blocker); + error_free(s->blocker); } static int64_t sd_getlength(BlockDriverState *bs)