Re: Re: Re: [PATCH] UBI: fix memory leak when use fastmap

[richard -rw- weinberger]

On Fri, May 3, 2013 at 5:31 AM,  <wang.bo116@xxxxxxxxxx> wrote:
> richard -rw- weinberger <richard.weinberger@xxxxxxxxx> write 2013-05-03
> 03:21:33:
>
>> On Thu, May 2, 2013 at 10:30 AM,  <wang.bo116@xxxxxxxxxx> wrote:
>> >> Hi!
>> >>
>> >> On Tue, Apr 16, 2013 at 10:53 AM,  <wang.bo116@xxxxxxxxxx> wrote:
>> >> > Hello,
>> >> >         Sorry, there is still something wrong with the previous
>> > patch's
>> >> > format, try to submit it again. When use ubi fastmap, there is a
>> > memory
>> >> > leak which will make destroy_ai() fail to free the slab alloced in
>> >> > scan_fast(). The following patch fix this problem by use a
> temporary
>> >> > "ubi_attach_info" variable in scan_fast().
>> >>
>> >> Thanks a lot for your patch!
>> >>
>> >> Did you test it well?
>> >> We need to make sure that it does the right thing for the following
>> > cases:
>> >> 1. fastmap disabled, attaching a non-fastmap volume
>> >> 1. fastmap disabled, attaching a fastmap volume
>> >> 3. fastmap enabled, attaching a non-fastmap volume
>> >> 4. fastmap enablled, attaching a fastmap volume
>> >>
>> >> --
>> >> Thanks,
>> >> //richard
>> >
>> >
>> > Thanks for your advice. I consider this problem like this:
>> > 1: if fastmap not config, ubi_attach() just call scan_all(ubi, ai, 0),
>> > there is nothing changed, so there is all right.
>> >
>> > 2: if fastmap config, and force_scan is 0, ubi_attach() will call
>> > scan_fast(), when scan_fast() return, temp ai used in scan_fast() has
> been
>> > free, and there will be two conditions:
>> > A: scan_fast() return UBI_NO_FASTMAP(may be flash is empty or
> attaching a
>> > non-fastmap volume), all ubi_attach() need is to call scan_all(ubi,
> ai, 0)
>> > to scan all blocks.
>> > B: scan_fast() return UBI_BAD_FASTMAP, ubi_attach() first free ai used
> in
>> > ubi_scan_fastmap(), then alloc a clean ai, at last call scan_all(ubi,
> ai,
>> > 0) to scan all blocks.
>> >
>> > 3: if fastmap config, and force_scan is 1, just call scan_all(ubi, ai,
> 0).
>> >
>> > This patch pass the following cases(include attach and detach):
>> > 1. fastmap config, flash is empty,fm_autoconvert is 1.
>> > 2. fastmap config, flash is empty,fm_autoconvert is 0.
>> > 3. fastmap config, attaching a fastmap volume
>> > 4. fastmap config, attaching a bad fastmap volume
>> > 5. fastmap config, attaching a non-fastmap volume
>> > 6. fastmap not config, attaching a fastmap volume
>> > 7. fastmap not config, attaching a non-fastmap volume
>> > 8. fastmap not config, flash is empty.
>>
>> Good. :)
>>
>> > By the way, the problem may cause ubi_attach() fail after ubi detach
> when
>> > CONFIG_DEBUG_VM config.
>> > The reason is that ubi_attach() will alloc a slab cache, but the
>> > kmem_cache_sanity_check() will find the slab cache is already
> exist(not
>> > been free after last detach), so slab cache alloc fail, and
> ubi_attach()
>> > fail, too.
>>
>> Doesn't your patch address this issue too?
>> If not, we should fix this.Th
>
> The patch had already fix the above problem mentioned.
> Now, all the slabs belong to the slab-cache named "ubi_aeb_slab_cache" can
> be rightly freed, so the slab-cache can be rightly freed when ubi_attach()
> return.
> When ubi attach again after detach, ubi can alloc a slab-cache named "
> ubi_aeb_slab_cache" again.

I tried to apply/test your patch.
It has lots of white spaces damages.
Can you please resend it using git send-email?
And also run checkpatch.pl before sending.

--
Thanks,
//richard
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/