From mboxrd@z Thu Jan 1 00:00:00 1970 From: Guangliang Zhao Subject: Re: [PATCH] rbd: fix the memory leak of bio_chain_clone Date: Wed, 4 Jul 2012 11:09:58 +0800 Message-ID: <20120704030958.GA8577@linux-b964.site> References: <1340609863-10971-1-git-send-email-gzhao@suse.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from user.194.126.222.zhong-ren.net ([222.126.194.154]:57643 "EHLO linux-b964.site" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1756744Ab2GDDT4 (ORCPT ); Tue, 3 Jul 2012 23:19:56 -0400 Content-Disposition: inline In-Reply-To: Sender: ceph-devel-owner@vger.kernel.org List-ID: To: Yehuda Sadeh Cc: ceph-devel@vger.kernel.org, Guangliang Zhao On Mon, Jul 02, 2012 at 03:48:14PM -0700, Yehuda Sadeh wrote: > On Mon, Jun 25, 2012 at 12:37 AM, Guangliang Zhao wrote: > > Signed-off-by: Guangliang Zhao > > --- > > drivers/block/rbd.c | 10 ++++------ > > 1 files changed, 4 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/block/rbd.c b/drivers/block/rbd.c > > index 65665c9..3d6dfc8 100644 > > --- a/drivers/block/rbd.c > > +++ b/drivers/block/rbd.c > > @@ -719,8 +719,6 @@ static struct bio *bio_chain_clone(struct bio **old, struct bio **next, > > goto err_out; > > > > if (total + old_chain->bi_size > len) { > > - struct bio_pair *bp; > > - > > /* > > * this split can only happen with a single paged bio, > > * split_bio will BUG_ON if this is not the case > > @@ -732,13 +730,13 @@ static struct bio *bio_chain_clone(struct bio **old, struct bio **next, > > > > /* split the bio. We'll release it either in the next > > call, or it will have to be released outside */ > > - bp = bio_split(old_chain, (len - total) / SECTOR_SIZE); > > - if (!bp) > > + *bp = bio_split(old_chain, (len - total) / SECTOR_SIZE); > > + if (!*bp) > > goto err_out; > > > > - __bio_clone(tmp, &bp->bio1); > > + __bio_clone(tmp, &(*bp)->bio1); > > > > - *next = &bp->bio2; > > + *next = &(*bp)->bio2; > > } else { > > __bio_clone(tmp, old_chain); > > *next = old_chain->bi_next; > > The above patch looks right, that is, fixes a leak. However, I'm not > too sure that we're not missing some bigger problem that was hidden > (due to the leak). Effectively we never called bio_pair_release() on > the bio_pair that we created in bio_chain_clone(). However, I think we > should only call that after we're done with sending the data. The bio_pair is only used to split the orgin bio and clone them to the tmps, no one will touch it again, so I think it is safe release it here. The bio_pair could be freed actually only after 3 times release, because the reference count of bio_pair is initialized to 3 when bio_split and bio_pair_release only drops the reference count. There are not enough release times for the bio_pair even apply either of these patches. I think transferring data with bio1 and bio2 of bio_pair instead of the tmp bio chain is a better way. It can not only save mem(for tmp bio chain), but also manage the reference count of bio_pair more comfortably(with callback of the bio1 and bio2). We can slove the above problems more easily with this approach. Thanks for you reply:) -- Best regards, Guangliang