From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49119) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1b6CVs-0006sq-Tr for qemu-devel@nongnu.org; Fri, 27 May 2016 03:48:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1b6CVq-00045x-Sw for qemu-devel@nongnu.org; Fri, 27 May 2016 03:48:55 -0400 Date: Fri, 27 May 2016 15:48:46 +0800 From: Fam Zheng Message-ID: <20160527074846.GB15113@ad.usersys.redhat.com> References: <1463470536-8981-1-git-send-email-famz@redhat.com> <1463470536-8981-8-git-send-email-famz@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH v5 07/27] block: Handle image locking during reopen List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, Kevin Wolf , Jeff Cody , Markus Armbruster , Eric Blake , John Snow , qemu-block@nongnu.org, berrange@redhat.com, pbonzini@redhat.com, den@openvz.org, stefanha@redhat.com On Tue, 05/24 18:28, Max Reitz wrote: > On 17.05.2016 09:35, Fam Zheng wrote: > > Stash the locking state into BDRVReopenState. If it was locked, unlock > > in prepare, and lock it again when commit or abort. > > > > Signed-off-by: Fam Zheng > > --- > > block.c | 11 +++++++++++ > > include/block/block.h | 1 + > > 2 files changed, 12 insertions(+) > > > > diff --git a/block.c b/block.c > > index ad3663c..2be42bb 100644 > > --- a/block.c > > +++ b/block.c > > @@ -2112,6 +2112,11 @@ int bdrv_reopen_prepare(BDRVReopenState *reopen_state, BlockReopenQueue *queue, > > } while ((entry = qdict_next(reopen_state->options, entry))); > > } > > > > + reopen_state->was_locked = reopen_state->bs->image_locked; > > + if (reopen_state->was_locked) { > > + bdrv_unlock_image(reopen_state->bs); > > + } > > + > > ret = 0; > > > > error: > > @@ -2136,6 +2141,9 @@ static void bdrv_reopen_commit(BDRVReopenState *reopen_state) > > if (drv->bdrv_reopen_commit) { > > drv->bdrv_reopen_commit(reopen_state); > > } > > + if (reopen_state->was_locked) { > > + bdrv_lock_image(reopen_state->bs); > > + } > > > > /* set BDS specific flags now */ > > QDECREF(reopen_state->bs->explicit_options); > > @@ -2162,6 +2170,9 @@ static void bdrv_reopen_abort(BDRVReopenState *reopen_state) > > if (drv->bdrv_reopen_abort) { > > drv->bdrv_reopen_abort(reopen_state); > > } > > + if (reopen_state->was_locked) { > > + bdrv_lock_image(reopen_state->bs); > > I'm not sure I'm quite happy with this, because this may fail; > therefore, it may happen that the operation done in _prepare() cannot be > undone. > > Of course, the same applies to _commit(): bdrv_lock_image() can just > fail. It's probably even worse there. I don't see a good reason why just > relocking the image in _abort() should fail; but it's very much > conceivable that locking the reopened image in _commit() fails. > > I think the correct way would be to rely on the drivers which implement > locking to handle reopening correctly, that is, try lock the reopened > image in _prepare() and unlock the old one before discarding it in > _commit(). > > However, I'm not sure myself whether it's worth the effort. It's very > simple to do it like you did here -- but then again, it doesn't seem > quite correct. You are right, this is driver specific. raw-posix should "update" the lock. Will update this patch. > > Also: Should bdrv_reopen_prepare() check that the locking flags are not > changed? Read only flag also matters in fcntl locks, so practically we almost always need some change on the lock. Fam