[v2,11/16] sheepdog: disable dataplane

Commit Message

Paolo Bonzini March 16, 2016, 2:16 p.m. UTC

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 <[e-mail]>
Signed-off-by: Paolo Bonzini <[e-mail]>
---
 block/sheepdog.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

Comments

Kevin Wolf March 23, 2016, 10:45 a.m. UTC | #1

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 <[e-mail]>
> Signed-off-by: Paolo Bonzini <[e-mail]>
> ---
>  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

Paolo Bonzini March 23, 2016, 11:07 a.m. UTC | #2

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

Patch

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)