From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48884) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1byubQ-00077L-Ba for qemu-devel@nongnu.org; Tue, 25 Oct 2016 01:48:49 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1byubP-0003dA-Bd for qemu-devel@nongnu.org; Tue, 25 Oct 2016 01:48:48 -0400 Date: Tue, 25 Oct 2016 13:48:38 +0800 From: Fam Zheng Message-ID: <20161025054838.GC5427@lemon> References: <1475237406-26917-1-git-send-email-famz@redhat.com> <1475237406-26917-4-git-send-email-famz@redhat.com> <36e4e4a0-b679-3b71-baf4-7513a38c594e@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <36e4e4a0-b679-3b71-baf4-7513a38c594e@redhat.com> Subject: Re: [Qemu-devel] [PATCH v8 03/36] block: Introduce image file locking List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Max Reitz Cc: qemu-devel@nongnu.org, berrange@redhat.com, John Snow , qemu-block@nongnu.org, Kevin Wolf , rjones@redhat.com, Jeff Cody , Markus Armbruster , stefanha@redhat.com, den@openvz.org, pbonzini@redhat.com, eblake@redhat.com On Fri, 10/21 23:04, Max Reitz wrote: > > +ImageLockMode bdrv_lock_mode_from_flags(int flags) > > +{ > > + if (flags & BDRV_O_NO_LOCK) { > > + return IMAGE_LOCK_MODE_NOLOCK; > > + } else if (flags & BDRV_O_SHARED_LOCK) { > > + return IMAGE_LOCK_MODE_SHARED; > > + } else if (flags & BDRV_O_EXCLUSIVE_LOCK) { > > + return IMAGE_LOCK_MODE_EXCLUSIVE; > > + } else { > > + return IMAGE_LOCK_MODE_AUTO; > > + } > > +} > > I don't know if there's been any discussion about the order of the flags > here, but I personally would order them exactly the other way around: > Asking for exclusive locking should override nolock, in my opinion. The idea was to assert no two bits are set at the same time. But I seem to have forgotten to actually add the assertion. > > > + > > +ImageLockMode bdrv_get_lock_mode(BlockDriverState *bs) > > +{ > > + return bs->cur_lock; > > +} > > + > > +int bdrv_set_lock_mode(BlockDriverState *bs, ImageLockMode mode) > > +{ > > + int ret; > > + > > + if (bs->cur_lock == mode) { > > + return 0; > > + } else if (!bs->drv) { > > + return -ENOMEDIUM; > > + } else if (!bs->drv->bdrv_lockf) { > > + if (bs->file) { > > + return bdrv_set_lock_mode(bs->file->bs, mode); > > + } > > + return 0; > > + } > > + ret = bs->drv->bdrv_lockf(bs, mode); > > + if (ret == -ENOTSUP) { > > + /* Handle it the same way as !bs->drv->bdrv_lockf */ > > + ret = 0; > > Yes, well, why do you handle both as success? Wouldn't returning > -ENOTSUP make more sense? > > I guess the caller can find out itself by checking whether bs->cur_lock > has changed, but... I can't think of a reason for any caller to do something different for -ENOTSUP from success, hence the check here. > > > + } else if (ret == 0) { > > + bs->cur_lock = mode; > > + } > > + return ret; > > +} > > + > > static QemuOptsList bdrv_runtime_opts = { > > .name = "bdrv_common", > > .head = QTAILQ_HEAD_INITIALIZER(bdrv_runtime_opts.head), > > @@ -1076,6 +1119,10 @@ static int bdrv_open_common(BlockDriverState *bs, BdrvChild *file, > > goto free_and_fail; > > } > > > > + if (open_flags & BDRV_O_INACTIVE) { > > + open_flags = (open_flags & ~BDRV_O_LOCK_MASK) & BDRV_O_NO_LOCK; > > I suppose the second & is supposed to be a |? Yes. Thanks for catching it. > > > + } > > + > > ret = refresh_total_sectors(bs, bs->total_sectors); > > if (ret < 0) { > > error_setg_errno(errp, -ret, "Could not refresh total sector count"); > > @@ -2273,6 +2320,7 @@ static void bdrv_close(BlockDriverState *bs) > > if (bs->drv) { > > BdrvChild *child, *next; > > > > + bdrv_set_lock_mode(bs, IMAGE_LOCK_MODE_NOLOCK); > > bs->drv->bdrv_close(bs); > > bs->drv = NULL; > > > > @@ -3188,6 +3236,9 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) > > This function's name is pretty weird... Maybe it would be better to > rename it to "bdrv_complete_incoming" or something. (Unrelated to this > series, of course.) > > > error_setg_errno(errp, -ret, "Could not refresh total sector count"); > > return; > > } > > + if (bs->cur_lock != IMAGE_LOCK_MODE__MAX) { > > + bdrv_set_lock_mode(bs, bs->cur_lock); > > + } > > } > > > > void bdrv_invalidate_cache_all(Error **errp) > > @@ -3230,6 +3281,7 @@ static int bdrv_inactivate_recurse(BlockDriverState *bs, > > } > > > > if (setting_flag) { > > + ret = bdrv_set_lock_mode(bs, IMAGE_LOCK_MODE_NOLOCK); > > Maybe it would make sense to do something with the return value...? :-) Yes, sounds good. Fam