JFFS2 update for 2.4: read_inode/clear_inode race.

JFFS2 update for 2.4: read_inode/clear_inode race.

[David Woodhouse]

JFFS2 gets somewhat unhappy when the VFS calls read_inode() and 
clear_inode() for the same inode simultaneously. This changeset (which can 
be fed through 'grep ^# | cut -c2-' to get a unified diff if anyone cares)
fixes that particular case.

I still haven't decided what to do in the stable branch about the other less
likely case where the VFS calls read_inode() read_inode() clear_inode()
clear_inode() in that order, all for the same inode number. By the time the
last clear_inode() is called, some of the data structures which belonged to
the physical inode in question may have been freed, although it's unlikely
as it actually requires a flash block erase to happen in the meantime too.


This BitKeeper patch contains the following changesets:
1.737

# This is a BitKeeper patch.  What follows are the unified diffs for the
# set of deltas contained in the patch.  The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User:	dwmw2
# Host:	infradead.org
# Root:	/mnt/bk/jffs2-2.4

#
#--- 1.4/fs/jffs2/gc.c	Sun May 12 19:34:14 2002
#+++ 1.5/fs/jffs2/gc.c	Thu Oct 10 15:34:48 2002
#@@ -31,7 +31,7 @@
#  * provisions above, a recipient may use your version of this file
#  * under either the RHEPL or the GPL.
#  *
#- * $Id: gc.c,v 1.52.2.3 2002/05/12 17:27:08 dwmw2 Exp $
#+ * $Id: gc.c,v 1.52.2.5 2002/10/10 13:18:38 dwmw2 Exp $
#  *
#  */
# 
#@@ -134,8 +134,10 @@
# 
# 	D1(printk(KERN_DEBUG "garbage collect from block at phys 0x%08x\n", jeb->offset));
# 
#-	if (!jeb->used_size)
#+	if (!jeb->used_size) {
#+		up(&c->alloc_sem);
# 		goto eraseit;
#+	}
# 
# 	raw = jeb->gc_node;
# 			
#@@ -156,6 +158,7 @@
# 		/* Inode-less node. Clean marker, snapshot or something like that */
# 		spin_unlock_bh(&c->erase_completion_lock);
# 		jffs2_mark_node_obsolete(c, raw);
#+		up(&c->alloc_sem);
# 		goto eraseit_lock;
# 	}
# 						     
#@@ -170,8 +173,8 @@
# 	if (is_bad_inode(inode)) {
# 		printk(KERN_NOTICE "Eep. read_inode() failed for ino #%u\n", inum);
# 		/* NB. This will happen again. We need to do something appropriate here. */
#-		iput(inode);
# 		up(&c->alloc_sem);
#+		iput(inode);
# 		return -EIO;
# 	}
# 
#@@ -234,6 +237,7 @@
# 	}
#  upnout:
# 	up(&f->sem);
#+	up(&c->alloc_sem);
# 	iput(inode);
# 
#  eraseit_lock:
#@@ -250,7 +254,6 @@
# 		jffs2_erase_pending_trigger(c);
# 	}
# 	spin_unlock_bh(&c->erase_completion_lock);
#-	up(&c->alloc_sem);
# 
# 	return ret;
# }
#@@ -507,7 +510,7 @@
# 	 * number as before. (Except in case of error -- see 'goto fill;' 
# 	 * above.)
# 	 */
#-	D1(if(unlikely(fn->frags <= 1)) {
#+	D1(if(fn->frags <= 1) {
# 		printk(KERN_WARNING "jffs2_garbage_collect_hole: Replacing fn with %d frag(s) but new ver %d != highest_version %d of ino #%d\n",
# 		       fn->frags, ri.version, f->highest_version, ri.ino);
# 	});
#
#--- 1.4/fs/jffs2/readinode.c	Fri Mar 22 09:01:31 2002
#+++ 1.5/fs/jffs2/readinode.c	Thu Oct 10 15:34:49 2002
#@@ -31,7 +31,7 @@
#  * provisions above, a recipient may use your version of this file
#  * under either the RHEPL or the GPL.
#  *
#- * $Id: readinode.c,v 1.58.2.5 2002/03/05 22:40:03 dwmw2 Exp $
#+ * $Id: readinode.c,v 1.58.2.6 2002/10/10 13:18:38 dwmw2 Exp $
#  *
#  */
# 
#@@ -468,15 +468,28 @@
# 	struct jffs2_node_frag *frag, *frags;
# 	struct jffs2_full_dirent *fd, *fds;
# 	struct jffs2_inode_info *f = JFFS2_INODE_INFO(inode);
#+        /* I don't think we care about the potential race due to reading this
#+           without f->sem. It can never get undeleted. */
#+        int deleted = f->inocache && !f->inocache->nlink;
# 
# 	D1(printk(KERN_DEBUG "jffs2_clear_inode(): ino #%lu mode %o\n", inode->i_ino, inode->i_mode));
# 
#+	/* If it's a deleted inode, grab the alloc_sem. This prevents
#+	   jffs2_garbage_collect_pass() from deciding that it wants to
#+	   garbage collect one of the nodes we're just about to mark 
#+	   obsolete -- by the time we drop alloc_sem and return, all
#+	   the nodes are marked obsolete, and jffs2_g_c_pass() won't
#+	   call iget() for the inode in question.
#+	*/
#+	if (deleted)
#+		down(&c->alloc_sem);
#+
# 	down(&f->sem);
# 
# 	frags = f->fraglist;
# 	fds = f->dents;
# 	if (f->metadata) {
#-		if (!f->inocache->nlink)
#+		if (deleted)
# 			jffs2_mark_node_obsolete(c, f->metadata->raw);
# 		jffs2_free_full_dnode(f->metadata);
# 	}
#@@ -488,7 +501,7 @@
# 
# 		if (frag->node && !(--frag->node->frags)) {
# 			/* Not a hole, and it's the final remaining frag of this node. Free the node */
#-			if (!f->inocache->nlink)
#+			if (deleted)
# 				jffs2_mark_node_obsolete(c, frag->node->raw);
# 
# 			jffs2_free_full_dnode(frag->node);
#@@ -502,5 +515,8 @@
# 	}
# 
# 	up(&f->sem);
#+
#+	if(deleted)
#+		up(&c->alloc_sem);
# };
# 
#

# Diff checksum=73bb1fd7


# Patch vers:	1.3
# Patch type:	REGULAR

== ChangeSet ==
torvalds@athlon.transmeta.com|ChangeSet|20020205173056|16047|c1d11a41ed024864
marcelo@freak.distro.conectiva|ChangeSet|20021010052138|26379
D 1.737 02/10/10 15:35:41+01:00 dwmw2@infradead.org +2 -0
B torvalds@athlon.transmeta.com|ChangeSet|20020205173056|16047|c1d11a41ed024864
C
c Deal with VFS calling clear_inode() and read_inode() simultaneously for the same inode.
K 25252
P ChangeSet
------------------------------------------------

0a0
> patch@athlon.transmeta.com|fs/jffs2/gc.c|20020205201839|10209|b6698581d0f53a8b dwmw2@infradead.org|fs/jffs2/gc.c|20021010143448|25453
> patch@athlon.transmeta.com|fs/jffs2/readinode.c|20020205201839|13519|d8985d847ab6293a dwmw2@infradead.org|fs/jffs2/readinode.c|20021010143449|21243

== fs/jffs2/gc.c ==
patch@athlon.transmeta.com|fs/jffs2/gc.c|20020205201839|10209|b6698581d0f53a8b
dwmw2@dwmw2.baythorne.internal|fs/jffs2/gc.c|20020512183414|22949
D 1.5 02/10/10 15:34:48+01:00 dwmw2@infradead.org +8 -5
B torvalds@athlon.transmeta.com|ChangeSet|20020205173056|16047|c1d11a41ed024864
C
c Rearrange the dropping of alloc_sem and iput() to allow for clear_inode() now requiring the alloc_sem.
K 25453
O -rw-rw-r--
P fs/jffs2/gc.c
------------------------------------------------

D34 1
I34 1
 * $Id: gc.c,v 1.52.2.5 2002/10/10 13:18:38 dwmw2 Exp $
D137 1
I137 2
	if (!jeb->used_size) {
		up(&c->alloc_sem);
I138 1
	}
I158 1
		up(&c->alloc_sem);
D173 1
I174 1
		iput(inode);
I236 1
	up(&c->alloc_sem);
D253 1
D510 1
I510 1
	D1(if(fn->frags <= 1) {

== fs/jffs2/readinode.c ==
patch@athlon.transmeta.com|fs/jffs2/readinode.c|20020205201839|13519|d8985d847ab6293a
dwmw2@infradead.org|fs/jffs2/readinode.c|20020322090131|37120
D 1.5 02/10/10 15:34:49+01:00 dwmw2@infradead.org +19 -3
B torvalds@athlon.transmeta.com|ChangeSet|20020205173056|16047|c1d11a41ed024864
C
c Grab alloc_sem in clear_inode().
K 21243
O -rw-rw-r--
P fs/jffs2/readinode.c
------------------------------------------------

D34 1
I34 1
 * $Id: readinode.c,v 1.58.2.6 2002/10/10 13:18:38 dwmw2 Exp $
I470 3
        /* I don't think we care about the potential race due to reading this
           without f->sem. It can never get undeleted. */
        int deleted = f->inocache && !f->inocache->nlink;
I473 10
	/* If it's a deleted inode, grab the alloc_sem. This prevents
	   jffs2_garbage_collect_pass() from deciding that it wants to
	   garbage collect one of the nodes we're just about to mark 
	   obsolete -- by the time we drop alloc_sem and return, all
	   the nodes are marked obsolete, and jffs2_g_c_pass() won't
	   call iget() for the inode in question.
	*/
	if (deleted)
		down(&c->alloc_sem);
\
D479 1
I479 1
		if (deleted)
D491 1
I491 1
			if (deleted)
I504 3
\
	if(deleted)
		up(&c->alloc_sem);

# Patch checksum=272430c7

--
dwmw2

Re: JFFS2 update for 2.4: read_inode/clear_inode race.

[Marcelo Tosatti]

On Thu, 10 Oct 2002, David Woodhouse wrote:

> JFFS2 gets somewhat unhappy when the VFS calls read_inode() and
> clear_inode() for the same inode simultaneously. This changeset (which can
> be fed through 'grep ^# | cut -c2-' to get a unified diff if anyone cares)
> fixes that particular case.

I do care. Next time please mind to send me unified diffs?

That makes me life easier :)

> I still haven't decided what to do in the stable branch about the other less
> likely case where the VFS calls read_inode() read_inode() clear_inode()
> clear_inode() in that order, all for the same inode number. By the time the
> last clear_inode() is called, some of the data structures which belonged to
> the physical inode in question may have been freed, although it's unlikely
> as it actually requires a flash block erase to happen in the meantime too.

Patch applied. Thanks

Re: JFFS2 update for 2.4: read_inode/clear_inode race.

[Marcelo Tosatti]

On Mon, 14 Oct 2002, Marcelo Tosatti wrote:

>
>
> On Thu, 10 Oct 2002, David Woodhouse wrote:
>
> > JFFS2 gets somewhat unhappy when the VFS calls read_inode() and
> > clear_inode() for the same inode simultaneously. This changeset (which can
> > be fed through 'grep ^# | cut -c2-' to get a unified diff if anyone cares)
> > fixes that particular case.
>
> I do care. Next time please mind to send me unified diffs?
>
> That makes me life easier :)

Err.

Ugh.