| Message ID | 1392068957-14184-2-git-send-email-benoit.canet@irqsave.net |
|---|---|
| State | New |
| Headers | show |
On Mon, 02/10 22:49, Benoît Canet wrote: > Take into account the fact that a block filter like quorum will be in bs->file > while a regular block driver device is really on the top level. > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > --- > block.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/block.c b/block.c > index 07ac50a..d04f535 100644 > --- a/block.c > +++ b/block.c > @@ -5400,14 +5400,16 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) > > /* walk down the bs forest recursively */ > QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > - bool perm; > - > - if (!bs->file) { > - continue; > + bool perm = false; > + > + if (bs->file && > + bs->file->drv && > + bs->file->drv->authorizations[BS_IS_A_FILTER]) { > + perm = bdrv_recurse_is_first_non_filter(bs->file, candidate); > + } else if (bs == candidate) { > + perm = true; > } > > - perm = bdrv_recurse_is_first_non_filter(bs->file, candidate); > - > /* candidate is the first non filter */ > if (perm) { > return true; With this change, if the top level driver has ->file, its implementation of .bdrv_recurse_is_first_non_filter() is skipped and bs->file is the start point. So we have an implication that single child block drivers (that has ->file) doesn't need to, and shouldn't implement this operation, as commentted above bdrv_generic_is_first_non_filter. Tested that this patch fixes the external snapshot problem, but didn't test the "quorum as bs->file case". Thanks, Reviewed-by: Fam Zheng <famz@redhat.com>
The Tuesday 11 Feb 2014 à 16:12:17 (+0800), Fam Zheng wrote : > On Mon, 02/10 22:49, Benoît Canet wrote: > > Take into account the fact that a block filter like quorum will be in bs->file > > while a regular block driver device is really on the top level. > > > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > > --- > > block.c | 14 ++++++++------ > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > diff --git a/block.c b/block.c > > index 07ac50a..d04f535 100644 > > --- a/block.c > > +++ b/block.c > > @@ -5400,14 +5400,16 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) > > > > /* walk down the bs forest recursively */ > > QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > > - bool perm; > > - > > - if (!bs->file) { > > - continue; > > + bool perm = false; > > + > > + if (bs->file && > > + bs->file->drv && > > + bs->file->drv->authorizations[BS_IS_A_FILTER]) { > > + perm = bdrv_recurse_is_first_non_filter(bs->file, candidate); > > + } else if (bs == candidate) { > > + perm = true; > > } > > > > - perm = bdrv_recurse_is_first_non_filter(bs->file, candidate); > > - > > /* candidate is the first non filter */ > > if (perm) { > > return true; > > With this change, if the top level driver has ->file, its implementation of > .bdrv_recurse_is_first_non_filter() is skipped and bs->file is the start point. > > So we have an implication that single child block drivers (that has ->file) > doesn't need to, and shouldn't implement this operation, as commentted above > bdrv_generic_is_first_non_filter. > > Tested that this patch fixes the external snapshot problem, but didn't test > the "quorum as bs->file case". > > Thanks, > > Reviewed-by: Fam Zheng <famz@redhat.com> After extensive testing of all type of quorum instanciations and snapshots I discovered that we are not done yet with this issue. When instantiating quorum from the command line the quorum driver is in bs->file->drv. When using QMP's blockdev_add at once or by using references the quorum driver is in bs->drv. As a result this patch work in half the cases: regular file and command line quorum but not when quorum is instantiated via QMP. I don't understand why the block layer have this irregular behavior regarding quorum instantiation. What would be the best way to fix this ? Best regards Benoît
The Wednesday 12 Feb 2014 à 20:49:18 (+0100), Benoît Canet wrote : > The Tuesday 11 Feb 2014 à 16:12:17 (+0800), Fam Zheng wrote : > > On Mon, 02/10 22:49, Benoît Canet wrote: > > > Take into account the fact that a block filter like quorum will be in bs->file > > > while a regular block driver device is really on the top level. > > > > > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > > > --- > > > block.c | 14 ++++++++------ > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > diff --git a/block.c b/block.c > > > index 07ac50a..d04f535 100644 > > > --- a/block.c > > > +++ b/block.c > > > @@ -5400,14 +5400,16 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) > > > > > > /* walk down the bs forest recursively */ > > > QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > > > - bool perm; > > > - > > > - if (!bs->file) { > > > - continue; > > > + bool perm = false; > > > + > > > + if (bs->file && > > > + bs->file->drv && > > > + bs->file->drv->authorizations[BS_IS_A_FILTER]) { > > > + perm = bdrv_recurse_is_first_non_filter(bs->file, candidate); > > > + } else if (bs == candidate) { > > > + perm = true; > > > } > > > > > > - perm = bdrv_recurse_is_first_non_filter(bs->file, candidate); > > > - > > > /* candidate is the first non filter */ > > > if (perm) { > > > return true; > > > > With this change, if the top level driver has ->file, its implementation of > > .bdrv_recurse_is_first_non_filter() is skipped and bs->file is the start point. > > > > So we have an implication that single child block drivers (that has ->file) > > doesn't need to, and shouldn't implement this operation, as commentted above > > bdrv_generic_is_first_non_filter. > > > > Tested that this patch fixes the external snapshot problem, but didn't test > > the "quorum as bs->file case". > > > > Thanks, > > > > Reviewed-by: Fam Zheng <famz@redhat.com> > > After extensive testing of all type of quorum instanciations and snapshots I > discovered that we are not done yet with this issue. > > When instantiating quorum from the command line the quorum driver is in > bs->file->drv. > When using QMP's blockdev_add at once or by using references the quorum driver > is in bs->drv. > > As a result this patch work in half the cases: regular file and command line > quorum but not when quorum is instantiated via QMP. > > I don't understand why the block layer have this irregular behavior regarding > quorum instantiation. > > What would be the best way to fix this ? I have a pending resping of this patch which tests for the two cases as a workaround. Should I post it ? Best regards Benoît > > Best regards > > Benoît >
The Wednesday 12 Feb 2014 à 22:43:15 (+0100), Benoît Canet wrote : > The Wednesday 12 Feb 2014 à 20:49:18 (+0100), Benoît Canet wrote : > > The Tuesday 11 Feb 2014 à 16:12:17 (+0800), Fam Zheng wrote : > > > On Mon, 02/10 22:49, Benoît Canet wrote: > > > > Take into account the fact that a block filter like quorum will be in bs->file > > > > while a regular block driver device is really on the top level. > > > > > > > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > > > > --- > > > > block.c | 14 ++++++++------ > > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > > > diff --git a/block.c b/block.c > > > > index 07ac50a..d04f535 100644 > > > > --- a/block.c > > > > +++ b/block.c > > > > @@ -5400,14 +5400,16 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) > > > > > > > > /* walk down the bs forest recursively */ > > > > QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > > > > - bool perm; > > > > - > > > > - if (!bs->file) { > > > > - continue; > > > > + bool perm = false; > > > > + > > > > + if (bs->file && > > > > + bs->file->drv && > > > > + bs->file->drv->authorizations[BS_IS_A_FILTER]) { > > > > + perm = bdrv_recurse_is_first_non_filter(bs->file, candidate); > > > > + } else if (bs == candidate) { > > > > + perm = true; > > > > } > > > > > > > > - perm = bdrv_recurse_is_first_non_filter(bs->file, candidate); > > > > - > > > > /* candidate is the first non filter */ > > > > if (perm) { > > > > return true; > > > > > > With this change, if the top level driver has ->file, its implementation of > > > .bdrv_recurse_is_first_non_filter() is skipped and bs->file is the start point. > > > > > > So we have an implication that single child block drivers (that has ->file) > > > doesn't need to, and shouldn't implement this operation, as commentted above > > > bdrv_generic_is_first_non_filter. > > > > > > Tested that this patch fixes the external snapshot problem, but didn't test > > > the "quorum as bs->file case". > > > > > > Thanks, > > > > > > Reviewed-by: Fam Zheng <famz@redhat.com> > > > > After extensive testing of all type of quorum instanciations and snapshots I > > discovered that we are not done yet with this issue. > > > > When instantiating quorum from the command line the quorum driver is in > > bs->file->drv. > > When using QMP's blockdev_add at once or by using references the quorum driver > > is in bs->drv. > > > > As a result this patch work in half the cases: regular file and command line > > quorum but not when quorum is instantiated via QMP. > > > > I don't understand why the block layer have this irregular behavior regarding > > quorum instantiation. > > > > What would be the best way to fix this ? > > I have a pending resping of this patch which tests for the two cases as a > workaround. > > Should I post it ? > > Best regards > > Benoît > > > > > Best regards > > > > Benoît > > > I found that the used the command line init in a wrong way. I will repost an updated version of the patch. Quorum will need to be reposted since one commit message showed the wrong way of doing the initialization. Best regards Benoît
Am 12.02.2014 um 20:49 hat Benoît Canet geschrieben: > The Tuesday 11 Feb 2014 à 16:12:17 (+0800), Fam Zheng wrote : > > On Mon, 02/10 22:49, Benoît Canet wrote: > > > Take into account the fact that a block filter like quorum will be in bs->file > > > while a regular block driver device is really on the top level. > > > > > > Signed-off-by: Benoit Canet <benoit@irqsave.net> > > > --- > > > block.c | 14 ++++++++------ > > > 1 file changed, 8 insertions(+), 6 deletions(-) > > > > > > diff --git a/block.c b/block.c > > > index 07ac50a..d04f535 100644 > > > --- a/block.c > > > +++ b/block.c > > > @@ -5400,14 +5400,16 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) > > > > > > /* walk down the bs forest recursively */ > > > QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > > > - bool perm; > > > - > > > - if (!bs->file) { > > > - continue; > > > + bool perm = false; > > > + > > > + if (bs->file && > > > + bs->file->drv && > > > + bs->file->drv->authorizations[BS_IS_A_FILTER]) { > > > + perm = bdrv_recurse_is_first_non_filter(bs->file, candidate); > > > + } else if (bs == candidate) { > > > + perm = true; > > > } > > > > > > - perm = bdrv_recurse_is_first_non_filter(bs->file, candidate); > > > - > > > /* candidate is the first non filter */ > > > if (perm) { > > > return true; > > > > With this change, if the top level driver has ->file, its implementation of > > .bdrv_recurse_is_first_non_filter() is skipped and bs->file is the start point. > > > > So we have an implication that single child block drivers (that has ->file) > > doesn't need to, and shouldn't implement this operation, as commentted above > > bdrv_generic_is_first_non_filter. > > > > Tested that this patch fixes the external snapshot problem, but didn't test > > the "quorum as bs->file case". > > > > Thanks, > > > > Reviewed-by: Fam Zheng <famz@redhat.com> > > After extensive testing of all type of quorum instanciations and snapshots I > discovered that we are not done yet with this issue. > > When instantiating quorum from the command line the quorum driver is in > bs->file->drv. > When using QMP's blockdev_add at once or by using references the quorum driver > is in bs->drv. I'm pretty sure this is a user error. Command line and blockdev-add are pretty much equivalent, they even use exactly the same code path once they have converted their input into an options QDict. Both ways should be able to instantiate both setups you mentioned. This is required, because these setups have different semantics: quorum above qcow2 redirects and compares the guest data, whereas below qcow2 it redirects image file accesses including metadata). Kevin
diff --git a/block.c b/block.c index 07ac50a..d04f535 100644 --- a/block.c +++ b/block.c @@ -5400,14 +5400,16 @@ bool bdrv_is_first_non_filter(BlockDriverState *candidate) /* walk down the bs forest recursively */ QTAILQ_FOREACH(bs, &bdrv_states, device_list) { - bool perm; - - if (!bs->file) { - continue; + bool perm = false; + + if (bs->file && + bs->file->drv && + bs->file->drv->authorizations[BS_IS_A_FILTER]) { + perm = bdrv_recurse_is_first_non_filter(bs->file, candidate); + } else if (bs == candidate) { + perm = true; } - perm = bdrv_recurse_is_first_non_filter(bs->file, candidate); - /* candidate is the first non filter */ if (perm) { return true;
Take into account the fact that a block filter like quorum will be in bs->file while a regular block driver device is really on the top level. Signed-off-by: Benoit Canet <benoit@irqsave.net> --- block.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-)