Re: [Qemu-block] [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 <address@hidden>
> ---
>   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