Re: JFFS2 deadlock with alloc_sem

[David Woodhouse <dwmw2@infradead.org>]

On Tue, 2007-07-31 at 13:10 +0100, David Woodhouse wrote:
> If that's the explanation, then the patch which Nathan tried (dropping
> f->sem before jffs2_gc_fetch_page(), followed by your cleanups¹) ought
> to have fixed the problem. And I'd be happier with that version rather
> than introducing a new read_cache_page_async_trylock() solely for
> JFFS2.

This is the tidied-up version of that patch...

diff --git a/fs/jffs2/README.Locking b/fs/jffs2/README.Locking
index d14d5a4..5836de7 100644
--- a/fs/jffs2/README.Locking
+++ b/fs/jffs2/README.Locking
@@ -69,6 +69,8 @@ Ordering constraints:
 	   any f->sem held.
 	2. Never attempt to lock two file semaphores in one thread.
 	   No ordering rules have been made for doing so.
+(Linux)	3. f->sem must always be locked _after_ the page lock of
+	   any page cache page belonging to the file in question.
 
 
 	erase_completion_lock spinlock
diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
index c253019..4c09531 100644
--- a/fs/jffs2/file.c
+++ b/fs/jffs2/file.c
@@ -69,7 +69,7 @@ const struct address_space_operations jffs2_file_address_operations =
 	.commit_write =	jffs2_commit_write
 };
 
-static int jffs2_do_readpage_nolock (struct inode *inode, struct page *pg)
+static int jffs2_do_readpage(struct inode *inode, struct page *pg)
 {
 	struct jffs2_inode_info *f = JFFS2_INODE_INFO(inode);
 	struct jffs2_sb_info *c = JFFS2_SB_INFO(inode->i_sb);
@@ -83,8 +83,11 @@ static int jffs2_do_readpage_nolock (struct inode *inode, struct page *pg)
 	pg_buf = kmap(pg);
 	/* FIXME: Can kmap fail? */
 
+	down(&f->sem);
+
 	ret = jffs2_read_inode_range(c, f, pg_buf, pg->index << PAGE_CACHE_SHIFT, PAGE_CACHE_SIZE);
 
+	up(&f->sem);
 	if (ret) {
 		ClearPageUptodate(pg);
 		SetPageError(pg);
@@ -102,21 +105,14 @@ static int jffs2_do_readpage_nolock (struct inode *inode, struct page *pg)
 
 int jffs2_do_readpage_unlock(struct inode *inode, struct page *pg)
 {
-	int ret = jffs2_do_readpage_nolock(inode, pg);
+	int ret = jffs2_do_readpage(inode, pg);
 	unlock_page(pg);
 	return ret;
 }
 
-
-static int jffs2_readpage (struct file *filp, struct page *pg)
+static int jffs2_readpage(struct file *filp, struct page *pg)
 {
-	struct jffs2_inode_info *f = JFFS2_INODE_INFO(pg->mapping->host);
-	int ret;
-
-	down(&f->sem);
-	ret = jffs2_do_readpage_unlock(pg->mapping->host, pg);
-	up(&f->sem);
-	return ret;
+	return jffs2_do_readpage_unlock(pg->mapping->host, pg);
 }
 
 static int jffs2_prepare_write (struct file *filp, struct page *pg,
@@ -194,11 +190,9 @@ static int jffs2_prepare_write (struct file *filp, struct page *pg,
 	}
 
 	/* Read in the page if it wasn't already present, unless it's a whole page */
-	if (!PageUptodate(pg) && (start || end < PAGE_CACHE_SIZE)) {
-		down(&f->sem);
-		ret = jffs2_do_readpage_nolock(inode, pg);
-		up(&f->sem);
-	}
+	if (!PageUptodate(pg) && (start || end < PAGE_CACHE_SIZE))
+		ret = jffs2_do_readpage(inode, pg);
+
 	D1(printk(KERN_DEBUG "end prepare_write(). pg->flags %lx\n", pg->flags));
 	return ret;
 }
diff --git a/fs/jffs2/fs.c b/fs/jffs2/fs.c
diff --git a/fs/jffs2/gc.c b/fs/jffs2/gc.c
index eded819..51cb3d1 100644
--- a/fs/jffs2/gc.c
+++ b/fs/jffs2/gc.c
@@ -1218,7 +1218,9 @@ static int jffs2_garbage_collect_dnode(struct jffs2_sb_info *c, struct jffs2_era
 	 *    page OK. We'll actually write it out again in commit_write, which is a little
 	 *    suboptimal, but at least we're correct.
 	 */
+	up(&f->sem);
 	pg_ptr = jffs2_gc_fetch_page(c, f, start, &pg);
+	down(&f->sem);
 
 	if (IS_ERR(pg_ptr)) {
 		printk(KERN_WARNING "read_cache_page() returned error: %ld\n", PTR_ERR(pg_ptr));
diff --git a/fs/jffs2/os-linux.h b/fs/jffs2/os-linux.h

-- 
dwmw2