The following changes since commit dd2db39d78431ab5a0b78777afaab3d61e94533e:

Merge remote-tracking branch 'remotes/ehabkost-gl/tags/x86-next-pull-request' into staging (2021-06-01 21:23:26 +0100)

for you to fetch changes up to b317006a3f1f04191a7981cef83417cb2477213b:

docs/secure-coding-practices: Describe how to use 'null-co' block driver (2021-06-02 14:29:14 +0200)

- NBD server: Fix crashes related to switching between AioContexts

- file-posix: Workaround for discard/write_zeroes on buggy filesystems

- Follow-up fixes for the reopen vs. permission changes

- quorum: Fix error handling for flush

- block-copy: Refactor copy_range handling

- docs: Describe how to use 'null-co' block driver

Lukas Straub (1):

block/quorum: Provide .bdrv_co_flush instead of .bdrv_co_flush_to_disk

Philippe Mathieu-Daudé (1):

docs/secure-coding-practices: Describe how to use 'null-co' block driver

Sergio Lopez (2):

block-backend: add drained_poll

nbd/server: Use drained block ops to quiesce the server

Thomas Huth (2):

block/file-posix: Fix problem with fallocate(PUNCH_HOLE) on GPFS

block/file-posix: Try other fallbacks after invalid FALLOC_FL_ZERO_RANGE

Vladimir Sementsov-Ogievskiy (14):

qemu-io-cmds: assert that we don't have .perm requested in no-blk case

block/vvfat: child_vvfat_qcow: add .get_parent_aio_context, fix crash

block/vvfat: fix vvfat_child_perm crash

block: consistently use bdrv_is_read_only()

block: drop BlockDriverState::read_only

block: drop BlockBackendRootState::read_only

block: document child argument of bdrv_attach_child_common()

block-backend: improve blk_root_get_parent_desc()

block: improve bdrv_child_get_parent_desc()

block/vvfat: inherit child_vvfat_qcow from child_of_bds

block: simplify bdrv_child_user_desc()

block: improve permission conflict error message

block-copy: fix block_copy_task_entry() progress update

block-copy: refactor copy_range handling

docs/devel/secure-coding-practices.rst | 9 ++++

include/block/block.h | 1 +

include/block/block_int.h | 2 -

include/sysemu/block-backend.h | 4 ++

block.c | 82 ++++++++++++++++++++--------------

block/block-backend.c | 26 +++++------

block/block-copy.c | 80 ++++++++++++++++++++++-----------

block/commit.c | 2 +-

block/file-posix.c | 29 ++++++++----

block/io.c | 4 +-

block/qapi.c | 2 +-

block/qcow2-snapshot.c | 2 +-

block/qcow2.c | 5 +--

block/quorum.c | 2 +-

block/snapshot.c | 2 +-

block/vhdx-log.c | 2 +-

block/vvfat.c | 14 +++---

blockdev.c | 3 +-

nbd/server.c | 82 +++++++++++++++++++++++++---------

qemu-io-cmds.c | 14 +++++-

tests/unit/test-block-iothread.c | 6 ---

tests/qemu-iotests/283.out | 2 +-

tests/qemu-iotests/307.out | 2 +-

tests/qemu-iotests/tests/qsd-jobs.out | 2 +-

24 files changed, 241 insertions(+), 138 deletions(-)

From: Thomas Huth <thuth@redhat.com>

A customer reported that running

qemu-img convert -t none -O qcow2 -f qcow2 input.qcow2 output.qcow2

fails for them with the following error message when the images are

stored on a GPFS file system :

qemu-img: error while writing sector 0: Invalid argument

After analyzing the strace output, it seems like the problem is in

handle_aiocb_write_zeroes(): The call to fallocate(FALLOC_FL_PUNCH_HOLE)

returns EINVAL, which can apparently happen if the file system has

a different idea of the granularity of the operation. It's arguably

a bug in GPFS, since the PUNCH_HOLE mode should not result in EINVAL

according to the man-page of fallocate(), but the file system is out

there in production and so we have to deal with it. In commit 294682cc3a

("block: workaround for unaligned byte range in fallocate()") we also

already applied the a work-around for the same problem to the earlier

fallocate(FALLOC_FL_ZERO_RANGE) call, so do it now similar with the

PUNCH_HOLE call. But instead of silently catching and returning

-ENOTSUP (which causes the caller to fall back to writing zeroes),

let's rather inform the user once about the buggy file system and

try the other fallback instead.

Signed-off-by: Thomas Huth <thuth@redhat.com>

Message-Id: <20210527172020.847617-2-thuth@redhat.com>

block/file-posix.c | 11 +++++++++++

1 file changed, 11 insertions(+)

@@ -XXX,XX +XXX,XX @@ static int handle_aiocb_write_zeroes(void *opaque)

return ret;

}

s->has_fallocate = false;

+ } else if (ret == -EINVAL) {

+ /*

+ * Some file systems like older versions of GPFS do not like un-

+ * aligned byte ranges, and return EINVAL in such a case, though

+ * they should not do it according to the man-page of fallocate().

+ * Warn about the bad filesystem and try the final fallback instead.

+ */

+ warn_report_once("Your file system is misbehaving: "

+ "fallocate(FALLOC_FL_PUNCH_HOLE) returned EINVAL. "

+ "Please report this bug to your file sytem "

+ "vendor.");

} else if (ret != -ENOTSUP) {

return ret;

} else {

2.30.2

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

The logic around **child is not obvious: this reference is used not

only to return resulting child, but also to rollback NULL value on

transaction abort.

So, let's add documentation and some assertions.

While being here, drop extra declaration of bdrv_attach_child_noperm().

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Message-Id: <20210601075218.79249-2-vsementsov@virtuozzo.com>

block.c | 24 +++++++++++++++---------

1 file changed, 15 insertions(+), 9 deletions(-)

-static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,

- BlockDriverState *child_bs,

- const char *child_name,

- const BdrvChildClass *child_class,

- BdrvChildRole child_role,

- BdrvChild **child,

- Transaction *tran,

- Error **errp);

static void bdrv_remove_filter_or_cow_child(BlockDriverState *bs,

Transaction *tran);

@@ -XXX,XX +XXX,XX @@ static TransactionActionDrv bdrv_attach_child_common_drv = {

/*

* Common part of attaching bdrv child to bs or to blk or to job

+ *

+ * Resulting new child is returned through @child.

+ * At start *@child must be NULL.

+ * @child is saved to a new entry of @tran, so that *@child could be reverted to

+ * NULL on abort(). So referenced variable must live at least until transaction

+ * end.

*/

static int bdrv_attach_child_common(BlockDriverState *child_bs,

const char *child_name,

@@ -XXX,XX +XXX,XX @@ static int bdrv_attach_child_common(BlockDriverState *child_bs,

return 0;

+/*

+ * Variable referenced by @child must live at least until transaction end.

+ * (see bdrv_attach_child_common() doc for details)

+ */

static int bdrv_attach_child_noperm(BlockDriverState *parent_bs,

BlockDriverState *child_bs,

const char *child_name,

@@ -XXX,XX +XXX,XX @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,

child_role, perm, shared_perm, opaque,

&child, tran, errp);

if (ret < 0) {

- assert(child == NULL);

goto out;

}

@@ -XXX,XX +XXX,XX @@ BdrvChild *bdrv_root_attach_child(BlockDriverState *child_bs,

out:

tran_finalize(tran, ret);

+ /* child is unset on failure by bdrv_attach_child_common_abort() */

+ assert((ret < 0) == !child);

bdrv_unref(child_bs);

return child;

@@ -XXX,XX +XXX,XX @@ BdrvChild *bdrv_attach_child(BlockDriverState *parent_bs,

out:

tran_finalize(tran, ret);

+ /* child is unset on failure by bdrv_attach_child_common_abort() */

+ assert((ret < 0) == !child);

bdrv_unref(child_bs);

2.30.2

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Now permissions are updated as follows:

1. do graph modifications ignoring permissions

2. do permission update

(of course, we rollback [1] if [2] fails)

So, on stage [2] we can't say which users are "old" and which are

"new" and exist only since [1]. And current error message is a bit

outdated. Let's improve it, to make everything clean.

While being here, add also a comment and some good assertions.

iotests 283, 307, qsd-jobs outputs are updated.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Message-Id: <20210601075218.79249-7-vsementsov@virtuozzo.com>

block.c | 29 ++++++++++++++++++++-------

tests/qemu-iotests/283.out | 2 +-

tests/qemu-iotests/307.out | 2 +-

tests/qemu-iotests/tests/qsd-jobs.out | 2 +-

4 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/block.c b/block.c

--- a/block.c

+++ b/block.c

@@ -XXX,XX +XXX,XX @@ static char *bdrv_child_user_desc(BdrvChild *c)

return c->klass->get_parent_desc(c);

+ * Check that @a allows everything that @b needs. @a and @b must reference same

+ * child node.

static bool bdrv_a_allow_b(BdrvChild *a, BdrvChild *b, Error **errp)

- g_autofree char *user = NULL;

- g_autofree char *perm_names = NULL;

+ const char *child_bs_name;

+ g_autofree char *a_user = NULL;

+ g_autofree char *b_user = NULL;

+ g_autofree char *perms = NULL;

+

+ assert(a->bs);

+ assert(a->bs == b->bs);

if ((b->perm & a->shared_perm) == b->perm) {

return true;

}

- perm_names = bdrv_perm_names(b->perm & ~a->shared_perm);

- user = bdrv_child_user_desc(a);

- error_setg(errp, "Conflicts with use by %s as '%s', which does not "

- "allow '%s' on %s",

- user, a->name, perm_names, bdrv_get_node_name(b->bs));

+ child_bs_name = bdrv_get_node_name(b->bs);

+ a_user = bdrv_child_user_desc(a);

+ b_user = bdrv_child_user_desc(b);

+ perms = bdrv_perm_names(b->perm & ~a->shared_perm);

+

+ error_setg(errp, "Permission conflict on node '%s': permissions '%s' are "

+ "both required by %s (uses node '%s' as '%s' child) and "

+ "unshared by %s (uses node '%s' as '%s' child).",

+ child_bs_name, perms,

+ b_user, child_bs_name, b->name,

+ a_user, child_bs_name, a->name);

return false;

}

diff --git a/tests/qemu-iotests/283.out b/tests/qemu-iotests/283.out

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/283.out

+++ b/tests/qemu-iotests/283.out

@@ -XXX,XX +XXX,XX @@

{"execute": "blockdev-add", "arguments": {"driver": "blkdebug", "image": "base", "node-name": "other", "take-child-perms": ["write"]}}

{"return": {}}

{"execute": "blockdev-backup", "arguments": {"device": "source", "sync": "full", "target": "target"}}

-{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Conflicts with use by node 'source' as 'image', which does not allow 'write' on base"}}

+{"error": {"class": "GenericError", "desc": "Cannot append backup-top filter: Permission conflict on node 'base': permissions 'write' are both required by node 'other' (uses node 'base' as 'image' child) and unshared by node 'source' (uses node 'base' as 'image' child)."}}

=== backup-top should be gone after job-finalize ===

diff --git a/tests/qemu-iotests/307.out b/tests/qemu-iotests/307.out

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/307.out

+++ b/tests/qemu-iotests/307.out

@@ -XXX,XX +XXX,XX @@ exports available: 1

=== Add a writable export ===

{"execute": "block-export-add", "arguments": {"description": "This is the writable second export", "id": "export1", "name": "export1", "node-name": "fmt", "type": "nbd", "writable": true, "writethrough": true}}

-{"error": {"class": "GenericError", "desc": "Conflicts with use by block device 'sda' as 'root', which does not allow 'write' on fmt"}}

+{"error": {"class": "GenericError", "desc": "Permission conflict on node 'fmt': permissions 'write' are both required by an unnamed block device (uses node 'fmt' as 'root' child) and unshared by block device 'sda' (uses node 'fmt' as 'root' child)."}}

{"execute": "device_del", "arguments": {"id": "sda"}}

{"return": {}}

{"data": {"device": "sda", "path": "/machine/peripheral/sda"}, "event": "DEVICE_DELETED", "timestamp": {"microseconds": "USECS", "seconds": "SECS"}}

diff --git a/tests/qemu-iotests/tests/qsd-jobs.out b/tests/qemu-iotests/tests/qsd-jobs.out

index XXXXXXX..XXXXXXX 100644

--- a/tests/qemu-iotests/tests/qsd-jobs.out

+++ b/tests/qemu-iotests/tests/qsd-jobs.out

@@ -XXX,XX +XXX,XX @@ QMP_VERSION

{"return": {}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "created", "id": "job0"}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "JOB_STATUS_CHANGE", "data": {"status": "null", "id": "job0"}}

-{"error": {"class": "GenericError", "desc": "Conflicts with use by stream job 'job0' as 'intermediate node', which does not allow 'write' on fmt_base"}}

+{"error": {"class": "GenericError", "desc": "Permission conflict on node 'fmt_base': permissions 'write' are both required by an unnamed block device (uses node 'fmt_base' as 'root' child) and unshared by stream job 'job0' (uses node 'fmt_base' as 'intermediate node' child)."}}

{"return": {}}

{"timestamp": {"seconds": TIMESTAMP, "microseconds": TIMESTAMP}, "event": "BLOCK_EXPORT_DELETED", "data": {"id": "export1"}}

*** done

2.30.2

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Don't report successful progress on failure, when call_state->ret is

set.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>

Message-Id: <20210528141628.44287-2-vsementsov@virtuozzo.com>

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

block/block-copy.c | 8 +++++---

1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/block/block-copy.c b/block/block-copy.c

--- a/block/block-copy.c

+++ b/block/block-copy.c

@@ -XXX,XX +XXX,XX @@ static coroutine_fn int block_copy_task_entry(AioTask *task)

ret = block_copy_do_copy(t->s, t->offset, t->bytes, t->zeroes,

&error_is_read);

- if (ret < 0 && !t->call_state->ret) {

- t->call_state->ret = ret;

- t->call_state->error_is_read = error_is_read;

+ if (ret < 0) {

+ if (!t->call_state->ret) {

+ t->call_state->ret = ret;

+ t->call_state->error_is_read = error_is_read;

+ }

} else {

progress_work_done(t->s->progress, t->bytes);

}

2.30.2

From: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>