Re: [Qemu-block] [Qemu-devel] [PATCH v7 05/20] dirty-bitmap: Check for size query failure during truncate

[Eric Blake]

On 09/13/2017 06:27 PM, John Snow wrote:
> 
> 
> On 09/12/2017 04:31 PM, Eric Blake wrote:
>> We've previously fixed several places where we failed to account
>> for possible errors from bdrv_nb_sectors().  Fix another one by
>> making bdrv_dirty_bitmap_truncate() report the error rather then
>> silently resizing bitmaps to -1.  Then adjust the sole caller
> 
> Nice failure mode. It was not immediately obvious to me that this could
> fail, but here we all are.
> 
>> bdrv_truncate() to both reduce the likelihood of failure (blindly
>> calling bdrv_dirty_bitmap_truncate() after refresh_total_sectors()
>> fails was not nice) as well as propagate any actual failures.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>>

>>      ret = drv->bdrv_truncate(bs, offset, prealloc, errp);
>> -    if (ret == 0) {
>> -        ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
>> -        bdrv_dirty_bitmap_truncate(bs);
>> -        bdrv_parent_cb_resize(bs);
>> -        atomic_inc(&bs->write_gen);
>> +    if (ret < 0) {
>> +        return ret;
>>      }
>> +    ret = refresh_total_sectors(bs, offset >> BDRV_SECTOR_BITS);
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, -ret, "Could not refresh total sector 
>> count");
>> +        return ret;
>> +    }
>> +    ret = bdrv_dirty_bitmap_truncate(bs);
>> +    if (ret < 0) {
>> +        error_setg_errno(errp, -ret, "Could not refresh total sector 
>> count");
> 
> You probably meant to write a different error message here.
> 
> Also, what happens if the actual truncate call works, but the bitmap
> resizing fails? What state does that leave us in?

Another option: bdrv_dirty_bitmap_truncate() can only fail if
bdrv_nb_sectors() can fail.  We WANT to use the actual size of the
device (which might be slightly different than the requested size passed
to bdrv_truncate in the first place), but we could change
bdrv_dirty_bitmap_truncate() to take an actual size as a parameter
instead of having to query bdrv_nb_sectors() for the size; and we can
change the contract of refresh_total_sectors() to query the actual size
before returning (remember, offset >> BDRV_SECTOR_BITS is only the hint
size, and may differ from the actual size).  That way, if
refresh_total_sectors() succeeds, then bdrv_dirty_bitmap_truncate()
cannot fail.

I'm not sure, however, how invasive it will be to make
refresh_total_sectors() guarantee that it can return the actual size
that was set even when that size differs from the hint.  Still, it
sounds like the right approach to take, so I'll play with it.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature