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