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

[Vaibhav Bhembre]

Thanks Eric! 

On Thu, Jul 14, 2016 at 4:28 PM, Eric Blake <address@hidden> 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 address@hidden 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 <address@hidden>
>
> 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