From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:47641) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VrJ40-000292-MS for qemu-devel@nongnu.org; Thu, 12 Dec 2013 22:05:22 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VrJ3u-0006kp-Fb for qemu-devel@nongnu.org; Thu, 12 Dec 2013 22:05:16 -0500 Received: from mx1.redhat.com ([209.132.183.28]:33331) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VrJ3u-0006ke-5K for qemu-devel@nongnu.org; Thu, 12 Dec 2013 22:05:10 -0500 Received: from int-mx09.intmail.prod.int.phx2.redhat.com (int-mx09.intmail.prod.int.phx2.redhat.com [10.5.11.22]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id rBD359Yb001537 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Thu, 12 Dec 2013 22:05:09 -0500 Message-ID: <52AA7955.2000505@redhat.com> Date: Fri, 13 Dec 2013 11:04:53 +0800 From: Fam Zheng MIME-Version: 1.0 References: <1386836626-6436-1-git-send-email-famz@redhat.com> <1386836626-6436-7-git-send-email-famz@redhat.com> <87vbyu89mi.fsf@blackfin.pond.sub.org> In-Reply-To: <87vbyu89mi.fsf@blackfin.pond.sub.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v7 06/10] block: Replace in_use with operation blocker List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Markus Armbruster Cc: kwolf@redhat.com, qemu-devel@nongnu.org, rjones@redhat.com, imain@redhat.com, stefanha@redhat.com, pbonzini@redhat.com On 2013=E5=B9=B412=E6=9C=8812=E6=97=A5 21:46, Markus Armbruster wrote: > Fam Zheng writes: > >> This drops BlockDriverState.in_use with op_blockers: >> >> - Call bdrv_op_block_all in place of bdrv_set_in_use(bs, 1). >> - Call bdrv_op_unblock_all in place of bdrv_set_in_use(bs, 0). >> - Check bdrv_op_is_blocked() in place of bdrv_in_use(bs). >> The specific types are used, e.g. in place of starting block back= up, >> bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP, ...). >> - Check bdrv_op_blocker_is_empty() in place of assert(!bs->in_use). >> >> Note: there is no block for only specific types for now, i.e. a caller >> blocks all or none. So although the checks are type specific, the abov= e >> changes can still be seen as identical in logic. > > PATCH 3/10 adds a new blocker that doesn't block all or none. It's not > related to existing in_use, though, and therefore doesn't contradict > your "no functional change" claim. > > I've always hated in_use. Its meaning is ill-defined, and its use is > confusing. Three cheers for getting rid of it! > Markus, Thank you very much for the thorough and nice review on this. The commit=20 message apparently went out dated, because I reordered the patch and did=20 numbers of rebase during my self reviewing and testing. I meant to make it mechanism, and will fix toward that as you suggested.=20 So I'll put it back to the first part of series in the next revision, to=20 avoid preceding patches introducing blocker (like you pointed out). >> Signed-off-by: Fam Zheng >> >> --- >> block-migration.c | 7 +++++-- >> block.c | 32 +++++++++++++++++++------------= - >> blockdev.c | 29 +++++++++++++++++++---------- >> blockjob.c | 12 +++++++----- >> hw/block/dataplane/virtio-blk.c | 16 ++++++++++------ >> include/block/block.h | 2 -- >> include/block/block_int.h | 1 - >> include/block/blockjob.h | 3 +++ >> 8 files changed, 63 insertions(+), 39 deletions(-) >> >> diff --git a/block-migration.c b/block-migration.c >> index 897fdba..bf9a25f 100644 >> --- a/block-migration.c >> +++ b/block-migration.c >> @@ -59,6 +59,7 @@ typedef struct BlkMigDevState { >> unsigned long *aio_bitmap; >> int64_t completed_sectors; >> BdrvDirtyBitmap *dirty_bitmap; >> + Error *blocker; >> } BlkMigDevState; >> >> typedef struct BlkMigBlock { >> @@ -346,7 +347,8 @@ static void init_blk_migration_it(void *opaque, Bl= ockDriverState *bs) >> bmds->completed_sectors =3D 0; >> bmds->shared_base =3D block_mig_state.shared_base; >> alloc_aio_bitmap(bmds); >> - bdrv_set_in_use(bs, 1); >> + error_setg(&bmds->blocker, "block device is in use by migrati= on"); >> + bdrv_op_block_all(bs, bmds->blocker); >> bdrv_ref(bs); >> >> block_mig_state.total_sector_sum +=3D sectors; >> @@ -584,7 +586,8 @@ static void blk_mig_cleanup(void) >> blk_mig_lock(); >> while ((bmds =3D QSIMPLEQ_FIRST(&block_mig_state.bmds_list)) !=3D= NULL) { >> QSIMPLEQ_REMOVE_HEAD(&block_mig_state.bmds_list, entry); >> - bdrv_set_in_use(bmds->bs, 0); >> + bdrv_op_unblock_all(bmds->bs, bmds->blocker); >> + error_free(bmds->blocker); >> bdrv_unref(bmds->bs); >> g_free(bmds->aio_bitmap); >> g_free(bmds); >> diff --git a/block.c b/block.c >> index 681d3be..f59f398 100644 >> --- a/block.c >> +++ b/block.c >> @@ -1652,15 +1652,17 @@ static void bdrv_move_feature_fields(BlockDriv= erState *bs_dest, >> bs_dest->refcnt =3D bs_src->refcnt; >> >> /* job */ >> - bs_dest->in_use =3D bs_src->in_use; >> bs_dest->job =3D bs_src->job; >> >> /* keep the same entry in bdrv_states */ >> pstrcpy(bs_dest->device_name, sizeof(bs_dest->device_name), >> bs_src->device_name); >> + memcpy(bs_dest->op_blockers, bs_src->op_blockers, >> + sizeof(bs_dest->op_blockers)); >> bs_dest->list =3D bs_src->list; >> } >> > > Doesn't the new memcpy() belong to PATCH 02/10? > Yes, thanks. >> +static bool bdrv_op_blocker_is_empty(BlockDriverState *bs); >> /* >> * Swap bs contents for two image chains while they are live, >> * while keeping required fields on the BlockDriverState that is >> @@ -1682,7 +1684,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDr= iverState *bs_old) >> assert(QLIST_EMPTY(&bs_new->dirty_bitmaps)); >> assert(bs_new->job =3D=3D NULL); >> assert(bs_new->dev =3D=3D NULL); >> - assert(bs_new->in_use =3D=3D 0); >> assert(bs_new->io_limits_enabled =3D=3D false); >> assert(!throttle_have_timer(&bs_new->throttle_state)); >> > > Why can you drop the !in_use assertion rather than replacing it by a > bdrv_op_blocker_is_empty() assertion like you do elsewhere? > I shouldn't, will fix. >> @@ -1701,7 +1702,6 @@ void bdrv_swap(BlockDriverState *bs_new, BlockDr= iverState *bs_old) >> /* Check a few fields that should remain attached to the device = */ >> assert(bs_new->dev =3D=3D NULL); >> assert(bs_new->job =3D=3D NULL); >> - assert(bs_new->in_use =3D=3D 0); >> assert(bs_new->io_limits_enabled =3D=3D false); >> assert(!throttle_have_timer(&bs_new->throttle_state)); >> > > Same question. Will fix too. > >> @@ -1738,7 +1738,7 @@ static void bdrv_delete(BlockDriverState *bs) >> { >> assert(!bs->dev); >> assert(!bs->job); >> - assert(!bs->in_use); >> + assert(bdrv_op_blocker_is_empty(bs)); >> assert(!bs->refcnt); >> assert(QLIST_EMPTY(&bs->dirty_bitmaps)); >> >> @@ -1912,6 +1912,7 @@ int bdrv_commit(BlockDriverState *bs) >> int ret =3D 0; >> uint8_t *buf; >> char filename[PATH_MAX]; >> + Error *local_err; > > Recommend to always initialize local_err =3D NULL on general principles. > > Your code doesn't strictly require it, because you use it only after > bdrv_op_is_blocked() set it. Initializing it is more obviously correct= , > and more robust against change. > > But: since your code doesn't do anything with local_err, simply drop it= , > and pass NULL to brdv_op_is_blocked(). > Good suggestion, thanks. >> >> if (!drv) >> return -ENOMEDIUM; >> @@ -1920,7 +1921,9 @@ int bdrv_commit(BlockDriverState *bs) >> return -ENOTSUP; >> } >> >> - if (bdrv_in_use(bs) || bdrv_in_use(bs->backing_hd)) { >> + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, &local_err) || >> + bdrv_op_is_blocked(bs->backing_hd, BLOCK_OP_TYPE_COMMIT, &loc= al_err)) { >> + error_free(local_err); >> return -EBUSY; >> } >> >> @@ -2935,6 +2938,7 @@ int coroutine_fn bdrv_co_write_zeroes(BlockDrive= rState *bs, >> int bdrv_truncate(BlockDriverState *bs, int64_t offset) >> { >> BlockDriver *drv =3D bs->drv; >> + Error *local_err; >> int ret; >> if (!drv) >> return -ENOMEDIUM; >> @@ -2942,8 +2946,10 @@ int bdrv_truncate(BlockDriverState *bs, int64_t= offset) >> return -ENOTSUP; >> if (bs->read_only) >> return -EACCES; >> - if (bdrv_in_use(bs)) >> + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_RESIZE, &local_err)) { >> + error_free(local_err); >> return -EBUSY; >> + } >> ret =3D drv->bdrv_truncate(bs, offset); >> if (ret =3D=3D 0) { >> ret =3D refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS= ); > > Likewise. > Yes. >> @@ -4711,15 +4717,15 @@ void bdrv_op_unblock_all(BlockDriverState *bs,= Error *reason) >> } >> } >> >> -void bdrv_set_in_use(BlockDriverState *bs, int in_use) >> +static bool bdrv_op_blocker_is_empty(BlockDriverState *bs) > > I'd call it bdrv_op_is_any_blocked(), for symmetry with > bdrv_op_is_any_blocked(). Your choice. > I'd like to keep it as is for now. Renaming is easy while there are=20 non-trivial fixes needed for the series. >> { >> - assert(bs->in_use !=3D in_use); >> - bs->in_use =3D in_use; >> -} >> + bool ret =3D true; >> + int i; >> >> -int bdrv_in_use(BlockDriverState *bs) >> -{ >> - return bs->in_use; >> + for (i =3D 0; i < BLOCK_OP_TYPE_MAX; i++) { >> + ret =3D ret && QLIST_EMPTY(&bs->op_blockers[i]); >> + } >> + return ret; > > I find this code simpler: > > int i; > > for (i =3D 0; i < BLOCK_OP_TYPE_MAX; i++) { > if (!QLIST_EMPTY(&bs->op_blockers[i])) { > return false; > } > } > return true; > Taken, thanks. > >> } >> >> void bdrv_iostatus_enable(BlockDriverState *bs) >> diff --git a/blockdev.c b/blockdev.c >> index 44755e1..369d8da 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -1224,8 +1224,8 @@ static void external_snapshot_prepare(BlkTransac= tionState *common, >> return; >> } >> >> - if (bdrv_in_use(state->old_bs)) { >> - error_set(errp, QERR_DEVICE_IN_USE, device); >> + if (bdrv_op_is_blocked(state->old_bs, >> + BLOCK_OP_TYPE_EXTERNAL_SNAPSHOT, errp)) { >> return; >> } >> > > This improves the error reported back to the caller, contradicting the > commit message's claim "no functional change". I don't mind, I just > want it noted in the commit message. > OK. >> @@ -1441,10 +1441,10 @@ exit: >> >> static void eject_device(BlockDriverState *bs, int force, Error **er= rp) >> { >> - if (bdrv_in_use(bs)) { >> - error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs))= ; >> + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_EJECT, errp)) { >> return; >> } >> + >> if (!bdrv_dev_has_removable_media(bs)) { >> error_set(errp, QERR_DEVICE_NOT_REMOVABLE, bdrv_get_device_n= ame(bs)); >> return; > > Likewise. > >> @@ -1637,14 +1637,16 @@ int do_drive_del(Monitor *mon, const QDict *qd= ict, QObject **ret_data) >> { >> const char *id =3D qdict_get_str(qdict, "id"); >> BlockDriverState *bs; >> + Error *local_err; >> >> bs =3D bdrv_find(id); >> if (!bs) { >> qerror_report(QERR_DEVICE_NOT_FOUND, id); >> return -1; >> } >> - if (bdrv_in_use(bs)) { >> - qerror_report(QERR_DEVICE_IN_USE, id); >> + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_DRIVE_DEL, &local_err)) = { >> + error_report("%s", error_get_pretty(local_err)); >> + error_free(local_err); > > Likewise. > > Caveat, emptor: even I am unsure about which of the many error reportin= g > interfaces is to be used in which context, but here goes anyway: This > isn't wrong, because do_drive_del() isn't reachable from QMP. Still, > qerror_report_err() would be more robust against change. > > Maybe Luiz can give more definite advice. > I'll keep the original error message and drop local_err. >> return -1; >> } >> >> @@ -1755,6 +1757,10 @@ void qmp_block_stream(const char *device, bool = has_base, >> return; >> } >> >> + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_STREAM, errp)) { >> + return; >> + } >> + >> if (base) { >> base_bs =3D bdrv_find_backing_image(bs, base); >> if (base_bs =3D=3D NULL) { > > Oh, here you *add* a blocker check! Why shouldn't this be a separate > patch? > >> @@ -1795,6 +1801,10 @@ void qmp_block_commit(const char *device, >> return; >> } >> >> + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_COMMIT, errp)) { >> + return; >> + } >> + >> /* default top_bs is the active layer */ >> top_bs =3D bs; >> > > Likewise. > These two are part of replacement for in_use check in=20 block_job_create(), where checking for bdrv_op_blocker_is_empty()=20 doesn't sound perfect, but works. Let's convert to that weird check=20 intermediately and improve through a separate patch. >> @@ -1881,8 +1891,7 @@ void qmp_drive_backup(const char *device, const = char *target, >> } >> } >> >> - if (bdrv_in_use(bs)) { >> - error_set(errp, QERR_DEVICE_IN_USE, device); >> + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_BACKUP_SOURCE, errp)) { >> return; >> } >> > > Another error message improvement. > >> @@ -2011,11 +2020,11 @@ void qmp_drive_mirror(const char *device, cons= t char *target, >> } >> } >> >> - if (bdrv_in_use(bs)) { >> - error_set(errp, QERR_DEVICE_IN_USE, device); >> + if (bdrv_op_is_blocked(bs, BLOCK_OP_TYPE_MIRROR, errp)) { > > Likewise. > >> return; >> } >> >> + > > Spurious whitespace change, please drop. > OK. >> flags =3D bs->open_flags | BDRV_O_RDWR; >> source =3D bs->backing_hd; >> if (!source && sync =3D=3D MIRROR_SYNC_MODE_TOP) { >> diff --git a/blockjob.c b/blockjob.c >> index 9e5fd5c..e198cb2 100644 >> --- a/blockjob.c >> +++ b/blockjob.c >> @@ -41,13 +41,11 @@ void *block_job_create(const BlockJobDriver *drive= r, BlockDriverState *bs, >> { >> BlockJob *job; >> >> - if (bs->job || bdrv_in_use(bs)) { >> + if (bs->job) { >> error_set(errp, QERR_DEVICE_IN_USE, bdrv_get_device_name(bs)= ); >> return NULL; >> } > > Why can you drop bdrv_in_use() without replacement here? > As explained above. >> bdrv_ref(bs); >> - bdrv_set_in_use(bs, 1); >> - >> job =3D g_malloc0(driver->instance_size); >> job->driver =3D driver; >> job->bs =3D bs; >> @@ -64,11 +62,14 @@ void *block_job_create(const BlockJobDriver *drive= r, BlockDriverState *bs, >> if (error_is_set(&local_err)) { >> bs->job =3D NULL; >> g_free(job); >> - bdrv_set_in_use(bs, 0); >> error_propagate(errp, local_err); >> return NULL; >> } >> } >> + error_setg(&job->blocker, "block device is in use by block job: %= s", >> + BlockJobType_lookup[driver->job_type]); >> + bdrv_op_block_all(bs, job->blocker); >> + >> return job; >> } >> > > The replacement for bdrv_set_in_use() is in a different place. Makes > sense to me, but it should be a separate patch, to keep this one as > mechanical as possible. > OK, makes sense. >> @@ -79,8 +80,9 @@ void block_job_completed(BlockJob *job, int ret) >> assert(bs->job =3D=3D job); >> job->cb(job->opaque, ret); >> bs->job =3D NULL; >> + bdrv_op_unblock_all(bs, job->blocker); >> + error_free(job->blocker); >> g_free(job); >> - bdrv_set_in_use(bs, 0); >> } >> >> void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) >> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virt= io-blk.c >> index f2d7350..0a7b759 100644 >> --- a/hw/block/dataplane/virtio-blk.c >> +++ b/hw/block/dataplane/virtio-blk.c >> @@ -69,6 +69,9 @@ struct VirtIOBlockDataPlane { >> queue */ >> >> unsigned int num_reqs; >> + >> + /* Operation blocker on BDS */ >> + Error *blocker; >> }; >> >> /* Raise an interrupt to signal guest, if necessary */ >> @@ -385,6 +388,7 @@ bool virtio_blk_data_plane_create(VirtIODevice *vd= ev, VirtIOBlkConf *blk, >> { >> VirtIOBlockDataPlane *s; >> int fd; >> + Error *local_err =3D NULL; >> >> *dataplane =3D NULL; >> >> @@ -406,8 +410,9 @@ bool virtio_blk_data_plane_create(VirtIODevice *vd= ev, VirtIOBlkConf *blk, >> /* If dataplane is (re-)enabled while the guest is running there= could be >> * block jobs that can conflict. >> */ >> - if (bdrv_in_use(blk->conf.bs)) { >> - error_report("cannot start dataplane thread while device is i= n use"); >> + if (bdrv_op_is_blocked(blk->conf.bs, BLOCK_OP_TYPE_DATAPLANE, &lo= cal_err)) { >> + error_report("%s", error_get_pretty(local_err)); >> + error_free(local_err); >> return false; >> } >> > > Error message change. The new one tells us why in more detail, but is > doesn't tell us what failed. Have you reproduced the error to make sur= e > the message still makes sense? > No, I'll concatenate the original test with local_err. >> @@ -422,9 +427,8 @@ bool virtio_blk_data_plane_create(VirtIODevice *vd= ev, VirtIOBlkConf *blk, >> s->vdev =3D vdev; >> s->fd =3D fd; >> s->blk =3D blk; >> - >> - /* Prevent block operations that conflict with data plane thread = */ >> - bdrv_set_in_use(blk->conf.bs, 1); >> + error_setg(&s->blocker, "block device is in use by data plane"); >> + bdrv_op_block_all(blk->conf.bs, s->blocker); >> >> *dataplane =3D s; >> return true; >> @@ -437,7 +441,7 @@ void virtio_blk_data_plane_destroy(VirtIOBlockData= Plane *s) >> } >> >> virtio_blk_data_plane_stop(s); >> - bdrv_set_in_use(s->blk->conf.bs, 0); >> + bdrv_op_unblock_all(s->blk->conf.bs, s->blocker); > > Don't you want to error_free(s->blocker)? > Good catch. Thanks. Fam