[Qemu-devel] Re: [PATCH 3/3] Drop internal bdrv_pread()/bdrv_pwrite() APIs

[Avi Kivity <avi@redhat.com>]

Anthony Liguori wrote:
> Avi Kivity wrote:
>> Now that scsi generic no longer uses bdrv_pread() and bdrv_pwrite(), 
>> we can
>> drop the corresponding internal APIs, which overlap 
>> bdrv_read()/bdrv_write()
>> and, being byte oriented, are unnatural for a block device.
>>
>> Signed-off-by: Avi Kivity <avi@redhat.com>
>>
>>  int bdrv_truncate(BlockDriverState *bs, int64_t offset)
>> diff --git a/block_int.h b/block_int.h
>> index e4630f0..cc9966b 100644
>> --- a/block_int.h
>> +++ b/block_int.h
>> @@ -58,10 +58,6 @@ struct BlockDriver {
>>      int aiocb_size;
>>  
>>      const char *protocol_name;
>> -    int (*bdrv_pread)(BlockDriverState *bs, int64_t offset,
>> -                      uint8_t *buf, int count);
>> -    int (*bdrv_pwrite)(BlockDriverState *bs, int64_t offset,
>> -                       const uint8_t *buf, int count);
>>   
>
> $ grep -l bdrv_pwrite *.c hw/*.c
> block.c
> block-qcow2.c
> block-qcow.c
> block-raw-posix.c
> block-raw-win32.c
> block-vmdk.c
> block-vpc.c
> savevm.c
> hw/scsi-generic.c
>
> So there's a lot of users other than scsi-generic.  Usually, these 
> callers are in the block layer to read/write metadata that isn't 
> always block aligned.  Some buffer adjustment could fix savevm.c to 
> ensure alignment.

It's more accurate to say that there are now no users that depend on the 
request size.  The other users will happily allow expansion of a small 
request to a sector.

> These users are now relegated to using emulated pread/pwrite?  Won't 
> that have a noticable impact on performance if updating small bits of 
> metadata.  For instance, I think updating qcow2 refcounts would look 
> bad since you have to read/write the full block to update 4 bytes of 
> data.  Granted it'll be cached, but...

I haven't measured but I'll bet the impact is unnoticable.  We're 
doubling the syscall count for writes, but the actual transfer (if 
cached) or the I/O (if uncached) will swamp that.  For reads, we're 
copying a bit more data, but that won't even tickle performance.

-- 
I have a truly marvellous patch that fixes the bug which this
signature is too narrow to contain.