qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v14 05/13] qapi: create BlockdevOptionsCor structure for COR


From: Vladimir Sementsov-Ogievskiy
Subject: Re: [PATCH v14 05/13] qapi: create BlockdevOptionsCor structure for COR driver
Date: Fri, 11 Dec 2020 15:32:43 +0300
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.5.1

11.12.2020 11:54, Max Reitz wrote:
On 10.12.20 19:30, Vladimir Sementsov-Ogievskiy wrote:
10.12.2020 20:43, Max Reitz wrote:
I don’t like this patch’s subject very much, because I find the implementation 
of the @bottom option to be more noteworthy than the addition of the QAPI 
structure.


On 04.12.20 23:07, Vladimir Sementsov-Ogievskiy wrote:
From: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>

Create the BlockdevOptionsCor structure for COR driver specific options
splitting it off form the BlockdevOptionsGenericFormat. The only option
'bottom' node in the structure denotes an image file that limits the
COR operations in the backing chain.
We are going to use the COR-filter for a block-stream job and will pass
a bottom node name to the COR driver. The bottom node is the first
non-filter overlay of the base. It was introduced because the base node
itself may change due to possible concurrent jobs.

Suggested-by: Max Reitz <mreitz@redhat.com>
Suggested-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
   [vsementsov: fix bdrv_is_allocated_above() usage]
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
  qapi/block-core.json | 21 +++++++++++++++-
  block/copy-on-read.c | 57 ++++++++++++++++++++++++++++++++++++++++++--
  2 files changed, 75 insertions(+), 3 deletions(-)

diff --git a/qapi/block-core.json b/qapi/block-core.json
index 8ef3df6767..04055ef50c 100644
--- a/qapi/block-core.json
+++ b/qapi/block-core.json
@@ -3942,6 +3942,25 @@
    'data': { 'throttle-group': 'str',
              'file' : 'BlockdevRef'
               } }
+
+##
+# @BlockdevOptionsCor:
+#
+# Driver specific block device options for the copy-on-read driver.
+#
+# @bottom: the name of a non-filter node (allocation-bearing layer) that limits
+#          the COR operations in the backing chain (inclusive).

This seems to me like something’s missing.  Perhaps technically there isn’t, 
but “limits the COR operations” begs the question (to me) “Limits them in what 
way?” (to which the answer is: No data below @bottom is copied).

Could you make it more verbose?  Perhaps something like “The name of a 
non-filter node (allocation-bearing layer) that limits the COR operations in 
the backing chain (inclusive), so that no data below this node will be copied 
by this filter”?

Sounds good for me.


+#          For the block-stream job, it will be the first non-filter overlay of
+#          the base node. We do not involve the base node into the COR
+#          operations because the base may change due to a concurrent
+#          block-commit job on the same backing chain.


I now see that paragraph conflicts with further introduce of "bottom" for 
stream job itself. I think it may be safely dropped. It's a wrong place to describe how 
block-stream works.

I think the default behavior should be mentioned here somewhere, i.e. that no 
limit is applied, so that data from all backing layers may be copied.

agree


+#
+# Since: 5.2

*6.0

+##
+{ 'struct': 'BlockdevOptionsCor',
+  'base': 'BlockdevOptionsGenericFormat',
+  'data': { '*bottom': 'str' } }
+
  ##
  # @BlockdevOptions:
  #

[...]

diff --git a/block/copy-on-read.c b/block/copy-on-read.c
index 618c4c4f43..67f61983c0 100644
--- a/block/copy-on-read.c
+++ b/block/copy-on-read.c

[...]

@@ -51,7 +56,17 @@ static int cor_open(BlockDriverState *bs, QDict *options, 
int flags,
          ((BDRV_REQ_FUA | BDRV_REQ_MAY_UNMAP | BDRV_REQ_NO_FALLBACK) &
              bs->file->bs->supported_zero_flags);
+    if (bottom_node) {
+        bottom_bs = bdrv_lookup_bs(NULL, bottom_node, errp);
+        if (!bottom_bs) {
+            error_setg(errp, "Bottom node '%s' not found", bottom_node);
+            qdict_del(options, "bottom");
+            return -EINVAL;
+        }

Should we verify that bottom_bs is not a filter, as required by the schema?


yes, thanks for the catch!


Hmm.. Interesting, we don't freeze the backing chain in cor filter open. And I 
think we shouldn't. But then, bottom node may disappear. We should handle it 
without a crash.

I suggest:

1. document, that if bottom node disappear from the backing chain of the filter, it 
continues to work like without any specified "bottom" node

2. do bdrv_ref/bdrv_unref of bottom_bs, to not work with dead pointer

3. check in cor_co_preadv_part() is bottom_bs is still in backing chain or not

Hm, right.

Alternatively, we could also freeze the chain until the bottom node and then 
allow changing the @bottom node through reopen.  Then it would have to be 
manually unset before the bottom node is allowed to disappear from the chain.

Would freezing the chain pose a problem?


Hmm. Then we'll just need not freeze it in block-stream, so freezing is done by 
filter.

It's just more restrictive.. But I can't imagine reasonable cases where user specify 
bottom node and than remove it. Forcing user to reopen the filter to change the bottom 
node may be more clean then "just don't care". OK, I think we can freeze the 
chain in the filter.


--
Best regards,
Vladimir



reply via email to

[Prev in Thread] Current Thread [Next in Thread]