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: Philippe Mathieu-Daudé <philmd@redhat.com>

Document that security reports must use 'null-co,read-zeroes=on'

because otherwise the memory is left uninitialized (which is an

on-purpose performance feature).

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>

Message-Id: <20210601162548.2076631-1-philmd@redhat.com>

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

1 file changed, 9 insertions(+)

diff --git a/docs/devel/secure-coding-practices.rst b/docs/devel/secure-coding-practices.rst

--- a/docs/devel/secure-coding-practices.rst

+++ b/docs/devel/secure-coding-practices.rst

@@ -XXX,XX +XXX,XX @@ structures and only process the local copy. This prevents

time-of-check-to-time-of-use (TOCTOU) race conditions that could cause QEMU to

crash when a vCPU thread modifies guest RAM while device emulation is

processing it.

+Use of null-co block drivers

+----------------------------

+

+The ``null-co`` block driver is designed for performance: its read accesses are

+not initialized by default. In case this driver has to be used for security

+research, it must be used with the ``read-zeroes=on`` option which fills read

+buffers with zeroes. Security issues reported with the default

+(``read-zeroes=off``) will be discarded.

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(-)

@@ -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>

All child classes have this callback. So, drop unreachable code.

Still add an assertion to bdrv_attach_child_common(), to early detect

bad classes.

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

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

block.c | 7 ++-----

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

@@ -XXX,XX +XXX,XX @@ bool bdrv_is_writable(BlockDriverState *bs)

static char *bdrv_child_user_desc(BdrvChild *c)

- if (c->klass->get_parent_desc) {

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

- }

-

- return g_strdup("another user");

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

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

assert(child);

assert(*child == NULL);

+ assert(child_class->get_parent_desc);

new_child = g_new(BdrvChild, 1);

*new_child = (BdrvChild) {

2.30.2

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

It's better to use accessor function instead of bs->read_only directly.

In some places use bdrv_is_writable() instead of

checking both BDRV_O_RDWR set and BDRV_O_INACTIVE not set.

In bdrv_open_common() it's a bit strange to add one more variable, but

we are going to drop bs->read_only in the next patch, so new ro local

variable substitutes it here.

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

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

block.c | 11 +++++++----

block/block-backend.c | 2 +-

block/commit.c | 2 +-

block/io.c | 4 ++--

block/qapi.c | 2 +-

block/qcow2-snapshot.c | 2 +-

block/qcow2.c | 5 ++---

block/snapshot.c | 2 +-

block/vhdx-log.c | 2 +-

9 files changed, 17 insertions(+), 15 deletions(-)

@@ -XXX,XX +XXX,XX @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,

QemuOpts *opts;

BlockDriver *drv;

Error *local_err = NULL;

+ bool ro;

assert(bs->file == NULL);

assert(options != NULL && bs->options != options);

@@ -XXX,XX +XXX,XX @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,

bs->read_only = !(bs->open_flags & BDRV_O_RDWR);

- if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, bs->read_only)) {

- if (!bs->read_only && bdrv_is_whitelisted(drv, true)) {

+ ro = bdrv_is_read_only(bs);

+

+ if (use_bdrv_whitelist && !bdrv_is_whitelisted(drv, ro)) {

+ if (!ro && bdrv_is_whitelisted(drv, true)) {

ret = bdrv_apply_auto_read_only(bs, NULL, NULL);

} else {

ret = -ENOTSUP;

}

if (ret < 0) {

error_setg(errp,

- !bs->read_only && bdrv_is_whitelisted(drv, true)

+ !ro && bdrv_is_whitelisted(drv, true)

? "Driver '%s' can only be used for read-only devices"

: "Driver '%s' is not whitelisted",

drv->format_name);

@@ -XXX,XX +XXX,XX @@ static int bdrv_open_common(BlockDriverState *bs, BlockBackend *file,

assert(qatomic_read(&bs->copy_on_read) == 0);

if (bs->open_flags & BDRV_O_COPY_ON_READ) {

- if (!bs->read_only) {

+ if (!ro) {

bdrv_enable_copy_on_read(bs);

} else {

error_setg(errp, "Can't use copy-on-read on read-only device");

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

index XXXXXXX..XXXXXXX 100644

--- a/block/block-backend.c

+++ b/block/block-backend.c

@@ -XXX,XX +XXX,XX @@ void blk_update_root_state(BlockBackend *blk)

assert(blk->root);

blk->root_state.open_flags = blk->root->bs->open_flags;

- blk->root_state.read_only = blk->root->bs->read_only;

+ blk->root_state.read_only = bdrv_is_read_only(blk->root->bs);

blk->root_state.detect_zeroes = blk->root->bs->detect_zeroes;

}

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

index XXXXXXX..XXXXXXX 100644

--- a/block/commit.c

+++ b/block/commit.c

@@ -XXX,XX +XXX,XX @@ int bdrv_commit(BlockDriverState *bs)

return -EBUSY;

- ro = backing_file_bs->read_only;

+ ro = bdrv_is_read_only(backing_file_bs);

if (ro) {

if (bdrv_reopen_set_read_only(backing_file_bs, false, NULL)) {

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

index XXXXXXX..XXXXXXX 100644

--- a/block/io.c

+++ b/block/io.c

@@ -XXX,XX +XXX,XX @@ bdrv_co_write_req_prepare(BdrvChild *child, int64_t offset, int64_t bytes,

bdrv_check_request(offset, bytes, &error_abort);

- if (bs->read_only) {

+ if (bdrv_is_read_only(bs)) {

return -EPERM;

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 void bdrv_replace_child_noperm(BdrvChild *child,

-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,

+/*

+ * 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);

+

2.30.2

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

We have different types of parents: block nodes, block backends and

jobs. So, it makes sense to specify type together with name.

Next, this handler us used to compose an error message about permission

conflict. And permission conflict occurs in a specific place of block

graph. We shouldn't report name of parent device (as it refers another

place in block graph), but exactly and only the name of the node. So,

use bdrv_get_node_name() directly.

iotest 283 output is updated.

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

Reviewed-by: Alberto Garcia <berto@igalia.com>

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

block.c | 2 +-

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

2 files changed, 2 insertions(+), 2 deletions(-)

@@ -XXX,XX +XXX,XX @@ int bdrv_parse_cache_mode(const char *mode, int *flags, bool *writethrough)

static char *bdrv_child_get_parent_desc(BdrvChild *c)

BlockDriverState *parent = c->opaque;

- return g_strdup(bdrv_get_device_or_node_name(parent));

+ return g_strdup_printf("node '%s'", bdrv_get_node_name(parent));

static void bdrv_child_cb_drained_begin(BdrvChild *child)

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 source as 'image', which does not allow 'write' on base"}}

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

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

2.30.2

From: Sergio Lopez <slp@redhat.com>

Before switching between AioContexts we need to make sure that we're

fully quiesced ("nb_requests == 0" for every client) when entering the

drained section.

To do this, we set "quiescing = true" for every client on

".drained_begin" to prevent new coroutines from being created, and

check if "nb_requests == 0" on ".drained_poll". Finally, once we're

exiting the drained section, on ".drained_end" we set "quiescing =

false" and call "nbd_client_receive_next_request()" to resume the

processing of new requests.

With these changes, "blk_aio_attach()" and "blk_aio_detach()" can be

reverted to be as simple as they were before f148ae7d36.

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1960137

Suggested-by: Kevin Wolf <kwolf@redhat.com>

Signed-off-by: Sergio Lopez <slp@redhat.com>

Message-Id: <20210602060552.17433-3-slp@redhat.com>

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

1 file changed, 61 insertions(+), 21 deletions(-)

diff --git a/nbd/server.c b/nbd/server.c

--- a/nbd/server.c

+++ b/nbd/server.c

@@ -XXX,XX +XXX,XX @@ static void nbd_request_put(NBDRequestData *req)

g_free(req);

client->nb_requests--;

+

+ if (client->quiescing && client->nb_requests == 0) {

+ aio_wait_kick();

+

nbd_client_receive_next_request(client);

nbd_client_put(client);

@@ -XXX,XX +XXX,XX @@ static void blk_aio_attached(AioContext *ctx, void *opaque)

QTAILQ_FOREACH(client, &exp->clients, next) {

qio_channel_attach_aio_context(client->ioc, ctx);

+ assert(client->nb_requests == 0);

assert(client->recv_coroutine == NULL);

assert(client->send_coroutine == NULL);

-

- if (client->quiescing) {

- client->quiescing = false;

- nbd_client_receive_next_request(client);

- }

}

}

-static void nbd_aio_detach_bh(void *opaque)

+static void blk_aio_detach(void *opaque)

NBDExport *exp = opaque;

NBDClient *client;

+ trace_nbd_blk_aio_detach(exp->name, exp->common.ctx);

+

QTAILQ_FOREACH(client, &exp->clients, next) {

qio_channel_detach_aio_context(client->ioc);

+

+ exp->common.ctx = NULL;

+}

+

+static void nbd_drained_begin(void *opaque)

+{

+ NBDExport *exp = opaque;

+ NBDClient *client;

+

+ QTAILQ_FOREACH(client, &exp->clients, next) {

client->quiescing = true;

+ }

+}

- if (client->recv_coroutine) {

- if (client->read_yielding) {

- qemu_aio_coroutine_enter(exp->common.ctx,

- client->recv_coroutine);

- } else {

- AIO_WAIT_WHILE(exp->common.ctx, client->recv_coroutine != NULL);

- }

- }

+static void nbd_drained_end(void *opaque)

+{

+ NBDExport *exp = opaque;

+ NBDClient *client;

- if (client->send_coroutine) {

- AIO_WAIT_WHILE(exp->common.ctx, client->send_coroutine != NULL);

- }

+ QTAILQ_FOREACH(client, &exp->clients, next) {

+ client->quiescing = false;

+ nbd_client_receive_next_request(client);

}

}

-static void blk_aio_detach(void *opaque)

+static bool nbd_drained_poll(void *opaque)

{

NBDExport *exp = opaque;

+ NBDClient *client;

- trace_nbd_blk_aio_detach(exp->name, exp->common.ctx);

+ QTAILQ_FOREACH(client, &exp->clients, next) {

+ if (client->nb_requests != 0) {

+ /*

+ * If there's a coroutine waiting for a request on nbd_read_eof()

+ * enter it here so we don't depend on the client to wake it up.

+ */

+ if (client->recv_coroutine != NULL && client->read_yielding) {

+ qemu_aio_coroutine_enter(exp->common.ctx,

+ client->recv_coroutine);

+ }

- aio_wait_bh_oneshot(exp->common.ctx, nbd_aio_detach_bh, exp);

+ return true;

+ }

+ }

- exp->common.ctx = NULL;

+ return false;

}

static void nbd_eject_notifier(Notifier *n, void *data)

@@ -XXX,XX +XXX,XX @@ void nbd_export_set_on_eject_blk(BlockExport *exp, BlockBackend *blk)

blk_add_remove_bs_notifier(blk, &nbd_exp->eject_notifier);

}

+static const BlockDevOps nbd_block_ops = {

+ .drained_begin = nbd_drained_begin,

+ .drained_end = nbd_drained_end,

+ .drained_poll = nbd_drained_poll,

+};

+

static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,

Error **errp)

{

@@ -XXX,XX +XXX,XX @@ static int nbd_export_create(BlockExport *blk_exp, BlockExportOptions *exp_args,

exp->allocation_depth = arg->allocation_depth;

+ /*

+ * We need to inhibit request queuing in the block layer to ensure we can

+ * be properly quiesced when entering a drained section, as our coroutines

+ * servicing pending requests might enter blk_pread().

+ */

+ blk_set_disable_request_queuing(blk, true);

+

blk_add_aio_context_notifier(blk, blk_aio_attached, blk_aio_detach, exp);

+ blk_set_dev_ops(blk, &nbd_block_ops, exp);

+

QTAILQ_INSERT_TAIL(&exports, exp, next);

return 0;

@@ -XXX,XX +XXX,XX @@ static void nbd_export_delete(BlockExport *blk_exp)

2.30.2

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

Currently we update s->use_copy_range and s->copy_size in

block_copy_do_copy().

It's not very good:

1. block_copy_do_copy() is intended to be a simple function, that wraps

bdrv_co_<io> functions for need of block copy. That's why we don't pass

BlockCopyTask into it. So, block_copy_do_copy() is bad place for

manipulation with generic state of block-copy process

2. We are going to make block-copy thread-safe. So, it's good to move

manipulation with state of block-copy to the places where we'll need

critical sections anyway, to not introduce extra synchronization

primitives in block_copy_do_copy().

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

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

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

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

1 file changed, 49 insertions(+), 23 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 @@ typedef struct BlockCopyTask {

int64_t offset;

int64_t bytes;

bool zeroes;

+ bool copy_range;

QLIST_ENTRY(BlockCopyTask) list;

CoQueue wait_queue; /* coroutines blocked on this task */

} BlockCopyTask;

@@ -XXX,XX +XXX,XX @@ static BlockCopyTask *block_copy_task_create(BlockCopyState *s,

.call_state = call_state,

.offset = offset,

.bytes = bytes,

+ .copy_range = s->use_copy_range,

qemu_co_queue_init(&task->wait_queue);

QLIST_INSERT_HEAD(&s->tasks, task, list);

@@ -XXX,XX +XXX,XX @@ static coroutine_fn int block_copy_task_run(AioTaskPool *pool,

* No sync here: nor bitmap neighter intersecting requests handling, only copy.

*

+ * @copy_range is an in-out argument: if *copy_range is false, copy_range is not

+ * done. If *copy_range is true, copy_range is attempted. If the copy_range

+ * attempt fails, the function falls back to the usual read+write and

+ * *copy_range is set to false. *copy_range and zeroes must not be true

+ * simultaneously.

* Returns 0 on success.

static int coroutine_fn block_copy_do_copy(BlockCopyState *s,

int64_t offset, int64_t bytes,

- bool zeroes, bool *error_is_read)

+ bool zeroes, bool *copy_range,

+ bool *error_is_read)

{

int64_t nbytes = MIN(offset + bytes, s->len) - offset;

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn block_copy_do_copy(BlockCopyState *s,

assert(offset + bytes <= s->len ||

offset + bytes == QEMU_ALIGN_UP(s->len, s->cluster_size));

assert(nbytes < INT_MAX);

+ assert(!(*copy_range && zeroes));

if (zeroes) {

ret = bdrv_co_pwrite_zeroes(s->target, offset, nbytes, s->write_flags &

@@ -XXX,XX +XXX,XX @@ static int coroutine_fn block_copy_do_copy(BlockCopyState *s,

return ret;

}

- if (s->use_copy_range) {

+ if (*copy_range) {

ret = bdrv_co_copy_range(s->source, offset, s->target, offset, nbytes,

0, s->write_flags);

if (ret < 0) {

trace_block_copy_copy_range_fail(s, offset, ret);

- s->use_copy_range = false;

- s->copy_size = MAX(s->cluster_size, BLOCK_COPY_MAX_BUFFER);

+ *copy_range = false;

/* Fallback to read+write with allocated buffer */

} else {

- if (s->use_copy_range) {

- /*

- * Successful copy-range. Now increase copy_size. copy_range

- * does not respect max_transfer (it's a TODO), so we factor

- * that in here.

- *

- * Note: we double-check s->use_copy_range for the case when

- * parallel block-copy request unsets it during previous

- * bdrv_co_copy_range call.

- */

- s->copy_size =

- MIN(MAX(s->cluster_size, BLOCK_COPY_MAX_COPY_RANGE),

- QEMU_ALIGN_DOWN(block_copy_max_transfer(s->source,

- s->target),

- s->cluster_size));

- }

- goto out;

+ return 0;

}

}

+static void block_copy_handle_copy_range_result(BlockCopyState *s,