xen_disk: fix memory leak
PATCH 1/1 xen_disk: fix memory leak
There are some code paths that clobber ioreq->buf, which leads to a huge memory leak after a few hours of runtime. One code path is qemu_aio_complete, which might be called recursive. Another one is ioreq_reset, which might clobber ioreq->buf as well.
Add wrappers to free ioreq->buf before reassignment.
--- hw/block/xen_disk.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)
hw/block/xen_disk.c +17 -5
@@ -103,12 +103,24 @@ struct XenBlkDev {/* ------------------------------------------------------------- */ +static void ioreq_buf_alloc (struct ioreq *ioreq,size_t alignment) +{ +if (ioreq->buf) +qemu_vfree (ioreq->buf); + ioreq->buf =qemu_memalign (XC_PAGE_SIZE , ioreq->size); +} +static void ioreq_buf_free (struct ioreq *ioreq) +{ +if (ioreq->buf) +qemu_vfree (ioreq->buf); + ioreq->buf =NULL ; +}static void ioreq_reset (struct ioreq *ioreq) { +ioreq_buf_free (ioreq);memset (&ioreq->req,0 ,sizeof (ioreq->req)); ioreq->status =0 ; ioreq->start =0 ; - ioreq->buf =NULL ; ioreq->size =0 ; ioreq->presync =0 ;
@@ -315,14 +327,14 @@ static void qemu_aio_complete(void *opaque, int ret)if (ret ==0 ) {ioreq_grant_copy (ioreq); } -qemu_vfree (ioreq->buf); +ioreq_buf_free (ioreq);break ;case BLKIF_OP_WRITE :case BLKIF_OP_FLUSH_DISKCACHE :if (!ioreq->req.nr_segments) {break ; } -qemu_vfree (ioreq->buf); +ioreq_buf_free (ioreq);break ;default :break ;
@@ -390,12 +402,12 @@ static int ioreq_runio_qemu_aio(struct ioreq *ioreq) {struct XenBlkDev *blkdev = ioreq->blkdev; - ioreq->buf =qemu_memalign (XC_PAGE_SIZE , ioreq->size); +ioreq_buf_alloc (ioreq,XC_PAGE_SIZE );if (ioreq->req.nr_segments && (ioreq->req.operation ==BLKIF_OP_WRITE || ioreq->req.operation ==BLKIF_OP_FLUSH_DISKCACHE ) &&ioreq_grant_copy (ioreq)) { -qemu_vfree (ioreq->buf); +ioreq_buf_free (ioreq); goto err; }
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Patchew URL: https://patchew.org/QEMU/[email protected]/
Patchew URL: https://patchew.org/QEMU/[email protected]/
Hi,
This series seems to have some coding style problems. See output below for more information:
Message-id: [email protected]
Subject: [Qemu-devel] [PATCH v1] xen_disk: fix memory leak
Type: series
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram
commits="$(git log --format=%H --reverse $BASE..)" for c in $commits; do
echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
aaf3499 xen_disk: fix memory leak
=== OUTPUT BEGIN ===
Checking PATCH 1/1: xen_disk: fix memory leak...
ERROR: braces {} are necessary for all arms of this statement #26: FILE: hw/block/xen_disk.c:108:
+ if (ioreq->buf)
[...]
ERROR: braces {} are necessary for all arms of this statement #32: FILE: hw/block/xen_disk.c:114:
+ if (ioreq->buf)
[...]
total: 2 errors, 0 warnings, 55 lines checked
Your patch has style problems, please review. If any of these errors are false positives report them to the maintainer, see CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
The full log is available at
http://patchew.org/logs/[email protected]/testing.checkpatch/?type=message.
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to [email protected]
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 3 lines · Olaf Hering
There are some code paths that clobber
ioreq->buf, which leads to a huge memory leak after a few hours of runtime. One code path isqemu_aio_complete, which might be called recursive. Another one is
I think it's s/recursive/recursively/.
Quoted 5 lines
ioreq_reset, which might clobberioreq->bufas well.Add wrappers to free
ioreq->bufbefore reassignment.Signed-off-by Olaf Hering <[email protected]>
That patch seems fine, with a few coding style issues, and is going to be needed to be forward ported to Paul's reimplementation (not yet merged).
hw/block/xen_disk.c struct XenBlkDev { · line 106
Quoted 13 lines
--- hw/block/xen_disk.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)hw/block/xen_disk.c +1
-0@@ -103,12 +103,24 @@ struct XenBlkDev {/* ------------------------------------------------------------- */ +static void ioreq_buf_alloc (struct ioreq *ioreq,size_t alignment)
You have the parameter alignment but don't actually use it, I don't think it's needed.
hw/block/xen_disk.c struct XenBlkDev { · line 109
Quoted 3 lines
hw/block/xen_disk.c +3
-0+{ +if (ioreq->buf) +qemu_vfree (ioreq->buf);
You could call ioreq_buf_free here instead of duplicating the code.
hw/block/xen_disk.c struct XenBlkDev { · line 117
Quoted 8 lines
hw/block/xen_disk.c +8
-0+ ioreq->buf =qemu_memalign (XC_PAGE_SIZE , ioreq->size); +} +static void ioreq_buf_free (struct ioreq *ioreq) +{ +if (ioreq->buf) +qemu_vfree (ioreq->buf); + ioreq->buf =NULL ; +}
Thanks,
Signature · 6 lines
Anthony PERARD _______________________________________________ Xen-devel mailing list [email protected] https://lists.xenproject.org/mailman/listinfo/xen-devel
Quoted 20 lines
-----Original Message-----
From: Xen-devel [mailto:[email protected]] On Behalf Of Anthony PERARD
Sent: 13 December 2018 11:34
To: Olaf Hering <[email protected]>
Cc: Kevin Wolf <[email protected]>; Stefano Stabellini <[email protected]>; open list:Block layer core <qemu- [email protected]>; [email protected]; Max Reitz <[email protected]>; open list:X86 <[email protected]>
Subject: Re: [Xen-devel] [PATCH v1]xen_disk: fix memory leakQuoted 3 lines · Olaf Hering
There are some code paths that clobber
ioreq->buf, which leads to a huge memory leak after a few hours of runtime. One code path isqemu_aio_complete, which might be called recursive. Another one isI think it's s/recursive/recursively/.
Quoted 2 lines
ioreq_reset, which might clobberioreq->bufas well.
Quoted 7 lines
Quoted 3 lines
Add wrappers to free
ioreq->bufbefore reassignment.Signed-off-by Olaf Hering <[email protected]>That patch seems fine, with a few coding style issues, and is going to be needed to be forward ported to Paul's reimplementation (not yet merged).
I already posted a patch from Tim Smith (re-based to the new xen-block datapath) that should fix this issue.
Paul
Quoted 35 lines
Quoted 10 lines
--- hw/block/xen_disk.c | 22 +++++++++++++++++----- 1 file changed, 17 insertions(+), 5 deletions(-)hw/block/xen_disk.c +0
-0hw/block/xen_disk.c struct XenBlkDev { · line 106
Quoted 4 lines
hw/block/xen_disk.c +1
-0@@ -103,12 +103,24 @@ struct XenBlkDev {/* ------------------------------------------------------------- */ +static void ioreq_buf_alloc (struct ioreq *ioreq,size_t alignment)You have the parameter
alignmentbut don't actually use it, I don't think it's needed.hw/block/xen_disk.c struct XenBlkDev { · line 109
Quoted 3 lines
hw/block/xen_disk.c +3
-0+{ +if (ioreq->buf) +qemu_vfree (ioreq->buf);You could call
ioreq_buf_freehere instead of duplicating the code.hw/block/xen_disk.c struct XenBlkDev { · line 117
Quoted 8 lines
hw/block/xen_disk.c +8
-0+ ioreq->buf =qemu_memalign (XC_PAGE_SIZE , ioreq->size); +} +static void ioreq_buf_free (struct ioreq *ioreq) +{ +if (ioreq->buf) +qemu_vfree (ioreq->buf); + ioreq->buf =NULL ; +}Thanks,
Quoted 7 lines
--
Anthony PERARD_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel
_______________________________________________
Xen-devel mailing list
[email protected]
https://lists.xenproject.org/mailman/listinfo/xen-devel