RE: [PATCH v3 1/3] hw/block/m25p80: Fix Numonyx NVCFG DIO and QIO bit polarity

[Joe Komlodi]

Hi Philippe,

-Original Message-
From: Philippe Mathieu-Daudé  
Sent: Monday, November 9, 2020 6:21 AM
To: Joe Komlodi ; qemu-de...@nongnu.org; Cédric Le Goater 
; Edgar Iglesias 
Cc: Francisco Eduardo Iglesias ; alist...@alistair23.me; 
philippe.mathieu.da...@gmail.com; qemu-block@nongnu.org; mre...@redhat.com
Subject: Re: [PATCH v3 1/3] hw/block/m25p80: Fix Numonyx NVCFG DIO and QIO bit 
polarity

On 11/6/20 2:32 AM, Joe Komlodi wrote:
> QIO and DIO modes should be enabled when the bits in NVCFG are set to 0.
> This matches the behavior of the other bits in the NVCFG register.

Is this material for the 5.2 release?

[Joe] If possible, yeah.  Otherwise if it's too late for 5.2 I have no problems 
pushing it to a later release.
Thanks!
Joe

> 
> Signed-off-by: Joe Komlodi 
> ---
>  hw/block/m25p80.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c index 
> 483925f..4255a6a 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -783,10 +783,10 @@ static void reset_memory(Flash *s)
>  s->enh_volatile_cfg |= EVCFG_OUT_DRIVER_STRENGTH_DEF;
>  s->enh_volatile_cfg |= EVCFG_VPP_ACCELERATOR;
>  s->enh_volatile_cfg |= EVCFG_RESET_HOLD_ENABLED;
> -if (s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK) {
> +if (!(s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK)) {
>  s->enh_volatile_cfg |= EVCFG_DUAL_IO_ENABLED;
>  }
> -if (s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK) {
> +if (!(s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK)) {
>  s->enh_volatile_cfg |= EVCFG_QUAD_IO_ENABLED;
>  }
>  if (!(s->nonvolatile_cfg & NVCFG_4BYTE_ADDR_MASK)) {
> 

Re: [PULL v2 00/15] Block patches for 5.2.0-rc1

[Peter Maydell]

On Mon, 9 Nov 2020 at 17:50, Max Reitz  wrote:
>
> The following changes since commit 2a190a7256a3e0563b29ffd67e0164097b4a6dac:
>
>   Merge remote-tracking branch 
> 'remotes/philmd-gitlab/tags/renesas-fixes-20201109' into staging (2020-11-09 
> 11:20:25 +)
>
> are available in the Git repository at:
>
>   https://github.com/XanClic/qemu.git tags/pull-block-2020-11-09-v2
>
> for you to fetch changes up to d669ed6ab028497d634e1f236c74a98725f9e45f:
>
>   block: make bdrv_drop_intermediate() less wrong (2020-11-09 18:43:31 +0100)
>
> 
> Block patches for 5.2.0-rc1:
> - Some nvme fixes (addressing problems spotted by Coverity)
> - Fix nfs compiling on mingw (and enable it in Cirrus)
> - Fix an error path in bdrv_co_invalidate_cache() (permission update
>   was initiated, but not aborted)
> - Fix (on-error) roll back in bdrv_drop_intermediate(): Instead of
>   inlining bdrv_replace_node() (wrongly), call that function
> - Fix for iotest 240
> - Fix error handling in bdrv_getlength()
> - Be more explicit about how QCowL2Meta objects are handled
> - Cleanups
>
> 
> v2:
> - Added missing Message-Id and Signed-off-by to patch 12


Applied, thanks.

Please update the changelog at https://wiki.qemu.org/ChangeLog/5.2
for any user-visible changes.

-- PMM

Re: [PULL 00/15] Block patches for 5.2.0-rc1

[Max Reitz]

On 09.11.20 18:38, Max Reitz wrote:

The following changes since commit 2a190a7256a3e0563b29ffd67e0164097b4a6dac:

   Merge remote-tracking branch 
'remotes/philmd-gitlab/tags/renesas-fixes-20201109' into staging (2020-11-09 
11:20:25 +)

are available in the Git repository at:

   https://github.com/XanClic/qemu.git tags/pull-block-2020-11-09

for you to fetch changes up to 8f88cb77b06be0c2e45ed31c9fd6817d2ce3836a:

   block: make bdrv_drop_intermediate() less wrong (2020-11-09 15:44:21 +0100)


Block patches for 5.2.0-rc1:
- Some nvme fixes (addressing problems spotted by Coverity)
- Fix nfs compiling on mingw (and enable it in Cirrus)
- Fix an error path in bdrv_co_invalidate_cache() (permission update
   was initiated, but not aborted)
- Fix (on-error) roll back in bdrv_drop_intermediate(): Instead of
   inlining bdrv_replace_node() (wrongly), call that function
- Fix for iotest 240
- Fix error handling in bdrv_getlength()
- Be more explicit about how QCowL2Meta objects are handled
- Cleanups


Sorry, NAK.  I applied patch 12 the wrong way (namely, just using 
git-am) and so it’s missing the Message-Id and my Signed-off-by.


v2 is on the way.

Max

[PULL v2 00/15] Block patches for 5.2.0-rc1

[Max Reitz]

The following changes since commit 2a190a7256a3e0563b29ffd67e0164097b4a6dac:

  Merge remote-tracking branch 
'remotes/philmd-gitlab/tags/renesas-fixes-20201109' into staging (2020-11-09 
11:20:25 +)

are available in the Git repository at:

  https://github.com/XanClic/qemu.git tags/pull-block-2020-11-09-v2

for you to fetch changes up to d669ed6ab028497d634e1f236c74a98725f9e45f:

  block: make bdrv_drop_intermediate() less wrong (2020-11-09 18:43:31 +0100)


Block patches for 5.2.0-rc1:
- Some nvme fixes (addressing problems spotted by Coverity)
- Fix nfs compiling on mingw (and enable it in Cirrus)
- Fix an error path in bdrv_co_invalidate_cache() (permission update
  was initiated, but not aborted)
- Fix (on-error) roll back in bdrv_drop_intermediate(): Instead of
  inlining bdrv_replace_node() (wrongly), call that function
- Fix for iotest 240
- Fix error handling in bdrv_getlength()
- Be more explicit about how QCowL2Meta objects are handled
- Cleanups


v2:
- Added missing Message-Id and Signed-off-by to patch 12


Alberto Garcia (1):
  qcow2: Document and enforce the QCowL2Meta invariants

AlexChen (1):
  block: Remove unused include

Eric Blake (1):
  block: Fix integer promotion error in bdrv_getlength()

Greg Kurz (1):
  block: Move bdrv_drain_all_end_quiesce() to block_int.h

Klaus Jensen (3):
  hw/block/nvme: fix null ns in register namespace
  hw/block/nvme: fix uint16_t use of uint32_t sgls member
  hw/block/nvme: fix free of array-typed value

Maxim Levitsky (2):
  iotests: add filter_qmp_virtio_scsi function
  iotests: rewrite iotest 240 in python

Vladimir Sementsov-Ogievskiy (3):
  block: add forgotten bdrv_abort_perm_update() to
bdrv_co_invalidate_cache()
  block: add bdrv_replace_node_common()
  block: make bdrv_drop_intermediate() less wrong

Yonggang Luo (2):
  block: Fixes nfs compiling error on msys2/mingw
  block: enable libnfs on msys2/mingw in cirrus.yml

shiliyang (1):
  block: Fix some code style problems, "foo* bar" should be "foo *bar"

 block/qcow2.h |  25 ++--
 include/block/block.h |   6 -
 include/block/block_int.h |   9 ++
 block.c   |  89 --
 block/blkdebug.c  |   2 +-
 block/dmg-lzfse.c |   1 -
 block/dmg.c   |   2 +-
 block/nfs.c   |  13 +-
 block/qcow2-cluster.c |   5 +-
 block/qcow2.c |  23 +++-
 block/vpc.c   |  10 +-
 hw/block/nvme.c   |   6 +-
 .cirrus.yml   |   1 +
 tests/qemu-iotests/240| 219 ++
 tests/qemu-iotests/240.out|  76 ++--
 tests/qemu-iotests/iotests.py |  10 ++
 16 files changed, 259 insertions(+), 238 deletions(-)

-- 
2.28.0

[PULL 15/15] block: make bdrv_drop_intermediate() less wrong

[Max Reitz]

From: Vladimir Sementsov-Ogievskiy 

First, permission update loop tries to do iterations transactionally,
but the whole update is not transactional: nobody roll-back successful
loop iterations when some iteration fails.

Second, in the iteration we have nested permission update:
c->klass->update_filename may point to bdrv_child_cb_update_filename()
which calls bdrv_backing_update_filename(), which may do node reopen to
RW.

Permission update system is not prepared to nested updates, at least it
has intermediate permission-update state stored in BdrvChild
structures: has_backup_perm, backup_perm and backup_shared_perm.

So, let's first do bdrv_replace_node_common() (which is more
transactional than open-coded update in bdrv_drop_intermediate()) and
then call update_filename() in separate. We still do not rollback
changes in case of update_filename() failure but it's not much worse
than pre-patch behavior.

Note that bdrv_replace_node_common() does check for frozen children,
so corresponding check is dropped in bdrv_drop_intermediate().

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20201106124241.16950-4-vsement...@virtuozzo.com>
Reviewed-by: Alberto Garcia 
Signed-off-by: Max Reitz 
---
 block.c | 54 --
 1 file changed, 24 insertions(+), 30 deletions(-)

diff --git a/block.c b/block.c
index 9a945a058d..f1cedac362 100644
--- a/block.c
+++ b/block.c
@@ -4910,9 +4910,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, 
BlockDriverState *base,
 {
 BlockDriverState *explicit_top = top;
 bool update_inherits_from;
-BdrvChild *c, *next;
+BdrvChild *c;
 Error *local_err = NULL;
 int ret = -EIO;
+g_autoptr(GSList) updated_children = NULL;
+GSList *p;
 
 bdrv_ref(top);
 bdrv_subtree_drained_begin(top);
@@ -4926,14 +4928,6 @@ int bdrv_drop_intermediate(BlockDriverState *top, 
BlockDriverState *base,
 goto exit;
 }
 
-/* This function changes all links that point to top and makes
- * them point to base. Check that none of them is frozen. */
-QLIST_FOREACH(c, &top->parents, next_parent) {
-if (c->frozen) {
-goto exit;
-}
-}
-
 /* If 'base' recursively inherits from 'top' then we should set
  * base->inherits_from to top->inherits_from after 'top' and all
  * other intermediate nodes have been dropped.
@@ -4950,36 +4944,36 @@ int bdrv_drop_intermediate(BlockDriverState *top, 
BlockDriverState *base,
 backing_file_str = base->filename;
 }
 
-QLIST_FOREACH_SAFE(c, &top->parents, next_parent, next) {
-/* Check whether we are allowed to switch c from top to base */
-GSList *ignore_children = g_slist_prepend(NULL, c);
-ret = bdrv_check_update_perm(base, NULL, c->perm, c->shared_perm,
- ignore_children, NULL, &local_err);
-g_slist_free(ignore_children);
-if (ret < 0) {
-error_report_err(local_err);
-goto exit;
-}
+QLIST_FOREACH(c, &top->parents, next_parent) {
+updated_children = g_slist_prepend(updated_children, c);
+}
+
+bdrv_replace_node_common(top, base, false, &local_err);
+if (local_err) {
+error_report_err(local_err);
+goto exit;
+}
+
+for (p = updated_children; p; p = p->next) {
+c = p->data;
 
-/* If so, update the backing file path in the image file */
 if (c->klass->update_filename) {
 ret = c->klass->update_filename(c, base, backing_file_str,
 &local_err);
 if (ret < 0) {
-bdrv_abort_perm_update(base);
+/*
+ * TODO: Actually, we want to rollback all previous iterations
+ * of this loop, and (which is almost impossible) previous
+ * bdrv_replace_node()...
+ *
+ * Note, that c->klass->update_filename may lead to permission
+ * update, so it's a bad idea to call it inside permission
+ * update transaction of bdrv_replace_node.
+ */
 error_report_err(local_err);
 goto exit;
 }
 }
-
-/*
- * Do the actual switch in the in-memory graph.
- * Completes bdrv_check_update_perm() transaction internally.
- * c->frozen is false, we have checked that above.
- */
-bdrv_ref(base);
-bdrv_replace_child(c, base);
-bdrv_unref(top);
 }
 
 if (update_inherits_from) {
-- 
2.28.0

[PULL 14/15] block: add bdrv_replace_node_common()

[Max Reitz]

From: Vladimir Sementsov-Ogievskiy 

Add new parameter to bdrv_replace_node(): auto_skip. With
auto_skip=false we'll have stricter behavior: update _all_ from
parents or fail. New behaviour will be used in the following commit in
block.c, so keep original function name as public interface.

Note: new error message is a bit funny in contrast with further
"Cannot" in case of frozen child, but we'd better keep some difference
to make it possible to distinguish one from another on failure. Still,
actually we'd better refactor should_update_child() call to distinguish
also different kinds of "should not". Let's do it later.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20201106124241.16950-3-vsement...@virtuozzo.com>
Reviewed-by: Alberto Garcia 
Signed-off-by: Max Reitz 
---
 block.c | 25 ++---
 1 file changed, 22 insertions(+), 3 deletions(-)

diff --git a/block.c b/block.c
index 1e68bd2f5f..9a945a058d 100644
--- a/block.c
+++ b/block.c
@@ -4563,8 +4563,16 @@ static bool should_update_child(BdrvChild *c, 
BlockDriverState *to)
 return ret;
 }
 
-void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
-   Error **errp)
+/*
+ * With auto_skip=true bdrv_replace_node_common skips updating from parents
+ * if it creates a parent-child relation loop or if parent is block-job.
+ *
+ * With auto_skip=false the error is returned if from has a parent which should
+ * not be updated.
+ */
+static void bdrv_replace_node_common(BlockDriverState *from,
+ BlockDriverState *to,
+ bool auto_skip, Error **errp)
 {
 BdrvChild *c, *next;
 GSList *list = NULL, *p;
@@ -4583,7 +4591,12 @@ void bdrv_replace_node(BlockDriverState *from, 
BlockDriverState *to,
 QLIST_FOREACH_SAFE(c, &from->parents, next_parent, next) {
 assert(c->bs == from);
 if (!should_update_child(c, to)) {
-continue;
+if (auto_skip) {
+continue;
+}
+error_setg(errp, "Should not change '%s' link to '%s'",
+   c->name, from->node_name);
+goto out;
 }
 if (c->frozen) {
 error_setg(errp, "Cannot change '%s' link to '%s'",
@@ -4623,6 +4636,12 @@ out:
 bdrv_unref(from);
 }
 
+void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to,
+   Error **errp)
+{
+return bdrv_replace_node_common(from, to, true, errp);
+}
+
 /*
  * Add new bs contents at the top of an image chain while the chain is
  * live, while keeping required fields on the top layer.
-- 
2.28.0

[PULL 13/15] block: add forgotten bdrv_abort_perm_update() to bdrv_co_invalidate_cache()

[Max Reitz]

From: Vladimir Sementsov-Ogievskiy 

Signed-off-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20201106124241.16950-2-vsement...@virtuozzo.com>
Reviewed-by: Alberto Garcia 
Signed-off-by: Max Reitz 
---
 block.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block.c b/block.c
index 2fd932154e..1e68bd2f5f 100644
--- a/block.c
+++ b/block.c
@@ -5787,6 +5787,7 @@ int coroutine_fn 
bdrv_co_invalidate_cache(BlockDriverState *bs, Error **errp)
 bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
 ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, errp);
 if (ret < 0) {
+bdrv_abort_perm_update(bs);
 bs->open_flags |= BDRV_O_INACTIVE;
 return ret;
 }
-- 
2.28.0

[PULL 09/15] block: Fixes nfs compiling error on msys2/mingw

[Max Reitz]

From: Yonggang Luo 

These compiling errors are fixed:
../block/nfs.c:27:10: fatal error: poll.h: No such file or directory
   27 | #include 
  |  ^~~~
compilation terminated.

../block/nfs.c:63:5: error: unknown type name 'blkcnt_t'
   63 | blkcnt_t st_blocks;
  | ^~~~
../block/nfs.c: In function 'nfs_client_open':
../block/nfs.c:550:27: error: 'struct _stat64' has no member named 'st_blocks'
  550 | client->st_blocks = st.st_blocks;
  |   ^
../block/nfs.c: In function 'nfs_get_allocated_file_size':
../block/nfs.c:751:41: error: 'struct _stat64' has no member named 'st_blocks'
  751 | return (task.ret < 0 ? task.ret : st.st_blocks * 512);
  | ^
../block/nfs.c: In function 'nfs_reopen_prepare':
../block/nfs.c:805:31: error: 'struct _stat64' has no member named 'st_blocks'
  805 | client->st_blocks = st.st_blocks;
  |   ^
../block/nfs.c: In function 'nfs_get_allocated_file_size':
../block/nfs.c:752:1: error: control reaches end of non-void function 
[-Werror=return-type]
  752 | }
  | ^

On msys2/mingw, there is no st_blocks in struct _stat64 yet, we disable the 
usage of it
on msys2/mingw, and create a typedef long long blkcnt_t; for further 
implementation

Signed-off-by: Yonggang Luo 
Message-Id: <20201105123116.674-2-luoyongg...@gmail.com>
Signed-off-by: Max Reitz 
---
 block/nfs.c | 13 -
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/block/nfs.c b/block/nfs.c
index f86e660374..77905f516d 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -24,7 +24,9 @@
 
 #include "qemu/osdep.h"
 
+#if !defined(_WIN32)
 #include 
+#endif
 #include "qemu/config-file.h"
 #include "qemu/error-report.h"
 #include "qapi/error.h"
@@ -58,7 +60,7 @@ typedef struct NFSClient {
 bool has_zero_init;
 AioContext *aio_context;
 QemuMutex mutex;
-blkcnt_t st_blocks;
+uint64_t st_blocks;
 bool cache_used;
 NFSServer *server;
 char *path;
@@ -545,7 +547,9 @@ static int64_t nfs_client_open(NFSClient *client, 
BlockdevOptionsNfs *opts,
 }
 
 ret = DIV_ROUND_UP(st.st_size, BDRV_SECTOR_SIZE);
+#if !defined(_WIN32)
 client->st_blocks = st.st_blocks;
+#endif
 client->has_zero_init = S_ISREG(st.st_mode);
 *strp = '/';
 goto out;
@@ -706,6 +710,7 @@ static int nfs_has_zero_init(BlockDriverState *bs)
 return client->has_zero_init;
 }
 
+#if !defined(_WIN32)
 /* Called (via nfs_service) with QemuMutex held.  */
 static void
 nfs_get_allocated_file_size_cb(int ret, struct nfs_context *nfs, void *data,
@@ -748,6 +753,7 @@ static int64_t nfs_get_allocated_file_size(BlockDriverState 
*bs)
 
 return (task.ret < 0 ? task.ret : st.st_blocks * 512);
 }
+#endif
 
 static int coroutine_fn
 nfs_file_co_truncate(BlockDriverState *bs, int64_t offset, bool exact,
@@ -800,7 +806,9 @@ static int nfs_reopen_prepare(BDRVReopenState *state,
nfs_get_error(client->context));
 return ret;
 }
+#if !defined(_WIN32)
 client->st_blocks = st.st_blocks;
+#endif
 }
 
 return 0;
@@ -869,7 +877,10 @@ static BlockDriver bdrv_nfs = {
 .create_opts= &nfs_create_opts,
 
 .bdrv_has_zero_init = nfs_has_zero_init,
+/* libnfs does not provide the allocated filesize of a file on win32. */
+#if !defined(_WIN32)
 .bdrv_get_allocated_file_size   = nfs_get_allocated_file_size,
+#endif
 .bdrv_co_truncate   = nfs_file_co_truncate,
 
 .bdrv_file_open = nfs_file_open,
-- 
2.28.0

[PULL 10/15] block: enable libnfs on msys2/mingw in cirrus.yml

[Max Reitz]

From: Yonggang Luo 

Initially, libnfs has not been enabled, and now it's fixed, so enable it
on cirrus.

Signed-off-by: Yonggang Luo 
Message-Id: <20201105123116.674-3-luoyongg...@gmail.com>
Signed-off-by: Max Reitz 
---
 .cirrus.yml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.cirrus.yml b/.cirrus.yml
index 900437dd2a..f0209b7a3e 100644
--- a/.cirrus.yml
+++ b/.cirrus.yml
@@ -109,6 +109,7 @@ windows_msys2_task:
   mingw-w64-x86_64-cyrus-sasl \
   mingw-w64-x86_64-curl \
   mingw-w64-x86_64-gnutls \
+  mingw-w64-x86_64-libnfs \
   "
 bitsadmin /transfer msys_download /dynamic /download /priority 
FOREGROUND `
   
https://repo.msys2.org/mingw/x86_64/mingw-w64-x86_64-python-sphinx-2.3.1-1-any.pkg.tar.xz
 `
-- 
2.28.0

[PULL 04/15] hw/block/nvme: fix null ns in register namespace

[Max Reitz]

From: Klaus Jensen 

Fix dereference after NULL check.

Reported-by: Coverity (CID 1436128)
Fixes: b20804946bce ("hw/block/nvme: update nsid when registered")
Signed-off-by: Klaus Jensen 
Message-Id: <20201104102248.32168-2-...@irrelevant.dk>
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Max Reitz 
---
 hw/block/nvme.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index fa2cba744b..080d782f1c 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2562,8 +2562,7 @@ int nvme_register_namespace(NvmeCtrl *n, NvmeNamespace 
*ns, Error **errp)
 
 if (!nsid) {
 for (int i = 1; i <= n->num_namespaces; i++) {
-NvmeNamespace *ns = nvme_ns(n, i);
-if (!ns) {
+if (!nvme_ns(n, i)) {
 nsid = ns->params.nsid = i;
 break;
 }
-- 
2.28.0

[PULL 12/15] block: Fix some code style problems, "foo* bar" should be "foo *bar"

[Max Reitz]

From: shiliyang 

There have some code style problems be found when read the block driver code.
So I fixes some problems of this error, ERROR: "foo* bar" should be "foo *bar".

Signed-off-by: Liyang Shi 
Reported-by: Euler Robot 
---
 block/qcow2.h|  6 +++---
 block/blkdebug.c |  2 +-
 block/dmg.c  |  2 +-
 block/qcow2.c|  4 ++--
 block/vpc.c  | 10 +-
 5 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 2e0272a7b8..0678073b74 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -343,8 +343,8 @@ typedef struct BDRVQcow2State {
 uint64_t l1_table_offset;
 uint64_t *l1_table;
 
-Qcow2Cache* l2_table_cache;
-Qcow2Cache* refcount_block_cache;
+Qcow2Cache *l2_table_cache;
+Qcow2Cache *refcount_block_cache;
 QEMUTimer *cache_clean_timer;
 unsigned cache_clean_interval;
 
@@ -394,7 +394,7 @@ typedef struct BDRVQcow2State {
 uint64_t autoclear_features;
 
 size_t unknown_header_fields_size;
-void* unknown_header_fields;
+void *unknown_header_fields;
 QLIST_HEAD(, Qcow2UnknownHeaderExtension) unknown_header_ext;
 QTAILQ_HEAD (, Qcow2DiscardRegion) discards;
 bool cache_discards;
diff --git a/block/blkdebug.c b/block/blkdebug.c
index 54da719dd1..5fe6172da9 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -173,7 +173,7 @@ static int add_rule(void *opaque, QemuOpts *opts, Error 
**errp)
 {
 struct add_rule_data *d = opaque;
 BDRVBlkdebugState *s = d->s;
-const char* event_name;
+const char *event_name;
 int event;
 struct BlkdebugRule *rule;
 int64_t sector;
diff --git a/block/dmg.c b/block/dmg.c
index 0d6c317296..ef35a505f2 100644
--- a/block/dmg.c
+++ b/block/dmg.c
@@ -559,7 +559,7 @@ static void dmg_refresh_limits(BlockDriverState *bs, Error 
**errp)
 bs->bl.request_alignment = BDRV_SECTOR_SIZE; /* No sub-sector I/O */
 }
 
-static inline int is_sector_in_chunk(BDRVDMGState* s,
+static inline int is_sector_in_chunk(BDRVDMGState *s,
 uint32_t chunk_num, uint64_t sector_num)
 {
 if (chunk_num >= s->n_chunks || s->sectors[chunk_num] > sector_num ||
diff --git a/block/qcow2.c b/block/qcow2.c
index 1b0733238b..3a90ef2786 100644
--- a/block/qcow2.c
+++ b/block/qcow2.c
@@ -269,7 +269,7 @@ static int qcow2_read_extensions(BlockDriverState *bs, 
uint64_t start_offset,
 
 case QCOW2_EXT_MAGIC_FEATURE_TABLE:
 if (p_feature_table != NULL) {
-void* feature_table = g_malloc0(ext.len + 2 * 
sizeof(Qcow2Feature));
+void *feature_table = g_malloc0(ext.len + 2 * 
sizeof(Qcow2Feature));
 ret = bdrv_pread(bs->file, offset , feature_table, ext.len);
 if (ret < 0) {
 error_setg_errno(errp, -ret, "ERROR: ext_feature_table: "
@@ -3388,7 +3388,7 @@ qcow2_co_create(BlockdevCreateOptions *create_options, 
Error **errp)
 size_t cluster_size;
 int version;
 int refcount_order;
-uint64_t* refcount_table;
+uint64_t *refcount_table;
 int ret;
 uint8_t compression_type = QCOW2_COMPRESSION_TYPE_ZLIB;
 
diff --git a/block/vpc.c b/block/vpc.c
index 890554277e..1ab55f9287 100644
--- a/block/vpc.c
+++ b/block/vpc.c
@@ -172,7 +172,7 @@ static QemuOptsList vpc_runtime_opts = {
 
 static QemuOptsList vpc_create_opts;
 
-static uint32_t vpc_checksum(uint8_t* buf, size_t size)
+static uint32_t vpc_checksum(uint8_t *buf, size_t size)
 {
 uint32_t res = 0;
 int i;
@@ -528,7 +528,7 @@ static inline int64_t get_image_offset(BlockDriverState 
*bs, uint64_t offset,
  *
  * Returns 0 on success and < 0 on error
  */
-static int rewrite_footer(BlockDriverState* bs)
+static int rewrite_footer(BlockDriverState *bs)
 {
 int ret;
 BDRVVPCState *s = bs->opaque;
@@ -548,7 +548,7 @@ static int rewrite_footer(BlockDriverState* bs)
  *
  * Returns the sectors' offset in the image file on success and < 0 on error
  */
-static int64_t alloc_block(BlockDriverState* bs, int64_t offset)
+static int64_t alloc_block(BlockDriverState *bs, int64_t offset)
 {
 BDRVVPCState *s = bs->opaque;
 int64_t bat_offset;
@@ -781,8 +781,8 @@ static int coroutine_fn 
vpc_co_block_status(BlockDriverState *bs,
  * the hardware EIDE and ATA-2 limit of 16 heads (max disk size of 127 GB)
  * and instead allow up to 255 heads.
  */
-static int calculate_geometry(int64_t total_sectors, uint16_t* cyls,
-uint8_t* heads, uint8_t* secs_per_cyl)
+static int calculate_geometry(int64_t total_sectors, uint16_t *cyls,
+uint8_t *heads, uint8_t *secs_per_cyl)
 {
 uint32_t cyls_times_heads;
 
-- 
2.28.0

[PULL 06/15] hw/block/nvme: fix free of array-typed value

[Max Reitz]

From: Klaus Jensen 

Since 7f0f1acedf15 ("hw/block/nvme: support multiple namespaces"), the
namespaces member of NvmeCtrl is no longer a dynamically allocated
array. Remove the free.

Fixes: 7f0f1acedf15 ("hw/block/nvme: support multiple namespaces")
Reported-by: Coverity (CID 1436131)
Signed-off-by: Klaus Jensen 
Message-Id: <20201104102248.32168-4-...@irrelevant.dk>
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Max Reitz 
---
 hw/block/nvme.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 2bdc50eb6f..01b657b1c5 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -2799,7 +2799,6 @@ static void nvme_exit(PCIDevice *pci_dev)
 NvmeCtrl *n = NVME(pci_dev);
 
 nvme_clear_ctrl(n);
-g_free(n->namespaces);
 g_free(n->cq);
 g_free(n->sq);
 g_free(n->aer_reqs);
-- 
2.28.0

[PULL 01/15] block: Remove unused include

[Max Reitz]

From: AlexChen 

The "qemu-common.h" include is not used, remove it.

Reported-by: Euler Robot 
Signed-off-by: AlexChen 
Message-Id: <5f8ffb94.3030...@huawei.com>
Signed-off-by: Max Reitz 
---
 block/dmg-lzfse.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/block/dmg-lzfse.c b/block/dmg-lzfse.c
index 19d25bc646..6798cf4fbf 100644
--- a/block/dmg-lzfse.c
+++ b/block/dmg-lzfse.c
@@ -22,7 +22,6 @@
  * THE SOFTWARE.
  */
 #include "qemu/osdep.h"
-#include "qemu-common.h"
 #include "dmg.h"
 #include 
 
-- 
2.28.0

[PULL 08/15] iotests: rewrite iotest 240 in python

[Max Reitz]

From: Maxim Levitsky 

The recent changes that brought RCU delayed device deletion,
broke few tests and this test breakage went unnoticed.

Fix this test by rewriting it in python
(which allows to wait for DEVICE_DELETED events before continuing).

Signed-off-by: Maxim Levitsky 
Tested-by: Christian Borntraeger 
Reviewed-by: Paolo Bonzini 
Message-Id: <20201104185025.434703-3-mlevi...@redhat.com>
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/240 | 219 +++--
 tests/qemu-iotests/240.out |  76 +++--
 2 files changed, 130 insertions(+), 165 deletions(-)

diff --git a/tests/qemu-iotests/240 b/tests/qemu-iotests/240
index 8b4337b58d..c0f71f0461 100755
--- a/tests/qemu-iotests/240
+++ b/tests/qemu-iotests/240
@@ -1,5 +1,5 @@
-#!/usr/bin/env bash
-#
+#!/usr/bin/env python3
+
 # Test hot plugging and unplugging with iothreads
 #
 # Copyright (C) 2019 Igalia, S.L.
@@ -17,133 +17,90 @@
 #
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see .
-#
 
-# creator
-owner=be...@igalia.com
-
-seq=`basename $0`
-echo "QA output created by $seq"
-
-status=1   # failure is the default!
-
-_cleanup()
-{
-rm -f "$SOCK_DIR/nbd"
-}
-trap "_cleanup; exit \$status" 0 1 2 3 15
-
-# get standard environment, filters and checks
-. ./common.rc
-. ./common.filter
-
-_supported_fmt generic
-_supported_proto generic
-
-do_run_qemu()
-{
-echo Testing: "$@"
-$QEMU -nographic -qmp stdio -serial none "$@"
-echo
-}
-
-# Remove QMP events from (pretty-printed) output. Doesn't handle
-# nested dicts correctly, but we don't get any of those in this test.
-_filter_qmp_events()
-{
-tr '\n' '\t' | sed -e \
-   
's/{\s*"timestamp":\s*{[^}]*},\s*"event":[^,}]*\(,\s*"data":\s*{[^}]*}\)\?\s*}\s*//g'
 \
-   | tr '\t' '\n'
-}
-
-run_qemu()
-{
-do_run_qemu "$@" 2>&1 | _filter_qmp | _filter_qmp_events
-}
-
-case "$QEMU_DEFAULT_MACHINE" in
-  s390-ccw-virtio)
-  virtio_scsi=virtio-scsi-ccw
-  ;;
-  *)
-  virtio_scsi=virtio-scsi-pci
-  ;;
-esac
-
-echo
-echo === Unplug a SCSI disk and then plug it again ===
-echo
-
-run_qemu <

[PULL 05/15] hw/block/nvme: fix uint16_t use of uint32_t sgls member

[Max Reitz]

From: Klaus Jensen 

nvme_map_sgl_data erroneously uses the sgls member of NvmeIdNs as a
uint16_t.

Reported-by: Coverity (CID 1436129)
Fixes: cba0a8a344fe ("hw/block/nvme: add support for scatter gather lists")
Signed-off-by: Klaus Jensen 
Message-Id: <20201104102248.32168-3-...@irrelevant.dk>
Reviewed-by: Philippe Mathieu-Daudé 
Signed-off-by: Max Reitz 
---
 hw/block/nvme.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 080d782f1c..2bdc50eb6f 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -452,7 +452,7 @@ static uint16_t nvme_map_sgl_data(NvmeCtrl *n, QEMUSGList 
*qsg,
  * segments and/or descriptors. The controller might accept
  * ignoring the rest of the SGL.
  */
-uint16_t sgls = le16_to_cpu(n->id_ctrl.sgls);
+uint32_t sgls = le32_to_cpu(n->id_ctrl.sgls);
 if (sgls & NVME_CTRL_SGLS_EXCESS_LENGTH) {
 break;
 }
-- 
2.28.0

[PULL 03/15] qcow2: Document and enforce the QCowL2Meta invariants

[Max Reitz]

From: Alberto Garcia 

The QCowL2Meta structure is used to store information about a part of
a write request that touches clusters that need changes in their L2
entries. This happens with newly-allocated clusters or subclusters.

This structure has changed a bit since it was first created and its
current documentation is not quite up-to-date.

A write request can span a region consisting of a combination of
clusters of different types, and qcow2_alloc_host_offset() can
repeatedly call handle_copied() and handle_alloc() to add more
clusters to the mix as long as they all are contiguous on the image
file.

Because of this a write request has a list of QCowL2Meta structures,
one for each part of the request that needs changes in the L2
metadata.

Each one of them spans nb_clusters and has two copy-on-write regions
located immediately before and after the middle region touched by that
part of the write request. Even when those regions themselves are
empty their offsets must be correct because they are used to know the
location of the middle region.

This was not always the case but it is not a problem anymore
because the only two places where QCowL2Meta structures are created
(calculate_l2_meta() and qcow2_co_truncate()) ensure that the
copy-on-write regions are correctly defined, and so do assertions like
the ones in perform_cow().

The conditional initialization of the 'written_to' variable is
therefore unnecessary and is removed by this patch.

Signed-off-by: Alberto Garcia 
Reviewed-by: Eric Blake 
Reviewed-by: Vladimir Sementsov-Ogievskiy 
Message-Id: <20201007161323.4667-1-be...@igalia.com>
Signed-off-by: Max Reitz 
---
 block/qcow2.h | 19 +++
 block/qcow2-cluster.c |  5 +++--
 block/qcow2.c | 19 +++
 3 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/block/qcow2.h b/block/qcow2.h
index 125ea9679b..2e0272a7b8 100644
--- a/block/qcow2.h
+++ b/block/qcow2.h
@@ -435,17 +435,18 @@ typedef struct Qcow2COWRegion {
 
 /**
  * Describes an in-flight (part of a) write request that writes to clusters
- * that are not referenced in their L2 table yet.
+ * that need to have their L2 table entries updated (because they are
+ * newly allocated or need changes in their L2 bitmaps)
  */
 typedef struct QCowL2Meta
 {
-/** Guest offset of the first newly allocated cluster */
+/** Guest offset of the first updated cluster */
 uint64_t offset;
 
-/** Host offset of the first newly allocated cluster */
+/** Host offset of the first updated cluster */
 uint64_t alloc_offset;
 
-/** Number of newly allocated clusters */
+/** Number of updated clusters */
 int nb_clusters;
 
 /** Do not free the old clusters */
@@ -458,14 +459,16 @@ typedef struct QCowL2Meta
 CoQueue dependent_requests;
 
 /**
- * The COW Region between the start of the first allocated cluster and the
- * area the guest actually writes to.
+ * The COW Region immediately before the area the guest actually
+ * writes to. This (part of the) write request starts at
+ * cow_start.offset + cow_start.nb_bytes.
  */
 Qcow2COWRegion cow_start;
 
 /**
- * The COW Region between the area the guest actually writes to and the
- * end of the last allocated cluster.
+ * The COW Region immediately after the area the guest actually
+ * writes to. This (part of the) write request ends at cow_end.offset
+ * (which must always be set even when cow_end.nb_bytes is 0).
  */
 Qcow2COWRegion cow_end;
 
diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index aa87d3e99b..485b4cb92e 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1049,6 +1049,8 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, 
QCowL2Meta *m)
 qcow2_cache_entry_mark_dirty(s->l2_table_cache, l2_slice);
 
 assert(l2_index + m->nb_clusters <= s->l2_slice_size);
+assert(m->cow_end.offset + m->cow_end.nb_bytes <=
+   m->nb_clusters << s->cluster_bits);
 for (i = 0; i < m->nb_clusters; i++) {
 uint64_t offset = cluster_offset + ((uint64_t)i << s->cluster_bits);
 /* if two concurrent writes happen to the same unallocated cluster
@@ -1070,8 +1072,7 @@ int qcow2_alloc_cluster_link_l2(BlockDriverState *bs, 
QCowL2Meta *m)
 if (has_subclusters(s) && !m->prealloc) {
 uint64_t l2_bitmap = get_l2_bitmap(s, l2_slice, l2_index + i);
 unsigned written_from = m->cow_start.offset;
-unsigned written_to = m->cow_end.offset + m->cow_end.nb_bytes ?:
-m->nb_clusters << s->cluster_bits;
+unsigned written_to = m->cow_end.offset + m->cow_end.nb_bytes;
 int first_sc, last_sc;
 /* Narrow written_from and written_to down to the current cluster 
*/
 written_from = MAX(written_from, i << s->cluster_bits);
diff --git a/block/qcow2.c b/block/qcow2.c
index 4274806a2a..1b0733238b 100644
--- a/block/qcow2.c
+++

[PULL 11/15] block: Fix integer promotion error in bdrv_getlength()

[Max Reitz]

From: Eric Blake 

Back in 2015, we attempted to fix error reporting for images that
claimed to have more than INT64_MAX/512 sectors, but due to the type
promotions caused by BDRV_SECTOR_SIZE being unsigned, this
inadvertently forces all negative ret values to be slammed into -EFBIG
rather than the original error.  While we're at it, we can avoid the
confusing ?: by spelling the logic more directly.

Fixes: 4a9c9ea0d3
Reported-by: Guoyi Tu 
Signed-off-by: Eric Blake 
Message-Id: <20201105155122.60943-1-ebl...@redhat.com>
Reviewed-by: Alberto Garcia 
Signed-off-by: Max Reitz 
---
 block.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/block.c b/block.c
index 56bacc9e9f..2fd932154e 100644
--- a/block.c
+++ b/block.c
@@ -5091,8 +5091,13 @@ int64_t bdrv_getlength(BlockDriverState *bs)
 {
 int64_t ret = bdrv_nb_sectors(bs);
 
-ret = ret > INT64_MAX / BDRV_SECTOR_SIZE ? -EFBIG : ret;
-return ret < 0 ? ret : ret * BDRV_SECTOR_SIZE;
+if (ret < 0) {
+return ret;
+}
+if (ret > INT64_MAX / BDRV_SECTOR_SIZE) {
+return -EFBIG;
+}
+return ret * BDRV_SECTOR_SIZE;
 }
 
 /* return 0 as number of sectors if no device present or error */
-- 
2.28.0

[PULL 00/15] Block patches for 5.2.0-rc1

[Max Reitz]

The following changes since commit 2a190a7256a3e0563b29ffd67e0164097b4a6dac:

  Merge remote-tracking branch 
'remotes/philmd-gitlab/tags/renesas-fixes-20201109' into staging (2020-11-09 
11:20:25 +)

are available in the Git repository at:

  https://github.com/XanClic/qemu.git tags/pull-block-2020-11-09

for you to fetch changes up to 8f88cb77b06be0c2e45ed31c9fd6817d2ce3836a:

  block: make bdrv_drop_intermediate() less wrong (2020-11-09 15:44:21 +0100)


Block patches for 5.2.0-rc1:
- Some nvme fixes (addressing problems spotted by Coverity)
- Fix nfs compiling on mingw (and enable it in Cirrus)
- Fix an error path in bdrv_co_invalidate_cache() (permission update
  was initiated, but not aborted)
- Fix (on-error) roll back in bdrv_drop_intermediate(): Instead of
  inlining bdrv_replace_node() (wrongly), call that function
- Fix for iotest 240
- Fix error handling in bdrv_getlength()
- Be more explicit about how QCowL2Meta objects are handled
- Cleanups


Alberto Garcia (1):
  qcow2: Document and enforce the QCowL2Meta invariants

AlexChen (1):
  block: Remove unused include

Eric Blake (1):
  block: Fix integer promotion error in bdrv_getlength()

Greg Kurz (1):
  block: Move bdrv_drain_all_end_quiesce() to block_int.h

Klaus Jensen (3):
  hw/block/nvme: fix null ns in register namespace
  hw/block/nvme: fix uint16_t use of uint32_t sgls member
  hw/block/nvme: fix free of array-typed value

Maxim Levitsky (2):
  iotests: add filter_qmp_virtio_scsi function
  iotests: rewrite iotest 240 in python

Vladimir Sementsov-Ogievskiy (3):
  block: add forgotten bdrv_abort_perm_update() to
bdrv_co_invalidate_cache()
  block: add bdrv_replace_node_common()
  block: make bdrv_drop_intermediate() less wrong

Yonggang Luo (2):
  block: Fixes nfs compiling error on msys2/mingw
  block: enable libnfs on msys2/mingw in cirrus.yml

shiliyang (1):
  block: Fix some code style problems, "foo* bar" should be "foo *bar"

 block/qcow2.h |  25 ++--
 include/block/block.h |   6 -
 include/block/block_int.h |   9 ++
 block.c   |  89 --
 block/blkdebug.c  |   2 +-
 block/dmg-lzfse.c |   1 -
 block/dmg.c   |   2 +-
 block/nfs.c   |  13 +-
 block/qcow2-cluster.c |   5 +-
 block/qcow2.c |  23 +++-
 block/vpc.c   |  10 +-
 hw/block/nvme.c   |   6 +-
 .cirrus.yml   |   1 +
 tests/qemu-iotests/240| 219 ++
 tests/qemu-iotests/240.out|  76 ++--
 tests/qemu-iotests/iotests.py |  10 ++
 16 files changed, 259 insertions(+), 238 deletions(-)

-- 
2.28.0

[PULL 02/15] block: Move bdrv_drain_all_end_quiesce() to block_int.h

[Max Reitz]

From: Greg Kurz 

This function is really an internal helper for bdrv_close(). Update its
doc comment to make this clear and make the function private.

Signed-off-by: Greg Kurz 
Message-Id: <160387245480.131299.13430357162209598411.stgit@bahia>
Reviewed-by: Stefan Hajnoczi 
Signed-off-by: Max Reitz 
---
 include/block/block.h | 6 --
 include/block/block_int.h | 9 +
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/include/block/block.h b/include/block/block.h
index 4bfe3b546b..c9d7c58765 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -781,12 +781,6 @@ void bdrv_drained_end(BlockDriverState *bs);
  */
 void bdrv_drained_end_no_poll(BlockDriverState *bs, int *drained_end_counter);
 
-/**
- * End all quiescent sections started by bdrv_drain_all_begin(). This is
- * only needed when deleting a BDS before bdrv_drain_all_end() is called.
- */
-void bdrv_drain_all_end_quiesce(BlockDriverState *bs);
-
 /**
  * End a quiescent section started by bdrv_subtree_drained_begin().
  */
diff --git a/include/block/block_int.h b/include/block/block_int.h
index 38cad9d15c..95d9333be1 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -1407,4 +1407,13 @@ static inline BlockDriverState 
*bdrv_primary_bs(BlockDriverState *bs)
 return child_bs(bdrv_primary_child(bs));
 }
 
+/**
+ * End all quiescent sections started by bdrv_drain_all_begin(). This is
+ * needed when deleting a BDS before bdrv_drain_all_end() is called.
+ *
+ * NOTE: this is an internal helper for bdrv_close() *only*. No one else
+ * should call it.
+ */
+void bdrv_drain_all_end_quiesce(BlockDriverState *bs);
+
 #endif /* BLOCK_INT_H */
-- 
2.28.0

[PULL 07/15] iotests: add filter_qmp_virtio_scsi function

[Max Reitz]

From: Maxim Levitsky 

filter_qmp_virtio_scsi can be used to filter virtio-scsi-pci/ccw differences.
Note that this patch was only tested on x86.

Suggested-by: Paolo Bonzini 
Signed-off-by: Maxim Levitsky 
Tested-by: Christian Borntraeger 
Reviewed-by: Paolo Bonzini 
Message-Id: <20201104185025.434703-2-mlevi...@redhat.com>
Signed-off-by: Max Reitz 
---
 tests/qemu-iotests/iotests.py | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index 814804a4c6..bcd4fe5b6f 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -392,6 +392,16 @@ def filter_qmp_testfiles(qmsg):
 return value
 return filter_qmp(qmsg, _filter)
 
+def filter_virtio_scsi(output: str) -> str:
+return re.sub(r'(virtio-scsi)-(ccw|pci)', r'\1', output)
+
+def filter_qmp_virtio_scsi(qmsg):
+def _filter(_key, value):
+if is_str(value):
+return filter_virtio_scsi(value)
+return value
+return filter_qmp(qmsg, _filter)
+
 def filter_generated_node_ids(msg):
 return re.sub("#block[0-9]+", "NODE_NAME", msg)
 
-- 
2.28.0

Re: [PATCH 0/2] Increase amount of data for monitor to read

[Andrey Shinkevich]

On 09.11.2020 13:04, Vladimir Sementsov-Ogievskiy wrote:

09.11.2020 11:50, Vladimir Sementsov-Ogievskiy wrote:

06.11.2020 15:42, Andrey Shinkevich wrote:

The subject was discussed here:
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg00206.html

This series is a solution for the issue with QMP monitor buffered input.
A little parser is introduced to throttle JSON commands read from the
buffer so that QMP requests do not overwhelm the monitor input queue.
A side effect raised in the test #247 was managed in the first patch.
It may be considered as a workaround. Any sane fix suggested will be
appreciated.

Note:
This series goes after the Vladimir's one:
'[PATCH v3 00/25] backup performance: block_status + async"'
To make the test #129 passed, the following patch should be applied 
first:

'[PATCH v3 01/25] iotests: 129 don't check backup "busy"'.



[...]




Positive thing: the patches do increase performance:

for me, the following command:

(echo "{ 'execute': 'qmp_capabilities' }"; for i in {1..1}; do echo 
"{ 'execute': 'query-block-jobs' }"; done; echo "{ 'execute': 'quit' }" 
) | time ./qemu-system-x86_64 -qmp stdio > /dev/null


shows 2.4s on master and 0.6s after patches




Thank you for testing it. I'd like to include the result to the patch 
description with "Tested-by: ..."


Andrey

Re: [PATCH v2 0/7] block: permission update fix & refactor

[Vladimir Sementsov-Ogievskiy]

09.11.2020 17:41, Max Reitz wrote:

On 06.11.20 13:42, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

These series supersedes "Fix nested permission update" and includes one
more fix (patch 01) and more improvements.

I think patch 01 is good to have in 5.2, 02 is probably OK for 5.2 and
the others are OK for next release. Still all may be taken to 5.2, up to
block maintainers.

Actually the series is a first part of my work announced in
https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg08386.html
to bring correct order to permission update (topological sort),
update permission only on updated graph (and rollback graph changes),
get rid of .active fields in block jobs.

Supersedes: <20201031123502.4558-1-vsement...@virtuozzo.com>

v2: all new except for 03:, which uses suggestions by Albert and more
strict version of bdrv_replace_node.


Thanks, I took 1 through 3 to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

and 4 through 7 to my block-next branch (squashing in the fix for 4, and fixing 
up the resulting conflict in 7):

https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next

Max



Thank you!

--
Best regards,
Vladimir

Re: [PATCH for-5.2] block/export/vhost-user-blk-server.c: Avoid potential integer overflow

[Max Reitz]

[Cc-ing Stefan]

On 09.11.20 16:05, Peter Maydell wrote:

In vu_blk_discard_write_zeroes(), we read a 32-bit sector count from
the descriptor and convert it to a 64-bit byte count. Coverity warns
that the left shift is done with 32-bit arithmetic so it might
overflow before the conversion to 64-bit happens. Add a cast to
avoid this.


This will silence Coverity, but both functions to which range[1] is then 
passed (blk_co_pdiscard() and blk_co_pwrite_zeroes()) only accept ints 
there, so this would only move the overflow to the function call.


Shouldn’t we verify that the number of sectors is in range and return an 
error if it isn’t?  (The same probably goes for the starting sector, 
then, too.)


Max


Fixes: Coverity CID 1435956
Signed-off-by: Peter Maydell 
---
Tested with 'make check' and 'make check-acceptance' only.
---
  block/export/vhost-user-blk-server.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/export/vhost-user-blk-server.c 
b/block/export/vhost-user-blk-server.c
index 62672d1cb95..e5749451e65 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -70,7 +70,7 @@ vu_blk_discard_write_zeroes(BlockBackend *blk, struct iovec 
*iov,
  }
  
  uint64_t range[2] = { le64_to_cpu(desc.sector) << 9,

-  le32_to_cpu(desc.num_sectors) << 9 };
+  (uint64_t)le32_to_cpu(desc.num_sectors) << 9 };
  if (type == VIRTIO_BLK_T_DISCARD) {
  if (blk_co_pdiscard(blk, range[0], range[1]) == 0) {
  return 0;

[PATCH for-5.2] block/export/vhost-user-blk-server.c: Avoid potential integer overflow

[Peter Maydell]

In vu_blk_discard_write_zeroes(), we read a 32-bit sector count from
the descriptor and convert it to a 64-bit byte count. Coverity warns
that the left shift is done with 32-bit arithmetic so it might
overflow before the conversion to 64-bit happens. Add a cast to
avoid this.

Fixes: Coverity CID 1435956
Signed-off-by: Peter Maydell 
---
Tested with 'make check' and 'make check-acceptance' only.
---
 block/export/vhost-user-blk-server.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/block/export/vhost-user-blk-server.c 
b/block/export/vhost-user-blk-server.c
index 62672d1cb95..e5749451e65 100644
--- a/block/export/vhost-user-blk-server.c
+++ b/block/export/vhost-user-blk-server.c
@@ -70,7 +70,7 @@ vu_blk_discard_write_zeroes(BlockBackend *blk, struct iovec 
*iov,
 }
 
 uint64_t range[2] = { le64_to_cpu(desc.sector) << 9,
-  le32_to_cpu(desc.num_sectors) << 9 };
+  (uint64_t)le32_to_cpu(desc.num_sectors) << 9 };
 if (type == VIRTIO_BLK_T_DISCARD) {
 if (blk_co_pdiscard(blk, range[0], range[1]) == 0) {
 return 0;
-- 
2.20.1

Re: [PATCH v2 0/7] block: permission update fix & refactor

[Max Reitz]

On 06.11.20 13:42, Vladimir Sementsov-Ogievskiy wrote:

Hi all!

These series supersedes "Fix nested permission update" and includes one
more fix (patch 01) and more improvements.

I think patch 01 is good to have in 5.2, 02 is probably OK for 5.2 and
the others are OK for next release. Still all may be taken to 5.2, up to
block maintainers.

Actually the series is a first part of my work announced in
https://lists.gnu.org/archive/html/qemu-devel/2020-09/msg08386.html
to bring correct order to permission update (topological sort),
update permission only on updated graph (and rollback graph changes),
get rid of .active fields in block jobs.

Supersedes: <20201031123502.4558-1-vsement...@virtuozzo.com>

v2: all new except for 03:, which uses suggestions by Albert and more
strict version of bdrv_replace_node.


Thanks, I took 1 through 3 to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

and 4 through 7 to my block-next branch (squashing in the fix for 4, and 
fixing up the resulting conflict in 7):


https://git.xanclic.moe/XanClic/qemu/commits/branch/block-next

Max

Re: [PATCH v3 1/3] hw/block/m25p80: Fix Numonyx NVCFG DIO and QIO bit polarity

[Philippe Mathieu-Daudé]

On 11/6/20 2:32 AM, Joe Komlodi wrote:
> QIO and DIO modes should be enabled when the bits in NVCFG are set to 0.
> This matches the behavior of the other bits in the NVCFG register.

Is this material for the 5.2 release?

> 
> Signed-off-by: Joe Komlodi 
> ---
>  hw/block/m25p80.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 483925f..4255a6a 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -783,10 +783,10 @@ static void reset_memory(Flash *s)
>  s->enh_volatile_cfg |= EVCFG_OUT_DRIVER_STRENGTH_DEF;
>  s->enh_volatile_cfg |= EVCFG_VPP_ACCELERATOR;
>  s->enh_volatile_cfg |= EVCFG_RESET_HOLD_ENABLED;
> -if (s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK) {
> +if (!(s->nonvolatile_cfg & NVCFG_DUAL_IO_MASK)) {
>  s->enh_volatile_cfg |= EVCFG_DUAL_IO_ENABLED;
>  }
> -if (s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK) {
> +if (!(s->nonvolatile_cfg & NVCFG_QUAD_IO_MASK)) {
>  s->enh_volatile_cfg |= EVCFG_QUAD_IO_ENABLED;
>  }
>  if (!(s->nonvolatile_cfg & NVCFG_4BYTE_ADDR_MASK)) {
> 

Re: [PATCH v2 7/7] block: drop tighten_restrictions

[Max Reitz]

On 06.11.20 13:42, Vladimir Sementsov-Ogievskiy wrote:

The only users of this thing are:
  1. bdrv_child_try_set_perm, to ignore failures on loosen restrictions
  2. assertion in bdrv_replace_child
  3. assertion in bdrv_inactivate_recurse

Assertions are not enough reason for overcomplication the permission
update system. So, look at bdrv_child_try_set_perm.

We are interested in tighten_restrictions only on failure. But on
failure this field is not reliable: we may fail in the middle of
permission update, some nodes are not touched and we don't know should
their permissions be tighten or not. So, we rely on the fact that if we
loose restrictions on some node (or BdrvChild), we'll not tighten
restriction in the whole subtree as part of this update (assertions 2
and 3 rely on this fact as well). And, if we rely on this fact anyway,
we can just check it on top, and don't pass additional pointer through
the whole recursive infrastructure.

Note also, that further patches will fix real bugs in permission update
system, so now is good time to simplify it, as a help for further
refactorings.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c | 88 +++--
  1 file changed, 17 insertions(+), 71 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH v2 4/7] block: add bdrv_refresh_perms() helper

[Vladimir Sementsov-Ogievskiy]

09.11.2020 10:04, Vladimir Sementsov-Ogievskiy wrote:

06.11.2020 18:14, Alberto Garcia wrote:

On Fri 06 Nov 2020 01:42:38 PM CET, Vladimir Sementsov-Ogievskiy wrote:

Make separate function for common pattern.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c | 60 -
  1 file changed, 30 insertions(+), 30 deletions(-)

diff --git a/block.c b/block.c
index 77a3f8f1e2..fc7633307f 100644
--- a/block.c
+++ b/block.c
@@ -2321,6 +2321,23 @@ static void bdrv_child_abort_perm_update(BdrvChild *c)
  bdrv_abort_perm_update(c->bs);
  }
+static int bdrv_refresh_perms(BlockDriverState *bs, bool *tighten_restrictions,
+  Error **errp)
+{
+    int ret;
+    uint64_t perm, shared_perm;
+
+    bdrv_get_cumulative_perm(bs, &perm, &shared_perm);
+    ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL,
errp);


Aren't you supposed to pass tighten_restrictions here ?



Oops, yes I should



So, squash-in:


diff --git a/block.c b/block.c
index fc7633307f..a96dc07364 100644
--- a/block.c
+++ b/block.c
@@ -2328,7 +2328,8 @@ static int bdrv_refresh_perms(BlockDriverState *bs, bool 
*tighten_restrictions,
 uint64_t perm, shared_perm;
 
 bdrv_get_cumulative_perm(bs, &perm, &shared_perm);

-ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL, NULL, errp);
+ret = bdrv_check_perm(bs, NULL, perm, shared_perm, NULL,
+  tighten_restrictions, errp);
 if (ret < 0) {
 bdrv_abort_perm_update(bs);
 return ret;



(produces simple conflict when applying "block: drop tighten_restrictions")

--
Best regards,
Vladimir

Re: [PATCH v2 6/7] block: bdrv_child_set_perm() drop redundant parameters.

[Max Reitz]

On 06.11.20 13:42, Vladimir Sementsov-Ogievskiy wrote:

We must set the permission used for _check_.  Assert that we have
backup and drop extra arguments.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c | 15 ---
  1 file changed, 4 insertions(+), 11 deletions(-)


Reviewed-by: Max Reitz 

Re: [PATCH v2 5/7] block: bdrv_set_perm() drop redundant parameters.

[Vladimir Sementsov-Ogievskiy]

09.11.2020 15:20, Max Reitz wrote:

On 06.11.20 13:42, Vladimir Sementsov-Ogievskiy wrote:

We should never set permissions other than cumulative permissions of
parents. During bdrv_reopen_multiple() we _check_ for synthetic
permissions but when we do _set_ the graph is already updated.
Add an assertion to bdrv_reopen_multiple(), other cases are more
obvious.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c | 29 +++--
  1 file changed, 15 insertions(+), 14 deletions(-)


(Perhaps bdrv_commit_perm() might be a better name then, but I’m afraid such a 
name change might be quite invasive (because AFAIR *_set_perm is used quite 
often).)

Reviewed-by: Max Reitz 



Thanks for reviewing!

Actually, I plan to split and organize in a similar transactional way 
graph-update operations:

 - aio context change
 - child replacement
 - permission update

So, we'll have a chance to discuss final names later. I think about prepare/commit/abort too, as it is more 
common than check/set/abort. Also, check now actually do set permissions in BdrvChild, so it isn't "just 
check" (and the fact that we should do "abort" after "check" was always a bit odd).

--
Best regards,
Vladimir

Re: [PATCH v3 11/25] qapi: backup: add max-chunk and max-workers to x-perf struct

[Vladimir Sementsov-Ogievskiy]

26.10.2020 20:18, Vladimir Sementsov-Ogievskiy wrote:

Add new parameters to configure future backup features. The patch
doesn't introduce aio backup requests (so we actually have only one
worker) neither requests larger than one cluster. Still, formally we
satisfy these maximums anyway, so add the parameters now, to facilitate
further patch which will really change backup job behavior.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  qapi/block-core.json | 11 ++-
  block/backup.c   | 28 +++-
  block/replication.c  |  2 +-
  blockdev.c   |  8 +++-
  4 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 71e6faa15c..5a21c24c1d 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1378,10 +1378,19 @@
  #
  # @use-copy-range: Use copy offloading. Default true.
  #
+# @max-workers: Maximum number of parallel requests for the sustained 
background
+#   copying process. Doesn't influence copy-before-write 
operations.
+#   Default 64.
+#
+# @max-chunk: Maximum request length for the sustained background copying
+# process. Doesn't influence copy-before-write operations.
+# 0 means unlimited. Default 0.
+#
  # Since: 5.2
  ##
  { 'struct': 'BackupPerf',
-  'data': { '*use-copy-range': 'bool' }}
+  'data': { '*use-copy-range': 'bool',
+'*max-workers': 'int', '*max-chunk': 'int64' } }
  
  ##

  # @BackupCommon:


squash-in:

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 5591189b75..276ad5e6a8 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -1406,7 +1406,9 @@
 #
 # @max-chunk: Maximum request length for the sustained background copying
 # process. Doesn't influence copy-before-write operations.
-# 0 means unlimited. Default 0.
+# 0 means unlimited. If max-chunk is non-zero then it should not be
+# less than job cluster size which is calculated as maximum of
+# target image cluster size and 64k. Default 0.
 #
 # Since: 5.2
 ##


--
Best regards,
Vladimir

Re: [PATCH v2 5/7] block: bdrv_set_perm() drop redundant parameters.

[Max Reitz]

On 06.11.20 13:42, Vladimir Sementsov-Ogievskiy wrote:

We should never set permissions other than cumulative permissions of
parents. During bdrv_reopen_multiple() we _check_ for synthetic
permissions but when we do _set_ the graph is already updated.
Add an assertion to bdrv_reopen_multiple(), other cases are more
obvious.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
  block.c | 29 +++--
  1 file changed, 15 insertions(+), 14 deletions(-)


(Perhaps bdrv_commit_perm() might be a better name then, but I’m afraid 
such a name change might be quite invasive (because AFAIR *_set_perm is 
used quite often).)


Reviewed-by: Max Reitz 

Re: [PATCH v2 13/20] iotests: 129: prepare for backup over block-copy

[Vladimir Sementsov-Ogievskiy]

04.11.2020 20:30, Max Reitz wrote:

On 22.10.20 23:10, Vladimir Sementsov-Ogievskiy wrote:

23.07.2020 11:03, Max Reitz wrote:

On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:

After introducing parallel async copy requests instead of plain
cluster-by-cluster copying loop, backup job may finish earlier than
final assertion in do_test_stop. Let's require slow backup explicitly
by specifying speed parameter.


Isn’t the problem really that block_set_io_throttle does absolutely
nothing?  (Which is a long-standing problem with 129.  I personally just
never run it, honestly.)


Hmm.. is it better to drop test_drive_backup() from here ?


I think the best would be to revisit this:

https://lists.nongnu.org/archive/html/qemu-block/2019-06/msg00499.html

Max



I've checked, no, that doesn't help. I suppose that throttling has same defect 
as
original block-job speed: it calculates throttling after request, and if we 
issue
simultaneously many requests they all will be handled (and after it we'll have 
long
throttling pause). New solution for backup in these series works better with 
parallel
requests (see patch [PATCH v2 07/20] block/block-copy: add ratelimit to 
block-copy).

So, I'd keep this patch for now.

--
Best regards,
Vladimir

Re: [PATCH v2 11/20] qapi: backup: add x-max-chunk and x-max-workers parameters

[Vladimir Sementsov-Ogievskiy]

04.11.2020 20:19, Max Reitz wrote:

On 22.10.20 22:35, Vladimir Sementsov-Ogievskiy wrote:

22.07.2020 15:22, Max Reitz wrote:

On 01.06.20 20:11, Vladimir Sementsov-Ogievskiy wrote:

Add new parameters to configure future backup features. The patch
doesn't introduce aio backup requests (so we actually have only one
worker) neither requests larger than one cluster. Still, formally we
satisfy these maximums anyway, so add the parameters now, to facilitate
further patch which will really change backup job behavior.

Options are added with x- prefixes, as the only use for them are some
very conservative iotests which will be updated soon.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
   qapi/block-core.json  |  9 -
   include/block/block_int.h |  7 +++
   block/backup.c    | 21 +
   block/replication.c   |  2 +-
   blockdev.c    |  5 +
   5 files changed, 42 insertions(+), 2 deletions(-)



[..]


@@ -422,6 +436,11 @@ BlockJob *backup_job_create(const char *job_id,
BlockDriverState *bs,
   if (cluster_size < 0) {
   goto error;
   }
+    if (max_chunk && max_chunk < cluster_size) {
+    error_setg(errp, "Required max-chunk (%" PRIi64") is less
than backup "


(missing a space after PRIi64)


+   "cluster size (%" PRIi64 ")", max_chunk,
cluster_size);


Should this be noted in the QAPI documentation?


Hmm.. It makes sense, but I don't know what to write: should be >= job
cluster_size? But then I'll have to describe the logic of
backup_calculate_cluster_size()...


Isn’t the logic basically just “cluster size of the target image file
(min 64k)”?  The other cases just cover error cases, and they always
return 64k, which would effectively be documented by stating that 64k is
the minimum.

But in the common cases (qcow2 or raw), we’ll never get an error anyway.

I think it’d be good to describe the cluster size somewhere, because
otherwise I find it a bit malicious to throw an error at the user that
they could not have anticipated because they have no idea what valid
values for the parameter are (until they try).


OK




  (And perhaps the fact
that without copy offloading, we’ll never copy anything bigger than one
cluster at a time anyway?)


This is a parameter for background copying. Look at
block_copy_task_create(), if call_state has own max_chunk, it's used
instead of common copy_size (derived from cluster_size). But at a moment
of this patch background process through async block-copy is not  yet
implemented, and the parameter doesn't work, which is described in
commit message.


Ah, OK.  Right.

Max




--
Best regards,
Vladimir

Re: [PATCH 6/7] qom: Add FIELD_PTR, a type-safe wrapper for object_field_prop_ptr()

[Cornelia Huck]

On Wed,  4 Nov 2020 12:25:11 -0500
Eduardo Habkost  wrote:

> Introduce a FIELD_PTR macro that will ensure the size of the area
> we are accessing has the correct size, and will return a pointer
> of the correct type.
> 
> Signed-off-by: Eduardo Habkost 
> ---
> Cc: Stefan Berger 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: Paolo Bonzini 
> Cc: "Daniel P. Berrangé" 
> Cc: Eduardo Habkost 
> Cc: Cornelia Huck 
> Cc: Thomas Huth 
> Cc: Halil Pasic 
> Cc: Christian Borntraeger 
> Cc: Richard Henderson 
> Cc: David Hildenbrand 
> Cc: Matthew Rosato 
> Cc: Alex Williamson 
> Cc: qemu-de...@nongnu.org
> Cc: xen-de...@lists.xenproject.org
> Cc: qemu-block@nongnu.org
> Cc: qemu-s3...@nongnu.org
> ---
>  include/qom/field-property.h | 21 ++-
>  backends/tpm/tpm_util.c  |  6 ++--
>  hw/block/xen-block.c |  4 +--
>  hw/core/qdev-properties-system.c | 50 +-
>  hw/s390x/css.c   |  4 +--
>  hw/s390x/s390-pci-bus.c  |  4 +--
>  hw/vfio/pci-quirks.c |  4 +--
>  qom/field-property.c |  3 +-
>  qom/property-types.c | 60 +---
>  9 files changed, 89 insertions(+), 67 deletions(-)

Acked-by: Cornelia Huck 

Re: [PATCH 4/7] qom: Replace void* parameter with Property* on field getters/setters

[Cornelia Huck]

On Wed,  4 Nov 2020 12:25:09 -0500
Eduardo Habkost  wrote:

> All field property getters and setters must interpret the fourth
> argument as Property*.  Change the function signature of field
> property getters and setters to indicate that.
> 
> Signed-off-by: Eduardo Habkost 
> ---
> Cc: Stefan Berger 
> Cc: Stefano Stabellini 
> Cc: Anthony Perard 
> Cc: Paul Durrant 
> Cc: Kevin Wolf 
> Cc: Max Reitz 
> Cc: Paolo Bonzini 
> Cc: "Daniel P. Berrangé" 
> Cc: Eduardo Habkost 
> Cc: Richard Henderson 
> Cc: David Hildenbrand 
> Cc: Cornelia Huck 
> Cc: Thomas Huth 
> Cc: Halil Pasic 
> Cc: Christian Borntraeger 
> Cc: Matthew Rosato 
> Cc: Alex Williamson 
> Cc: Mark Cave-Ayland 
> Cc: Artyom Tarasenko 
> Cc: qemu-de...@nongnu.org
> Cc: xen-de...@lists.xenproject.org
> Cc: qemu-block@nongnu.org
> Cc: qemu-s3...@nongnu.org
> ---
>  include/qom/field-property-internal.h |   8 +-
>  include/qom/field-property.h  |  26 ---
>  backends/tpm/tpm_util.c   |  11 ++-
>  hw/block/xen-block.c  |   6 +-
>  hw/core/qdev-properties-system.c  |  86 +-
>  hw/s390x/css.c|   6 +-
>  hw/s390x/s390-pci-bus.c   |   6 +-
>  hw/vfio/pci-quirks.c  |  10 +--
>  qom/property-types.c  | 102 +-
>  target/sparc/cpu.c|   4 +-
>  10 files changed, 105 insertions(+), 160 deletions(-)

Acked-by: Cornelia Huck 

Re: Qemu first time contribution

[Stefan Hajnoczi]

On Sun, Nov 08, 2020 at 12:21:33PM +, Harshavardhan Unnibhavi wrote:
> Thank you for the reply! Yes, I understand that gsoc is over for 2020,
> and projects for 2021 will come up next year. I was thinking of
> contributing outside of gsoc(for which I won't be eligible anyways for
> next year). Anyway, I will work on some of the bite sized tasks, and
> get back to you for some other concrete project ideas that require
> somebody to work on, in qemu.

Hi Harsha,
Here is an idea you could explore:

The Linux AIO API was extended to support fsync(2)/fdatasync(2) in the
following commit from 2018:

  commit a3c0d439e4d92411c2b4b21a526a4de720d0806b
  Author: Christoph Hellwig 
  Date:   Tue Mar 27 19:18:57 2018 +0200

  aio: implement IOCB_CMD_FSYNC and IOCB_CMD_FDSYNC

QEMU's Linux AIO code does not take advantage of this feature yet.
Instead it invokes the traditional fdatasync(2) system call from a
thread pool because it assumes the Linux AIO API doesn't support the
operation. The function where this happens is
block/file-posix.c:raw_co_flush_to_disk().

The goal is to implement IO_CMD_FDSYNC support in block/linux-aio.c
using io_prep_fdsync() and update
block/file-posix.c:raw_co_flush_to_disk() to use this when the feature
is available. See  for the Linux AIO library API.

Keep in mind that old host kernels may not support IO_CMD_FDSYNC. In
that case QEMU should continue to use the thread pool.

Taking advantage of the Linux AIO API means QEMU will spawn fewer
worker threads and disk flush performance may improve. You can benchmark
performance using the fio(1) tool. Configure it with ioengine=pvsync2
rw=randwrite direct=1 fdatasync=1 bs=4k to measure the peformance of 4
KB writes followed by fdatasync. For more information about disk I/O
benchmarking, including example fio jobs, see:
https://blog.vmsplice.net/2017/11/common-disk-benchmarking-mistakes.html

Stefan


signature.asc
Description: PGP signature

Re: [PATCH 0/2] Increase amount of data for monitor to read

[Vladimir Sementsov-Ogievskiy]

09.11.2020 11:50, Vladimir Sementsov-Ogievskiy wrote:

06.11.2020 15:42, Andrey Shinkevich wrote:

The subject was discussed here:
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg00206.html

This series is a solution for the issue with QMP monitor buffered input.
A little parser is introduced to throttle JSON commands read from the
buffer so that QMP requests do not overwhelm the monitor input queue.
A side effect raised in the test #247 was managed in the first patch.
It may be considered as a workaround. Any sane fix suggested will be
appreciated.

Note:
This series goes after the Vladimir's one:
'[PATCH v3 00/25] backup performance: block_status + async"'
To make the test #129 passed, the following patch should be applied first:
'[PATCH v3 01/25] iotests: 129 don't check backup "busy"'.



Hi!

I tried the following test:

start qemu:

   ./qemu-system-x86_64 -qmp stdio

type the following in one line:

   { 'execute': 'qmp_capabilities' }{ 'execute': 'quit' }

press Enter.

Without your patches, the qemu quits immediately, printing double "{"return": 
{}}".
With your patches applied qemu prints "{"return": {}}" only once and doesn't 
quit, until I press Enter the second time.



Positive thing: the patches do increase performance:

for me, the following command:

(echo "{ 'execute': 'qmp_capabilities' }"; for i in {1..1}; do echo "{ 'execute': 
'query-block-jobs' }"; done; echo "{ 'execute': 'quit' }" ) | time ./qemu-system-x86_64 -qmp 
stdio > /dev/null

shows 2.4s on master and 0.6s after patches






Andrey Shinkevich (2):
   iotests: add another bash sleep command to 247
   monitor: increase amount of data for monitor to read

  chardev/char-fd.c  | 64 +-
  chardev/char-socket.c  | 54 +++---
  chardev/char.c | 40 +
  include/chardev/char.h | 15 +++
  monitor/monitor.c  |  2 +-
  tests/qemu-iotests/247 |  2 ++
  tests/qemu-iotests/247.out |  1 +
  7 files changed, 161 insertions(+), 17 deletions(-)







--
Best regards,
Vladimir

Re: [PATCH 2/2] monitor: increase amount of data for monitor to read

[Vladimir Sementsov-Ogievskiy]

06.11.2020 15:42, Andrey Shinkevich wrote:

QMP and HMP monitors read one byte at a time from the socket or stdin,
which is very inefficient. With 100+ VMs on the host, this results in
multiple extra system calls and CPU overuse.
This patch increases the amount of read data up to 4096 bytes that fits
the buffer size on the channel level.

Suggested-by: Denis V. Lunev 
Signed-off-by: Andrey Shinkevich 
---
  chardev/char-fd.c  | 64 +-
  chardev/char-socket.c  | 54 +++---
  chardev/char.c | 40 +
  include/chardev/char.h | 15 +++
  monitor/monitor.c  |  2 +-
  tests/qemu-iotests/247.out |  2 +-
  6 files changed, 159 insertions(+), 18 deletions(-)


Please, add

[diff]
orderFile = /path/to/qemu/scripts/git.orderfile

to your git config, to keep files in good order in the patch.



diff --git a/chardev/char-fd.c b/chardev/char-fd.c
index 1cd62f2..6194fe6 100644
--- a/chardev/char-fd.c
+++ b/chardev/char-fd.c
@@ -33,6 +33,8 @@
  #include "chardev/char-fd.h"
  #include "chardev/char-io.h"
  
+#include "monitor/monitor-internal.h"

+
  /* Called with chr_write_lock held.  */
  static int fd_chr_write(Chardev *chr, const uint8_t *buf, int len)
  {
@@ -41,7 +43,7 @@ static int fd_chr_write(Chardev *chr, const uint8_t *buf, int 
len)
  return io_channel_send(s->ioc_out, buf, len);
  }
  
-static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)

+static gboolean fd_chr_read_hmp(QIOChannel *chan, void *opaque)
  {
  Chardev *chr = CHARDEV(opaque);
  FDChardev *s = FD_CHARDEV(opaque);
@@ -71,6 +73,66 @@ static gboolean fd_chr_read(QIOChannel *chan, GIOCondition 
cond, void *opaque)
  return TRUE;
  }
  
+static gboolean fd_chr_read_qmp(QIOChannel *chan, void *opaque)

+{
+static JSONthrottle thl = {0};


Hmm static cache? I don't think that is good idea. What if function called with 
another chan?

On the other hand, we shouldn't have more than one QMP monitor, and function is 
used only for qmp monitor. Still, could it be reopened or somehow switched, and 
we'll have outdated cache? I don't know..


+uint8_t *start;
+Chardev *chr = CHARDEV(opaque);
+FDChardev *s = FD_CHARDEV(opaque);
+int len, size, pos;
+ssize_t ret;
+
+if (!thl.load) {
+len = sizeof(thl.buf);
+if (len > s->max_size) {
+len = s->max_size;
+}
+if (len == 0) {
+return TRUE;
+}
+
+ret = qio_channel_read(
+chan, (gchar *)thl.buf, len, NULL);
+if (ret == 0) {
+remove_fd_in_watch(chr);
+qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
+thl = (const JSONthrottle){0};
+return FALSE;
+}
+if (ret < 0) {
+return TRUE;
+}


large code chunk is shared with fd_chr_read_hmp(). Would be not bad to avoid 
duplication..


+thl.load = ret;
+thl.cursor = 0;
+}
+
+size = thl.load;
+start = thl.buf + thl.cursor;


you may use uint8_t* pointer type for thl.curser and get rid of size and start 
variables.


+pos = qemu_chr_end_position((const char *) start, size, &thl);


Why should we stop at paired bracked? What's wrong if we pass more data? This 
is the
main thing of the patch, would be good to describe it in the commit message.


+if (pos >= 0) {
+size = pos + 1;
+}
+
+qemu_chr_be_write(chr, start, size);
+thl.cursor += size;
+thl.load -= size;
+
+return TRUE;
+}
+
+static gboolean fd_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
+{
+Chardev *chr = CHARDEV(opaque);
+CharBackend *be = chr->be;
+Monitor *mon = (Monitor *)be->opaque;
+
+if (monitor_is_qmp(mon)) {
+return fd_chr_read_qmp(chan, opaque);
+}
+
+return fd_chr_read_hmp(chan, opaque);
+}
+
  static int fd_chr_read_poll(void *opaque)
  {
  Chardev *chr = CHARDEV(opaque);
diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 213a4c8..8335e8c 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -520,30 +520,54 @@ static void tcp_chr_disconnect(Chardev *chr)
  
  static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)

  {
+static JSONthrottle thl = {0};
+uint8_t *start;
  Chardev *chr = CHARDEV(opaque);
  SocketChardev *s = SOCKET_CHARDEV(opaque);
-uint8_t buf[CHR_READ_BUF_LEN];
-int len, size;
+int len, size, pos;
  
  if ((s->state != TCP_CHARDEV_STATE_CONNECTED) ||

  s->max_size <= 0) {
  return TRUE;
  }
-len = sizeof(buf);
-if (len > s->max_size) {
-len = s->max_size;
-}
-size = tcp_chr_recv(chr, (void *)buf, len);
-if (size == 0 || (size == -1 && errno != EAGAIN)) {
-/* connection closed */
-tcp_chr_disconnect(chr);
-} else if (size > 0) {
-if (s->do_telnetopt) {

Re: [PATCH] block: Fix some code style problems, "foo* bar" should be "foo *bar"

[Max Reitz]

On 30.10.20 04:35, shiliyang wrote:

There have some code style problems be found when read the block driver code.
So I fixes some problems of this error, ERROR: "foo* bar" should be "foo *bar".

Signed-off-by: Liyang Shi 
Reported-by: Euler Robot 
---
  block/blkdebug.c |  2 +-
  block/dmg.c  |  2 +-
  block/qcow2.c|  4 ++--
  block/qcow2.h|  6 +++---
  block/vpc.c  | 10 +-
  5 files changed, 12 insertions(+), 12 deletions(-)


Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max

Re: [PATCH 0/2] Increase amount of data for monitor to read

[Vladimir Sementsov-Ogievskiy]

06.11.2020 15:42, Andrey Shinkevich wrote:

The subject was discussed here:
https://lists.gnu.org/archive/html/qemu-devel/2017-05/msg00206.html

This series is a solution for the issue with QMP monitor buffered input.
A little parser is introduced to throttle JSON commands read from the
buffer so that QMP requests do not overwhelm the monitor input queue.
A side effect raised in the test #247 was managed in the first patch.
It may be considered as a workaround. Any sane fix suggested will be
appreciated.

Note:
This series goes after the Vladimir's one:
'[PATCH v3 00/25] backup performance: block_status + async"'
To make the test #129 passed, the following patch should be applied first:
'[PATCH v3 01/25] iotests: 129 don't check backup "busy"'.



Hi!

I tried the following test:

start qemu:

  ./qemu-system-x86_64 -qmp stdio

type the following in one line:

  { 'execute': 'qmp_capabilities' }{ 'execute': 'quit' }

press Enter.

Without your patches, the qemu quits immediately, printing double "{"return": 
{}}".
With your patches applied qemu prints "{"return": {}}" only once and doesn't 
quit, until I press Enter the second time.



Andrey Shinkevich (2):
   iotests: add another bash sleep command to 247
   monitor: increase amount of data for monitor to read

  chardev/char-fd.c  | 64 +-
  chardev/char-socket.c  | 54 +++---
  chardev/char.c | 40 +
  include/chardev/char.h | 15 +++
  monitor/monitor.c  |  2 +-
  tests/qemu-iotests/247 |  2 ++
  tests/qemu-iotests/247.out |  1 +
  7 files changed, 161 insertions(+), 17 deletions(-)




--
Best regards,
Vladimir

Re: [PATCH 0/2] block: Remove unused BlockDeviceMapEntry

[Markus Armbruster]

Max Reitz  writes:

> Hi,
>
> Markus has revived a rather old patch to remove an unused QAPI
> structure:
>
> https://lists.nongnu.org/archive/html/qemu-block/2020-10/msg01902.html
>
> He quoted a response of mine to the original patch, where I noted that
> removing this structure is OK because it is superseded by another
> structure (MapEntry), but that other structure’s documentation is worse.
> So we should merge some of BlockDeviceMapEntry’s documentation into
> MapEntry before removing it.
>
> This series combines said merge with Markus’s patch to drop
> BlockDeviceMapEntry.

Queued, thanks!