qemu-devel
[Top][All Lists]
Advanced

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

Re: [PATCH v15 09/13] stream: rework backing-file changing


From: Max Reitz
Subject: Re: [PATCH v15 09/13] stream: rework backing-file changing
Date: Tue, 22 Dec 2020 16:59:40 +0100
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.5.0

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

Stream in stream_prepare calls bdrv_change_backing_file() to change
backing-file in the metadata of bs.

It may use either backing-file parameter given by user or just take
filename of base on job start.

Backing file format is determined by base on job finish.

There are some problems with this design, we solve only two by this
patch:

1. Consider scenario with backing-file unset. Current concept of stream
supports changing of the base during the job (we don't freeze link to
the base). So, we should not save base filename at job start,

   - let's determine name of the base on job finish.

2. Using direct base to determine filename and format is not very good:
base node may be a filter, so its filename may be JSON, and format_name
is not good for storing into qcow2 metadata as backing file format.

   - let's use unfiltered_base

Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
   [vsementsov: change commit subject, change logic in stream_prepare]
---
  block/stream.c | 9 +++++----
  blockdev.c     | 8 +-------
  2 files changed, 6 insertions(+), 11 deletions(-)

diff --git a/block/stream.c b/block/stream.c
index 6e281c71ac..6a525a5edf 100644
--- a/block/stream.c
+++ b/block/stream.c

[...]

@@ -73,10 +74,10 @@ static int stream_prepare(Job *job)
if (bdrv_cow_child(unfiltered_bs)) {
          const char *base_id = NULL, *base_fmt = NULL;
-        if (base) {
-            base_id = s->backing_file_str;
-            if (base->drv) {
-                base_fmt = base->drv->format_name;
+        if (unfiltered_base) {
+            base_id = s->backing_file_str ?: unfiltered_base->filename;
+            if (unfiltered_base->drv) {
+                base_fmt = unfiltered_base->drv->format_name;
              }
          }
          bdrv_set_backing_hd(unfiltered_bs, base, &local_err);

I think I preferred the v14 behavior of not setting a backing file format if backing_file_str is nowhere to be found in the current backing chain. (I just noticed, I had a typo in my reply to v14, though; the “continuing on with setting a backing_fmt” should have read “continuing on *without* setting a backing_fmt”...)

Anyway, this is still an improvement on the pre-patch behavior, so:

Reviewed-by: Max Reitz <mreitz@redhat.com>

(And as we discussed, the best would be for the user to specify a backing format through a yet-to-be-added option.)




reply via email to

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