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

[Anthony PERARD <anthony.perard@xxxxxxxxxx>]

On Thu, Apr 02, 2020 at 03:27:22PM +0100, Paul Durrant wrote:
> > -----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()).

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