Re: [Qemu-devel] [PATCH v2] rbd: reload ceph config for block device

[Vaibhav Bhembre <vaibhav@digitalocean.com>]

Thanks Eric!

On Thu, Jul 14, 2016 at 4:28 PM, Eric Blake <eblake@redhat.com> wrote:

> On 07/14/2016 01:32 PM, Vaibhav Bhembre wrote:
> > This patch adds ability to reload ceph configuration for an attached RBD
> > block device. This is necessary for the cases where rebooting a VM and/or
> > detaching-reattaching a RBD drive is not an easy option.
>
> Probably worth including qemu-block@nongnu.org if you resend this. I've
> added them in cc now, per the output of:
>  scripts/get_maintainer.pl -f block/rbd
>
> >
> > The reload mechanism relies on the bdrv_reopen_* calls to provide a
> transactional
> > guarantee (using 2PC) for pulling in new configuration parameters. In
> the _prepare
> > phase we do the grunt-work of creating and establishing new connection
> and open
> > another instance of the same RBD image. If any issues are observed while
> creating a
> > connection using the new parameters we _abort the reload. The original
> connection to
> > the cluster is kept available and all ongoing I/O on it should be fine.
> >
> > Once the _prepare phase completes successfully we enter the _commit
> phase. In this phase
> > we simple move the I/O over to the new fd for the corresponding image we
> have already
> > created in the _prepare phase and reclaim the old rados I/O context and
> connection.
> >
> > It is important to note that because we want to use this feature when a
> QEMU VM is already
> > running, we need to switch the logic to have values in ceph.conf
> override the ones present
> > in the -drive file=* string in order for new changes to take place, for
> same keys present
> > in both places.
> >
> > Signed-off-by: Vaibhav Bhembre <vaibhav@digitalocean.com>
> >
> > diff --git a/block/rbd.c b/block/rbd.c
>
> Your patch is missing the usual --- separator and diffstat that 'git
> format-patch' (and 'git send-email') normally produce. Without that, I
> don't have a good up-front idea on what it touches.
>
> Also, you titled this v2, in which case it's nice to mention what you
> changed since v1.
>
> You may want to look at http://wiki.qemu.org/Contribute/SubmitAPatch for
> other hints on writing a patch that is easier to review.
>
> ​Noted. The changes made from v1 are: (a) version change from 2.5 to 2.7,
and (b) using node as an input to the monitor command versus device when
reloading config.

>
> > +++ b/qapi-schema.json
> > @@ -4317,3 +4317,16 @@
> >  # Since: 2.7
> >  ##
> >  { 'command': 'query-hotpluggable-cpus', 'returns': ['HotpluggableCPU'] }
> > +
> > +##
> > +# @reload-rbd-config
> > +#
> > +# Reload the ceph config for a given RBD block device attached to the
> VM.
> > +#
> > +# @node: Name of the node.
> > +#
> > +# Returns: nothing on success.
> > +#
> > +# Since: 2.7
>
> v1 was posted June 17, before soft freeze on June 28, so this may still
> make hard freeze if someone picks it up before hard freeze on July 19.
> But we're getting close.
>
> ​Hoping so! ​


>
> > +++ b/qmp.c
> > @@ -707,3 +707,34 @@ ACPIOSTInfoList *qmp_query_acpi_ospm_status(Error
> **errp)
> >
> >      return head;
> >  }
> > +
> > +void qmp_reload_rbd_config(const char *node, Error **errp)
> > +{
> > +    BlockBackend *blk;
> > +    BlockDriverState *bs;
> > +    Error *local_err = NULL;
> > +    int ret;
> > +
> > +    blk = blk_by_name(node);
> > +    if (!blk) {
> > +        error_setg(errp, QERR_INVALID_PARAMETER, "node");
> > +        return;
> > +    }
> > +
>
> We may want to rebase this on top of Kevin's series that adds
> qmp_get_root_bs()
> https://lists.gnu.org/archive/html/qemu-devel/2016-07/msg03086.html
>
> ​This is exactly what I need. Should I wait for other reviews before
making this change or should I push it right-away?
​


> > +    bs = blk_bs(blk);
> > +    if (!bs) {
> > +        error_setg(errp, "no BDS found");
> > +        return;
> > +    }
> > +
> > +    ret = bdrv_reopen(bs, bdrv_get_flags(bs), &local_err);
>
> This seems pretty generic - would it be better to just have a general
> 'block-reopen' command, instead of making it specific to rbd?
>
> I'll let other block maintainers do a deeper review (I just focused on
> the interface).
>
> ​I was thinking about that earlier, but refrained from adding it in since
my use-case was very specific. If it will indeed be useful to have a
top-level block-reopen operation I am all ready to add it in if you guys
think so.

--
> Eric Blake   eblake redhat com    +1-919-301-3266
> Libvirt virtualization library http://libvirt.org
>
>