Some bs->options fixes

Hi everyone,

this is part of the blockdev-reopen work that I'm doing, but since I'll be away during most of July I thought that I could send already a couple of patches that I think are ready and don't need anything else from the rest of the series.

There's two main fixes here:

1) bs->options are not kept up to date after an image is reopened and no longer reflect its state.

2) bs->options and bs->explicit_options also contain the options of a BDS's children, so there's data that is duplicated and will be inconsistent as soon as you change the children's options directly.

The fix for (2) involves removing all children options from both QDicts. In the cases of node name references ("backing": "node-name") those remain in the QDict (they're technically parent options). I think we don't really need them and it should be possible to get rid of them, but it's a little more complicated (we need them during bdrv_reopen() to ensure that the user didn't try to change any of them).

Regards,

Berto

Alberto Garcia (5):
qdict: Make qdict_extract_subqdict() accept dst = NULL
block: Remove children options from bs->{options,explicit_options}
block: Simplify bdrv_reopen_abort()
block: Update bs->options if bdrv_reopen() succeeds
block: Simplify append_open_options()

 block.c               | 42 +++++++++++++++++++++++++++++++-----------
 qobject/block-qdict.c | 11 ++++++++---
 2 files changed, 39 insertions(+), 14 deletions(-)

Signature · 1 line

2.11.0

If a bdrv_reopen_multiple() call fails, then the explicit_options QDict has to be deleted for every entry in the reopen queue. This must happen regardless of whether that entry's bdrv_reopen_prepare() call succeeded or not.

This patch simplifies the cleanup code a bit.

Signed-off-by Alberto Garcia <[email protected]>

---
 block.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

block.c +4 -3

index 5aaed424b9..48e8f4814c 100644
@@ -3060,9 +3060,10 @@ int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **er
 
 cleanup:
     QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
-        if (ret && bs_entry->prepared) {
-            bdrv_reopen_abort(&bs_entry->state);
-        } else if (ret) {
+        if (ret) {
+            if (bs_entry->prepared) {
+                bdrv_reopen_abort(&bs_entry->state);
+            }
             qobject_unref(bs_entry->state.explicit_options);
         }
         qobject_unref(bs_entry->state.options);

This function returns a BDS's driver-specific options, excluding also those from its children. Since we have just removed all children options from bs->options there's no need to do this last step.

We allow references to children, though ("backing": "node0"), so those we still have to remove.

Signed-off-by Alberto Garcia <[email protected]>

Am 29.06.2018 um 13:37 hat Alberto Garcia geschrieben:

Quoted 34 lines

If a bdrv_reopen_multiple() call fails, then the explicit_options QDict has to be deleted for every entry in the reopen queue. This must happen regardless of whether that entry's bdrv_reopen_prepare() call succeeded or not.

This patch simplifies the cleanup code a bit.

Signed-off-by Alberto Garcia <[email protected]>

---
 block.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

block.c +4 -3

index 5aaed424b9..48e8f4814c 100644
@@ -3060,9 +3060,10 @@ int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **er
 
 cleanup:
     QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
-        if (ret && bs_entry->prepared) {
-            bdrv_reopen_abort(&bs_entry->state);
-        } else if (ret) {
+        if (ret) {
+            if (bs_entry->prepared) {
+                bdrv_reopen_abort(&bs_entry->state);
+            }
             qobject_unref(bs_entry->state.explicit_options);
         }
         qobject_unref(bs_entry->state.options);

@@ -3351,8 +3352,6 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
         drv->bdrv_reopen_abort(reopen_state);
     }
 

block.c void bdrv_reopen_abort(BDRVReopenState *reopen_state) · line 3356

Quoted 4 lines

block.c +0 -2

-    qobject_unref(reopen_state->explicit_options);
-
     bdrv_abort_perm_update(reopen_state->bs);
 }

I think this is only correct if we make bdrv_reopen_prepare/commit/abort private. At the moment they are public interfaces, so we must have thought that some callers might want to integrate them into other transactions.

Am 29.06.2018 um 13:37 hat Alberto Garcia geschrieben:

Quoted 8 lines

This function returns a BDS's driver-specific options, excluding also those from its children. Since we have just removed all children options from bs->options there's no need to do this last step.

We allow references to children, though ("backing": "node0"), so those we still have to remove.

Signed-off-by Alberto Garcia <[email protected]>

Hmm, yes, but if I compare this with the example you gave in an earlier patch:

      $ qemu-img create -f qcow2 hd0.qcow2 10M
      $ qemu-img create -f qcow2 -b hd0.qcow2 hd1.qcow2
      $ qemu-img create -f qcow2 -b hd1.qcow2 hd2.qcow2

      $ $QEMU -drive file=hd2.qcow2,node-name=hd2,backing.node-name=hd1

This opens a chain of images hd0 <- hd1 <- hd2. Now let's remove hd1 using block_stream:

(qemu) block_stream hd2 0 hd0.qcow2

After this hd2 contains backing.node-name=hd1, which is no longer correct because hd1 doesn't exist anymore.

Doesn't backing=hd1 actually result in the same kind of inconsistency? That is, bs->option will still have backing=hd1, but in reality we reference hd0 now?

Should we get rid of child references in bs->(explicit_)options as well?

Quoted 35 lines

Am 29.06.2018 um 13:37 hat Alberto Garcia geschrieben:

Quoted 34 lines

If a bdrv_reopen_multiple() call fails, then the explicit_options QDict has to be deleted for every entry in the reopen queue. This must happen regardless of whether that entry's bdrv_reopen_prepare() call succeeded or not.

This patch simplifies the cleanup code a bit.

Signed-off-by Alberto Garcia <[email protected]>

---
 block.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

block.c +4 -3

index 5aaed424b9..48e8f4814c 100644
@@ -3060,9 +3060,10 @@ int bdrv_reopen_multiple(AioContext *ctx, BlockReopenQueue *bs_queue, Error **er
 
 cleanup:
     QSIMPLEQ_FOREACH_SAFE(bs_entry, bs_queue, entry, next) {
-        if (ret && bs_entry->prepared) {
-            bdrv_reopen_abort(&bs_entry->state);
-        } else if (ret) {
+        if (ret) {
+            if (bs_entry->prepared) {
+                bdrv_reopen_abort(&bs_entry->state);
+            }
             qobject_unref(bs_entry->state.explicit_options);
         }
         qobject_unref(bs_entry->state.options);

@@ -3351,8 +3352,6 @@ void bdrv_reopen_abort(BDRVReopenState *reopen_state)
         drv->bdrv_reopen_abort(reopen_state);
     }
 

Quoted 9 lines

block.c void bdrv_reopen_abort(BDRVReopenState *reopen_state) · line 3356

Quoted 4 lines

block.c +0 -2

-    qobject_unref(reopen_state->explicit_options);
-
     bdrv_abort_perm_update(reopen_state->bs);
 }

I think this is only correct if we make bdrv_reopen_prepare/commit/abort private. At the moment they are public interfaces, so we must have thought that some callers might want to integrate them into other transactions.

But that's not a problem as long as the new callers unref state.explicit_options when bdrv_reopen_prepare() fails. They need to do it with state.options anyway.

Quoted 27 lines

Am 29.06.2018 um 13:37 hat Alberto Garcia geschrieben:

Quoted 8 lines

This function returns a BDS's driver-specific options, excluding also those from its children. Since we have just removed all children options from bs->options there's no need to do this last step.

We allow references to children, though ("backing": "node0"), so those we still have to remove.

Signed-off-by Alberto Garcia <[email protected]>

Hmm, yes, but if I compare this with the example you gave in an earlier patch:

      $ qemu-img create -f qcow2 hd0.qcow2 10M
      $ qemu-img create -f qcow2 -b hd0.qcow2 hd1.qcow2
      $ qemu-img create -f qcow2 -b hd1.qcow2 hd2.qcow2

      $ $QEMU -drive file=hd2.qcow2,node-name=hd2,backing.node-name=hd1

This opens a chain of images hd0 <- hd1 <- hd2. Now let's remove hd1 using block_stream:

(qemu) block_stream hd2 0 hd0.qcow2

After this hd2 contains backing.node-name=hd1, which is no longer correct because hd1 doesn't exist anymore.

Quoted 6 lines

Doesn't backing=hd1 actually result in the same kind of inconsistency? That is, bs->option will still have backing=hd1, but in reality we reference hd0 now?

Should we get rid of child references in bs->(explicit_)options as well?

I don't think so, at least not in general.

The main difference is that if you set backing.node-name=foo then it means that 'node-name=foo' is an option of the child, the option doesn't belong in the parent at all. So once it's transferred to the child there's no reason why it shoud remain in the parent. It's redundant and can interfere with the reopen operation (e.g. you change the option in the child but when you reopen the parent it will try to revert the child option to the previous value).

In the case of 'backing=foo' that's clearly an option of the parent, there's no other place to put it. We could probably remove it as well, but we have to see carefully that no parent needs to keep that information. I think we want to keep child references because in general we don't allow them to be changed in reopen.

Example: you cannot pass 'file=bar' on reopen unless 'bar' was already the existing value of 'file' (i.e. you're not changing anything). We need to look for the previous value in bs->options in order to know that.

After x-blockdev-reopen is ready, 'backing' will perhaps be the first/only exception, because we'll be able to change it and the previous value doesn't matter.

Quoted 7 lines

The main difference is that if you set backing.node-name=foo then it means that 'node-name=foo' is an option of the child, the option doesn't belong in the parent at all. So once it's transferred to the child there's no reason why it shoud remain in the parent. It's redundant and can interfere with the reopen operation (e.g. you change the option in the child but when you reopen the parent it will try to revert the child option to the previous value).

That's a rather reopen-centric point of view, though.

Redundant information isn't nice already, but what really makes it a problem is that it's potentially incorrect information because it isn't kept up to date. There is no point in keeping incorrect information, so I agree that deleting it is best.

Quoted 10 lines

In the case of 'backing=foo' that's clearly an option of the parent, there's no other place to put it. We could probably remove it as well, but we have to see carefully that no parent needs to keep that information. I think we want to keep child references because in general we don't allow them to be changed in reopen.

Example: you cannot pass 'file=bar' on reopen unless 'bar' was already the existing value of 'file' (i.e. you're not changing anything). We need to look for the previous value in bs->options in order to know that.

My point is the backing=foo has the same problem: Storing it in bs->options is not only redundant, but potentially incorrect because we never update it when we modify the graph. There is no point in keeping potentially incorrect information.

When you actually use that incorrect information, you've got a bug. Reopen with file=bar doesn't have to check whether 'file=bar' is in bs->options (because that may be outdated information), but whether the BdrvChild with the name 'file' points to a node called 'bar'.

Quoted 3 lines

After x-blockdev-reopen is ready, 'backing' will perhaps be the first/only exception, because we'll be able to change it and the previous value doesn't matter.

I certainly hope that it will not be the only kind of node references that you can change. Adding/removing filter nodes requires to be able to change more or less any kind of node references. But we'll see.

Quoted 11 lines

Quoted 10 lines

In the case of 'backing=foo' that's clearly an option of the parent, there's no other place to put it. We could probably remove it as well, but we have to see carefully that no parent needs to keep that information. I think we want to keep child references because in general we don't allow them to be changed in reopen.

Example: you cannot pass 'file=bar' on reopen unless 'bar' was already the existing value of 'file' (i.e. you're not changing anything). We need to look for the previous value in bs->options in order to know that.

Quoted 4 lines

My point is the backing=foo has the same problem: Storing it in bs->options is not only redundant, but potentially incorrect because we never update it when we modify the graph. There is no point in keeping potentially incorrect information.

I tend to agree, but there's one reason why it's not exactly the same
case: with "backing=foo" we know not only that the backing node name is 'foo', but that it was added using a reference rather than with backing.node-name=foo. I'm not sure if that's information that we need for anything, however (probably not).

Quoted 4 lines

When you actually use that incorrect information, you've got a bug. Reopen with file=bar doesn't have to check whether 'file=bar' is in bs->options (because that may be outdated information), but whether the BdrvChild with the name 'file' points to a node called 'bar'.

Again, this is correct if we open the BDS with

file.node-name=bar

and then we allow reopening it with

file=bar

Different set of options, but the result still makes sense. For this we need a specific check to verify that this is correct. For the current behavior we don't need anything now: we only allow the exact same option.

Quoted 7 lines

Quoted 3 lines

After x-blockdev-reopen is ready, 'backing' will perhaps be the first/only exception, because we'll be able to change it and the previous value doesn't matter.

I certainly hope that it will not be the only kind of node references that you can change. Adding/removing filter nodes requires to be able to change more or less any kind of node references. But we'll see.

No, but anything that we allow changing has to be done on a case-by-case basis. You can't simply allow changing any child reference, the specific driver has to be modified in order to allow that.

Until the driver is changed, the current behavior prevails: if an option was modified, we return an error.

Quoted 9 lines

Quoted 4 lines

When you actually use that incorrect information, you've got a bug. Reopen with file=bar doesn't have to check whether 'file=bar' is in bs->options (because that may be outdated information), but whether the BdrvChild with the name 'file' points to a node called 'bar'.

Again, this is correct if we open the BDS with

file.node-name=bar

Quoted 8 lines

and then we allow reopening it with

file=bar

Different set of options, but the result still makes sense. For this we need a specific check to verify that this is correct. For the current behavior we don't need anything now: we only allow the exact same option.

That's yet another case and another reason why we want to check BdrvChild instead. If we take BlockdevOptions for blockdev-reopen, you need to put something there for nodes that you created inline originally, and just putting the node name there probably makes the most sense.

Anyway, the case that I had in mind is the case where you removed a backing file with a block job:

base <- mid <- top

You stream top into mid, so at the end of the job, mid goes away and base is the backing file of top. But since you opened top with backing=mid, that's still what you get when you look at bs->options.

base <- top
(backing=mid)

If you now reopen top, and bdrv_reopen looks at bs->options to check whether the operation is valid, you must specify backing=mid instead of the correct backing=base so that reopen thinks it's unchanged.

Quoted 12 lines

Quoted 7 lines

Quoted 3 lines

After x-blockdev-reopen is ready, 'backing' will perhaps be the first/only exception, because we'll be able to change it and the previous value doesn't matter.

I certainly hope that it will not be the only kind of node references that you can change. Adding/removing filter nodes requires to be able to change more or less any kind of node references. But we'll see.

No, but anything that we allow changing has to be done on a case-by-case basis. You can't simply allow changing any child reference, the specific driver has to be modified in order to allow that.

Quoted 2 lines

Until the driver is changed, the current behavior prevails: if an option was modified, we return an error.

Makes sense to err on the safe side, though I expect that most drivers don't need to do much more than just allowing the switch.

Maybe, if we want to keep things a bit safer, what we can do is check that the same node is addressed when you skip all filters.

Hi everyone,

this is part of the blockdev-reopen work that I'm doing, but since I'll be away during most of July I thought that I could send already a couple of patches that I think are ready and don't need anything else from the rest of the series.

There's two main fixes here:

1) bs->options are not kept up to date after an image is reopened and no longer reflect its state.

2) bs->options and bs->explicit_options also contain the options of a BDS's children, so there's data that is duplicated and will be inconsistent as soon as you change the children's options directly.

The fix for (2) involves removing all children options from both QDicts. In the cases of node name references ("backing": "node-name") those remain in the QDict (they're technically parent options). I think we don't really need them and it should be possible to get rid of them, but it's a little more complicated (we need them during bdrv_reopen() to ensure that the user didn't try to change any of them).