* [Qemu-devel] [PATCH] block: bdrv_invalidate_cache: invalidate children first
@ 2017-01-31 11:23 Vladimir Sementsov-Ogievskiy
2017-01-31 11:26 ` Vladimir Sementsov-Ogievskiy
` (3 more replies)
0 siblings, 4 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-01-31 11:23 UTC (permalink / raw
To: qemu-block, qemu-devel
Cc: kwolf, mreitz, armbru, eblake, jsnow, famz, den, stefanha,
vsementsov, pbonzini
Current implementation invalidates firstly parent bds and then its
children. This leads to the following bug:
after incoming migration, in bdrv_invalidate_cache_all:
1. invalidate parent bds - reopen it with BDRV_O_INACTIVE cleared
2. child is not yet invalidated
3. parent check that its BDRV_O_INACTIVE is cleared
4. parent writes to child
5. assert in bdrv_co_pwritev, as BDRV_O_INACTIVE is set for child
This patch fixes it by just changing invalidate sequence: invalidate
children first.
Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
v2: I've missed that bdrv_invalidate_cache is already recursive, so we
can change sequence here. Also v1 doesn't cover the case when
bdrv_invalidate_cache is called not from bdrv_invalidate_cache_all.
block.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/block.c b/block.c
index a0346c80c6..dce1dc02af 100644
--- a/block.c
+++ b/block.c
@@ -3235,19 +3235,18 @@ void bdrv_invalidate_cache(BlockDriverState *bs, Error **errp)
if (!(bs->open_flags & BDRV_O_INACTIVE)) {
return;
}
- bs->open_flags &= ~BDRV_O_INACTIVE;
- if (bs->drv->bdrv_invalidate_cache) {
- bs->drv->bdrv_invalidate_cache(bs, &local_err);
+ 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;
}
}
- QLIST_FOREACH(child, &bs->children, next) {
- bdrv_invalidate_cache(child->bs, &local_err);
+ bs->open_flags &= ~BDRV_O_INACTIVE;
+ if (bs->drv->bdrv_invalidate_cache) {
+ bs->drv->bdrv_invalidate_cache(bs, &local_err);
if (local_err) {
bs->open_flags |= BDRV_O_INACTIVE;
error_propagate(errp, local_err);
--
2.11.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] block: bdrv_invalidate_cache: invalidate children first
2017-01-31 11:23 [Qemu-devel] [PATCH] block: bdrv_invalidate_cache: invalidate children first Vladimir Sementsov-Ogievskiy
@ 2017-01-31 11:26 ` Vladimir Sementsov-Ogievskiy
2017-02-01 2:03 ` Max Reitz
` (2 subsequent siblings)
3 siblings, 0 replies; 5+ messages in thread
From: Vladimir Sementsov-Ogievskiy @ 2017-01-31 11:26 UTC (permalink / raw
To: qemu-block, qemu-devel
Cc: kwolf, mreitz, armbru, eblake, jsnow, famz, den, stefanha,
pbonzini
v2 missed in topic, sorry for that.
First version was "[PATCH] block: bdrv_invalidate_cache_all: invalidate
children first"
--
Best regards,
Vladimir
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] block: bdrv_invalidate_cache: invalidate children first
2017-01-31 11:23 [Qemu-devel] [PATCH] block: bdrv_invalidate_cache: invalidate children first Vladimir Sementsov-Ogievskiy
2017-01-31 11:26 ` Vladimir Sementsov-Ogievskiy
@ 2017-02-01 2:03 ` Max Reitz
2017-02-01 13:31 ` Stefan Hajnoczi
2017-02-01 20:14 ` Max Reitz
3 siblings, 0 replies; 5+ messages in thread
From: Max Reitz @ 2017-02-01 2:03 UTC (permalink / raw
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: kwolf, armbru, eblake, jsnow, famz, den, stefanha, pbonzini
[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]
On 31.01.2017 12:23, Vladimir Sementsov-Ogievskiy wrote:
> Current implementation invalidates firstly parent bds and then its
> children. This leads to the following bug:
>
> after incoming migration, in bdrv_invalidate_cache_all:
> 1. invalidate parent bds - reopen it with BDRV_O_INACTIVE cleared
> 2. child is not yet invalidated
> 3. parent check that its BDRV_O_INACTIVE is cleared
> 4. parent writes to child
> 5. assert in bdrv_co_pwritev, as BDRV_O_INACTIVE is set for child
>
> This patch fixes it by just changing invalidate sequence: invalidate
> children first.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> v2: I've missed that bdrv_invalidate_cache is already recursive, so we
> can change sequence here. Also v1 doesn't cover the case when
> bdrv_invalidate_cache is called not from bdrv_invalidate_cache_all.
>
> block.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
Reviewed-by: Max Reitz <mreitz@redhat.com>
I'll wait a bit until I apply this patch (please remind me next week if
I forget it...), though, because migration-related things are not
exactly my forte, so maybe someone else has something to say.
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] block: bdrv_invalidate_cache: invalidate children first
2017-01-31 11:23 [Qemu-devel] [PATCH] block: bdrv_invalidate_cache: invalidate children first Vladimir Sementsov-Ogievskiy
2017-01-31 11:26 ` Vladimir Sementsov-Ogievskiy
2017-02-01 2:03 ` Max Reitz
@ 2017-02-01 13:31 ` Stefan Hajnoczi
2017-02-01 20:14 ` Max Reitz
3 siblings, 0 replies; 5+ messages in thread
From: Stefan Hajnoczi @ 2017-02-01 13:31 UTC (permalink / raw
To: Vladimir Sementsov-Ogievskiy
Cc: qemu-block, qemu-devel, kwolf, mreitz, armbru, eblake, jsnow,
famz, den, pbonzini
[-- Attachment #1: Type: text/plain, Size: 1048 bytes --]
On Tue, Jan 31, 2017 at 02:23:08PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> Current implementation invalidates firstly parent bds and then its
> children. This leads to the following bug:
>
> after incoming migration, in bdrv_invalidate_cache_all:
> 1. invalidate parent bds - reopen it with BDRV_O_INACTIVE cleared
> 2. child is not yet invalidated
> 3. parent check that its BDRV_O_INACTIVE is cleared
> 4. parent writes to child
> 5. assert in bdrv_co_pwritev, as BDRV_O_INACTIVE is set for child
>
> This patch fixes it by just changing invalidate sequence: invalidate
> children first.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> v2: I've missed that bdrv_invalidate_cache is already recursive, so we
> can change sequence here. Also v1 doesn't cover the case when
> bdrv_invalidate_cache is called not from bdrv_invalidate_cache_all.
>
> block.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [Qemu-devel] [PATCH] block: bdrv_invalidate_cache: invalidate children first
2017-01-31 11:23 [Qemu-devel] [PATCH] block: bdrv_invalidate_cache: invalidate children first Vladimir Sementsov-Ogievskiy
` (2 preceding siblings ...)
2017-02-01 13:31 ` Stefan Hajnoczi
@ 2017-02-01 20:14 ` Max Reitz
3 siblings, 0 replies; 5+ messages in thread
From: Max Reitz @ 2017-02-01 20:14 UTC (permalink / raw
To: Vladimir Sementsov-Ogievskiy, qemu-block, qemu-devel
Cc: kwolf, armbru, eblake, jsnow, famz, den, stefanha, pbonzini
[-- Attachment #1: Type: text/plain, Size: 983 bytes --]
On 31.01.2017 12:23, Vladimir Sementsov-Ogievskiy wrote:
> Current implementation invalidates firstly parent bds and then its
> children. This leads to the following bug:
>
> after incoming migration, in bdrv_invalidate_cache_all:
> 1. invalidate parent bds - reopen it with BDRV_O_INACTIVE cleared
> 2. child is not yet invalidated
> 3. parent check that its BDRV_O_INACTIVE is cleared
> 4. parent writes to child
> 5. assert in bdrv_co_pwritev, as BDRV_O_INACTIVE is set for child
>
> This patch fixes it by just changing invalidate sequence: invalidate
> children first.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>
> v2: I've missed that bdrv_invalidate_cache is already recursive, so we
> can change sequence here. Also v1 doesn't cover the case when
> bdrv_invalidate_cache is called not from bdrv_invalidate_cache_all.
Thanks, applied to my block tree:
https://github.com/XanClic/qemu/commits/block
Max
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 512 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2017-02-01 20:14 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-01-31 11:23 [Qemu-devel] [PATCH] block: bdrv_invalidate_cache: invalidate children first Vladimir Sementsov-Ogievskiy
2017-01-31 11:26 ` Vladimir Sementsov-Ogievskiy
2017-02-01 2:03 ` Max Reitz
2017-02-01 13:31 ` Stefan Hajnoczi
2017-02-01 20:14 ` Max Reitz
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.