Date: Sat, 02 Apr 2011 00:04:09 +0300 From: Mikolaj Golub <trociny@freebsd.org> To: Victor Balada Diaz <victor@bsdes.net> Cc: Kostik Belousov <kib@FreeBSD.org>, stable@freebsd.org, Pawel Jakub Dawidek <pjd@FreeBSD.org> Subject: Re: geli(4) memory leak Message-ID: <86pqp53cqe.fsf@kopusha.home.net> In-Reply-To: <20110401174354.GE1289@equilibrium.bsdes.net> (Victor Balada Diaz's message of "Fri, 1 Apr 2011 19:43:54 %2B0200") References: <20110326003348.GQ36706@equilibrium.bsdes.net> <20110401174354.GE1289@equilibrium.bsdes.net>
next in thread | previous in thread | raw e-mail | index | archive | help
[-- Attachment #1 --]
VBD> On Sat, Mar 26, 2011 at 01:33:48AM +0100, Victor Balada Diaz wrote:
>> Hello,
>>
>> I'm trying to setup a new geli disk and i'm seeing what looks like a memory leak.
>> After initializing the device i've tried to do the dd command from /dev/random
>> like this one:
>>
>> dd if=/dev/random of=/dev/da0p1.eli bs=1m
>>
VBD> Hello again,
VBD> I've found the cause of the memory leak and i attach a patch to fix it. I hope
VBD> the patch is good enough to get committed or at least helps someone made a better
VBD> patch and commit it. Patched file is src/sys/geom/eli/g_eli.c
VBD> The problem happens when you're using data integrity verification and you need
VBD> to write more than MAXPHYS. If you look at g_eli_integrity.c:314 you'll
VBD> see that geli creates a second request to write all that's needed.
VBD> Each of the request get the callback to g_eli_write_done once they're done. The
VBD> first request will get up to g_eli.c:209 and find that there are still requests
VBD> pending so instead of calling g_io_deliver to notify it's written data, it just
VBD> returns and waits until all requests are done to say everything's OK. The problem
VBD> is that once you return, you're leaking this g_bio. You can see with vmstat -z how
VBD> g_bio increases and never releases memory.
VBD> I just destroy the current bio before returning and that prevents the memory leak.
For me your patch look correct. But the same issue is for read :-). Also, to
avoid the leak I think we can just do g_destroy_bio() before "all sectors"
check. See the attached patch (had some testing).
--
Mikolaj Golub
[-- Attachment #2 --]
Index: sys/geom/eli/g_eli.c
===================================================================
--- sys/geom/eli/g_eli.c (revision 220168)
+++ sys/geom/eli/g_eli.c (working copy)
@@ -160,13 +160,13 @@ g_eli_read_done(struct bio *bp)
pbp = bp->bio_parent;
if (pbp->bio_error == 0)
pbp->bio_error = bp->bio_error;
+ g_destroy_bio(bp);
/*
* Do we have all sectors already?
*/
pbp->bio_inbed++;
if (pbp->bio_inbed < pbp->bio_children)
return;
- g_destroy_bio(bp);
sc = pbp->bio_to->geom->softc;
if (pbp->bio_error != 0) {
G_ELI_LOGREQ(0, pbp, "%s() failed", __func__);
@@ -202,6 +202,7 @@ g_eli_write_done(struct bio *bp)
if (bp->bio_error != 0)
pbp->bio_error = bp->bio_error;
}
+ g_destroy_bio(bp);
/*
* Do we have all sectors already?
*/
@@ -215,7 +216,6 @@ g_eli_write_done(struct bio *bp)
pbp->bio_error);
pbp->bio_completed = 0;
}
- g_destroy_bio(bp);
/*
* Write is finished, send it up.
*/
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?86pqp53cqe.fsf>