| Message ID | 624533945b182710437b6ed04595bf560c1d9f5f.1551895814.git.berto@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add a 'x-blockdev-reopen' QMP command | expand |
06.03.2019 21:11, Alberto Garcia wrote: > Our permission system is useful to define what operations are allowed > on a certain block node and includes things like BLK_PERM_WRITE or > BLK_PERM_RESIZE among others. > > One of the permissions is BLK_PERM_GRAPH_MOD which allows "changing > the node that this BdrvChild points to". The exact meaning of this has > never been very clear, but it can be understood as "change any of the > links connected to the node". This can be used to prevent changing a > backing link, but it's too coarse. > > This patch adds a new 'frozen' attribute to BdrvChild, which forbids > detaching the link from the node it points to, and new API to freeze > and unfreeze a backing chain. > > After this change a few functions can fail, so they need additional > checks. > > Signed-off-by: Alberto Garcia <berto@igalia.com> > --- > block.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++ > include/block/block.h | 5 ++++ > include/block/block_int.h | 6 ++++ > 3 files changed, 87 insertions(+) > > diff --git a/block.c b/block.c > index 35e78e2172..6e9c72f0cd 100644 > --- a/block.c > +++ b/block.c > @@ -2127,6 +2127,8 @@ static void bdrv_replace_child_noperm(BdrvChild *child, > BlockDriverState *old_bs = child->bs; > int i; > > + assert(!child->frozen); > + > if (old_bs && new_bs) { > assert(bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs)); > } > @@ -2343,6 +2345,10 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd, > bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) && > bdrv_inherits_from_recursive(backing_hd, bs); > > + if (bdrv_is_backing_chain_frozen(bs, backing_bs(bs), errp)) { > + return; > + } > + > if (backing_hd) { > bdrv_ref(backing_hd); > } > @@ -3712,6 +3718,11 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, > if (!should_update_child(c, to)) { > continue; > } > + if (c->frozen) { > + error_setg(errp, "Cannot change '%s' link to '%s'", > + c->name, from->node_name); > + goto out; > + } > list = g_slist_prepend(list, c); > perm |= c->perm; > shared &= c->shared_perm; > @@ -3924,6 +3935,63 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs) > } > > /* > + * Return true if at least one of the backing links between @bs and > + * @base is frozen. @errp is set if that's the case. > + */ > +bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base, > + Error **errp) > +{ > + BlockDriverState *i; > + > + for (i = bs; i != base && i->backing; i = backing_bs(i)) { > + if (i->backing->frozen) { > + error_setg(errp, "Cannot change '%s' link from '%s' to '%s'", > + i->backing->name, i->node_name, > + backing_bs(i)->node_name); > + return true; > + } > + } > + > + return false; > +} > + > +/* > + * Freeze all backing links between @bs and @base. > + * If any of the links is already frozen the operation is aborted and > + * none of the links are modified. > + * Returns 0 on success. On failure returns < 0 and sets @errp. > + */ > +int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, > + Error **errp) > +{ > + BlockDriverState *i; > + > + if (bdrv_is_backing_chain_frozen(bs, base, errp)) { > + return -EPERM; > + } > + > + for (i = bs; i != base && i->backing; i = backing_bs(i)) { > + i->backing->frozen = true; > + } > + > + return 0; > +} > + > +/* > + * Unfreeze all backing links between @bs and @base. The caller must > + * ensure that all links are frozen before using this function. > + */ > +void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base) > +{ > + BlockDriverState *i; > + > + for (i = bs; i != base && i->backing; i = backing_bs(i)) { > + assert(i->backing->frozen); > + i->backing->frozen = false; > + } > +} > + > +/* > * Drops images above 'base' up to and including 'top', and sets the image > * above 'top' to have base as its backing file. > * > @@ -3972,6 +4040,14 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, > goto exit; > } > > + /* This function changes all links that point to top and makes > + * them point to base. Check that none of them is frozen. */ > + QLIST_FOREACH(c, &top->parents, next_parent) { > + if (c->frozen) { > + goto exit; > + } > + } > + > /* If 'base' recursively inherits from 'top' then we should set > * base->inherits_from to top->inherits_from after 'top' and all > * other intermediate nodes have been dropped. Hmm, looking at this I have a bit unrelated questions about bdrv_drop_intermediate: 1. if we fail on some iteration of QLIST_FOREACH_SAFE loop, we finish up with partly updated graph?? I don't see any kind of roll-back in this case.. Why it don't operate like bdrv_replace_node, which firstly check all permissions, and then update all parents in fail-free loop? 2. And therefore, why we just don't call bdrv_replace_node(top, base, errp) from bdrv_drop_intermediate? 3. About updating inherits_from: is it possible for some reason that base->inherits_from points to some node inside removed backing chain, but bdrv_inherits_from_recursive(base, explicit_top) is false? In this case we'll finish with dead-pointer in base->inherits_from. 4. Should bdrv_replace_node do something around inherits_from? > diff --git a/include/block/block.h b/include/block/block.h > index 5b5cf868df..e4a25674b1 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -353,6 +353,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, > BlockDriverState *bdrv_find_overlay(BlockDriverState *active, > BlockDriverState *bs); > BlockDriverState *bdrv_find_base(BlockDriverState *bs); > +bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base, > + Error **errp); > +int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, > + Error **errp); > +void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base); > > > typedef struct BdrvCheckResult { > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 836d67c1ae..c8a83c3eea 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -709,6 +709,12 @@ struct BdrvChild { > uint64_t backup_perm; > uint64_t backup_shared_perm; > > + /* > + * This link is frozen: the child can neither be replaced nor > + * detached from the parent. > + */ > + bool frozen; > + > QLIST_ENTRY(BdrvChild) next; > QLIST_ENTRY(BdrvChild) next_parent; > }; > Hmm, also, I have a related reproducer, which lead to crash, when use stream base as commit top: #3 __GI___assert_fail (assertion=0x55fbd2218818 "bdrv_op_blocker_is_empty(bs)", file=0x55fbd22175e0 "block.c", line=3795, function=0x55fbd2219422 <__PRETTY_FUNCTION__.31496> "bdrv_delete") at assert.c:101 #4 bdrv_delete (bs=0x55fbd4593cf0) at block.c:3795 #5 bdrv_unref (bs=0x55fbd4593cf0) at block.c:5051 #6 bdrv_drop_intermediate (top=0x55fbd4593cf0, base=0x55fbd3b36260, backing_file_str=0x55fbd3b36291 "img0") at block.c:4034 #7 commit_prepare (job=0x55fbd4612000) at block/commit.c:78 #8 job_prepare (job=0x55fbd4612000) at job.c:771 #9 job_txn_apply (txn=0x55fbd4359a50, fn=0x55fbd1eae5c7 <job_prepare>) at job.c:146 #10 job_do_finalize (job=0x55fbd4612000) at job.c:788 to reproduce: qemu-img create -f qcow2 img0 $size; for i in {1..4}; do qemu-img create -f qcow2 -b img$((i-1)) img$i $size; done for i in {1,2}; do qemu-io -f qcow2 -c "write 0 $size" img$i; done ./x86_64-softmmu/qemu-system-x86_64 -nographic -qmp-pretty stdio -nodefaults -serial none -drive file=img4,node-name=img4,backing.node-name=img3,backing.backing.node-name=img2,backing.backing.backing.node-name=img1,backing.backing.backing.backing.node-name=img0 and copy the following input: { "execute": "qmp_capabilities" } {"execute": "block-commit", "arguments": {"device": "img4", "top-node": "img1", "job-id": "commit0", "speed": 1000000, "base-node": "img0"}} {"execute": "block-stream", "arguments": {"device": "img3", "job-id": "stream0", "speed": 500000, "base-node": "img1"}} after a bit waiting, I get qemu-system-x86_64: block.c:3795: bdrv_delete: Assertion `bdrv_op_blocker_is_empty(bs)' failed. Aborted (core dumped) But with your patches 01-04 applied, this reproducer gives { "error": { "class": "GenericError", "desc": "Cannot change 'backing' link from '#block507' to 'img1'" } } somewhere, and don't crash!! Could we consider it as a reason to apply 01-04 (may be with some updates, I didn't review them carefully yet) to v4.0 as a bug fix? -------- Also, related. We've faced a lot of related problems, trying to implement top-filter for stream job. It lead to failures in 30 iotest which want use parallel stream jobs, sharing a node to be base for one stream and top for another. It also touches the fact that stream stores pointer to base, but don't increase its refcount, so it's illegal pointer. And with your series we at least freeze backing child which points to base. But copy-on-read filter which is going to be used as stream top is .file child based, not .backing. So, it will not work. May be freeze_backing_chain should be (not now, but may be later) updated to handle such filters, may be we should support backing child in copy-on-read...
On Tue 12 Mar 2019 05:12:53 PM CET, Vladimir Sementsov-Ogievskiy wrote: >> + /* This function changes all links that point to top and makes >> + * them point to base. Check that none of them is frozen. */ >> + QLIST_FOREACH(c, &top->parents, next_parent) { >> + if (c->frozen) { >> + goto exit; >> + } >> + } >> + > Hmm, looking at this I have a bit unrelated questions about > bdrv_drop_intermediate: > > 1. if we fail on some iteration of QLIST_FOREACH_SAFE loop, we finish > up with partly updated graph?? > I don't see any kind of roll-back in this case.. Why it don't > operate like bdrv_replace_node, which firstly check all > permissions, and then update all parents in fail-free loop? I think you're right. If you don't have something written already I could try to do it. > 2. And therefore, why we just don't call bdrv_replace_node(top, base, > errp) from bdrv_drop_intermediate? I think that would not call role->update_filename(). > 3. About updating inherits_from: is it possible for some reason that > base->inherits_from points to some node inside removed backing chain, > but bdrv_inherits_from_recursive(base, explicit_top) is false? In this > case we'll finish with dead-pointer in base->inherits_from. I'm not sure about that particular case, but I realize that if base->inherits_from is a dead pointer then this can crash bdrv_inherits_from_recursive(). This would not happen before because inherits_from was only used in comparisons and never dereferenced. > 4. Should bdrv_replace_node do something around inherits_from? Probably, yes. > Hmm, also, I have a related reproducer, which lead to crash, when use > stream base as commit top: [...] > But with your patches 01-04 applied, this reproducer gives > > { > "error": { > "class": "GenericError", > "desc": "Cannot change 'backing' link from '#block507' to 'img1'" > } > } > > somewhere, and don't crash!! > > Could we consider it as a reason to apply 01-04 (may be with some > updates, I didn't review them carefully yet) to v4.0 as a bug fix? It seems that there's a merge request for the whole series already :) > Also, related. We've faced a lot of related problems, trying to > implement top-filter for stream job. It lead to failures in 30 iotest > which want use parallel stream jobs, sharing a node to be base for one > stream and top for another. Yeah I imagine that that iotest needs a few changes if we block the base node. > It also touches the fact that stream stores pointer to base, but don't > increase its refcount, so it's illegal pointer. You mean in the StreamBlockJob structure? What's the problem exactly? Berto
Am 13.03.2019 um 15:14 hat Alberto Garcia geschrieben: > On Tue 12 Mar 2019 05:12:53 PM CET, Vladimir Sementsov-Ogievskiy wrote: > >> + /* This function changes all links that point to top and makes > >> + * them point to base. Check that none of them is frozen. */ > >> + QLIST_FOREACH(c, &top->parents, next_parent) { > >> + if (c->frozen) { > >> + goto exit; > >> + } > >> + } > >> + > > > Hmm, looking at this I have a bit unrelated questions about > > bdrv_drop_intermediate: > > > > 1. if we fail on some iteration of QLIST_FOREACH_SAFE loop, we finish > > up with partly updated graph?? > > > I don't see any kind of roll-back in this case.. Why it don't > > operate like bdrv_replace_node, which firstly check all > > permissions, and then update all parents in fail-free loop? > > I think you're right. If you don't have something written already I > could try to do it. > > > 2. And therefore, why we just don't call bdrv_replace_node(top, base, > > errp) from bdrv_drop_intermediate? > > I think that would not call role->update_filename(). And role->update_filename() involves I/O, so you can't roll back across it anyway. I think this was the reason why we didn't roll back the in-memory state either, it would become inconsistent with what is on the disk. Kevin
On Wed 13 Mar 2019 03:29:36 PM CET, Kevin Wolf wrote: > Am 13.03.2019 um 15:14 hat Alberto Garcia geschrieben: >> On Tue 12 Mar 2019 05:12:53 PM CET, Vladimir Sementsov-Ogievskiy wrote: >> >> + /* This function changes all links that point to top and makes >> >> + * them point to base. Check that none of them is frozen. */ >> >> + QLIST_FOREACH(c, &top->parents, next_parent) { >> >> + if (c->frozen) { >> >> + goto exit; >> >> + } >> >> + } >> >> + >> >> > Hmm, looking at this I have a bit unrelated questions about >> > bdrv_drop_intermediate: >> > >> > 1. if we fail on some iteration of QLIST_FOREACH_SAFE loop, we finish >> > up with partly updated graph?? >> >> > I don't see any kind of roll-back in this case.. Why it don't >> > operate like bdrv_replace_node, which firstly check all >> > permissions, and then update all parents in fail-free loop? >> >> I think you're right. If you don't have something written already I >> could try to do it. >> >> > 2. And therefore, why we just don't call bdrv_replace_node(top, base, >> > errp) from bdrv_drop_intermediate? >> >> I think that would not call role->update_filename(). > > And role->update_filename() involves I/O, so you can't roll back > across it anyway. I think this was the reason why we didn't roll back > the in-memory state either, it would become inconsistent with what is > on the disk. But I think at least for the bdrv_check_update_perm() case we should call bdrv_abort_perm_update() on all nodes if things fail, and we're currently not doing it. Berto
13.03.2019 17:14, Alberto Garcia wrote: > On Tue 12 Mar 2019 05:12:53 PM CET, Vladimir Sementsov-Ogievskiy wrote: >>> + /* This function changes all links that point to top and makes >>> + * them point to base. Check that none of them is frozen. */ >>> + QLIST_FOREACH(c, &top->parents, next_parent) { >>> + if (c->frozen) { >>> + goto exit; >>> + } >>> + } >>> + > >> Hmm, looking at this I have a bit unrelated questions about >> bdrv_drop_intermediate: >> >> 1. if we fail on some iteration of QLIST_FOREACH_SAFE loop, we finish >> up with partly updated graph?? > >> I don't see any kind of roll-back in this case.. Why it don't >> operate like bdrv_replace_node, which firstly check all >> permissions, and then update all parents in fail-free loop? > > I think you're right. If you don't have something written already I > could try to do it. No, I don't. > >> 2. And therefore, why we just don't call bdrv_replace_node(top, base, >> errp) from bdrv_drop_intermediate? > > I think that would not call role->update_filename(). Honestly, I'm lost about filenames in the graph.. > >> 3. About updating inherits_from: is it possible for some reason that >> base->inherits_from points to some node inside removed backing chain, >> but bdrv_inherits_from_recursive(base, explicit_top) is false? In this >> case we'll finish with dead-pointer in base->inherits_from. > > I'm not sure about that particular case, but I realize that if > base->inherits_from is a dead pointer then this can crash > bdrv_inherits_from_recursive(). This would not happen before because > inherits_from was only used in comparisons and never dereferenced. > >> 4. Should bdrv_replace_node do something around inherits_from? > > Probably, yes. > >> Hmm, also, I have a related reproducer, which lead to crash, when use >> stream base as commit top: > [...] >> But with your patches 01-04 applied, this reproducer gives >> >> { >> "error": { >> "class": "GenericError", >> "desc": "Cannot change 'backing' link from '#block507' to 'img1'" >> } >> } >> >> somewhere, and don't crash!! >> >> Could we consider it as a reason to apply 01-04 (may be with some >> updates, I didn't review them carefully yet) to v4.0 as a bug fix? > > It seems that there's a merge request for the whole series already :) Yes and that's cool) > >> Also, related. We've faced a lot of related problems, trying to >> implement top-filter for stream job. It lead to failures in 30 iotest >> which want use parallel stream jobs, sharing a node to be base for one >> stream and top for another. > > Yeah I imagine that that iotest needs a few changes if we block the base > node. > >> It also touches the fact that stream stores pointer to base, but don't >> increase its refcount, so it's illegal pointer. > > You mean in the StreamBlockJob structure? What's the problem exactly? I hope it anyway should be handled be freeze
On Wed 13 Mar 2019 03:32:09 PM CET, Alberto Garcia wrote: >>> > 2. And therefore, why we just don't call bdrv_replace_node(top, >>> > base, errp) from bdrv_drop_intermediate? >>> >>> I think that would not call role->update_filename(). >> >> And role->update_filename() involves I/O, so you can't roll back >> across it anyway. I think this was the reason why we didn't roll back >> the in-memory state either, it would become inconsistent with what is >> on the disk. > > But I think at least for the bdrv_check_update_perm() case we should > call bdrv_abort_perm_update() on all nodes if things fail, and we're > currently not doing it. Forget this, bdrv_replace_child() already sets the new permissions after every iteration of the loop, so the only place that needs to call bdrv_abort_perm_update() is where we are doing it already. Berto
diff --git a/block.c b/block.c index 35e78e2172..6e9c72f0cd 100644 --- a/block.c +++ b/block.c @@ -2127,6 +2127,8 @@ static void bdrv_replace_child_noperm(BdrvChild *child, BlockDriverState *old_bs = child->bs; int i; + assert(!child->frozen); + if (old_bs && new_bs) { assert(bdrv_get_aio_context(old_bs) == bdrv_get_aio_context(new_bs)); } @@ -2343,6 +2345,10 @@ void bdrv_set_backing_hd(BlockDriverState *bs, BlockDriverState *backing_hd, bool update_inherits_from = bdrv_chain_contains(bs, backing_hd) && bdrv_inherits_from_recursive(backing_hd, bs); + if (bdrv_is_backing_chain_frozen(bs, backing_bs(bs), errp)) { + return; + } + if (backing_hd) { bdrv_ref(backing_hd); } @@ -3712,6 +3718,11 @@ void bdrv_replace_node(BlockDriverState *from, BlockDriverState *to, if (!should_update_child(c, to)) { continue; } + if (c->frozen) { + error_setg(errp, "Cannot change '%s' link to '%s'", + c->name, from->node_name); + goto out; + } list = g_slist_prepend(list, c); perm |= c->perm; shared &= c->shared_perm; @@ -3924,6 +3935,63 @@ BlockDriverState *bdrv_find_base(BlockDriverState *bs) } /* + * Return true if at least one of the backing links between @bs and + * @base is frozen. @errp is set if that's the case. + */ +bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base, + Error **errp) +{ + BlockDriverState *i; + + for (i = bs; i != base && i->backing; i = backing_bs(i)) { + if (i->backing->frozen) { + error_setg(errp, "Cannot change '%s' link from '%s' to '%s'", + i->backing->name, i->node_name, + backing_bs(i)->node_name); + return true; + } + } + + return false; +} + +/* + * Freeze all backing links between @bs and @base. + * If any of the links is already frozen the operation is aborted and + * none of the links are modified. + * Returns 0 on success. On failure returns < 0 and sets @errp. + */ +int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, + Error **errp) +{ + BlockDriverState *i; + + if (bdrv_is_backing_chain_frozen(bs, base, errp)) { + return -EPERM; + } + + for (i = bs; i != base && i->backing; i = backing_bs(i)) { + i->backing->frozen = true; + } + + return 0; +} + +/* + * Unfreeze all backing links between @bs and @base. The caller must + * ensure that all links are frozen before using this function. + */ +void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base) +{ + BlockDriverState *i; + + for (i = bs; i != base && i->backing; i = backing_bs(i)) { + assert(i->backing->frozen); + i->backing->frozen = false; + } +} + +/* * Drops images above 'base' up to and including 'top', and sets the image * above 'top' to have base as its backing file. * @@ -3972,6 +4040,14 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, goto exit; } + /* This function changes all links that point to top and makes + * them point to base. Check that none of them is frozen. */ + QLIST_FOREACH(c, &top->parents, next_parent) { + if (c->frozen) { + goto exit; + } + } + /* If 'base' recursively inherits from 'top' then we should set * base->inherits_from to top->inherits_from after 'top' and all * other intermediate nodes have been dropped. diff --git a/include/block/block.h b/include/block/block.h index 5b5cf868df..e4a25674b1 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -353,6 +353,11 @@ int bdrv_drop_intermediate(BlockDriverState *top, BlockDriverState *base, BlockDriverState *bdrv_find_overlay(BlockDriverState *active, BlockDriverState *bs); BlockDriverState *bdrv_find_base(BlockDriverState *bs); +bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base, + Error **errp); +int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base, + Error **errp); +void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base); typedef struct BdrvCheckResult { diff --git a/include/block/block_int.h b/include/block/block_int.h index 836d67c1ae..c8a83c3eea 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -709,6 +709,12 @@ struct BdrvChild { uint64_t backup_perm; uint64_t backup_shared_perm; + /* + * This link is frozen: the child can neither be replaced nor + * detached from the parent. + */ + bool frozen; + QLIST_ENTRY(BdrvChild) next; QLIST_ENTRY(BdrvChild) next_parent; };
Our permission system is useful to define what operations are allowed on a certain block node and includes things like BLK_PERM_WRITE or BLK_PERM_RESIZE among others. One of the permissions is BLK_PERM_GRAPH_MOD which allows "changing the node that this BdrvChild points to". The exact meaning of this has never been very clear, but it can be understood as "change any of the links connected to the node". This can be used to prevent changing a backing link, but it's too coarse. This patch adds a new 'frozen' attribute to BdrvChild, which forbids detaching the link from the node it points to, and new API to freeze and unfreeze a backing chain. After this change a few functions can fail, so they need additional checks. Signed-off-by: Alberto Garcia <berto@igalia.com> --- block.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++ include/block/block.h | 5 ++++ include/block/block_int.h | 6 ++++ 3 files changed, 87 insertions(+)