Re: [Qemu-devel] [Xen-devel] [PATCH v1] xen_disk: fix memory leak

[Paul Durrant <Paul.Durrant@citrix.com>]

> -----Original Message-----
> From: Xen-devel [mailto:xen-devel-bounces@lists.xenproject.org] On Behalf
> Of Anthony PERARD
> Sent: 13 December 2018 11:34
> To: Olaf Hering <olaf@aepfle.de>
> Cc: Kevin Wolf <kwolf@redhat.com>; Stefano Stabellini
> <sstabellini@kernel.org>; open list:Block layer core <qemu-
> block@nongnu.org>; qemu-devel@nongnu.org; Max Reitz <mreitz@redhat.com>;
> open list:X86 <xen-devel@lists.xenproject.org>
> Subject: Re: [Xen-devel] [PATCH v1] xen_disk: fix memory leak
> 
> On Tue, Dec 11, 2018 at 05:02:24PM +0100, Olaf Hering wrote:
> > 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
> 
> I think it's s/recursive/recursively/.
> 
> > ioreq_reset, which might clobber ioreq->buf as well.
> >
> > Add wrappers to free ioreq->buf before reassignment.
> >
> > Signed-off-by: Olaf Hering <olaf@aepfle.de>
> 
> 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

> 
> > ---
> >  hw/block/xen_disk.c | 22 +++++++++++++++++-----
> >  1 file changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/hw/block/xen_disk.c b/hw/block/xen_disk.c
> > index 36eff94f84..e15eefe625 100644
> > --- a/hw/block/xen_disk.c
> > +++ b/hw/block/xen_disk.c
> > @@ -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.
> 
> > +{
> > +    if (ioreq->buf)
> > +        qemu_vfree(ioreq->buf);
> 
> You could call ioreq_buf_free here instead of duplicating the code.
> 
> > +    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,
> 
> --
> Anthony PERARD
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xenproject.org
> https://lists.xenproject.org/mailman/listinfo/xen-devel