Re: [Qemu-devel] [PATCH v3 21/29] block: Assert serialisation assumptions in pwritev

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

Le Friday 17 Jan 2014 à 15:15:11 (+0100), Kevin Wolf a écrit :
> If a request calls wait_serialising_requests() and actually has to wait
> in this function (i.e. a coroutine yield), other requests can run and
> previously read data (like the head or tail buffer) could become
> outdated. In this case, we would have to restart from the beginning to
> read in the updated data.
> 
> However, we're lucky and don't actually need to do that: A request can
> only wait in the first call of wait_serialising_requests() because we
> mark it as serialising before that call, so any later requests would
> wait. So as we don't wait in practice, we don't have to reload the data.
> 
> This is an important assumption that may not be broken or data
> corruption will happen. Document it with some assertions.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/block.c b/block.c
> index 859e1aa..53d9bd5 100644
> --- a/block.c
> +++ b/block.c
> @@ -2123,14 +2123,15 @@ static bool tracked_request_overlaps(BdrvTrackedRequest *req,
>      return true;
>  }
>  
> -static void coroutine_fn wait_serialising_requests(BdrvTrackedRequest *self)
> +static bool coroutine_fn wait_serialising_requests(BdrvTrackedRequest *self)
>  {
>      BlockDriverState *bs = self->bs;
>      BdrvTrackedRequest *req;
>      bool retry;
> +    bool waited = false;
>  
>      if (!bs->serialising_in_flight) {
> -        return;
> +        return false;
>      }
>  
>      do {
> @@ -2156,11 +2157,14 @@ static void coroutine_fn wait_serialising_requests(BdrvTrackedRequest *self)
>                      qemu_co_queue_wait(&req->wait_queue);
>                      self->waiting_for = NULL;
>                      retry = true;
> +                    waited = true;
>                      break;
>                  }
>              }
>          }
>      } while (retry);
> +
> +    return waited;
>  }
>  
>  /*
> @@ -3011,6 +3015,7 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
>      QEMUIOVector *qiov, int flags)
>  {
>      BlockDriver *drv = bs->drv;
> +    bool waited;
>      int ret;
>  
>      int64_t sector_num = offset >> BDRV_SECTOR_BITS;
> @@ -3019,7 +3024,8 @@ static int coroutine_fn bdrv_aligned_pwritev(BlockDriverState *bs,
>      assert((offset & (BDRV_SECTOR_SIZE - 1)) == 0);
>      assert((bytes & (BDRV_SECTOR_SIZE - 1)) == 0);
>  
> -    wait_serialising_requests(req);
> +    waited = wait_serialising_requests(req);
> +    assert(!waited || !req->serialising);

I we apply De Morgan's law to the expression we have:

    assert(!(waited && req->serialising));

Is it really the condition we want ?

Best regards

Benoît

>  
>      ret = notifier_with_return_list_notify(&bs->before_write_notifiers, req);
>  
> @@ -3119,9 +3125,11 @@ static int coroutine_fn bdrv_co_do_pwritev(BlockDriverState *bs,
>          QEMUIOVector tail_qiov;
>          struct iovec tail_iov;
>          size_t tail_bytes;
> +        bool waited;
>  
>          mark_request_serialising(&req, align);
> -        wait_serialising_requests(&req);
> +        waited = wait_serialising_requests(&req);
> +        assert(!waited || !use_local_qiov);
>  
>          tail_buf = qemu_blockalign(bs, align);
>          tail_iov = (struct iovec) {
> -- 
> 1.8.1.4
> 
>