From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44585) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b2k2i-0005ly-9S for qemu-devel@nongnu.org; Tue, 17 May 2016 14:48:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b2k2e-0008Bj-5D for qemu-devel@nongnu.org; Tue, 17 May 2016 14:48:32 -0400 References: <3f8edd94-280c-8aa7-0f07-ab674ee4667b@gmail.com> <20160513084548.GA5529@noname.redhat.com> From: Josh Durgin Message-ID: <573B676F.2080302@redhat.com> Date: Tue, 17 May 2016 11:48:15 -0700 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH] block/rbd: add .bdrv_reopen_prepare() stub List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: =?UTF-8?Q?Sebastian_F=c3=a4rber?= , Kevin Wolf Cc: qemu-block@nongnu.org, qemu-devel@nongnu.org, jcody@redhat.com, mreitz@redhat.com On 05/17/2016 03:03 AM, Sebastian F=C3=A4rber wrote: > Hi Kevin, > >> A correct reopen implementation must consider all options and flags th= at >> .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. Updating this setting on an open image won't do anything, but if you rbd_close() and rbd_open() it again the setting will take effect. rbd_close() will force a flush of any pending I/O in librbd and free the memory for librbd's ImageCtx, which may or may not be desired here. > 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. There's no need to reset the librados state, so connections to the cluster can stick around. I'm a bit unclear on the bdrv_reopen_* functions though - what is their intended use and semantics? > Sebastian > > -- >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=C3=A4rber > --- > 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 clu= ster > + - it is effectively a NOP. */ > +static int qemu_rbd_reopen_prepare(BDRVReopenState *state, > + BlockReopenQueue *queue, Error **er= rp) > +{ > + if (state->flags & BDRV_O_NOCACHE && > + ((state->bs->open_flags & BDRV_O_NOCACHE) =3D=3D 0)) { > + error_setg(errp, "Cannot turn off rbd_cache during reopen"); > + return -EINVAL; > + } > + return 0; > +} > + > static void qemu_rbd_close(BlockDriverState *bs) > { > BDRVRBDState *s =3D bs->opaque; > @@ -976,6 +989,7 @@ static BlockDriver bdrv_rbd =3D { > .instance_size =3D sizeof(BDRVRBDState), > .bdrv_needs_filename =3D true, > .bdrv_file_open =3D qemu_rbd_open, > + .bdrv_reopen_prepare =3D qemu_rbd_reopen_prepare, > .bdrv_close =3D qemu_rbd_close, > .bdrv_create =3D qemu_rbd_create, > .bdrv_has_zero_init =3D bdrv_has_zero_init_1, > -- > 1.8.3.1 >