On Thu, Apr 02, 2020 at 03:27:22PM +0100, Paul Durrant wrote:
> > -----Original Message-----
> > From: Anthony PERARD <anthony.per...@citrix.com>
> > Sent: 02 April 2020 14:08
> > To: qemu-de...@nongnu.org
> > Cc: qemu-sta...@nongnu.org; Anthony PERARD <anthony.per...@citrix.com>;
> > Stefano Stabellini
> > <sstabell...@kernel.org>; Paul Durrant <p...@xen.org>; Stefan Hajnoczi
> > <stefa...@redhat.com>; Kevin
> > Wolf <kw...@redhat.com>; Max Reitz <mre...@redhat.com>;
> > xen-de...@lists.xenproject.org; qemu-
> > bl...@nongnu.org
> > 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.per...@citrix.com>
>
> 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()).
I'm thinking of going further than that. I've notice another bug, in
case of error in xen_block_do_aio(), xen_block_finish_request() is
called without ever calling send_response() or release_request(). I
think that mean a leak of request.
So, I'm thinking of creating a function that would do finish_request(),
send_response(), release_request(), has I believe those operations needs
to be done together anyway.
I'll rework the patch.
--
Anthony PERARD
