Re: [Qemu-devel] [PATCH v4 3/8] block: add request tracking

[Marcelo Tosatti]

On Wed, Nov 23, 2011 at 11:47:53AM +0000, Stefan Hajnoczi wrote:
> The block layer does not know about pending requests.  This information
> is necessary for copy-on-read since overlapping requests must be
> serialized to prevent races that corrupt the image.
> 
> The BlockDriverState gets a new tracked_request list field which
> contains all pending requests.  Each request is a BdrvTrackedRequest
> record with sector_num, nb_sectors, and is_write fields.
> 
> Note that request tracking is always enabled but hopefully this extra
> work is so small that it doesn't justify adding an enable/disable flag.
> 
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  block.c     |   48 +++++++++++++++++++++++++++++++++++++++++++++++-
>  block_int.h |    4 ++++
>  2 files changed, 51 insertions(+), 1 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 0df7eb9..27c4e84 100644
> --- a/block.c
> +++ b/block.c
> @@ -1071,6 +1071,42 @@ void bdrv_commit_all(void)
>      }
>  }
>  
> +struct BdrvTrackedRequest {
> +    BlockDriverState *bs;
> +    int64_t sector_num;
> +    int nb_sectors;
> +    bool is_write;
> +    QLIST_ENTRY(BdrvTrackedRequest) list;
> +};
> +
> +/**
> + * Remove an active request from the tracked requests list
> + *
> + * This function should be called when a tracked request is completing.
> + */
> +static void tracked_request_end(BdrvTrackedRequest *req)
> +{
> +    QLIST_REMOVE(req, list);
> +}
> +
> +/**
> + * Add an active request to the tracked requests list
> + */
> +static void tracked_request_begin(BdrvTrackedRequest *req,
> +                                  BlockDriverState *bs,
> +                                  int64_t sector_num,
> +                                  int nb_sectors, bool is_write)
> +{
> +    *req = (BdrvTrackedRequest){
> +        .bs = bs,
> +        .sector_num = sector_num,
> +        .nb_sectors = nb_sectors,
> +        .is_write = is_write,
> +    };
> +
> +    QLIST_INSERT_HEAD(&bs->tracked_requests, req, list);
> +}
> +
>  /*
>   * Return values:
>   * 0        - success
> @@ -1350,6 +1386,8 @@ static int coroutine_fn 
> bdrv_co_do_readv(BlockDriverState *bs,
>      int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
>  {
>      BlockDriver *drv = bs->drv;
> +    BdrvTrackedRequest req;
> +    int ret;
>  
>      if (!drv) {
>          return -ENOMEDIUM;
> @@ -1363,7 +1401,10 @@ static int coroutine_fn 
> bdrv_co_do_readv(BlockDriverState *bs,
>          bdrv_io_limits_intercept(bs, false, nb_sectors);
>      }
>  
> -    return drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
> +    tracked_request_begin(&req, bs, sector_num, nb_sectors, false);
> +    ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
> +    tracked_request_end(&req);
> +    return ret;
>  }
>  
>  int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
> @@ -1381,6 +1422,7 @@ static int coroutine_fn 
> bdrv_co_do_writev(BlockDriverState *bs,
>      int64_t sector_num, int nb_sectors, QEMUIOVector *qiov)
>  {
>      BlockDriver *drv = bs->drv;
> +    BdrvTrackedRequest req;
>      int ret;
>  
>      if (!bs->drv) {
> @@ -1398,6 +1440,8 @@ static int coroutine_fn 
> bdrv_co_do_writev(BlockDriverState *bs,
>          bdrv_io_limits_intercept(bs, true, nb_sectors);
>      }
>  
> +    tracked_request_begin(&req, bs, sector_num, nb_sectors, true);
> +
>      ret = drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
>  
>      if (bs->dirty_bitmap) {
> @@ -1408,6 +1452,8 @@ static int coroutine_fn 
> bdrv_co_do_writev(BlockDriverState *bs,
>          bs->wr_highest_sector = sector_num + nb_sectors - 1;
>      }
>  
> +    tracked_request_end(&req);
> +
>      return ret;
>  }

There is no need to worry about synchronous read/write requests
bypassing this interface, correct?