Re: [Qemu-devel] [PATCH 1/3 v4] Expose a mechanisem to trace block writes

[Anthony Liguori]

Hi Liran,

address@hidden wrote:
> To support live migration without shared storage we need to be able to trace
> writes to disk while migrating. This Patch expose handler registration for 
> above components to be notified about block writes.
>
> diff --git a/block.c b/block.c
> index 33f3d65..bf5f7a6 100644
> --- a/block.c
> +++ b/block.c
> @@ -61,6 +61,8 @@ BlockDriverState *bdrv_first;
>  
>  static BlockDriver *first_drv;
>  
> +static BlockDriverDirtyHandler *bdrv_dirty_handler = NULL;
> +
>   

Should be a property of a BlockDriverState.  IOW, we should register a 
dirty callback for each block device we're interested in.

>  int path_is_absolute(const char *path)
>  {
>      const char *p;
> @@ -626,6 +628,10 @@ int bdrv_write(BlockDriverState *bs, int64_t sector_num,
>      if (bdrv_check_request(bs, sector_num, nb_sectors))
>          return -EIO;
>  
> +    if(bdrv_dirty_handler != NULL) {
> +      bdrv_dirty_handler(bs, sector_num, nb_sectors);
> +    }
> +    
>      return drv->bdrv_write(bs, sector_num, buf, nb_sectors);
>  }
>   

CodingStyle seems off.

We have to be careful in these cases to check for whether we're dealing 
with BDRV_FILE.  In the case of something like qcow2, you would get two 
dirty callbacks as this code stands.  The first would be what the guest 
actually writes and then the second (and potentially third) would be 
qcow2 metadata updates along with writing the actual data to disk.

In terms of an interface, I think it would be better to register a 
bitmap and to poll the block driver periodically to see which bits have 
changed.  This is how ram dirty tracking works and I think keeping these 
interfaces consistent is a good thing.

I'd suggest tracking dirtiness in relatively large chunks (at least 2MB).

>  
> @@ -1359,6 +1370,10 @@ BlockDriverAIOCB *bdrv_aio_writev(BlockDriverState *bs, int64_t sector_num,
>      if (bdrv_check_request(bs, sector_num, nb_sectors))
>          return NULL;
>  
> +    if(bdrv_dirty_handler != NULL) {
> +      bdrv_dirty_handler(bs, sector_num, nb_sectors);
> +    }
> +    
>      ret = drv->bdrv_aio_writev(bs, sector_num, qiov, nb_sectors,
>                                 cb, opaque);
>   

The check should be in the completion callback as AIO requests can be 
canceled.  You're potentially giving a false positive.

Regards,

Anthony Liguori