| Message ID | 1461030177-29144-2-git-send-email-famz@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 04/19/2016 09:42 AM, Fam Zheng wrote: > Currently we only recurse to bs->file, which will miss the children in quorum > and VMDK. > > Recurse into the whole subtree to avoid that. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/block.c b/block.c > index d4939b4..fa8b38f 100644 > --- a/block.c > +++ b/block.c > @@ -3201,6 +3201,7 @@ void bdrv_init_with_whitelist(void) > > void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) > { > + BdrvChild *child; > Error *local_err = NULL; > int ret; > > @@ -3215,13 +3216,20 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) > > if (bs->drv->bdrv_invalidate_cache) { > bs->drv->bdrv_invalidate_cache(bs, &local_err); Kevin's commit 3456a8d1 introduced unexpected two spaces in this function, would you like remove it? - if (bs->drv && bs->drv->bdrv_invalidate_cache) { + if (!bs->drv) { + return; + } > - } else if (bs->file) { > - bdrv_invalidate_cache(bs->file->bs, &local_err); > + if (local_err) { > + bs->open_flags |= BDRV_O_INACTIVE; > + error_propagate(errp, local_err); > + return; > + } > } > - if (local_err) { > - bs->open_flags |= BDRV_O_INACTIVE; > - error_propagate(errp, local_err); > - return; > + > + QLIST_FOREACH(child, &bs->children, next) { > + bdrv_invalidate_cache(child->bs, &local_err); > + if (local_err) { > + bs->open_flags |= BDRV_O_INACTIVE; > + error_propagate(errp, local_err); > + return; If we really need return here? I just mean for example, Quorum A has 3 children(children.0, children.1, children.2), if we invalidate children.1 failed, then we are not going to invalidate children.2 > + } > } > > ret = refresh_total_sectors(bs, bs->total_sectors); >
On Tue, 04/19 16:44, Changlong Xie wrote: > On 04/19/2016 09:42 AM, Fam Zheng wrote: > >Currently we only recurse to bs->file, which will miss the children in quorum > >and VMDK. > > > >Recurse into the whole subtree to avoid that. > > > >Signed-off-by: Fam Zheng <famz@redhat.com> > >--- > > block.c | 20 ++++++++++++++------ > > 1 file changed, 14 insertions(+), 6 deletions(-) > > > >diff --git a/block.c b/block.c > >index d4939b4..fa8b38f 100644 > >--- a/block.c > >+++ b/block.c > >@@ -3201,6 +3201,7 @@ void bdrv_init_with_whitelist(void) > > > > void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) > > { > >+ BdrvChild *child; > > Error *local_err = NULL; > > int ret; > > > >@@ -3215,13 +3216,20 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) > > > > if (bs->drv->bdrv_invalidate_cache) { > > bs->drv->bdrv_invalidate_cache(bs, &local_err); > > Kevin's commit 3456a8d1 introduced unexpected two spaces in this function, > would you like remove it? No, that's what to do in this patch. I'd just leave it. > > - if (bs->drv && bs->drv->bdrv_invalidate_cache) { > + if (!bs->drv) { > + return; > + } > > >- } else if (bs->file) { > >- bdrv_invalidate_cache(bs->file->bs, &local_err); > >+ if (local_err) { > >+ bs->open_flags |= BDRV_O_INACTIVE; > >+ error_propagate(errp, local_err); > >+ return; > >+ } > > } > >- if (local_err) { > >- bs->open_flags |= BDRV_O_INACTIVE; > >- error_propagate(errp, local_err); > >- return; > >+ > >+ QLIST_FOREACH(child, &bs->children, next) { > >+ bdrv_invalidate_cache(child->bs, &local_err); > >+ if (local_err) { > >+ bs->open_flags |= BDRV_O_INACTIVE; > >+ error_propagate(errp, local_err); > >+ return; > > If we really need return here? I just mean for example, Quorum A has 3 > children(children.0, children.1, children.2), if we invalidate children.1 > failed, then we are not going to invalidate children.2 We report error anyway and the caller will abort the operation, it's not very useful to continue. Fam > > >+ } > > } > > > > ret = refresh_total_sectors(bs, bs->total_sectors); > > > > >
Am 19.04.2016 um 03:42 hat Fam Zheng geschrieben: > Currently we only recurse to bs->file, which will miss the children in quorum > and VMDK. > > Recurse into the whole subtree to avoid that. > > Signed-off-by: Fam Zheng <famz@redhat.com> > --- > block.c | 20 ++++++++++++++------ > 1 file changed, 14 insertions(+), 6 deletions(-) > > diff --git a/block.c b/block.c > index d4939b4..fa8b38f 100644 > --- a/block.c > +++ b/block.c > @@ -3201,6 +3201,7 @@ void bdrv_init_with_whitelist(void) > > void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) > { > + BdrvChild *child; > Error *local_err = NULL; > int ret; > > @@ -3215,13 +3216,20 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) > > if (bs->drv->bdrv_invalidate_cache) { > bs->drv->bdrv_invalidate_cache(bs, &local_err); > - } else if (bs->file) { > - bdrv_invalidate_cache(bs->file->bs, &local_err); The old behaviour was that we only recurse for bs->file if the block driver doesn't have its own implementation. This means that in qcow2, for example, we call bdrv_invalidate_cache() explicitly for bs->file. If we can already invalidate it here, the call inside qcow2 and probably other drivers could go away. Kevin
On Wed, 05/04 12:10, Kevin Wolf wrote: > Am 19.04.2016 um 03:42 hat Fam Zheng geschrieben: > > Currently we only recurse to bs->file, which will miss the children in quorum > > and VMDK. > > > > Recurse into the whole subtree to avoid that. > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > --- > > block.c | 20 ++++++++++++++------ > > 1 file changed, 14 insertions(+), 6 deletions(-) > > > > diff --git a/block.c b/block.c > > index d4939b4..fa8b38f 100644 > > --- a/block.c > > +++ b/block.c > > @@ -3201,6 +3201,7 @@ void bdrv_init_with_whitelist(void) > > > > void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) > > { > > + BdrvChild *child; > > Error *local_err = NULL; > > int ret; > > > > @@ -3215,13 +3216,20 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) > > > > if (bs->drv->bdrv_invalidate_cache) { > > bs->drv->bdrv_invalidate_cache(bs, &local_err); > > - } else if (bs->file) { > > - bdrv_invalidate_cache(bs->file->bs, &local_err); > > The old behaviour was that we only recurse for bs->file if the block > driver doesn't have its own implementation. > > This means that in qcow2, for example, we call bdrv_invalidate_cache() > explicitly for bs->file. If we can already invalidate it here, the call > inside qcow2 and probably other drivers could go away. Yes, will update. Thanks. Fam
diff --git a/block.c b/block.c index d4939b4..fa8b38f 100644 --- a/block.c +++ b/block.c @@ -3201,6 +3201,7 @@ void bdrv_init_with_whitelist(void) void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) { + BdrvChild *child; Error *local_err = NULL; int ret; @@ -3215,13 +3216,20 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp) if (bs->drv->bdrv_invalidate_cache) { bs->drv->bdrv_invalidate_cache(bs, &local_err); - } else if (bs->file) { - bdrv_invalidate_cache(bs->file->bs, &local_err); + if (local_err) { + bs->open_flags |= BDRV_O_INACTIVE; + error_propagate(errp, local_err); + return; + } } - if (local_err) { - bs->open_flags |= BDRV_O_INACTIVE; - error_propagate(errp, local_err); - return; + + QLIST_FOREACH(child, &bs->children, next) { + bdrv_invalidate_cache(child->bs, &local_err); + if (local_err) { + bs->open_flags |= BDRV_O_INACTIVE; + error_propagate(errp, local_err); + return; + } } ret = refresh_total_sectors(bs, bs->total_sectors);
Currently we only recurse to bs->file, which will miss the children in quorum and VMDK. Recurse into the whole subtree to avoid that. Signed-off-by: Fam Zheng <famz@redhat.com> --- block.c | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-)