spin_lock() needed ?

spin_lock() needed ?

[Artem B. Bityuckiy]

Hello,

In JFFS2 I have mentioned the following:

When new node is successfully written to the flash, its node_ref is 
insert to the correspondent inode's node_ref list. Tis is done as following:

jffs2_add_physical_node_ref(c, raw);
raw->next_in_ino = f->inocache->nodes;
f->inocache->nodes = raw;



For example, see functions jffs2_write_dirent() and jffs2_write_dnode() 
in the write.c file.

I am not sure, but it seems there is a race here. The f->inocache->nodes 
may be obsolete node which is in the block pending for erase. So, this 
node may be removed when the correspondent block is erased.



I mean the following.

Suppose we have inode with two nodes. The first node is obsolete and is 
physically located to the block (say block number A), which is currently 
in the c->erase_pending_list. So, suppose:


jffs2_add_physical_node_ref(c, raw);
raw->next_in_ino = f->inocache->nodes; /* We save the address of the 
first obsolete node */

/* Suppose we are preempted here and the another process calls the 
jffs2_erase_pending_blocks() function, which erases the block A. Before 
erasing, it removes all the node_ref structures corresponding to nodes 
in this block A (see the implementation of jffs2_erase_pending_blocks(), 
i.e., the call to jffs2_free_all_node_refs()). Thus, the first node will 
be removed from list */

f->inocache->nodes = raw;
/* Now the first node_ref corresponds to new (3rd) node, but 
f->inocache->nodes->next_in_ino points to wrong place */



So, I think we should hold the c->erase_completion_lock here. I mean:

jffs2_add_physical_node_ref(c, raw);
spin_lock(&c->erase_completion_lock); /* <--------- this */
raw->next_in_ino = f->inocache->nodes;
f->inocache->nodes = raw;
spin_unlock(&c->erase_completion_lock); /* <--------- and this */


Can anybody comment this please?

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.

Re: spin_lock() needed ?

[David Woodhouse]

On Thu, 2004-11-11 at 21:44 +0300, Artem B. Bityuckiy wrote:
> So, I think we should hold the c->erase_completion_lock here. I mean:
> 
> jffs2_add_physical_node_ref(c, raw);
> spin_lock(&c->erase_completion_lock); /* <--------- this */
> raw->next_in_ino = f->inocache->nodes;
> f->inocache->nodes = raw;
> spin_unlock(&c->erase_completion_lock); /* <--------- and this */
> 
> 
> Can anybody comment this please?

I think you're right -- if the first node in the per-inode list is
obsolete, then this can happen. README.Locking agrees with this
assessment. 

But I'm not sure the first node _can_ be obsolete. After all, nodes
become obsolete because we do something to make them so, and we can only
do that by writing a _new_ node... which ends up at the head of the
list.

But it certainly wants a bloody great comment to that effect, at the
very least -- I suppose we might as well just take the lock.

In fact I suspect it may be possible to get an obsolete node at the head
of the list, immediately after booting. There's no real ordering on the
list at that point. 

Well spotted.

-- 
dwmw2

Re: spin_lock() needed ?

[Artem Bityuckiy]

> I think you're right -- if the first node in the per-inode list is
> obsolete, then this can happen. README.Locking agrees with this
> assessment. 
> 
> But I'm not sure the first node _can_ be obsolete. After all, nodes
> become obsolete because we do something to make them so, and we can only
> do that by writing a _new_ node... which ends up at the head of the
> list.
Hmm, true.
> 
> But it certainly wants a bloody great comment to that effect, at the
> very least -- I suppose we might as well just take the lock.
> 
> In fact I suspect it may be possible to get an obsolete node at the head
> of the list, immediately after booting. There's no real ordering on the
> list at that point. 
Yes, and it seems not only "immediately after booting", but as long as a 
file wasn't changed after booting...

So, it seems the lock would be good.

Thanks for comment. I'll try to fix this.


-- 
Best regards, Artem B. Bityuckiy
Oktet Labs (St. Petersburg), Software Engineer.
+78124286709 (office) +79112449030 (mobile)
E-mail: dedekind@oktetlabs.ru, web: http://www.oktetlabs.ru