| Message ID | 1455615450-15138-2-git-send-email-xiecl.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Sorry that I wasn't so pedantic last time; or maybe I should rather be sorry that I'm so pedantic this time. On 16.02.2016 10:37, Changlong Xie wrote: > From: Wen Congyang <wency@cn.fujitsu.com> > > In some cases, we want to take a quorum child offline, and take > another child online. > > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> > Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> > Signed-off-by: Gonglei <arei.gonglei@huawei.com> > Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com> > Reviewed-by: Eric Blake <eblake@redhat.com> > Reviewed-by: Alberto Garcia <berto@igalia.com> > --- > block.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++ > include/block/block.h | 5 +++++ > include/block/block_int.h | 5 +++++ > 3 files changed, 60 insertions(+) > > diff --git a/block.c b/block.c > index efc3c43..08aa979 100644 > --- a/block.c > +++ b/block.c > @@ -4399,3 +4399,53 @@ void bdrv_refresh_filename(BlockDriverState *bs) > QDECREF(json); > } > } > + > +/* > + * Hot add/remove a BDS's child. So the user can take a child offline when > + * it is broken and take a new child online > + */ > +void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs, > + Error **errp) > +{ > + > + if (!parent_bs->drv || !parent_bs->drv->bdrv_add_child) { > + error_setg(errp, "The node %s doesn't support adding a child", As I said in my reply to v9's patch 3 (and I see you've followed it), I don't quite like contractions in error messages, so I'd rather like this to be "does not" instead of "doesn't". If you don't decide to change this patch, then feel free to leave this as it is, because that way you can keep Eric's and Berto's R-bs. > + bdrv_get_device_or_node_name(parent_bs)); > + return; > + } > + > + if (!QLIST_EMPTY(&child_bs->parents)) { > + error_setg(errp, "The node %s already has a parent", > + child_bs->node_name); > + return; > + } > + > + parent_bs->drv->bdrv_add_child(parent_bs, child_bs, errp); > +} > + > +void bdrv_del_child(BlockDriverState *parent_bs, BlockDriverState *child_bs, > + Error **errp) I wondered before and now I'm wondering why I didn't say anything. Why is this function taking a BDS pointer as child_bs? Why not just "BdrvChild *child"? Its only caller will be qmp_x_blockdev_change() which gets the child BDS from bdrv_find_child(), which could just as well return a BdrvChild instead of a BDS pointer. I see two advantages to this: First, it's a bit clearer since this operation actually does not delete the child *BDS* but only the *edge* between parent and child, and that edge is what a BdrvChild is. And second, some block drivers may prefer the BdrvChild over the BDS itself. They can always trivially get the BDS from the BdrvChild structure, but the other way around is difficult. The only disadvantage I can see is that it then becomes asymmetric to bdrv_add_child(). But that's fine, it's a different function after all. bdrv_add_child() creates a BdrvChild object (an edge), bdrv_del_child() deletes it. (I don't know whether you had this discussion with someone else before, though. Sorry if you did, I missed it, then.) > +{ > + BdrvChild *child; > + > + if (!parent_bs->drv || !parent_bs->drv->bdrv_del_child) { > + error_setg(errp, "The node %s doesn't support removing a child", > + bdrv_get_device_or_node_name(parent_bs)); Again, optional s/doesn't/does not/. > + return; > + } > + > + QLIST_FOREACH(child, &parent_bs->children, next) { > + if (child->bs == child_bs) { > + break; > + } > + } > + > + if (!child) { > + error_setg(errp, "The node %s is not a child of %s", > + bdrv_get_device_or_node_name(child_bs), > + bdrv_get_device_or_node_name(parent_bs)); Is there a special reason why you are using bdrv_get_device_or_node_name() for the child here, while in bdrv_add_child() you directly use the node name? Neither seems wrong to me. A child is unlikely to have a BB of its own, but if it does, printing its name instead of the node name may be appropriate. I'm just wondering about the difference between bdrv_add_child() and bdrv_del_child(). Max > + return; > + } > + > + parent_bs->drv->bdrv_del_child(parent_bs, child_bs, errp); > +} > diff --git a/include/block/block.h b/include/block/block.h > index 1c4f4d8..ecde190 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -582,4 +582,9 @@ void bdrv_drained_begin(BlockDriverState *bs); > */ > void bdrv_drained_end(BlockDriverState *bs); > > +void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, > + Error **errp); > +void bdrv_del_child(BlockDriverState *parent, BlockDriverState *child, > + Error **errp); > + > #endif > diff --git a/include/block/block_int.h b/include/block/block_int.h > index 9ef823a..89ec138 100644 > --- a/include/block/block_int.h > +++ b/include/block/block_int.h > @@ -305,6 +305,11 @@ struct BlockDriver { > */ > void (*bdrv_drain)(BlockDriverState *bs); > > + void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child, > + Error **errp); > + void (*bdrv_del_child)(BlockDriverState *parent, BlockDriverState *child, > + Error **errp); > + > QLIST_ENTRY(BlockDriver) list; > }; > >
On 03/06/2016 01:27 AM, Max Reitz wrote: > Sorry that I wasn't so pedantic last time; or maybe I should rather be > sorry that I'm so pedantic this time. Hi Max Welcome all your comments : ) > > On 16.02.2016 10:37, Changlong Xie wrote: >> From: Wen Congyang <wency@cn.fujitsu.com> >> >> In some cases, we want to take a quorum child offline, and take >> another child online. >> >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com> >> Reviewed-by: Eric Blake <eblake@redhat.com> >> Reviewed-by: Alberto Garcia <berto@igalia.com> >> --- >> block.c | 50 +++++++++++++++++++++++++++++++++++++++++++++++ >> include/block/block.h | 5 +++++ >> include/block/block_int.h | 5 +++++ >> 3 files changed, 60 insertions(+) >> >> diff --git a/block.c b/block.c >> index efc3c43..08aa979 100644 >> --- a/block.c >> +++ b/block.c >> @@ -4399,3 +4399,53 @@ void bdrv_refresh_filename(BlockDriverState *bs) >> QDECREF(json); >> } >> } >> + >> +/* >> + * Hot add/remove a BDS's child. So the user can take a child offline when >> + * it is broken and take a new child online >> + */ >> +void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs, >> + Error **errp) >> +{ >> + >> + if (!parent_bs->drv || !parent_bs->drv->bdrv_add_child) { >> + error_setg(errp, "The node %s doesn't support adding a child", > > As I said in my reply to v9's patch 3 (and I see you've followed it), I > don't quite like contractions in error messages, so I'd rather like this > to be "does not" instead of "doesn't". Okay > > If you don't decide to change this patch, then feel free to leave this > as it is, because that way you can keep Eric's and Berto's R-bs. > >> + bdrv_get_device_or_node_name(parent_bs)); >> + return; >> + } >> + >> + if (!QLIST_EMPTY(&child_bs->parents)) { >> + error_setg(errp, "The node %s already has a parent", >> + child_bs->node_name); >> + return; >> + } >> + >> + parent_bs->drv->bdrv_add_child(parent_bs, child_bs, errp); >> +} >> + >> +void bdrv_del_child(BlockDriverState *parent_bs, BlockDriverState *child_bs, >> + Error **errp) > > I wondered before and now I'm wondering why I didn't say anything. Why > is this function taking a BDS pointer as child_bs? Why not just > "BdrvChild *child"? Its only caller will be qmp_x_blockdev_change() > which gets the child BDS from bdrv_find_child(), which could just as > well return a BdrvChild instead of a BDS pointer. > > I see two advantages to this: First, it's a bit clearer since this > operation actually does not delete the child *BDS* but only the *edge* > between parent and child, and that edge is what a BdrvChild is. That's convincing. > > And second, some block drivers may prefer the BdrvChild over the BDS > itself. They can always trivially get the BDS from the BdrvChild > structure, but the other way around is difficult. > > The only disadvantage I can see is that it then becomes asymmetric to > bdrv_add_child(). But that's fine, it's a different function after all. As you said, they are different fuctions at all. So i don't think it's a big deal. > bdrv_add_child() creates a BdrvChild object (an edge), bdrv_del_child() > deletes it. > > (I don't know whether you had this discussion with someone else before, > though. Sorry if you did, I missed it, then.) > >> +{ >> + BdrvChild *child; >> + >> + if (!parent_bs->drv || !parent_bs->drv->bdrv_del_child) { >> + error_setg(errp, "The node %s doesn't support removing a child", >> + bdrv_get_device_or_node_name(parent_bs)); > > Again, optional s/doesn't/does not/. okay > >> + return; >> + } >> + >> + QLIST_FOREACH(child, &parent_bs->children, next) { >> + if (child->bs == child_bs) { >> + break; >> + } >> + } >> + >> + if (!child) { >> + error_setg(errp, "The node %s is not a child of %s", >> + bdrv_get_device_or_node_name(child_bs), >> + bdrv_get_device_or_node_name(parent_bs)); > > Is there a special reason why you are using > bdrv_get_device_or_node_name() for the child here, while in > bdrv_add_child() you directly use the node name? > Although we directly use the node name in bdrv_add_child(), but we still need treat bdrv_del_child() as a separate function, right? In up condition, we can't determine if child->bs supports BB or not, so i think bdrv_get_device_or_node_name(child->bs) is ok here. > Neither seems wrong to me. A child is unlikely to have a BB of its own, > but if it does, printing its name instead of the node name may be bdrv_get_device_or_node_name() can do that. Thanks -Xie > appropriate. I'm just wondering about the difference between > bdrv_add_child() and bdrv_del_child(). > > Max > >> + return; >> + } >> + >> + parent_bs->drv->bdrv_del_child(parent_bs, child_bs, errp); >> +} >> diff --git a/include/block/block.h b/include/block/block.h >> index 1c4f4d8..ecde190 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -582,4 +582,9 @@ void bdrv_drained_begin(BlockDriverState *bs); >> */ >> void bdrv_drained_end(BlockDriverState *bs); >> >> +void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, >> + Error **errp); >> +void bdrv_del_child(BlockDriverState *parent, BlockDriverState *child, >> + Error **errp); >> + >> #endif >> diff --git a/include/block/block_int.h b/include/block/block_int.h >> index 9ef823a..89ec138 100644 >> --- a/include/block/block_int.h >> +++ b/include/block/block_int.h >> @@ -305,6 +305,11 @@ struct BlockDriver { >> */ >> void (*bdrv_drain)(BlockDriverState *bs); >> >> + void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child, >> + Error **errp); >> + void (*bdrv_del_child)(BlockDriverState *parent, BlockDriverState *child, >> + Error **errp); >> + >> QLIST_ENTRY(BlockDriver) list; >> }; >> >> > >
On 07.03.2016 05:16, Changlong Xie wrote: > On 03/06/2016 01:27 AM, Max Reitz wrote: >> Sorry that I wasn't so pedantic last time; or maybe I should rather be >> sorry that I'm so pedantic this time. > > Hi Max > Welcome all your comments : ) Good :-) >> On 16.02.2016 10:37, Changlong Xie wrote: >>> From: Wen Congyang <wency@cn.fujitsu.com> >>> >>> In some cases, we want to take a quorum child offline, and take >>> another child online. >>> >>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com> >>> Signed-off-by: Gonglei <arei.gonglei@huawei.com> >>> Signed-off-by: Changlong Xie <xiecl.fnst@cn.fujitsu.com> >>> Reviewed-by: Eric Blake <eblake@redhat.com> >>> Reviewed-by: Alberto Garcia <berto@igalia.com> >>> --- >>> block.c | 50 >>> +++++++++++++++++++++++++++++++++++++++++++++++ >>> include/block/block.h | 5 +++++ >>> include/block/block_int.h | 5 +++++ >>> 3 files changed, 60 insertions(+) >>> >>> diff --git a/block.c b/block.c >>> index efc3c43..08aa979 100644 >>> --- a/block.c >>> +++ b/block.c [...] >>> + return; >>> + } >>> + >>> + QLIST_FOREACH(child, &parent_bs->children, next) { >>> + if (child->bs == child_bs) { >>> + break; >>> + } >>> + } >>> + >>> + if (!child) { >>> + error_setg(errp, "The node %s is not a child of %s", >>> + bdrv_get_device_or_node_name(child_bs), >>> + bdrv_get_device_or_node_name(parent_bs)); >> >> Is there a special reason why you are using >> bdrv_get_device_or_node_name() for the child here, while in >> bdrv_add_child() you directly use the node name? >> > > Although we directly use the node name in bdrv_add_child(), but we still > need treat bdrv_del_child() as a separate function, right? In up > condition, we can't determine if child->bs supports BB or not, so i > think bdrv_get_device_or_node_name(child->bs) is ok here. I just realized that in order to invoke bdrv_add_child() one passes a node name to x-blockdev-change, whereas for bdrv_del_child() name of the child is passed (which is not a node name). So it makes perfect sense to always emit the node name in bdrv_add_child(), regardless of whether the BDS has a BB, because the node name was the parameter that had been given to x-blockdev-change. In contrast, the supposedly child node passed to bdrv_del_child() is not identified by its node name, so it makes sense not to limit the output to the node name but to print the BB's name if present. So indeed, this is completely fine as it is in this patch. Max >> Neither seems wrong to me. A child is unlikely to have a BB of its own, >> but if it does, printing its name instead of the node name may be > > bdrv_get_device_or_node_name() can do that. > > Thanks > -Xie > >> appropriate. I'm just wondering about the difference between >> bdrv_add_child() and bdrv_del_child(). >> >> Max >> >>> + return; >>> + } >>> + >>> + parent_bs->drv->bdrv_del_child(parent_bs, child_bs, errp); >>> +} >>> diff --git a/include/block/block.h b/include/block/block.h >>> index 1c4f4d8..ecde190 100644 >>> --- a/include/block/block.h >>> +++ b/include/block/block.h >>> @@ -582,4 +582,9 @@ void bdrv_drained_begin(BlockDriverState *bs); >>> */ >>> void bdrv_drained_end(BlockDriverState *bs); >>> >>> +void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, >>> + Error **errp); >>> +void bdrv_del_child(BlockDriverState *parent, BlockDriverState *child, >>> + Error **errp); >>> + >>> #endif >>> diff --git a/include/block/block_int.h b/include/block/block_int.h >>> index 9ef823a..89ec138 100644 >>> --- a/include/block/block_int.h >>> +++ b/include/block/block_int.h >>> @@ -305,6 +305,11 @@ struct BlockDriver { >>> */ >>> void (*bdrv_drain)(BlockDriverState *bs); >>> >>> + void (*bdrv_add_child)(BlockDriverState *parent, >>> BlockDriverState *child, >>> + Error **errp); >>> + void (*bdrv_del_child)(BlockDriverState *parent, >>> BlockDriverState *child, >>> + Error **errp); >>> + >>> QLIST_ENTRY(BlockDriver) list; >>> }; >>> >>> >> >> > >
diff --git a/block.c b/block.c index efc3c43..08aa979 100644 --- a/block.c +++ b/block.c @@ -4399,3 +4399,53 @@ void bdrv_refresh_filename(BlockDriverState *bs) QDECREF(json); } } + +/* + * Hot add/remove a BDS's child. So the user can take a child offline when + * it is broken and take a new child online + */ +void bdrv_add_child(BlockDriverState *parent_bs, BlockDriverState *child_bs, + Error **errp) +{ + + if (!parent_bs->drv || !parent_bs->drv->bdrv_add_child) { + error_setg(errp, "The node %s doesn't support adding a child", + bdrv_get_device_or_node_name(parent_bs)); + return; + } + + if (!QLIST_EMPTY(&child_bs->parents)) { + error_setg(errp, "The node %s already has a parent", + child_bs->node_name); + return; + } + + parent_bs->drv->bdrv_add_child(parent_bs, child_bs, errp); +} + +void bdrv_del_child(BlockDriverState *parent_bs, BlockDriverState *child_bs, + Error **errp) +{ + BdrvChild *child; + + if (!parent_bs->drv || !parent_bs->drv->bdrv_del_child) { + error_setg(errp, "The node %s doesn't support removing a child", + bdrv_get_device_or_node_name(parent_bs)); + return; + } + + QLIST_FOREACH(child, &parent_bs->children, next) { + if (child->bs == child_bs) { + break; + } + } + + if (!child) { + error_setg(errp, "The node %s is not a child of %s", + bdrv_get_device_or_node_name(child_bs), + bdrv_get_device_or_node_name(parent_bs)); + return; + } + + parent_bs->drv->bdrv_del_child(parent_bs, child_bs, errp); +} diff --git a/include/block/block.h b/include/block/block.h index 1c4f4d8..ecde190 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -582,4 +582,9 @@ void bdrv_drained_begin(BlockDriverState *bs); */ void bdrv_drained_end(BlockDriverState *bs); +void bdrv_add_child(BlockDriverState *parent, BlockDriverState *child, + Error **errp); +void bdrv_del_child(BlockDriverState *parent, BlockDriverState *child, + Error **errp); + #endif diff --git a/include/block/block_int.h b/include/block/block_int.h index 9ef823a..89ec138 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -305,6 +305,11 @@ struct BlockDriver { */ void (*bdrv_drain)(BlockDriverState *bs); + void (*bdrv_add_child)(BlockDriverState *parent, BlockDriverState *child, + Error **errp); + void (*bdrv_del_child)(BlockDriverState *parent, BlockDriverState *child, + Error **errp); + QLIST_ENTRY(BlockDriver) list; };