Re: [Qemu-devel] [PATCH 01/10] block/io: add bdrv_co_write_compressed

[Pavel Butsykin <pbutsykin@virtuozzo.com>]

On 16.05.2016 19:52, Eric Blake wrote:
> On 05/14/2016 06:45 AM, Denis V. Lunev wrote:
>> From: Pavel Butsykin <pbutsykin@virtuozzo.com>
>>
>> This patch just adds the interface to the bdrv_co_write_compressed, which
>> is currently not used but will be useful for safe implementation of the
>> bdrv_co_write_compressed callback in format drivers.
>>
>> Signed-off-by: Pavel Butsykin <pbutsykin@virtuozzo.com>
>> Signed-off-by: Denis V. Lunev <den@openvz.org>
>> CC: Jeff Cody <jcody@redhat.com>
>> CC: Markus Armbruster <armbru@redhat.com>
>> CC: Eric Blake <eblake@redhat.com>
>> CC: John Snow <jsnow@redhat.com>
>> CC: Stefan Hajnoczi <stefanha@redhat.com>
>> CC: Kevin Wolf <kwolf@redhat.com>
>> ---
>
>> +++ b/block/io.c
>> @@ -1828,8 +1828,8 @@ int bdrv_is_allocated_above(BlockDriverState *top,
>>       return 0;
>>   }
>>
>> -int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>> -                          const uint8_t *buf, int nb_sectors)
>> +int bdrv_co_write_compressed(BlockDriverState *bs, int64_t sector_num,
>> +                             int nb_sectors, QEMUIOVector *qiov)
>
> As long as we're adding a new public interface, I'd really like us to
> make it byte-based.  int64_t sector_num might be better represented as a
> byte offset, and int nb_sectors seems redundant with qiov->size.
>
>>   {
>>       BlockDriver *drv = bs->drv;
>>       int ret;
>> @@ -1837,7 +1837,7 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>       if (!drv) {
>>           return -ENOMEDIUM;
>>       }
>> -    if (!drv->bdrv_write_compressed) {
>> +    if (!drv->bdrv_co_write_compressed) {
>>           return -ENOTSUP;
>>       }
>>       ret = bdrv_check_request(bs, sector_num, nb_sectors);
>> @@ -1846,8 +1846,71 @@ int bdrv_write_compressed(BlockDriverState *bs, int64_t sector_num,
>>       }
>>
>>       assert(QLIST_EMPTY(&bs->dirty_bitmaps));
>> +    assert(qemu_in_coroutine());
>> +
>> +    return drv->bdrv_co_write_compressed(bs, sector_num, nb_sectors, qiov);
>
> Of course, if you make the public interface byte-based, then calling
> into the back end will have to scale back to sectors (after first
> asserting that we aren't violating the scaling); see how Kevin did it in
> commit 166fe9605.
>
>> +}
>> +
>> +typedef struct BdrvWriteCompressedCo {
>> +    BlockDriverState *bs;
>> +    int64_t sector_num;
>
> Again, I think a byte offset is smarter than a sector number.
>

Kevin used the byte offset for functions bdrv_driver_pread/_pwrite(It
looks like just an additional interface), which is not the same thing.
Here the bdrv_co/bdrv_write_compressed functions are analogues of the
bdrv_co/bdrv_write functions that still use sectors in the arguments.
So I'm not sure that the interface there needs to be some other.


Kevin, what do you think about this?