Re: [Qemu-devel] [RFC 1/6] block: add request tracking

[Zhi Yong Wu]

On Mon, Oct 17, 2011 at 11:47 PM, Stefan Hajnoczi
<address@hidden> 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.
>
> Add a simple mechanism to enable/disable request tracking.  By default
> request tracking is disabled.
>
> 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.
>
> Signed-off-by: Stefan Hajnoczi <address@hidden>
> ---
>  block.c     |   83 
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  block_int.h |    8 +++++
>  2 files changed, 90 insertions(+), 1 deletions(-)
>
> diff --git a/block.c b/block.c
> index 9873b57..2d2c62a 100644
> --- a/block.c
> +++ b/block.c
> @@ -978,6 +978,77 @@ 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
> + *
> + * If req is NULL, no operation is performed.
> + *
> + * This function should be called when a tracked request is completing.
> + */
> +static void tracked_request_remove(BdrvTrackedRequest *req)
> +{
> +    if (req) {
> +        QLIST_REMOVE(req, list);
> +        g_free(req);
> +    }
> +}
> +
> +/**
> + * Add an active request to the tracked requests list
> + *
> + * Return NULL if request tracking is disabled, non-NULL otherwise.
> + */
> +static BdrvTrackedRequest *tracked_request_add(BlockDriverState *bs,
> +                                               int64_t sector_num,
> +                                               int nb_sectors, bool is_write)
> +{
> +    BdrvTrackedRequest *req;
> +
> +    if (!bs->request_tracking) {
> +        return NULL;
> +    }
> +
> +    req = g_malloc(sizeof(*req));
> +    req->bs = bs;
> +    req->sector_num = sector_num;
> +    req->nb_sectors = nb_sectors;
> +    req->is_write = is_write;
> +
> +    QLIST_INSERT_HEAD(&bs->tracked_requests, req, list);
> +
> +    return req;
> +}
> +
> +/**
> + * Enable tracking of incoming requests
> + *
> + * Request tracking can be safely used by multiple users at the same time,
> + * there is an internal reference count to match start and stop calls.
> + */
> +void bdrv_start_request_tracking(BlockDriverState *bs)
> +{
> +    bs->request_tracking++;
> +}
> +
> +/**
> + * Disable tracking of incoming requests
> + *
> + * Note that in-flight requests are still tracked, this function only stops
> + * tracking incoming requests.
> + */
> +void bdrv_stop_request_tracking(BlockDriverState *bs)
> +{
> +    bs->request_tracking--;
> +}
I don't understand what the real intention for the above functions is.
IMHO, why can we not drop them?


> +
>  /*
>  * Return values:
>  * 0        - success
> @@ -1262,6 +1333,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;
> @@ -1270,7 +1343,10 @@ static int coroutine_fn 
> bdrv_co_do_readv(BlockDriverState *bs,
>         return -EIO;
>     }
>
> -    return drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
> +    req = tracked_request_add(bs, sector_num, nb_sectors, false);
> +    ret = drv->bdrv_co_readv(bs, sector_num, nb_sectors, qiov);
> +    tracked_request_remove(req);
> +    return ret;
>  }
>
>  int coroutine_fn bdrv_co_readv(BlockDriverState *bs, int64_t sector_num,
> @@ -1288,6 +1364,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) {
> @@ -1300,6 +1377,8 @@ static int coroutine_fn 
> bdrv_co_do_writev(BlockDriverState *bs,
>         return -EIO;
>     }
>
> +    req = tracked_request_add(bs, sector_num, nb_sectors, true);
> +
>     ret = drv->bdrv_co_writev(bs, sector_num, nb_sectors, qiov);
>
>     if (bs->dirty_bitmap) {
> @@ -1310,6 +1389,8 @@ static int coroutine_fn 
> bdrv_co_do_writev(BlockDriverState *bs,
>         bs->wr_highest_sector = sector_num + nb_sectors - 1;
>     }
>
> +    tracked_request_remove(req);
> +
>     return ret;
>  }
>
> diff --git a/block_int.h b/block_int.h
> index f2f4f2d..87ce8b4 100644
> --- a/block_int.h
> +++ b/block_int.h
> @@ -43,6 +43,8 @@
>  #define BLOCK_OPT_PREALLOC      "preallocation"
>  #define BLOCK_OPT_SUBFMT        "subformat"
>
> +typedef struct BdrvTrackedRequest BdrvTrackedRequest;
> +
>  typedef struct AIOPool {
>     void (*cancel)(BlockDriverAIOCB *acb);
>     int aiocb_size;
> @@ -206,6 +208,9 @@ struct BlockDriverState {
>     int in_use; /* users other than guest access, eg. block migration */
>     QTAILQ_ENTRY(BlockDriverState) list;
>     void *private;
> +
> +    int request_tracking;       /* reference count, 0 means off */
> +    QLIST_HEAD(, BdrvTrackedRequest) tracked_requests;
>  };
>
>  struct BlockDriverAIOCB {
> @@ -216,6 +221,9 @@ struct BlockDriverAIOCB {
>     BlockDriverAIOCB *next;
>  };
>
> +void bdrv_start_request_tracking(BlockDriverState *bs);
> +void bdrv_stop_request_tracking(BlockDriverState *bs);
> +
>  void get_tmp_filename(char *filename, int size);
>
>  void *qemu_aio_get(AIOPool *pool, BlockDriverState *bs,
> --
> 1.7.6.3
>
>
>



-- 
Regards,

Zhi Yong Wu