Thanks Eric! On Thu, Jul 14, 2016 at 4:28 PM, Eric Blake <ebl...@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-bl...@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 <vaib...@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 > >