Re: [PATCH v2 for-5.0] xen-block: Fix double qlist remove and request leak
On 06.04.20 16:02, Anthony PERARD wrote:
> 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.
>
> That commit also introduced a leak of request in xen_block_do_aio().
> That function calls xen_block_finish_request() but the request is
> never released after that.
>
> To fix both issue, we do two changes:
> - we squash finish_request() and release_request() together as we want
> to remove a request from 'inflight' list to add it to 'freelist'.
> - before releasing a request, we need to let now the result to the
> other end, thus we should call xen_block_send_response() before
> releasing a request.
>
> The first change fix the double QLIST_REMOVE() as we remove the extra
> call. The second change makes the leak go away because if we want to
> call finish_request(), we need to call a function that do all of
> finish, send response, and release.
>
> Fixes: bfd0d6366043 ("xen-block: improve response latency")
> Signed-off-by: Anthony PERARD
> ---
> hw/block/dataplane/xen-block.c | 48 --
> 1 file changed, 16 insertions(+), 32 deletions(-)
I’m going to send a pull request today anyway, so I hope you won’t mind
and let me take this patch to my branch (with Paul’s suggestions
incorporated):
https://git.xanclic.moe/XanClic/qemu/commits/branch/block
Max
signature.asc
Description: OpenPGP digital signature
RE: [PATCH v2 for-5.0] xen-block: Fix double qlist remove and request leak
> -Original Message-
> From: Anthony PERARD
> Sent: 06 April 2020 15:02
> To: qemu-de...@nongnu.org
> Cc: qemu-sta...@nongnu.org; Anthony PERARD ;
> Stefano Stabellini
> ; Paul Durrant ; Stefan Hajnoczi
> ; Kevin
> Wolf ; Max Reitz ;
> xen-de...@lists.xenproject.org; qemu-
> bl...@nongnu.org
> Subject: [PATCH v2 for-5.0] xen-block: Fix double qlist remove and request
> leak
>
> 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
NIT: I guess you should quote the patch title here as well.
> , where a `finished'
> list was removed.
>
> That commit also introduced a leak of request in xen_block_do_aio().
> That function calls xen_block_finish_request() but the request is
> never released after that.
>
> To fix both issue, we do two changes:
> - we squash finish_request() and release_request() together as we want
> to remove a request from 'inflight' list to add it to 'freelist'.
> - before releasing a request, we need to let now the result to the
> other end,
"we need to let the other end know the result"
> thus we should call xen_block_send_response() before
> releasing a request.
>
> The first change fix the double QLIST_REMOVE() as we remove the extra
s/fix/fixes
> call. The second change makes the leak go away because if we want to
> call finish_request(), we need to call a function that do all of
s/do/does
> finish, send response, and release.
>
> Fixes: bfd0d6366043 ("xen-block: improve response latency")
> Signed-off-by: Anthony PERARD
The code looks ok, so with the cosmetic fixes...
Reviewed-by: Paul Durrant
