[Qemu-devel] [PULL for-2.4 v2 0/6] Block patches

[Qemu-devel] [PULL for-2.4 v2 0/6] Block patches

[Stefan Hajnoczi]

The following changes since commit f6e3035f75e5c6a73485335765ae070304c7a110:

  Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream-smm' into staging (2015-07-06 23:37:53 +0100)

are available in the git repository at:

  git://github.com/stefanha/qemu.git tags/block-pull-request

for you to fetch changes up to 970311646a701eecb103eb28093e8924d2fa6861:

  blockjob: add block_job_release function (2015-07-07 14:27:14 +0100)

----------------------------------------------------------------
Pull request

v2:
 * Drop block/nfs patch since it exposes an unfinished QAPI interface [kwolf]

----------------------------------------------------------------

Alberto Garcia (1):
  qcow2: remove unnecessary check

Fam Zheng (2):
  block: Initialize local_err in bdrv_append_temp_snapshot
  block: Use bdrv_drain to replace uncessary bdrv_drain_all

Richard W.M. Jones (1):
  block/raw-posix: Don't think /dev/fd/<NN> is a floppy drive.

Stefan Hajnoczi (1):
  block: update bdrv_drain_all()/bdrv_drain() comments

Ting Wang (1):
  blockjob: add block_job_release function

 block.c                  |  8 ++++----
 block/io.c               | 20 ++++++++++----------
 block/mirror.c           |  2 ++
 block/qcow2-cache.c      |  3 ---
 block/raw-posix.c        |  3 ++-
 block/snapshot.c         |  2 +-
 blockjob.c               | 20 ++++++++++++--------
 include/block/blockjob.h |  8 ++++++++
 migration/block.c        |  2 +-
 9 files changed, 40 insertions(+), 28 deletions(-)

-- 
2.4.3

[Qemu-devel] [PULL for-2.4 v2 1/6] qcow2: remove unnecessary check

[Stefan Hajnoczi]

From: Alberto Garcia <berto@igalia.com>

The value of 'i' is guaranteed to be >= 0

Signed-off-by: Alberto Garcia <berto@igalia.com>
Message-id: 1435824371-2660-1-git-send-email-berto@igalia.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block/qcow2-cache.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/block/qcow2-cache.c b/block/qcow2-cache.c
index ed92a09..53b8afc 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) {
-- 
2.4.3

[Qemu-devel] [PULL for-2.4 v2 2/6] block: update bdrv_drain_all()/bdrv_drain() comments

[Stefan Hajnoczi]

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 <armbru@redhat.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
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 305e0d9..d4bc83b 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;
 
-- 
2.4.3

[Qemu-devel] [PULL for-2.4 v2 3/6] block: Initialize local_err in bdrv_append_temp_snapshot

[Stefan Hajnoczi]

From: Fam Zheng <famz@redhat.com>

Cc: qemu-stable@nongnu.org
Signed-off-by: Fam Zheng <famz@redhat.com>
Message-id: 1436156684-16526-1-git-send-email-famz@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 block.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block.c b/block.c
index 7e130cc..42eb8e3 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
-- 
2.4.3

[Qemu-devel] [PULL for-2.4 v2 4/6] block: Use bdrv_drain to replace uncessary bdrv_drain_all

[Stefan Hajnoczi]

From: Fam Zheng <famz@redhat.com>

There callers work on a single BlockDriverState subtree, where using
bdrv_drain() is more accurate.

Signed-off-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 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 42eb8e3..5e80336 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 19395ae..49e143e 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 ddb59cc..ed865ed 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();
         }
-- 
2.4.3

[Qemu-devel] [PULL for-2.4 v2 5/6] block/raw-posix: Don't think /dev/fd/<NN> is a floppy drive.

[Stefan Hajnoczi]

From: "Richard W.M. Jones" <rjones@redhat.com>

In libguestfs we use /dev/fd/<NN> 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 <rjones@redhat.com>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Message-id: 1435761614-31358-1-git-send-email-rjones@redhat.com
Fixes: https://bugs.launchpad.net/qemu/+bug/1470536
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 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 cbe6574..855febe 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;
     }
 
-- 
2.4.3

[Qemu-devel] [PULL for-2.4 v2 6/6] blockjob: add block_job_release function

[Stefan Hajnoczi]

From: Ting Wang <kathy.wangting@huawei.com>

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 <kathy.wangting@huawei.com>
Reviewed-by: John Snow <jsnow@redhat.com>
Message-id: 1435311455-56048-1-git-send-email-kathy.wangting@huawei.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 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 8888cea..d409337 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 ec46fad..62bb906 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,18 +74,25 @@ void *block_job_create(const BlockJobDriver *driver, BlockDriverState *bs,
     return job;
 }
 
-void block_job_completed(BlockJob *job, int ret)
+void block_job_release(BlockDriverState *bs)
 {
-    BlockDriverState *bs = job->bs;
+    BlockJob *job = bs->job;
 
-    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);
 }
 
+void block_job_completed(BlockJob *job, int ret)
+{
+    BlockDriverState *bs = job->bs;
+
+    assert(bs->job == job);
+    job->cb(job->opaque, ret);
+    block_job_release(bs);
+}
+
 void block_job_set_speed(BlockJob *job, int64_t speed, Error **errp)
 {
     Error *local_err = NULL;
diff --git a/include/block/blockjob.h b/include/block/blockjob.h
index 57d8ef1..dd9d5e6 100644
--- a/include/block/blockjob.h
+++ b/include/block/blockjob.h
@@ -166,6 +166,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.
  * @ret: The status code.
-- 
2.4.3

Re: [Qemu-devel] [PULL for-2.4 v2 0/6] Block patches

[Peter Maydell]

On 7 July 2015 at 14:29, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> The following changes since commit f6e3035f75e5c6a73485335765ae070304c7a110:
>
>   Merge remote-tracking branch 'remotes/bonzini/tags/for-upstream-smm' into staging (2015-07-06 23:37:53 +0100)
>
> are available in the git repository at:
>
>   git://github.com/stefanha/qemu.git tags/block-pull-request
>
> for you to fetch changes up to 970311646a701eecb103eb28093e8924d2fa6861:
>
>   blockjob: add block_job_release function (2015-07-07 14:27:14 +0100)
>
> ----------------------------------------------------------------
> Pull request
>
> v2:
>  * Drop block/nfs patch since it exposes an unfinished QAPI interface [kwolf]
>
> ----------------------------------------------------------------

Applied, thanks.

-- PMM