Re: JFFS2 oops when writing to two partitions simultaneously

Orjan Friberg
2012-01-26T12:51:33Z

On 01/26/2012 12:53 PM, Joakim Tjernlund wrote:
> /* Allocating memory for output buffer if necessary */
> if ((this->compr_buf_size< orig_slen)&& (this->compr_buf)) {
> spin_unlock(&jffs2_compressor_list_lock);
>
> kfree(this->compr_buf);
> spin_lock(&jffs2_compressor_list_lock);
> this->compr_buf_size=0;
> this->compr_buf=NULL;
> }
>
> if 2 threads are competing here, I don't think you can drop the spin lock
> temporarily as this routine do.

Agreed. Both the freeing of this->compr_buf and the usage of it when
calling the compressor looks weird (because another process holding the
lock could decide that the buffer is too small and allocate a new one):

spin_unlock(&jffs2_compressor_list_lock);
*datalen = orig_slen;
*cdatalen = orig_dlen;
compr_ret = this->compress(data_in, this->compr_buf, datalen, cdatalen);
spin_lock(&jffs2_compressor_list_lock);

I'm not sure I'm crazy about the allocation either, come to think of it:

if (!this->compr_buf) {
spin_unlock(&jffs2_compressor_list_lock);
tmp_buf = kmalloc(orig_slen, GFP_KERNEL);
spin_lock(&jffs2_compressor_list_lock);
if (!tmp_buf) {
printk(KERN_WARNING "JFFS2: No memory for compressor allocation. (%d
bytes)\n", orig_slen);
continue;
}
else {
this->compr_buf = tmp_buf;
this->compr_buf_size = orig_slen;
}
}

Even though we hold the lock when assigning the new buffer, things could
have been changed while we're doing the kmalloc. In this case, maybe
just dropping the unlock/lock and allocating with GFP_ATOMIC would solve it.

I'm not sure I see why compr_buf has to belong to the compressor. To
not have to kmalloc a buffer each and every time?