On 06/29/2018 10:15 AM, Vladimir Sementsov-Ogievskiy wrote:
We need to synchronize backup job with reading from fleecing image
like it was done in block/replication.c.
Otherwise, the following situation is theoretically possible:
Grammar suggestions:
1. client start reading
client starts reading
2. client understand, that there is no corresponding cluster in
fleecing image
3. client is going to read from backing file (i.e. active image)
client sees that no corresponding cluster has been allocated in the
fleecing image, so the request is forwarded to the backing file
4. guest writes to active image
5. this write is stopped by backup(sync=none) and cluster is copied to
fleecing image
6. guest write continues...
7. and client reads _new_ (or partly new) date from active image
Interesting race. Can it actually happen, or does our read code
already serialize writes to the same area while a read is underway?
In short, I see what problem you are claiming exists: the moment the
client starts reading from the backing file, that portion of the
backing file must remain unchanged until after the client is done
reading. But I don't know enough details of the block layer to know
if this is actually a problem, or if adding the new filter is just
overhead.
So, this fleecing-filter should be above fleecing image, the whole
picture of fleecing looks like this:
+-------+ +------------+
| | | |
| guest | | NBD client +<------+
| | | | |
++-----++ +------------+ |only read
| ^ |
| IO | |
v | +-----+------+
++-----+---------+ | |
| | | internal |
| active image +----+ | NBD server |
| | | | |
+-+--------------+ |backup +-+----------+
^ |sync=none ^
|backing | |only read
| | |
+-+--------------+ | +------+----------+
| | | | |
| fleecing image +<---+ | fleecing filter |
| | | |
+--------+-------+ +-----+-----------+
^ |
| |
+--------------------------+
file
Can you also show the sequence of QMP commands to set up this
structure (or maybe you do in 3/3; which I haven't looked at yet).
Signed-off-by: Vladimir Sementsov-Ogievskiy <address@hidden>
---
qapi/block-core.json | 6 ++--
block/fleecing-filter.c | 80
+++++++++++++++++++++++++++++++++++++++++++++++++
block/Makefile.objs | 1 +
3 files changed, 85 insertions(+), 2 deletions(-)
create mode 100644 block/fleecing-filter.c
diff --git a/qapi/block-core.json b/qapi/block-core.json
index 577ce5e999..43872c3d79 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -2542,7 +2542,8 @@
'host_device', 'http', 'https', 'iscsi', 'luks', 'nbd',
'nfs',
'null-aio', 'null-co', 'nvme', 'parallels', 'qcow',
'qcow2', 'qed',
'quorum', 'raw', 'rbd', 'replication', 'sheepdog', 'ssh',
- 'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat',
'vxhs' ] }
+ 'throttle', 'vdi', 'vhdx', 'vmdk', 'vpc', 'vvfat', 'vxhs',
+ 'fleecing-filter' ] }
Missing a 'since 3.0' documentation blurb; also, this enum has been
kept sorted, so your new filter needs to come earlier.
##
# @BlockdevOptionsFile:
@@ -3594,7 +3595,8 @@
'vmdk': 'BlockdevOptionsGenericCOWFormat',
'vpc': 'BlockdevOptionsGenericFormat',
'vvfat': 'BlockdevOptionsVVFAT',
- 'vxhs': 'BlockdevOptionsVxHS'
+ 'vxhs': 'BlockdevOptionsVxHS',
+ 'fleecing-filter': 'BlockdevOptionsGenericFormat'
Again, this has been kept sorted.
+static coroutine_fn int fleecing_co_preadv(BlockDriverState *bs,
+ uint64_t offset, uint64_t
bytes,
+ QEMUIOVector *qiov, int flags)
+{
+ int ret;
+ BlockJob *job = bs->file->bs->backing->bs->job;
+ CowRequest req;
+
+ backup_wait_for_overlapping_requests(job, offset, bytes);
+ backup_cow_request_begin(&req, job, offset, bytes);
+
+ ret = bdrv_co_preadv(bs->file, offset, bytes, qiov, flags);
+
+ backup_cow_request_end(&req);
+
+ return ret;
+}
So the idea here is that you force a serializing request to ensure
that there are no other writes to the area in the meantime.
+
+static coroutine_fn int fleecing_co_pwritev(BlockDriverState *bs,
+ uint64_t offset,
uint64_t bytes,
+ QEMUIOVector *qiov, int flags)
+{
+ return -EINVAL;
and you force this to be a read-only interface. (Does the block layer
actually require us to provide a pwritev callback, or can we leave it
NULL instead?)
+BlockDriver bdrv_fleecing_filter = {
+ .format_name = "fleecing-filter",
+ .protocol_name = "fleecing-filter",
+ .instance_size = 0,
+
+ .bdrv_open = fleecing_open,
+ .bdrv_close = fleecing_close,
+
+ .bdrv_getlength = fleecing_getlength,
+ .bdrv_co_preadv = fleecing_co_preadv,
+ .bdrv_co_pwritev = fleecing_co_pwritev,
+
+ .is_filter = true,
+ .bdrv_recurse_is_first_non_filter =
fleecing_recurse_is_first_non_filter,
+ .bdrv_child_perm = bdrv_filter_default_perms,
No .bdrv_co_block_status callback? That probably hurts querying for
sparse regions.