RE: [PATCH for-5.0] xen-block: Fix double qlist remove

[Paul Durrant <xadimgnik@xxxxxxxxx>]

> -----Original Message-----
> From: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> Sent: 02 April 2020 14:08
> To: qemu-devel@xxxxxxxxxx
> Cc: qemu-stable@xxxxxxxxxx; Anthony PERARD <anthony.perard@xxxxxxxxxx>; 
> Stefano Stabellini
> <sstabellini@xxxxxxxxxx>; Paul Durrant <paul@xxxxxxx>; Stefan Hajnoczi 
> <stefanha@xxxxxxxxxx>; Kevin
> Wolf <kwolf@xxxxxxxxxx>; Max Reitz <mreitz@xxxxxxxxxx>; 
> xen-devel@xxxxxxxxxxxxxxxxxxxx; qemu-
> block@xxxxxxxxxx
> Subject: [PATCH for-5.0] xen-block: Fix double qlist remove
> 
> Commit a31ca6801c02 ("qemu/queue.h: clear linked list pointers on
> remove") revealed that a request was removed twice from a list, once
> in xen_block_finish_request() and a second time in
> xen_block_release_request() when both function are called from
> xen_block_complete_aio(). But also, the `requests_inflight' counter is
> decreased twice, and thus became negative.
> 
> This is a bug that was introduced in bfd0d6366043, where a `finished'
> list was removed.
> 
> This patch simply re-add the `finish' parameter of
> xen_block_release_request() so that we can distinguish when we need to
> remove a request from the inflight list and when not.
> 
> Fixes: bfd0d6366043 ("xen-block: improve response latency")
> Signed-off-by: Anthony PERARD <anthony.perard@xxxxxxxxxx>

It looks to me like it would just be more straightforward to simply drop the 
QLIST_REMOVE and requests_inflight-- from
xen_block_release_request() and simply insist that xen_block_finish_request() 
is called in all cases (which I think means adding one
extra call to it in xen_block_handle_requests()).

  Paul

> ---
>  hw/block/dataplane/xen-block.c | 14 +++++++++-----
>  1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/block/dataplane/xen-block.c b/hw/block/dataplane/xen-block.c
> index 288a87a814ad..6cc089fc561f 100644
> --- a/hw/block/dataplane/xen-block.c
> +++ b/hw/block/dataplane/xen-block.c
> @@ -123,15 +123,19 @@ static void xen_block_finish_request(XenBlockRequest 
> *request)
>      dataplane->requests_inflight--;
>  }
> 
> -static void xen_block_release_request(XenBlockRequest *request)
> +static void xen_block_release_request(XenBlockRequest *request, bool finish)
>  {
>      XenBlockDataPlane *dataplane = request->dataplane;
> 
> -    QLIST_REMOVE(request, list);
> +    if (!finish) {
> +        QLIST_REMOVE(request, list);
> +    }
>      reset_request(request);
>      request->dataplane = dataplane;
>      QLIST_INSERT_HEAD(&dataplane->freelist, request, list);
> -    dataplane->requests_inflight--;
> +    if (!finish) {
> +        dataplane->requests_inflight--;
> +    }
>  }
> 
>  /*
> @@ -316,7 +320,7 @@ static void xen_block_complete_aio(void *opaque, int ret)
>              error_report_err(local_err);
>          }
>      }
> -    xen_block_release_request(request);
> +    xen_block_release_request(request, true);
> 
>      if (dataplane->more_work) {
>          qemu_bh_schedule(dataplane->bh);
> @@ -585,7 +589,7 @@ static bool xen_block_handle_requests(XenBlockDataPlane 
> *dataplane)
>                      error_report_err(local_err);
>                  }
>              }
> -            xen_block_release_request(request);
> +            xen_block_release_request(request, false);
>              continue;
>          }
> 
> --
> Anthony PERARD