Re: [Qemu-devel] [PATCH 3/6] block: Prepare bdrv_aligned_pwritev() for byte-aligned requests

[Eric Blake <eblake@redhat.com>]

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

On 06/08/2016 08:10 AM, Kevin Wolf wrote:
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/io.c | 13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> diff --git a/block/io.c b/block/io.c
> index 2fd88cb..4af9c59 100644
> --- a/block/io.c
> +++ b/block/io.c
> @@ -1241,11 +1241,9 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
>      bool waited;
>      int ret;
>  
> -    int64_t sector_num = offset >> BDRV_SECTOR_BITS;
> -    unsigned int nb_sectors = bytes >> BDRV_SECTOR_BITS;
> +    int64_t start_sector = offset >> BDRV_SECTOR_BITS;
> +    int64_t end_sector = DIV_ROUND_UP(offset + bytes, BDRV_SECTOR_SIZE);
>  
> -    assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
> -    assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);

I wonder if we should add an 'unsigned int align' parameter to this
function, to mirror bdrv_aligned_preadv() - that way, we can assert that
any divisions we do based on 'align' are indeed aligned. If we do that,
then just as in the previous patch, I think we would want to keep
'assert((offset & (align - 1)) == 0)'.  But at the moment, I don't see
any divisions based on align, so I think you are okay.


>      if (ret >= 0) {
> -        bs->total_sectors = MAX(bs->total_sectors, sector_num + nb_sectors);
> +        bs->total_sectors = MAX(bs->total_sectors, end_sector);

Someday, we may want bs->total_bytes instead of total_sectors, but
unrelated to this patch.

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

-- 
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 --]