[Qemu-devel] [PATCH] block: record new size in bdrv_dirty_bitmap_truncate

[Qemu-devel] [PATCH] block: record new size in bdrv_dirty_bitmap_truncate

[John Snow]

ce1ffea8 neglected to update the BdrvDirtyBitmap structure
itself for internal consistency. It's currently not an issue,
but for migration and persistence series this will cause headaches.

Signed-off-by: John Snow <jsnow@redhat.com>
---
 block.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block.c b/block.c
index 2b9ceae..2786e47 100644
--- a/block.c
+++ b/block.c
@@ -3224,6 +3224,7 @@ static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
             continue;
         }
         hbitmap_truncate(bitmap->bitmap, size);
+        bitmap->size = size;
     }
 }
 
-- 
2.1.0

Re: [Qemu-devel] [PATCH] block: record new size in bdrv_dirty_bitmap_truncate

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 852 bytes --]

On 06/08/2015 02:49 PM, John Snow wrote:
> ce1ffea8 neglected to update the BdrvDirtyBitmap structure
> itself for internal consistency. It's currently not an issue,
> but for migration and persistence series this will cause headaches.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>
> ---
>  block.c | 1 +
>  1 file changed, 1 insertion(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

> 
> diff --git a/block.c b/block.c
> index 2b9ceae..2786e47 100644
> --- a/block.c
> +++ b/block.c
> @@ -3224,6 +3224,7 @@ static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>              continue;
>          }
>          hbitmap_truncate(bitmap->bitmap, size);
> +        bitmap->size = size;
>      }
>  }
>  
> 

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH] block: record new size in bdrv_dirty_bitmap_truncate

[Kevin Wolf]

Am 08.06.2015 um 22:49 hat John Snow geschrieben:
> ce1ffea8 neglected to update the BdrvDirtyBitmap structure
> itself for internal consistency. It's currently not an issue,
> but for migration and persistence series this will cause headaches.
> 
> Signed-off-by: John Snow <jsnow@redhat.com>

I know nothing about dirty bitmaps, but this one looks obvious enough,
I'll apply it.

> diff --git a/block.c b/block.c
> index 2b9ceae..2786e47 100644
> --- a/block.c
> +++ b/block.c
> @@ -3224,6 +3224,7 @@ static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>              continue;
>          }
>          hbitmap_truncate(bitmap->bitmap, size);
> +        bitmap->size = size;
>      }
>  }

However, I'm left wondering whether that 'continue' in the context of
that hunk is right. More context:

    QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
        if (bdrv_dirty_bitmap_frozen(bitmap)) {
            continue;
        }
        hbitmap_truncate(bitmap->bitmap, size);
    }

If the image just shrunk, the frozen bitmap covers parts of the image
that don't exist any more. When they are read out for the backup, that
request would fail.

If the image was extended, the frozen bitmap covers only part of the
image. There are a few bitmap functions that don't check the size and
would just work beyond the end of the bitmap if called with a now valid
sector number that is outside the image.

In practice, I don't think any of these happen because of op blockers
that prevent resizing while a backup is in progress, but should
!bdrv_dirty_bitmap_frozen(bitmap) be asserted then rather than just
skipping the bitmap?

Kevin

Re: [Qemu-devel] [PATCH] block: record new size in bdrv_dirty_bitmap_truncate

[John Snow]

On 06/09/2015 05:24 AM, Kevin Wolf wrote:
> Am 08.06.2015 um 22:49 hat John Snow geschrieben:
>> ce1ffea8 neglected to update the BdrvDirtyBitmap structure
>> itself for internal consistency. It's currently not an issue,
>> but for migration and persistence series this will cause headaches.
>>
>> Signed-off-by: John Snow <jsnow@redhat.com>
> 
> I know nothing about dirty bitmaps, but this one looks obvious enough,
> I'll apply it.
> 
>> diff --git a/block.c b/block.c
>> index 2b9ceae..2786e47 100644
>> --- a/block.c
>> +++ b/block.c
>> @@ -3224,6 +3224,7 @@ static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
>>              continue;
>>          }
>>          hbitmap_truncate(bitmap->bitmap, size);
>> +        bitmap->size = size;
>>      }
>>  }
> 
> However, I'm left wondering whether that 'continue' in the context of
> that hunk is right. More context:
> 
>     QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
>         if (bdrv_dirty_bitmap_frozen(bitmap)) {
>             continue;
>         }
>         hbitmap_truncate(bitmap->bitmap, size);
>     }
> 
> If the image just shrunk, the frozen bitmap covers parts of the image
> that don't exist any more. When they are read out for the backup, that
> request would fail.
> 
> If the image was extended, the frozen bitmap covers only part of the
> image. There are a few bitmap functions that don't check the size and
> would just work beyond the end of the bitmap if called with a now valid
> sector number that is outside the image.
> 
> In practice, I don't think any of these happen because of op blockers
> that prevent resizing while a backup is in progress, but should
> !bdrv_dirty_bitmap_frozen(bitmap) be asserted then rather than just
> skipping the bitmap?
> 
> Kevin
> 

Yeah, that won't hurt anything and will read cleaner. I'll just v2 this
patch, thanks.

--js

Re: [Qemu-devel] [PATCH] block: record new size in bdrv_dirty_bitmap_truncate

[Kevin Wolf]

Am 09.06.2015 um 17:46 hat John Snow geschrieben:
> 
> 
> On 06/09/2015 05:24 AM, Kevin Wolf wrote:
> > Am 08.06.2015 um 22:49 hat John Snow geschrieben:
> >> ce1ffea8 neglected to update the BdrvDirtyBitmap structure
> >> itself for internal consistency. It's currently not an issue,
> >> but for migration and persistence series this will cause headaches.
> >>
> >> Signed-off-by: John Snow <jsnow@redhat.com>
> > 
> > I know nothing about dirty bitmaps, but this one looks obvious enough,
> > I'll apply it.
> > 
> >> diff --git a/block.c b/block.c
> >> index 2b9ceae..2786e47 100644
> >> --- a/block.c
> >> +++ b/block.c
> >> @@ -3224,6 +3224,7 @@ static void bdrv_dirty_bitmap_truncate(BlockDriverState *bs)
> >>              continue;
> >>          }
> >>          hbitmap_truncate(bitmap->bitmap, size);
> >> +        bitmap->size = size;
> >>      }
> >>  }
> > 
> > However, I'm left wondering whether that 'continue' in the context of
> > that hunk is right. More context:
> > 
> >     QLIST_FOREACH(bitmap, &bs->dirty_bitmaps, list) {
> >         if (bdrv_dirty_bitmap_frozen(bitmap)) {
> >             continue;
> >         }
> >         hbitmap_truncate(bitmap->bitmap, size);
> >     }
> > 
> > If the image just shrunk, the frozen bitmap covers parts of the image
> > that don't exist any more. When they are read out for the backup, that
> > request would fail.
> > 
> > If the image was extended, the frozen bitmap covers only part of the
> > image. There are a few bitmap functions that don't check the size and
> > would just work beyond the end of the bitmap if called with a now valid
> > sector number that is outside the image.
> > 
> > In practice, I don't think any of these happen because of op blockers
> > that prevent resizing while a backup is in progress, but should
> > !bdrv_dirty_bitmap_frozen(bitmap) be asserted then rather than just
> > skipping the bitmap?
> > 
> > Kevin
> > 
> 
> Yeah, that won't hurt anything and will read cleaner. I'll just v2 this
> patch, thanks.

It's unrelated to this patch (except for touching the same function), so
I'd suggest to make it a separate patch.

Kevin