Re: [RFC Patch v3 09/18] block-remus: fix memory leak

[Wen Congyang <wency@cn.fujitsu.com>]

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
>>
>