Re: UBI leb_write_unlock NULL pointer Oops (continuation)

[Richard Weinberger <richard@nod.at>]

Am 24.02.2014 16:09, schrieb Bill Pringlemeir:
> On 22 Feb 2014, richard@nod.at wrote:
> 
>> Am 22.02.2014 01:49, schrieb Bill Pringlemeir:
> 
>>> I think the 'ubi_eba_copy_leb()' may have an issue.
>>>
>>> int ubi_eba_copy_leb(struct ubi_device *ubi, int from, int to,
>>> 		     struct ubi_vid_hdr *vid_hdr)
>>> {
>>> ...
>>>
>>> 	err = leb_write_trylock(ubi, vol_id, lnum);
>>>
>>> static int leb_write_trylock(struct ubi_device *ubi, int vol_id, int lnum)
>>> {
>>> ..
>>> 	le = ltree_add_entry(ubi, vol_id, lnum);  /* users + 1 */
>>> 	if (IS_ERR(le))
>>> 		return PTR_ERR(le);
>>> 	if (down_write_trylock(&le->mutex))
>>> 		return 0;
>>>
>>> 	/* Contention, cancel */
>>> 	spin_lock(&ubi->ltree_lock);
>>> 	le->users -= 1;                           /* user - 1 */
>>> ...
>>> 	spin_unlock(&ubi->ltree_lock);           
>>>
>>> 	return 1;
>>> }
>>>
>>> 	if (err)...
>>>
> 	if (vol->eba_tbl[lnum] != from) {
> 		dbg_wl("LEB %d:%d is no longer mapped to PEB %d, mapped to PEB %d, cancel",
> 		       vol_id, lnum, from, vol->eba_tbl[lnum]);
> 		err = MOVE_CANCEL_RACE;
> 		goto out_unlock_leb;
> 	}
> ...
> out_unlock_leb:
> 	leb_write_unlock(ubi, vol_id, lnum);     /* user - 1 */
> 
> 
>>> Didn't the count have to bump up in this case?  This isn't in Thorsten's
>>> traces, but neither are any 'down_read' or 'up_read' traces; maybe
>>> everything is in cache?
> 
>> Hmm, I'm not sure whether I was able to follow your thought.  But
>> leb_write_unlock() is balanced with leb_write_trylock() in
>> ubi_eba_copy_leb() which makes perfectly sense to me.  What exactly is
>> the problem?
> 
> There are two things that must be balanced.  The 'reference count'
> ubi_ltree_entry -> users and the rw_semaphore down/up.  You are right,
> the trylock needs to be balanced by the 'leb_write_unlock'.  However,
> the 'leb_write_trylock()' has already decremented 'users' in preperation
> to move the 'lnum'.  However, in the case of contention,
> 'ubi_eba_copy_leb' bails and does the 'leb_write_unlock()', which
> balances the 'trylock', but unbalances the 'users' reference count (I
> added some comments on the lines).

My first thought here is "If this is true, why does ubi_assert(le->users >= 0) not trigger"?

Thanks,
//richard