From mboxrd@z Thu Jan  1 00:00:00 1970
From: Wen Congyang <wency@cn.fujitsu.com>
Subject: Re: [RFC Patch v3 09/18] block-remus: fix memory leak
Date: Thu, 25 Sep 2014 13:23:04 +0800
Message-ID: <5423A6B8.4050001@cn.fujitsu.com>
References: <1409908261-18682-1-git-send-email-wency@cn.fujitsu.com>	<1409908261-18682-10-git-send-email-wency@cn.fujitsu.com>
	<CAP8mzPOFEiycJv4cH4xekVAQVzJbDj187u3vjybDTRvZ9JuDUw@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <xen-devel-bounces@lists.xen.org>
In-Reply-To: <CAP8mzPOFEiycJv4cH4xekVAQVzJbDj187u3vjybDTRvZ9JuDUw@mail.gmail.com>
List-Unsubscribe: <http://lists.xen.org/cgi-bin/mailman/options/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=unsubscribe>
List-Post: <mailto:xen-devel@lists.xen.org>
List-Help: <mailto:xen-devel-request@lists.xen.org?subject=help>
List-Subscribe: <http://lists.xen.org/cgi-bin/mailman/listinfo/xen-devel>,
	<mailto:xen-devel-request@lists.xen.org?subject=subscribe>
Sender: xen-devel-bounces@lists.xen.org
Errors-To: xen-devel-bounces@lists.xen.org
To: rshriram@cs.ubc.ca
Cc: Lai Jiangshan <laijs@cn.fujitsu.com>, Ian Jackson <Ian.Jackson@eu.citrix.com>, Jiang Yunhong <yunhong.jiang@intel.com>, Dong Eddie <eddie.dong@intel.com>, xen devel <xen-devel@lists.xen.org>, Yang Hongyang <yanghy@cn.fujitsu.com>, Ian Campbell <ian.campbell@citrix.com>
List-Id: xen-devel@lists.xenproject.org

On 09/25/2014 03:37 AM, Shriram Rajagopalan wrote:
> On Sep 5, 2014 5:15 AM, "Wen Congyang" <wency@cn.fujitsu.com> wrote:
>>
>> Fix the following two memory leak:
>> 1. If s->ramdisk.prev is not NULL, we merge the write requests in
>>    s->ramdisk.h into s->ramdisk.prev, and then destroy s->ramdisk.h.
>>    But we forget to free hash value when destroying s->ramdisk.h.
> 
> I am responding from my phone. So I don't have the code in hand to validate
> your claim. I think the merge function reuses the value block (write
> request) instead of doing a memcpy. In which case, this patch will be
> freeing the buffer that is queued for write. Is that right?

No, if we have cached the sector in s->ramdisk.prev, we just use memcpy.
Otherwise, we alloc memory and use memcpy. Why we don't reuse the buff?
Because the buff may be in the stack...

> 
>> 2. When write requests is finished, replicated_write_callback() will
>>    be called. We forget free the buff in this function.
> 
> Wasn't that done explicitly in the write req done function, where a
> free(buf) was introduced? (Vague recollection... I am not sure if I pushed
> that fix upstream either :( )

Sorry, I forgot to update the commit message. Bug 2 has been fixed...

> 
> Either way, if you have a working Remus setup, can you confirm that this
> patch does not cause double free error? (Just this patch and no other Remus
> related fixes).

Hmm, I don't test it for newest xen, because only drbd is supported now.

Thanks
Wen COngyang

> 
>>
>> Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
>> Signed-off-by: Jiang Yunhong <yunhong.jiang@intel.com>
>> Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
>> Cc: Shriram Rajagopalan <rshriram@cs.ubc.ca>
>> ---
>>  tools/blktap2/drivers/block-remus.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/blktap2/drivers/block-remus.c
> b/tools/blktap2/drivers/block-remus.c
>> index 079588d..4ce9dbe 100644
>> --- a/tools/blktap2/drivers/block-remus.c
>> +++ b/tools/blktap2/drivers/block-remus.c
>> @@ -602,7 +602,7 @@ static int ramdisk_start_flush(td_driver_t *driver)
>>                 }
>>                 free(sectors);
>>
>> -               hashtable_destroy (s->ramdisk.h, 0);
>> +               hashtable_destroy (s->ramdisk.h, 1);
>>         } else
>>                 s->ramdisk.prev = s->ramdisk.h;
>>
>> --
>> 1.9.3
>>
>