From 1bd84ee717bf146c19281cce48a36a2f4d71748d Mon Sep 17 00:00:00 2001 From: Alberto Garcia Date: Thu, 2 Jul 2015 11:06:11 +0300 Subject: [PATCH 1/6] qcow2: remove unnecessary check The value of 'i' is guaranteed to be >= 0 Signed-off-by: Alberto Garcia Message-id: 1435824371-2660-1-git-send-email-berto@igalia.com Signed-off-by: Stefan Hajnoczi --- block/qcow2-cache.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c index ed92a098c4..53b8afc3d3 100644 --- a/block/qcow2-cache.c +++ b/block/qcow2-cache.c @@ -281,9 +281,6 @@ static int qcow2_cache_do_get(BlockDriverState *bs, Qcow2Cache *c, i = min_lru_index; trace_qcow2_cache_get_replace_entry(qemu_coroutine_self(), c == s->l2_table_cache, i); - if (i < 0) { - return i; - } ret = qcow2_cache_entry_flush(bs, c, i); if (ret < 0) { From 7a63f3cdc44230109c91cdc0ee912c3cc7837141 Mon Sep 17 00:00:00 2001 From: Stefan Hajnoczi Date: Thu, 2 Jul 2015 17:24:41 +0100 Subject: [PATCH 2/6] block: update bdrv_drain_all()/bdrv_drain() comments The doc comments for bdrv_drain_all() and bdrv_drain() are outdated: * The bdrv_drain() comment is a poor man's bdrv_lock()/bdrv_unlock() which Fam Zheng is currently developing. Unfortunately this warning was never really enough because devices keep submitting I/O and op blockers don't prevent that. * The bdrv_drain_all() comment is still partially correct but reflects the nature of the implementation rather than API documentation. Do make it clear that bdrv_drain() is only appropriate within an AioContext. For anything spanning AioContexts you need bdrv_drain_all(). Cc: Markus Armbruster Cc: Paolo Bonzini Signed-off-by: Stefan Hajnoczi Reviewed-by: Markus Armbruster Reviewed-by: Fam Zheng Message-id: 1435854281-6078-1-git-send-email-stefanha@redhat.com --- block/io.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/block/io.c b/block/io.c index 305e0d952e..d4bc83b33b 100644 --- a/block/io.c +++ b/block/io.c @@ -236,12 +236,12 @@ static bool bdrv_requests_pending(BlockDriverState *bs) /* * Wait for pending requests to complete on a single BlockDriverState subtree * - * See the warning in bdrv_drain_all(). This function can only be called if - * you are sure nothing can generate I/O because you have op blockers - * installed. - * * Note that unlike bdrv_drain_all(), the caller must hold the BlockDriverState * AioContext. + * + * Only this BlockDriverState's AioContext is run, so in-flight requests must + * not depend on events in other AioContexts. In that case, use + * bdrv_drain_all() instead. */ void bdrv_drain(BlockDriverState *bs) { @@ -260,12 +260,6 @@ void bdrv_drain(BlockDriverState *bs) * * This function does not flush data to disk, use bdrv_flush_all() for that * after calling this function. - * - * Note that completion of an asynchronous I/O operation can trigger any - * number of other I/O operations on other devices---for example a coroutine - * can be arbitrarily complex and a constant flow of I/O can come until the - * coroutine is complete. Because of this, it is not possible to have a - * function to drain a single device's I/O queue. */ void bdrv_drain_all(void) { @@ -288,6 +282,12 @@ void bdrv_drain_all(void) } } + /* Note that completion of an asynchronous I/O operation can trigger any + * number of other I/O operations on other devices---for example a + * coroutine can submit an I/O request to another device in response to + * request completion. Therefore we must keep looping until there was no + * more activity rather than simply draining each device independently. + */ while (busy) { busy = false; From c2e0dbbfd7265eb9a7170ab195d8f9f8a1cbd1af Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Mon, 6 Jul 2015 12:24:44 +0800 Subject: [PATCH 3/6] block: Initialize local_err in bdrv_append_temp_snapshot Cc: qemu-stable@nongnu.org Signed-off-by: Fam Zheng Message-id: 1436156684-16526-1-git-send-email-famz@redhat.com Signed-off-by: Stefan Hajnoczi --- block.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/block.c b/block.c index 7e130cc528..42eb8e3610 100644 --- a/block.c +++ b/block.c @@ -1271,7 +1271,7 @@ int bdrv_append_temp_snapshot(BlockDriverState *bs, int flags, Error **errp) QemuOpts *opts = NULL; QDict *snapshot_options; BlockDriverState *bs_snapshot; - Error *local_err; + Error *local_err = NULL; int ret; /* if snapshot, we create a temporary backing file and open it From 53ec73e264f481b79b52efcadc9ceb8f8996975c Mon Sep 17 00:00:00 2001 From: Fam Zheng Date: Fri, 29 May 2015 18:53:14 +0800 Subject: [PATCH 4/6] block: Use bdrv_drain to replace uncessary bdrv_drain_all There callers work on a single BlockDriverState subtree, where using bdrv_drain() is more accurate. Signed-off-by: Fam Zheng Signed-off-by: Stefan Hajnoczi --- block.c | 6 +++--- block/snapshot.c | 2 +- migration/block.c | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/block.c b/block.c index 42eb8e3610..5e80336e9f 100644 --- a/block.c +++ b/block.c @@ -1841,9 +1841,9 @@ void bdrv_close(BlockDriverState *bs) if (bs->job) { block_job_cancel_sync(bs->job); } - bdrv_drain_all(); /* complete I/O */ + bdrv_drain(bs); /* complete I/O */ bdrv_flush(bs); - bdrv_drain_all(); /* in case flush left pending I/O */ + bdrv_drain(bs); /* in case flush left pending I/O */ notifier_list_notify(&bs->close_notifiers, bs); if (bs->drv) { @@ -3906,7 +3906,7 @@ void bdrv_attach_aio_context(BlockDriverState *bs, void bdrv_set_aio_context(BlockDriverState *bs, AioContext *new_context) { - bdrv_drain_all(); /* ensure there are no in-flight requests */ + bdrv_drain(bs); /* ensure there are no in-flight requests */ bdrv_detach_aio_context(bs); diff --git a/block/snapshot.c b/block/snapshot.c index 19395ae014..49e143e991 100644 --- a/block/snapshot.c +++ b/block/snapshot.c @@ -239,7 +239,7 @@ int bdrv_snapshot_delete(BlockDriverState *bs, } /* drain all pending i/o before deleting snapshot */ - bdrv_drain_all(); + bdrv_drain(bs); if (drv->bdrv_snapshot_delete) { return drv->bdrv_snapshot_delete(bs, snapshot_id, name, errp); diff --git a/migration/block.c b/migration/block.c index ddb59ccf87..ed865ed23b 100644 --- a/migration/block.c +++ b/migration/block.c @@ -457,7 +457,7 @@ static int mig_save_device_dirty(QEMUFile *f, BlkMigDevState *bmds, blk_mig_lock(); if (bmds_aio_inflight(bmds, sector)) { blk_mig_unlock(); - bdrv_drain_all(); + bdrv_drain(bmds->bs); } else { blk_mig_unlock(); } From 25d9747b6427de8253221d544b45e50888d4cef7 Mon Sep 17 00:00:00 2001 From: "Richard W.M. Jones" Date: Wed, 1 Jul 2015 15:40:14 +0100 Subject: [PATCH 5/6] block/raw-posix: Don't think /dev/fd/ is a floppy drive. In libguestfs we use /dev/fd/ to pass pre-opened file descriptors to qemu-img. Lately I've discovered that although this works, qemu believes that these are floppy disk images. That in itself isn't much of a problem, but now qemu prints a warning about host floppy pass-thru being deprecated. Extend the existing test so that it ignores /dev/fd/ as well as /dev/fdset/ A simple test of this, if you are using the bash shell, is: qemu-img info <( cat /dev/null ) without this patch: $ qemu-img info <( cat /dev/null ) qemu-img: Host floppy pass-through is deprecated Support for it will be removed in a future release. qemu-img: Could not open '/dev/fd/63': Could not refresh total sector count: Illegal seek with this patch: $ qemu-img info <( cat /dev/null ) qemu-img: Could not open '/dev/fd/63': Could not refresh total sector count: Illegal seek Signed-off-by: Richard W.M. Jones Reviewed-by: Markus Armbruster Message-id: 1435761614-31358-1-git-send-email-rjones@redhat.com Fixes: https://bugs.launchpad.net/qemu/+bug/1470536 Signed-off-by: Stefan Hajnoczi --- block/raw-posix.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/block/raw-posix.c b/block/raw-posix.c index cbe6574bf4..855febed52 100644 --- a/block/raw-posix.c +++ b/block/raw-posix.c @@ -2430,7 +2430,8 @@ static int floppy_probe_device(const char *filename) struct stat st; if (strstart(filename, "/dev/fd", NULL) && - !strstart(filename, "/dev/fdset/", NULL)) { + !strstart(filename, "/dev/fdset/", NULL) && + !strstart(filename, "/dev/fd/", NULL)) { prio = 50; } From 970311646a701eecb103eb28093e8924d2fa6861 Mon Sep 17 00:00:00 2001 From: Ting Wang Date: Fri, 26 Jun 2015 17:37:35 +0800 Subject: [PATCH 6/6] blockjob: add block_job_release function There is job resource leak in function mirror_start_job, although bdrv_create_dirty_bitmap is unlikely failed. Add block_job_release for each release when needed. Signed-off-by: Ting Wang Reviewed-by: John Snow Message-id: 1435311455-56048-1-git-send-email-kathy.wangting@huawei.com Signed-off-by: Stefan Hajnoczi --- block/mirror.c | 2 ++ blockjob.c | 20 ++++++++++++-------- include/block/blockjob.h | 8 ++++++++ 3 files changed, 22 insertions(+), 8 deletions(-) diff --git a/block/mirror.c b/block/mirror.c index 8888cea952..d409337c4a 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -708,6 +708,8 @@ static void mirror_start_job(BlockDriverState *bs, BlockDriverState *target, s->dirty_bitmap = bdrv_create_dirty_bitmap(bs, granularity, NULL, errp); if (!s->dirty_bitmap) { + g_free(s->replaces); + block_job_release(bs); return; } bdrv_set_enable_write_cache(s->target, true); diff --git a/blockjob.c b/blockjob.c index ec46fad2f1..62bb906634 100644 --- a/blockjob.c +++ b/blockjob.c @@ -66,10 +66,7 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs, block_job_set_speed(job, speed, &local_err); if (local_err) { - bs->job = NULL; - bdrv_op_unblock_all(bs, job->blocker); - error_free(job->blocker); - g_free(job); + block_job_release(bs); error_propagate(errp, local_err); return NULL; } @@ -77,16 +74,23 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs, return job; } +void block_job_release(BlockDriverState *bs) +{ + BlockJob *job = bs->job; + + bs->job = NULL; + bdrv_op_unblock_all(bs, job->blocker); + error_free(job->blocker); + g_free(job); +} + void block_job_completed(BlockJob *job, int ret) { BlockDriverState *bs = job->bs; assert(bs->job == job); job->cb(job->opaque, ret); - bs->job = NULL; - bdrv_op_unblock_all(bs, job->blocker); - error_free(job->blocker); - g_free(job); + block_job_release(bs); } void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp) diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 57d8ef13e2..dd9d5e6aad 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -165,6 +165,14 @@ void block_job_sleep_ns(BlockJob *job, QEMUClockType type, int64_t ns); */ void block_job_yield(BlockJob *job); +/** + * block_job_release: + * @bs: The block device. + * + * Release job resources when an error occurred or job completed. + */ +void block_job_release(BlockDriverState *bs); + /** * block_job_completed: * @job: The job being completed.