From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:57418) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b2u5Q-0006O1-MF for qemu-devel@nongnu.org; Wed, 18 May 2016 01:32:01 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b2u5O-0001OY-Gg for qemu-devel@nongnu.org; Wed, 18 May 2016 01:31:59 -0400 Date: Wed, 18 May 2016 01:31:48 -0400 From: Jeff Cody Message-ID: <20160518053148.GA3668@localhost.localdomain> References: <3f8edd94-280c-8aa7-0f07-ab674ee4667b@gmail.com> <20160513084548.GA5529@noname.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline In-Reply-To: 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: Sebastian =?iso-8859-1?Q?F=E4rber?= Cc: Kevin Wolf , qemu-block@nongnu.org, qemu-devel@nongnu.org, jdurgin@redhat.com, mreitz@redhat.com On Tue, May 17, 2016 at 12:03:53PM +0200, Sebastian F=E4rber wrote: > Hi Kevin, >=20 > > A correct reopen implementation must consider all options and flags t= hat > > .bdrv_open() looked at. > >=20 > > 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 flag= s > > BDRV_O_NOCACHE makes a difference: > >=20 > > if (flags & BDRV_O_NOCACHE) { > > rados_conf_set(s->cluster, "rbd_cache", "false"); > > } else { > > rados_conf_set(s->cluster, "rbd_cache", "true"); > > } > >=20 > > 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. >=20 > 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. >=20 > Sebastian >=20 Is there any reason you cannot create a new connection, and then tear dow= n the old one, and switch over to use the new connection? The reopen functions have a three operations: * _prepare() Parses flags, creates new connections / file descriptors, etc., but does= not do anything that cannot be undone, or anything that makes the reopen operation "live". * _commit() (if applicable) switches over to the new fd, and makes the reopen operation complete (and cleans up anything that needs to be clean= ed up from _prepare()). * _abort() cleanly undoes anything done in _prepare() - called if there is an error= in _prepare(), or if the reopen operation is part of a larger transaction o= f multiple reopens, one of which failed. For instance, it may be useful to look at how it is handled in the gluste= r driver, and see if you can do something similar here in ceph. For gluste= r, we create a new gluster connection in the _prepare(), and in the _commit(= ) we close the old fd, and switch the image driver instance state fd over t= o the new one. > -- >8 -- > Subject: [PATCH] block/rbd: add .bdrv_reopen_prepare() stub >=20 > Add support for reopen() by adding the .bdrv_reopen_prepare() stub >=20 > Signed-off-by: Sebastian F=E4rber > --- > block/rbd.c | 14 ++++++++++++++ > 1 file changed, 14 insertions(+) >=20 > 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; > } >=20 > +/* 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 >=20