Re: JFFS2 oops when writing to two partitions simultaneously

[Orjan Friberg <of@flatfrog.com>]

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?

-- 
Orjan Friberg
FlatFrog Laboratories AB