Re: JFFS2 oops when writing to two partitions simultaneously

Joakim Tjernlund
2012-01-26T15:09:30Z

>
> Orjan Friberg <of@flatfrog.com> wrote on 2012/01/26 15:07:07:
> >
> > On 01/26/2012 02:16 PM, Joakim Tjernlund wrote:
> > > Anyhow, I think it is stupid (and probably buggy) to have kfree and kmalloc
> > > as separate. Why is it not done at the same time?
> >
> > To me it looks like the lock must be held the entire time. We can't
> > allow two contexts using (i.e. freeing/allocating/writing to) the
> > compressor's compr_buf.
> >
> > Maybe the lock used here should be on a per-compressor basis, rather
> > than on the list as a whole. (The list lock is still needed when adding
> > to/removing from the list of course.)
>
> Did look some more and to me it seems like the whole kmalloc/kfree dance is pointless.
> The end result is the same as for JFFS2_COMPR_MODE_PRIORITY w.r.t buffer mgmt as the
> compressor doesn't own the buffer anyway. It gets freed by jffs2_free_comprbuf()
> after it has been written to flash.
>
> Maybe I am missing something?

Yes, I did. This dance is there to save you an extra kmalloc in most cases.

Maybe you can get away with removing the extra lock dropping around kfree/kmalloc
and if you do, you can simply the code to something like:

if ((this->compr_buf_size < orig_slen) || !this->compr_buf) {
tmp_buf = kmalloc(orig_slen, GFP_ATOMIC);
if (!tmp_buf) {
printk(KERN_WARNING "JFFS2: No memory for compressor allocation. (%d bytes)\n", orig_slen);
continue;
}
kfree(this->compr_buf);
this->compr_buf = tmp_buf;
this->compr_buf_size = orig_slen;
}

Then have a good look at whether to use orig_slen or orig_dlen

Jocke