| Message ID | 1439279489-13338-4-git-send-email-wency@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On 08/11/2015 01:51 AM, Wen Congyang wrote: > 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> > Reviewed-by: Alberto Garcia <berto@igalia.com> > --- > block.c | 43 +++++++++++++++++++++++++++++++++++++++++++ > include/block/block.h | 4 ++++ > include/block/block_int.h | 5 +++++ > 3 files changed, 52 insertions(+) > > + * 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 *bs, QDict *options, Error **errp) > +{ > + > + if (!bs->drv || !bs->drv->bdrv_add_child) { > + error_setg(errp, "The BDS %s doesn't support adding a child", > + bdrv_get_device_or_node_name(bs)); > + return; > + } > + > + bs->drv->bdrv_add_child(bs, options, errp); Should this also check that bs is not already a child of something? Or a bit looser, we may want to allow a BDS to be a child of multiple trees (a common shared backing file), but we still definitely don't want to allow nonsensical loops such as trying to make a BDS be hot-added as its own child.
On 09/01/2015 01:40 AM, Eric Blake wrote: > On 08/11/2015 01:51 AM, Wen Congyang wrote: >> 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> >> Reviewed-by: Alberto Garcia <berto@igalia.com> >> --- >> block.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >> include/block/block.h | 4 ++++ >> include/block/block_int.h | 5 +++++ >> 3 files changed, 52 insertions(+) >> > >> + * 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 *bs, QDict *options, Error **errp) >> +{ >> + >> + if (!bs->drv || !bs->drv->bdrv_add_child) { >> + error_setg(errp, "The BDS %s doesn't support adding a child", >> + bdrv_get_device_or_node_name(bs)); >> + return; >> + } >> + >> + bs->drv->bdrv_add_child(bs, options, errp); > > Should this also check that bs is not already a child of something? Or > a bit looser, we may want to allow a BDS to be a child of multiple trees > (a common shared backing file), but we still definitely don't want to > allow nonsensical loops such as trying to make a BDS be hot-added as its > own child. > hot-added BDS is a new BDS, but it is OK to check it here. I will update it in the next version. Thanks Wen Congyang
On 09/01/2015 01:40 AM, Eric Blake wrote: > On 08/11/2015 01:51 AM, Wen Congyang wrote: >> 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> >> Reviewed-by: Alberto Garcia <berto@igalia.com> >> --- >> block.c | 43 +++++++++++++++++++++++++++++++++++++++++++ >> include/block/block.h | 4 ++++ >> include/block/block_int.h | 5 +++++ >> 3 files changed, 52 insertions(+) >> > >> + * 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 *bs, QDict *options, Error **errp) >> +{ >> + >> + if (!bs->drv || !bs->drv->bdrv_add_child) { >> + error_setg(errp, "The BDS %s doesn't support adding a child", >> + bdrv_get_device_or_node_name(bs)); >> + return; >> + } >> + >> + bs->drv->bdrv_add_child(bs, options, errp); > > Should this also check that bs is not already a child of something? Or > a bit looser, we may want to allow a BDS to be a child of multiple trees > (a common shared backing file), but we still definitely don't want to > allow nonsensical loops such as trying to make a BDS be hot-added as its > own child. > bs is parent, and the child is options, and will be opened by the parent. So there is no need to check it. Thanks Wen Congyang
On 08/31/2015 06:44 PM, Wen Congyang wrote: >> >>> + * 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 *bs, QDict *options, Error **errp) >>> +{ >>> + >>> + if (!bs->drv || !bs->drv->bdrv_add_child) { >>> + error_setg(errp, "The BDS %s doesn't support adding a child", >>> + bdrv_get_device_or_node_name(bs)); >>> + return; >>> + } >>> + >>> + bs->drv->bdrv_add_child(bs, options, errp); >> >> Should this also check that bs is not already a child of something? Or >> a bit looser, we may want to allow a BDS to be a child of multiple trees >> (a common shared backing file), but we still definitely don't want to >> allow nonsensical loops such as trying to make a BDS be hot-added as its >> own child. >> > > hot-added BDS is a new BDS, but it is OK to check it here. I will update it > in the next version. Design-wise, I think we really want to have the add-child operation be handed a pre-opened BDS, rather than the options dictionary to open the BDS itself. That is, we should use the existing blockdev-add (and enhance it to support everything) to open the BDS, and then this command should just attach that BDS as the new child (which is why it IS important that we validate that the new BDS being added doesn't create an invalid loop).
On 09/01/2015 11:30 PM, Eric Blake wrote: > On 08/31/2015 06:44 PM, Wen Congyang wrote: > >>> >>>> + * 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 *bs, QDict *options, Error **errp) >>>> +{ >>>> + >>>> + if (!bs->drv || !bs->drv->bdrv_add_child) { >>>> + error_setg(errp, "The BDS %s doesn't support adding a child", >>>> + bdrv_get_device_or_node_name(bs)); >>>> + return; >>>> + } >>>> + >>>> + bs->drv->bdrv_add_child(bs, options, errp); >>> >>> Should this also check that bs is not already a child of something? Or >>> a bit looser, we may want to allow a BDS to be a child of multiple trees >>> (a common shared backing file), but we still definitely don't want to >>> allow nonsensical loops such as trying to make a BDS be hot-added as its >>> own child. >>> >> >> hot-added BDS is a new BDS, but it is OK to check it here. I will update it >> in the next version. > > Design-wise, I think we really want to have the add-child operation be > handed a pre-opened BDS, rather than the options dictionary to open the > BDS itself. That is, we should use the existing blockdev-add (and > enhance it to support everything) to open the BDS, and then this command > should just attach that BDS as the new child (which is why it IS > important that we validate that the new BDS being added doesn't create > an invalid loop). > How to check it? The parent BDS can get all children. But the child doesn't know if it is some BDS's child. Thanks Wen Congyang
On 09/08/2015 03:10 AM, Wen Congyang wrote: >> Design-wise, I think we really want to have the add-child operation be >> handed a pre-opened BDS, rather than the options dictionary to open the >> BDS itself. That is, we should use the existing blockdev-add (and >> enhance it to support everything) to open the BDS, and then this command >> should just attach that BDS as the new child (which is why it IS >> important that we validate that the new BDS being added doesn't create >> an invalid loop). >> > > How to check it? The parent BDS can get all children. But the child doesn't > know if it is some BDS's child. If I'm not mistaken, a child DOES know what its parent(s) are, once we have Max's series for NULL BDS representing a BB without media.
On 09/08/2015 11:52 PM, Eric Blake wrote: > On 09/08/2015 03:10 AM, Wen Congyang wrote: > >>> Design-wise, I think we really want to have the add-child operation be >>> handed a pre-opened BDS, rather than the options dictionary to open the >>> BDS itself. That is, we should use the existing blockdev-add (and >>> enhance it to support everything) to open the BDS, and then this command >>> should just attach that BDS as the new child (which is why it IS >>> important that we validate that the new BDS being added doesn't create >>> an invalid loop). >>> >> >> How to check it? The parent BDS can get all children. But the child doesn't >> know if it is some BDS's child. > > If I'm not mistaken, a child DOES know what its parent(s) are, once we > have Max's series for NULL BDS representing a BB without media. > Which patch? I don't find it. Thanks Wen Congyang
diff --git a/block.c b/block.c index d088ee0..1746d99 100644 --- a/block.c +++ b/block.c @@ -4251,3 +4251,46 @@ BlockAcctStats *bdrv_get_stats(BlockDriverState *bs) { return &bs->stats; } + +/* + * 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 *bs, QDict *options, Error **errp) +{ + + if (!bs->drv || !bs->drv->bdrv_add_child) { + error_setg(errp, "The BDS %s doesn't support adding a child", + bdrv_get_device_or_node_name(bs)); + return; + } + + bs->drv->bdrv_add_child(bs, options, errp); +} + +void bdrv_del_child(BlockDriverState *bs, BlockDriverState *child_bs, + Error **errp) +{ + BdrvChild *child; + + if (!bs->drv || !bs->drv->bdrv_del_child) { + error_setg(errp, "The BDS %s doesn't support removing a child", + bdrv_get_device_or_node_name(bs)); + return; + } + + QLIST_FOREACH(child, &bs->children, next) { + if (child->bs == child_bs) { + break; + } + } + + if (!child) { + error_setg(errp, "The BDS %s is not the BDS %s's child", + bdrv_get_device_or_node_name(child_bs), + bdrv_get_device_or_node_name(bs)); + return; + } + + bs->drv->bdrv_del_child(bs, child_bs, errp); +} diff --git a/include/block/block.h b/include/block/block.h index 37916f7..4a03fb6 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -616,4 +616,8 @@ void bdrv_flush_io_queue(BlockDriverState *bs); BlockAcctStats *bdrv_get_stats(BlockDriverState *bs); +void bdrv_add_child(BlockDriverState *bs, QDict *options, Error **errp); +void bdrv_del_child(BlockDriverState *bs, BlockDriverState *child, + Error **errp); + #endif diff --git a/include/block/block_int.h b/include/block/block_int.h index 14ad4c3..b6f2905 100644 --- a/include/block/block_int.h +++ b/include/block/block_int.h @@ -288,6 +288,11 @@ struct BlockDriver { */ int (*bdrv_probe_geometry)(BlockDriverState *bs, HDGeometry *geo); + void (*bdrv_add_child)(BlockDriverState *bs, QDict *options, + Error **errp); + void (*bdrv_del_child)(BlockDriverState *bs, BlockDriverState *child, + Error **errp); + QLIST_ENTRY(BlockDriver) list; };