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

[Eric Blake]

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.


> +++ 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.


> +++ 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

> +    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).

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

signature.asc
Description: OpenPGP digital signature