Re: [Qemu-devel] [PATCH v3 17/29] block: Generalise and optimise COR serialisation

["Benoît Canet" <benoit.canet@irqsave.net>]

Le Friday 17 Jan 2014 à 15:15:07 (+0100), Kevin Wolf a écrit :
> Change the API so that specific requests can be marked serialising. Only

I find the spelling "serialising" instead of "serializing" really odd since QEMU
code is full of z like the word virtualization.

Reviewed-by: Benoit Canet <benoit@irqsave.net>

> these requests are checked for overlaps then.
> 
> This means that during a Copy on Read operation, not all requests
> overlapping other requests are serialised any more, but only those that
> actually overlap with the specific COR request.
> 
> Also remove COR from function and variable names because this
> functionality can be useful in other contexts.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
>  block.c                   | 48 ++++++++++++++++++++++++++++-------------------
>  include/block/block_int.h |  5 +++--
>  2 files changed, 32 insertions(+), 21 deletions(-)
> 
> diff --git a/block.c b/block.c
> index d7156ce..efa8979 100644
> --- a/block.c
> +++ b/block.c
> @@ -2028,6 +2028,10 @@ int bdrv_commit_all(void)
>   */
>  static void tracked_request_end(BdrvTrackedRequest *req)
>  {
> +    if (req->serialising) {
> +        req->bs->serialising_in_flight--;
> +    }
> +
>      QLIST_REMOVE(req, list);
>      qemu_co_queue_restart_all(&req->wait_queue);
>  }
> @@ -2042,10 +2046,11 @@ static void tracked_request_begin(BdrvTrackedRequest *req,
>  {
>      *req = (BdrvTrackedRequest){
>          .bs = bs,
> -        .offset = offset,
> -        .bytes = bytes,
> -        .is_write = is_write,
> -        .co = qemu_coroutine_self(),
> +        .offset         = offset,
> +        .bytes          = bytes,
> +        .is_write       = is_write,
> +        .co             = qemu_coroutine_self(),
> +        .serialising    = false,
>      };
>  
>      qemu_co_queue_init(&req->wait_queue);
> @@ -2053,6 +2058,14 @@ static void tracked_request_begin(BdrvTrackedRequest *req,
>      QLIST_INSERT_HEAD(&bs->tracked_requests, req, list);
>  }
>  
> +static void mark_request_serialising(BdrvTrackedRequest *req)
> +{
> +    if (!req->serialising) {
> +        req->bs->serialising_in_flight++;
> +        req->serialising = true;
> +    }
> +}
> +
>  /**
>   * Round a region to cluster boundaries
>   */
> @@ -2105,26 +2118,31 @@ static bool tracked_request_overlaps(BdrvTrackedRequest *req,
>      return true;
>  }
>  
> -static void coroutine_fn wait_for_overlapping_requests(BlockDriverState *bs,
> -        BdrvTrackedRequest *self, int64_t offset, unsigned int bytes)
> +static void coroutine_fn wait_serialising_requests(BdrvTrackedRequest *self)
>  {
> +    BlockDriverState *bs = self->bs;
>      BdrvTrackedRequest *req;
>      int64_t cluster_offset;
>      unsigned int cluster_bytes;
>      bool retry;
>  
> +    if (!bs->serialising_in_flight) {
> +        return;
> +    }
> +
>      /* If we touch the same cluster it counts as an overlap.  This guarantees
>       * that allocating writes will be serialized and not race with each other
>       * for the same cluster.  For example, in copy-on-read it ensures that the
>       * CoR read and write operations are atomic and guest writes cannot
>       * interleave between them.
>       */
> -    round_bytes_to_clusters(bs, offset, bytes, &cluster_offset, &cluster_bytes);
> +    round_bytes_to_clusters(bs, self->offset, self->bytes,
> +                            &cluster_offset, &cluster_bytes);
>  
>      do {
>          retry = false;
>          QLIST_FOREACH(req, &bs->tracked_requests, list) {
> -            if (req == self) {
> +            if (req == self || (!req->serialising && !self->serialising)) {
>                  continue;
>              }
>              if (tracked_request_overlaps(req, cluster_offset, cluster_bytes)) {
> @@ -2743,12 +2761,10 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
>  
>      /* Handle Copy on Read and associated serialisation */
>      if (flags & BDRV_REQ_COPY_ON_READ) {
> -        bs->copy_on_read_in_flight++;
> +        mark_request_serialising(req);
>      }
>  
> -    if (bs->copy_on_read_in_flight) {
> -        wait_for_overlapping_requests(bs, req, offset, bytes);
> -    }
> +    wait_serialising_requests(req);
>  
>      if (flags & BDRV_REQ_COPY_ON_READ) {
>          int pnum;
> @@ -2797,10 +2813,6 @@ static int coroutine_fn bdrv_aligned_preadv(BlockDriverState *bs,
>      }
>  
>  out:
> -    if (flags & BDRV_REQ_COPY_ON_READ) {
> -        bs->copy_on_read_in_flight--;
> -    }
> -
>      return ret;
>  }
>  
> @@ -2999,9 +3011,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
>      assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
>      assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
>  
> -    if (bs->copy_on_read_in_flight) {
> -        wait_for_overlapping_requests(bs, req, offset, bytes);
> -    }
> +    wait_serialising_requests(req);
>  
>      ret = notifier_with_return_list_notify(&bs->before_write_notifiers, req);
>  
> diff --git a/include/block/block_int.h b/include/block/block_int.h
> index 97a4d23..d8443df 100644
> --- a/include/block/block_int.h
> +++ b/include/block/block_int.h
> @@ -60,6 +60,7 @@ typedef struct BdrvTrackedRequest {
>      int64_t offset;
>      unsigned int bytes;
>      bool is_write;
> +    bool serialising;
>      QLIST_ENTRY(BdrvTrackedRequest) list;
>      Coroutine *co; /* owner, used for deadlock detection */
>      CoQueue wait_queue; /* coroutines blocked on this request */
> @@ -296,8 +297,8 @@ struct BlockDriverState {
>      /* Callback before write request is processed */
>      NotifierWithReturnList before_write_notifiers;
>  
> -    /* number of in-flight copy-on-read requests */
> -    unsigned int copy_on_read_in_flight;
> +    /* number of in-flight serialising requests */
> +    unsigned int serialising_in_flight;
>  
>      /* I/O throttling */
>      ThrottleState throttle_state;
> -- 
> 1.8.1.4
> 
>