| Message ID | 558BA5CE.80507@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Jun 25, 2015 at 02:55:10PM +0800, Wen Congyang wrote: > Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> > --- > block.c | 5 +++-- > block/mirror.c | 3 ++- > blockdev.c | 2 +- > include/block/block.h | 3 ++- > 4 files changed, 8 insertions(+), 5 deletions(-) This patch is missing a commit description. What is the justification for this change? Leaving this patch to Jeff since I'm CC only but the email is to Jeff. > diff --git a/block.c b/block.c > index 7168575..70ee0f6 100644 > --- a/block.c > +++ b/block.c > @@ -4033,7 +4033,8 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) > return false; > } > > -BlockDriverState *check_to_replace_node(const char *node_name, Error **errp) > +BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, > + const char *node_name, Error **errp) > { > BlockDriverState *to_replace_bs = bdrv_find_node(node_name); > AioContext *aio_context; > @@ -4056,7 +4057,7 @@ BlockDriverState *check_to_replace_node(const char *node_name, Error **errp) > * Another benefit is that this tests exclude backing files which are > * blocked by the backing blockers. > */ > - if (!bdrv_is_first_non_filter(to_replace_bs)) { > + if (!bdrv_recurse_is_first_non_filter(parent_bs, to_replace_bs)) { > error_setg(errp, "Only top most non filter can be replaced"); > to_replace_bs = NULL; > goto out; > diff --git a/block/mirror.c b/block/mirror.c > index 0d06cc2..f132f35 100644 > --- a/block/mirror.c > +++ b/block/mirror.c > @@ -612,7 +612,8 @@ static void mirror_complete(BlockJob *job, Error **errp) > if (s->replaces) { > AioContext *replace_aio_context; > > - s->to_replace = check_to_replace_node(s->replaces, &local_err); > + s->to_replace = check_to_replace_node(s->common.bs, s->replaces, > + &local_err); > if (!s->to_replace) { > error_propagate(errp, local_err); > return; > diff --git a/blockdev.c b/blockdev.c > index f3a3097..a0e13b0 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -2756,7 +2756,7 @@ void qmp_drive_mirror(const char *device, const char *target, > goto out; > } > > - to_replace_bs = check_to_replace_node(replaces, &local_err); > + to_replace_bs = check_to_replace_node(bs, replaces, &local_err); > > if (!to_replace_bs) { > error_propagate(errp, local_err); > diff --git a/include/block/block.h b/include/block/block.h > index 07bb724..e0dad54 100644 > --- a/include/block/block.h > +++ b/include/block/block.h > @@ -311,7 +311,8 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs, > bool bdrv_is_first_non_filter(BlockDriverState *candidate); > > /* check if a named node can be replaced when doing drive-mirror */ > -BlockDriverState *check_to_replace_node(const char *node_name, Error **errp); > +BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, > + const char *node_name, Error **errp); > > /* async block I/O */ > typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector,
On 07/01/2015 04:39 PM, Stefan Hajnoczi wrote: > On Thu, Jun 25, 2015 at 02:55:10PM +0800, Wen Congyang wrote: >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >> --- >> block.c | 5 +++-- >> block/mirror.c | 3 ++- >> blockdev.c | 2 +- >> include/block/block.h | 3 ++- >> 4 files changed, 8 insertions(+), 5 deletions(-) > > This patch is missing a commit description. What is the justification > for this change? Sorry, I forgot to add commit messages.. Without this patch, the replace node can be any node, and it can be top BDS with BB, or another quorum's child. With this patch, the replace node must be this quorum's child. Thanks Wen Congyang > > Leaving this patch to Jeff since I'm CC only but the email is to Jeff. > >> diff --git a/block.c b/block.c >> index 7168575..70ee0f6 100644 >> --- a/block.c >> +++ b/block.c >> @@ -4033,7 +4033,8 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) >> return false; >> } >> >> -BlockDriverState *check_to_replace_node(const char *node_name, Error **errp) >> +BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, >> + const char *node_name, Error **errp) >> { >> BlockDriverState *to_replace_bs = bdrv_find_node(node_name); >> AioContext *aio_context; >> @@ -4056,7 +4057,7 @@ BlockDriverState *check_to_replace_node(const char *node_name, Error **errp) >> * Another benefit is that this tests exclude backing files which are >> * blocked by the backing blockers. >> */ >> - if (!bdrv_is_first_non_filter(to_replace_bs)) { >> + if (!bdrv_recurse_is_first_non_filter(parent_bs, to_replace_bs)) { >> error_setg(errp, "Only top most non filter can be replaced"); >> to_replace_bs = NULL; >> goto out; >> diff --git a/block/mirror.c b/block/mirror.c >> index 0d06cc2..f132f35 100644 >> --- a/block/mirror.c >> +++ b/block/mirror.c >> @@ -612,7 +612,8 @@ static void mirror_complete(BlockJob *job, Error **errp) >> if (s->replaces) { >> AioContext *replace_aio_context; >> >> - s->to_replace = check_to_replace_node(s->replaces, &local_err); >> + s->to_replace = check_to_replace_node(s->common.bs, s->replaces, >> + &local_err); >> if (!s->to_replace) { >> error_propagate(errp, local_err); >> return; >> diff --git a/blockdev.c b/blockdev.c >> index f3a3097..a0e13b0 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -2756,7 +2756,7 @@ void qmp_drive_mirror(const char *device, const char *target, >> goto out; >> } >> >> - to_replace_bs = check_to_replace_node(replaces, &local_err); >> + to_replace_bs = check_to_replace_node(bs, replaces, &local_err); >> >> if (!to_replace_bs) { >> error_propagate(errp, local_err); >> diff --git a/include/block/block.h b/include/block/block.h >> index 07bb724..e0dad54 100644 >> --- a/include/block/block.h >> +++ b/include/block/block.h >> @@ -311,7 +311,8 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs, >> bool bdrv_is_first_non_filter(BlockDriverState *candidate); >> >> /* check if a named node can be replaced when doing drive-mirror */ >> -BlockDriverState *check_to_replace_node(const char *node_name, Error **errp); >> +BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, >> + const char *node_name, Error **errp); >> >> /* async block I/O */ >> typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector,
On Wed, Jul 01, 2015 at 04:49:40PM +0800, Wen Congyang wrote: > On 07/01/2015 04:39 PM, Stefan Hajnoczi wrote: > > On Thu, Jun 25, 2015 at 02:55:10PM +0800, Wen Congyang wrote: > >> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> > >> --- > >> block.c | 5 +++-- > >> block/mirror.c | 3 ++- > >> blockdev.c | 2 +- > >> include/block/block.h | 3 ++- > >> 4 files changed, 8 insertions(+), 5 deletions(-) > > > > This patch is missing a commit description. What is the justification > > for this change? > > Sorry, I forgot to add commit messages.. > > Without this patch, the replace node can be any node, and it can be > top BDS with BB, or another quorum's child. With this patch, the replace node > must be this quorum's child. I think the point of the replace operation was to swap a quorum child with a new drive. It sounds like this patch will break that use case? The idea was that a failed child needs to be replaced. The user adds a new -drive and then uses the mirror+replace to include it into the quorum. I think the new child is not be part of the quorum BDS graph until replacement occurs. Stefan
On 07/02/2015 10:48 PM, Stefan Hajnoczi wrote: > On Wed, Jul 01, 2015 at 04:49:40PM +0800, Wen Congyang wrote: >> On 07/01/2015 04:39 PM, Stefan Hajnoczi wrote: >>> On Thu, Jun 25, 2015 at 02:55:10PM +0800, Wen Congyang wrote: >>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> >>>> --- >>>> block.c | 5 +++-- >>>> block/mirror.c | 3 ++- >>>> blockdev.c | 2 +- >>>> include/block/block.h | 3 ++- >>>> 4 files changed, 8 insertions(+), 5 deletions(-) >>> >>> This patch is missing a commit description. What is the justification >>> for this change? >> >> Sorry, I forgot to add commit messages.. >> >> Without this patch, the replace node can be any node, and it can be >> top BDS with BB, or another quorum's child. With this patch, the replace node >> must be this quorum's child. > > I think the point of the replace operation was to swap a quorum child > with a new drive. It sounds like this patch will break that use case? > > The idea was that a failed child needs to be replaced. The user adds a > new -drive and then uses the mirror+replace to include it into the > quorum. I think the new child is not be part of the quorum BDS graph > until replacement occurs. bs/s->common.bs is quorum, and to_replace is the broken child. The new child is target_bs. With this patch, we just check if the broken child is part of the quorum BDS. Do I misunderstand something? Thanks Wen Congyang > > Stefan >
On Mon, Jul 13, 2015 at 10:41:53AM +0800, Wen Congyang wrote: > On 07/02/2015 10:48 PM, Stefan Hajnoczi wrote: > > On Wed, Jul 01, 2015 at 04:49:40PM +0800, Wen Congyang wrote: > >> On 07/01/2015 04:39 PM, Stefan Hajnoczi wrote: > >>> On Thu, Jun 25, 2015 at 02:55:10PM +0800, Wen Congyang wrote: > >>>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> > >>>> --- > >>>> block.c | 5 +++-- > >>>> block/mirror.c | 3 ++- > >>>> blockdev.c | 2 +- > >>>> include/block/block.h | 3 ++- > >>>> 4 files changed, 8 insertions(+), 5 deletions(-) > >>> > >>> This patch is missing a commit description. What is the justification > >>> for this change? > >> > >> Sorry, I forgot to add commit messages.. > >> > >> Without this patch, the replace node can be any node, and it can be > >> top BDS with BB, or another quorum's child. With this patch, the replace node > >> must be this quorum's child. > > > > I think the point of the replace operation was to swap a quorum child > > with a new drive. It sounds like this patch will break that use case? > > > > The idea was that a failed child needs to be replaced. The user adds a > > new -drive and then uses the mirror+replace to include it into the > > quorum. I think the new child is not be part of the quorum BDS graph > > until replacement occurs. > > bs/s->common.bs is quorum, and to_replace is the broken child. > The new child is target_bs. With this patch, we just check if > the broken child is part of the quorum BDS. > > Do I misunderstand something? Thanks for explaining. That makes sense. Please resend with a commit description. Stefan
diff --git a/block.c b/block.c index 7168575..70ee0f6 100644 --- a/block.c +++ b/block.c @@ -4033,7 +4033,8 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) return false; } -BlockDriverState *check_to_replace_node(const char *node_name, Error **errp) +BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, + const char *node_name, Error **errp) { BlockDriverState *to_replace_bs = bdrv_find_node(node_name); AioContext *aio_context; @@ -4056,7 +4057,7 @@ BlockDriverState *check_to_replace_node(const char *node_name, Error **errp) * Another benefit is that this tests exclude backing files which are * blocked by the backing blockers. */ - if (!bdrv_is_first_non_filter(to_replace_bs)) { + if (!bdrv_recurse_is_first_non_filter(parent_bs, to_replace_bs)) { error_setg(errp, "Only top most non filter can be replaced"); to_replace_bs = NULL; goto out; diff --git a/block/mirror.c b/block/mirror.c index 0d06cc2..f132f35 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -612,7 +612,8 @@ static void mirror_complete(BlockJob *job, Error **errp) if (s->replaces) { AioContext *replace_aio_context; - s->to_replace = check_to_replace_node(s->replaces, &local_err); + s->to_replace = check_to_replace_node(s->common.bs, s->replaces, + &local_err); if (!s->to_replace) { error_propagate(errp, local_err); return; diff --git a/blockdev.c b/blockdev.c index f3a3097..a0e13b0 100644 --- a/blockdev.c +++ b/blockdev.c @@ -2756,7 +2756,7 @@ void qmp_drive_mirror(const char *device, const char *target, goto out; } - to_replace_bs = check_to_replace_node(replaces, &local_err); + to_replace_bs = check_to_replace_node(bs, replaces, &local_err); if (!to_replace_bs) { error_propagate(errp, local_err); diff --git a/include/block/block.h b/include/block/block.h index 07bb724..e0dad54 100644 --- a/include/block/block.h +++ b/include/block/block.h @@ -311,7 +311,8 @@ bool bdrv_recurse_is_first_non_filter(BlockDriverState *bs, bool bdrv_is_first_non_filter(BlockDriverState *candidate); /* check if a named node can be replaced when doing drive-mirror */ -BlockDriverState *check_to_replace_node(const char *node_name, Error **errp); +BlockDriverState *check_to_replace_node(BlockDriverState *parent_bs, + const char *node_name, Error **errp); /* async block I/O */ typedef void BlockDriverDirtyHandler(BlockDriverState *bs, int64_t sector,
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com> --- block.c | 5 +++-- block/mirror.c | 3 ++- blockdev.c | 2 +- include/block/block.h | 3 ++- 4 files changed, 8 insertions(+), 5 deletions(-)