[Qemu-devel] [PATCH] block/rbd: add .bdrv_reopen_prepare() stub

[Qemu-devel] [PATCH] block/rbd: add .bdrv_reopen_prepare() stub

[Sebastian Färber]

Add support for reopen() by adding the .bdrv_reopen_prepare() stub

Signed-off-by: Sebastian Färber <sfaerber82@gmail.com>
Tested-by: Sebastian Färber <sfaerber82@gmail.com>
---
 block/rbd.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/block/rbd.c b/block/rbd.c
index 5bc5b32..5f121b5 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -577,6 +577,14 @@ failed_opts:
     return r;
 }

+/* Note that this will not re-establish a connection with the Ceph cluster
+   - it is effectively a NOP.  */
+static int qemu_rbd_reopen_prepare(BDRVReopenState *state,
+                                   BlockReopenQueue *queue, Error **errp)
+{
+    return 0;
+}
+
 static void qemu_rbd_close(BlockDriverState *bs)
 {
     BDRVRBDState *s = bs->opaque;
@@ -976,6 +984,7 @@ static BlockDriver bdrv_rbd = {
     .instance_size      = sizeof(BDRVRBDState),
     .bdrv_needs_filename = true,
     .bdrv_file_open     = qemu_rbd_open,
+    .bdrv_reopen_prepare = qemu_rbd_reopen_prepare,
     .bdrv_close         = qemu_rbd_close,
     .bdrv_create        = qemu_rbd_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
--
1.8.3.1

Re: [Qemu-devel] [PATCH] block/rbd: add .bdrv_reopen_prepare() stub

[Kevin Wolf]

Am 13.05.2016 um 10:29 hat Sebastian Färber geschrieben:
> Add support for reopen() by adding the .bdrv_reopen_prepare() stub
> 
> Signed-off-by: Sebastian Färber <sfaerber82@gmail.com>
> Tested-by: Sebastian Färber <sfaerber82@gmail.com>
> ---
>  block/rbd.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 5bc5b32..5f121b5 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -577,6 +577,14 @@ failed_opts:
>      return r;
>  }
> 
> +/* Note that this will not re-establish a connection with the Ceph cluster
> +   - it is effectively a NOP.  */
> +static int qemu_rbd_reopen_prepare(BDRVReopenState *state,
> +                                   BlockReopenQueue *queue, Error **errp)
> +{
> +    return 0;
> +}

A correct reopen implementation must consider all options and flags that
.bdrv_open() looked at.

The options are okay, as both "filename" and "password-secret" aren't
things that we want to allow a reopen to change. However, in the flags
BDRV_O_NOCACHE makes a difference:

    if (flags & BDRV_O_NOCACHE) {
        rados_conf_set(s->cluster, "rbd_cache", "false");
    } else {
        rados_conf_set(s->cluster, "rbd_cache", "true");
    }

A reopen must either update the setting, or if it can't (e.g. because
librbd doesn't support it) any attempt to change the flag must fail.

Kevin

Re: [Qemu-devel] [PATCH] block/rbd: add .bdrv_reopen_prepare() stub

[Sebastian Färber]

Hi Kevin,

> A correct reopen implementation must consider all options and flags that
> .bdrv_open() looked at.
> 
> The options are okay, as both "filename" and "password-secret" aren't
> things that we want to allow a reopen to change. However, in the flags
> BDRV_O_NOCACHE makes a difference:
> 
>     if (flags & BDRV_O_NOCACHE) {
>         rados_conf_set(s->cluster, "rbd_cache", "false");
>     } else {
>         rados_conf_set(s->cluster, "rbd_cache", "true");
>     }
> 
> A reopen must either update the setting, or if it can't (e.g. because
> librbd doesn't support it) any attempt to change the flag must fail.

Thanks for the feedback.
As far as i can tell it's not possible to update the cache settings
without reconnecting. I've added a check in the following patch.
Would be great if someone who knows the internals of ceph/rbd could
have a look as well.

Sebastian

-- >8 --
Subject: [PATCH] block/rbd: add .bdrv_reopen_prepare() stub

Add support for reopen() by adding the .bdrv_reopen_prepare() stub

Signed-off-by: Sebastian Färber <sfaerber82@gmail.com>
---
 block/rbd.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/block/rbd.c b/block/rbd.c
index 5bc5b32..8ecf096 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -577,6 +577,19 @@ failed_opts:
     return r;
 }

+/* Note that this will not re-establish a connection with the Ceph cluster
+   - it is effectively a NOP.  */
+static int qemu_rbd_reopen_prepare(BDRVReopenState *state,
+                                   BlockReopenQueue *queue, Error **errp)
+{
+    if (state->flags & BDRV_O_NOCACHE &&
+        ((state->bs->open_flags & BDRV_O_NOCACHE) == 0)) {
+        error_setg(errp, "Cannot turn off rbd_cache during reopen");
+        return -EINVAL;
+    }
+    return 0;
+}
+
 static void qemu_rbd_close(BlockDriverState *bs)
 {
     BDRVRBDState *s = bs->opaque;
@@ -976,6 +989,7 @@ static BlockDriver bdrv_rbd = {
     .instance_size      = sizeof(BDRVRBDState),
     .bdrv_needs_filename = true,
     .bdrv_file_open     = qemu_rbd_open,
+    .bdrv_reopen_prepare = qemu_rbd_reopen_prepare,
     .bdrv_close         = qemu_rbd_close,
     .bdrv_create        = qemu_rbd_create,
     .bdrv_has_zero_init = bdrv_has_zero_init_1,
--
1.8.3.1

Re: [Qemu-devel] [PATCH] block/rbd: add .bdrv_reopen_prepare() stub

[Jason Dillaman]

On Tue, May 17, 2016 at 6:03 AM, Sebastian Färber <sfaerber82@gmail.com> wrote:
> Hi Kevin,
>
>> A correct reopen implementation must consider all options and flags that
>> .bdrv_open() looked at.
>>
>> The options are okay, as both "filename" and "password-secret" aren't
>> things that we want to allow a reopen to change. However, in the flags
>> BDRV_O_NOCACHE makes a difference:
>>
>>     if (flags & BDRV_O_NOCACHE) {
>>         rados_conf_set(s->cluster, "rbd_cache", "false");
>>     } else {
>>         rados_conf_set(s->cluster, "rbd_cache", "true");
>>     }
>>
>> A reopen must either update the setting, or if it can't (e.g. because
>> librbd doesn't support it) any attempt to change the flag must fail.
>
> Thanks for the feedback.
> As far as i can tell it's not possible to update the cache settings
> without reconnecting. I've added a check in the following patch.
> Would be great if someone who knows the internals of ceph/rbd could
> have a look as well.

Correct, you cannot currently dynamically enable nor disable the
cache.  You would need to close the image and re-open it to change the
cache settings.

> Sebastian
>
> -- >8 --
> Subject: [PATCH] block/rbd: add .bdrv_reopen_prepare() stub
>
> Add support for reopen() by adding the .bdrv_reopen_prepare() stub
>
> Signed-off-by: Sebastian Färber <sfaerber82@gmail.com>
> ---
>  block/rbd.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index 5bc5b32..8ecf096 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -577,6 +577,19 @@ failed_opts:
>      return r;
>  }
>
> +/* Note that this will not re-establish a connection with the Ceph cluster
> +   - it is effectively a NOP.  */
> +static int qemu_rbd_reopen_prepare(BDRVReopenState *state,
> +                                   BlockReopenQueue *queue, Error **errp)
> +{
> +    if (state->flags & BDRV_O_NOCACHE &&
> +        ((state->bs->open_flags & BDRV_O_NOCACHE) == 0)) {
> +        error_setg(errp, "Cannot turn off rbd_cache during reopen");
> +        return -EINVAL;
> +    }
> +    return 0;
> +}
> +
>  static void qemu_rbd_close(BlockDriverState *bs)
>  {
>      BDRVRBDState *s = bs->opaque;
> @@ -976,6 +989,7 @@ static BlockDriver bdrv_rbd = {
>      .instance_size      = sizeof(BDRVRBDState),
>      .bdrv_needs_filename = true,
>      .bdrv_file_open     = qemu_rbd_open,
> +    .bdrv_reopen_prepare = qemu_rbd_reopen_prepare,
>      .bdrv_close         = qemu_rbd_close,
>      .bdrv_create        = qemu_rbd_create,
>      .bdrv_has_zero_init = bdrv_has_zero_init_1,
> --
> 1.8.3.1
>
>



-- 
Jason

Re: [Qemu-devel] [PATCH] block/rbd: add .bdrv_reopen_prepare() stub

[Josh Durgin]

On 05/17/2016 03:03 AM, Sebastian Färber wrote:
> Hi Kevin,
>
>> A correct reopen implementation must consider all options and flags that
>> .bdrv_open() looked at.
>>
>> The options are okay, as both "filename" and "password-secret" aren't
>> things that we want to allow a reopen to change. However, in the flags
>> BDRV_O_NOCACHE makes a difference:
>>
>>      if (flags & BDRV_O_NOCACHE) {
>>          rados_conf_set(s->cluster, "rbd_cache", "false");
>>      } else {
>>          rados_conf_set(s->cluster, "rbd_cache", "true");
>>      }
>>
>> A reopen must either update the setting, or if it can't (e.g. because
>> librbd doesn't support it) any attempt to change the flag must fail.

Updating this setting on an open image won't do anything, but if you
rbd_close() and rbd_open() it again the setting will take effect.
rbd_close() will force a flush of any pending I/O in librbd and
free the memory for librbd's ImageCtx, which may or may not be desired
here.

> Thanks for the feedback.
> As far as i can tell it's not possible to update the cache settings
> without reconnecting. I've added a check in the following patch.
> Would be great if someone who knows the internals of ceph/rbd could
> have a look as well.

There's no need to reset the librados state, so connections to the
cluster can stick around. I'm a bit unclear on the bdrv_reopen_*
functions though - what is their intended use and semantics?

> Sebastian
>
> -- >8 --
> Subject: [PATCH] block/rbd: add .bdrv_reopen_prepare() stub
>
> Add support for reopen() by adding the .bdrv_reopen_prepare() stub
>
> Signed-off-by: Sebastian Färber <sfaerber82@gmail.com>
> ---
>   block/rbd.c | 14 ++++++++++++++
>   1 file changed, 14 insertions(+)
>
> diff --git a/block/rbd.c b/block/rbd.c
> index 5bc5b32..8ecf096 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -577,6 +577,19 @@ failed_opts:
>       return r;
>   }
>
> +/* Note that this will not re-establish a connection with the Ceph cluster
> +   - it is effectively a NOP.  */
> +static int qemu_rbd_reopen_prepare(BDRVReopenState *state,
> +                                   BlockReopenQueue *queue, Error **errp)
> +{
> +    if (state->flags & BDRV_O_NOCACHE &&
> +        ((state->bs->open_flags & BDRV_O_NOCACHE) == 0)) {
> +        error_setg(errp, "Cannot turn off rbd_cache during reopen");
> +        return -EINVAL;
> +    }
> +    return 0;
> +}
> +
>   static void qemu_rbd_close(BlockDriverState *bs)
>   {
>       BDRVRBDState *s = bs->opaque;
> @@ -976,6 +989,7 @@ static BlockDriver bdrv_rbd = {
>       .instance_size      = sizeof(BDRVRBDState),
>       .bdrv_needs_filename = true,
>       .bdrv_file_open     = qemu_rbd_open,
> +    .bdrv_reopen_prepare = qemu_rbd_reopen_prepare,
>       .bdrv_close         = qemu_rbd_close,
>       .bdrv_create        = qemu_rbd_create,
>       .bdrv_has_zero_init = bdrv_has_zero_init_1,
> --
> 1.8.3.1
>

Re: [Qemu-devel] [PATCH] block/rbd: add .bdrv_reopen_prepare() stub

[Sebastian Färber]

> 
> There's no need to reset the librados state, so connections to the
> cluster can stick around. I'm a bit unclear on the bdrv_reopen_*
> functions though - what is their intended use and semantics?

My motivation for implementing this basic reopen support is getting
active block commit in qemu (driven by libvirt) to work for rbd.
For example:
- VM is running and using rbd device as primary disk
- Create external local qcow2 snapshot with rbd as backing device
- All writes end up in local qcow2 file
- Do active block commit to get changes back to rbd after a while

Without the patch the last part (active block commit) fails in qemu:
Block format 'rbd' used by device '' does not support reopening files

I think, in this case, block commit wants to switch the rbd device to RW
to make sure it can write back the changes. Which is unnecessary in this case 
because it already is writeable.

Unfortunately i don't think i can implement proper/full-blown reopen support (like gluster)
for rbd - this would be way above my head ...

Sebastian

Re: [Qemu-devel] [PATCH] block/rbd: add .bdrv_reopen_prepare() stub

[Kevin Wolf]

Am 17.05.2016 um 20:48 hat Josh Durgin geschrieben:
> On 05/17/2016 03:03 AM, Sebastian Färber wrote:
> >Hi Kevin,
> >
> >>A correct reopen implementation must consider all options and flags that
> >>.bdrv_open() looked at.
> >>
> >>The options are okay, as both "filename" and "password-secret" aren't
> >>things that we want to allow a reopen to change. However, in the flags
> >>BDRV_O_NOCACHE makes a difference:
> >>
> >>     if (flags & BDRV_O_NOCACHE) {
> >>         rados_conf_set(s->cluster, "rbd_cache", "false");
> >>     } else {
> >>         rados_conf_set(s->cluster, "rbd_cache", "true");
> >>     }
> >>
> >>A reopen must either update the setting, or if it can't (e.g. because
> >>librbd doesn't support it) any attempt to change the flag must fail.
> 
> Updating this setting on an open image won't do anything, but if you
> rbd_close() and rbd_open() it again the setting will take effect.
> rbd_close() will force a flush of any pending I/O in librbd and
> free the memory for librbd's ImageCtx, which may or may not be desired
> here.

First rbd_close() and then rbd_open() risks that the rbd_open() fails
and we end up with no usable image at all. Can we open a second instance
of the image first and only close the first one if that succeeded?

We already flush all requests before calling this, so that part
shouldn't make a difference.

> >Thanks for the feedback.
> >As far as i can tell it's not possible to update the cache settings
> >without reconnecting. I've added a check in the following patch.
> >Would be great if someone who knows the internals of ceph/rbd could
> >have a look as well.
> 
> There's no need to reset the librados state, so connections to the
> cluster can stick around. I'm a bit unclear on the bdrv_reopen_*
> functions though - what is their intended use and semantics?

They change the options and flags that were specified in .bdrv_open().
The most important use case today is switching between read-only and
read-write, but changing the cache mode or any other option can be
requested as well.

> >Sebastian
> >
> >-- >8 --
> >Subject: [PATCH] block/rbd: add .bdrv_reopen_prepare() stub
> >
> >Add support for reopen() by adding the .bdrv_reopen_prepare() stub
> >
> >Signed-off-by: Sebastian Färber <sfaerber82@gmail.com>
> >---
> >  block/rbd.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> >diff --git a/block/rbd.c b/block/rbd.c
> >index 5bc5b32..8ecf096 100644
> >--- a/block/rbd.c
> >+++ b/block/rbd.c
> >@@ -577,6 +577,19 @@ failed_opts:
> >      return r;
> >  }
> >
> >+/* Note that this will not re-establish a connection with the Ceph cluster
> >+   - it is effectively a NOP.  */
> >+static int qemu_rbd_reopen_prepare(BDRVReopenState *state,
> >+                                   BlockReopenQueue *queue, Error **errp)
> >+{
> >+    if (state->flags & BDRV_O_NOCACHE &&
> >+        ((state->bs->open_flags & BDRV_O_NOCACHE) == 0)) {

This misses the other direction, where you try to turn on caching. If we
don't implement the real functionality, we should always error out if
the bit changes. The most readable check is probably:

(state->flags & BDRV_O_NOCACHE) != (state->bs->open_flags & BDRV_O_NOCACHE)

> >+        error_setg(errp, "Cannot turn off rbd_cache during reopen");
> >+        return -EINVAL;
> >+    }
> >+    return 0;
> >+}

Kevin

Re: [Qemu-devel] [PATCH] block/rbd: add .bdrv_reopen_prepare() stub

[Jason Dillaman]

On Wed, May 18, 2016 at 4:19 AM, Kevin Wolf <kwolf@redhat.com> wrote:
>> Updating this setting on an open image won't do anything, but if you
>> rbd_close() and rbd_open() it again the setting will take effect.
>> rbd_close() will force a flush of any pending I/O in librbd and
>> free the memory for librbd's ImageCtx, which may or may not be desired
>> here.
>
> First rbd_close() and then rbd_open() risks that the rbd_open() fails
> and we end up with no usable image at all. Can we open a second instance
> of the image first and only close the first one if that succeeded?
>
> We already flush all requests before calling this, so that part
> shouldn't make a difference.


You can open the same image twice without issue.  It's perfectly fine
to rbd_open() with the new settings and upon success rbd_close() the
original image handle.

-- 
Jason

Re: [Qemu-devel] [PATCH] block/rbd: add .bdrv_reopen_prepare() stub

[Jeff Cody]

On Tue, May 17, 2016 at 12:03:53PM +0200, Sebastian Färber wrote:
> Hi Kevin,
> 
> > A correct reopen implementation must consider all options and flags that
> > .bdrv_open() looked at.
> > 
> > The options are okay, as both "filename" and "password-secret" aren't
> > things that we want to allow a reopen to change. However, in the flags
> > BDRV_O_NOCACHE makes a difference:
> > 
> >     if (flags & BDRV_O_NOCACHE) {
> >         rados_conf_set(s->cluster, "rbd_cache", "false");
> >     } else {
> >         rados_conf_set(s->cluster, "rbd_cache", "true");
> >     }
> > 
> > A reopen must either update the setting, or if it can't (e.g. because
> > librbd doesn't support it) any attempt to change the flag must fail.
> 
> Thanks for the feedback.
> As far as i can tell it's not possible to update the cache settings
> without reconnecting. I've added a check in the following patch.
> Would be great if someone who knows the internals of ceph/rbd could
> have a look as well.
> 
> Sebastian
> 

Is there any reason you cannot create a new connection, and then tear down
the old one, and switch over to use the new connection?

The reopen functions have a three operations:

*  _prepare()
 Parses flags, creates new connections / file descriptors, etc., but does not
 do anything that cannot be undone, or anything that makes the reopen
 operation "live".

* _commit()
 (if applicable) switches over to the new fd, and makes the
 reopen operation complete (and cleans up anything that needs to be cleaned
 up from _prepare()).

* _abort()
 cleanly undoes anything done in _prepare() - called if there is an error in
 _prepare(), or if the reopen operation is part of a larger transaction of
 multiple reopens, one of which failed.


For instance, it may be useful to look at how it is handled in the gluster
driver, and see if you can do something similar here in ceph.  For gluster,
we create a new gluster connection in the _prepare(), and in the _commit()
we close the old fd, and switch the image driver instance state fd over to
the new one.


> -- >8 --
> Subject: [PATCH] block/rbd: add .bdrv_reopen_prepare() stub
> 
> Add support for reopen() by adding the .bdrv_reopen_prepare() stub
> 
> Signed-off-by: Sebastian Färber <sfaerber82@gmail.com>
> ---
>  block/rbd.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/block/rbd.c b/block/rbd.c
> index 5bc5b32..8ecf096 100644
> --- a/block/rbd.c
> +++ b/block/rbd.c
> @@ -577,6 +577,19 @@ failed_opts:
>      return r;
>  }
> 
> +/* Note that this will not re-establish a connection with the Ceph cluster
> +   - it is effectively a NOP.  */
> +static int qemu_rbd_reopen_prepare(BDRVReopenState *state,
> +                                   BlockReopenQueue *queue, Error **errp)
> +{
> +    if (state->flags & BDRV_O_NOCACHE &&
> +        ((state->bs->open_flags & BDRV_O_NOCACHE) == 0)) {
> +        error_setg(errp, "Cannot turn off rbd_cache during reopen");
> +        return -EINVAL;
> +    }
> +    return 0;
> +}
> +
>  static void qemu_rbd_close(BlockDriverState *bs)
>  {
>      BDRVRBDState *s = bs->opaque;
> @@ -976,6 +989,7 @@ static BlockDriver bdrv_rbd = {
>      .instance_size      = sizeof(BDRVRBDState),
>      .bdrv_needs_filename = true,
>      .bdrv_file_open     = qemu_rbd_open,
> +    .bdrv_reopen_prepare = qemu_rbd_reopen_prepare,
>      .bdrv_close         = qemu_rbd_close,
>      .bdrv_create        = qemu_rbd_create,
>      .bdrv_has_zero_init = bdrv_has_zero_init_1,
> --
> 1.8.3.1
>