Patchwork JFFS2 deadlock, kernel 3.4.11

login
register
mail settings
Submitter Joakim Tjernlund
Date Oct. 4, 2012, 6:39 p.m.
Message ID <OFB615AF11.05DABF7D-ONC1257A8D.00661C69-C1257A8D.0066832C@transmode.se>
Download mbox | patch
Permalink /patch/189246/
State New
Headers show

Comments

Joakim Tjernlund - Oct. 4, 2012, 6:39 p.m.
Thomas.Betker@rohde-schwarz.com wrote on 2012/10/04 18:51:58:
>
> Hello Joakim:
>
> > Try Reply With Internet-style History (We use Notes here too)
>
> Got it. Thanks!
>
> > > jffs2_reserve_space() must not be called with f->sem held because it
> > > acquires c->alloc_sem.
> >
> > hmm, are you sure? Did it fail?
> > As far as I can see jffs2_garbage_collect_live() does this.
>
> jffs2_reserve_space() does mutex_lock(&c->alloc_sem) first thing, and
> README.Locking says "Never attempt to allocate space or lock alloc_sem
> with any f->sem held.". So I didn't even try; yes, I am a coward. (:-)
> Also, all the code I checked carefully releases f->sem before calling
> jffs2_reserve_space().

I see, thanks.

>
> jffs2_garbage_collect_live() doesn't call jffs2_reserve_space() directly.
> Is it called indirectly somehow?

hmm, misread the code a bit so forget this comment.

>
> > > So I have moved mutex_lock(&f->sem) and grab_cache_page_write_begin()
> > > after jffs2_reserve_space(). Attached is my 3.4.11 patch (which is
> based
> > > on your patch) for review; I hope it is not mangled by Lotus Notes ...
> >
> > don't have time to look ATM
>
> Okay. When the tests succeed, I will simply mail it to the list as a
> regular patch, for general review.

I think your patch will work, but I don't like having 2 call sites
to grab_cache_page_write_begin(mapping, index, flags) so I came
up with this instead(untested as well):
Joakim Tjernlund - Oct. 4, 2012, 6:55 p.m.
>
> Thomas.Betker@rohde-schwarz.com wrote on 2012/10/04 18:51:58:
> >
> > Hello Joakim:
> >
> > > Try Reply With Internet-style History (We use Notes here too)
> >
> > Got it. Thanks!
> >
> > > > jffs2_reserve_space() must not be called with f->sem held because it
> > > > acquires c->alloc_sem.
> > >
> > > hmm, are you sure? Did it fail?
> > > As far as I can see jffs2_garbage_collect_live() does this.
> >
> > jffs2_reserve_space() does mutex_lock(&c->alloc_sem) first thing, and
> > README.Locking says "Never attempt to allocate space or lock alloc_sem
> > with any f->sem held.". So I didn't even try; yes, I am a coward. (:-)
> > Also, all the code I checked carefully releases f->sem before calling
> > jffs2_reserve_space().
>
> I see, thanks.
>
> >
> > jffs2_garbage_collect_live() doesn't call jffs2_reserve_space() directly.
> > Is it called indirectly somehow?
>
> hmm, misread the code a bit so forget this comment.
>
> >
> > > > So I have moved mutex_lock(&f->sem) and grab_cache_page_write_begin()
> > > > after jffs2_reserve_space(). Attached is my 3.4.11 patch (which is
> > based
> > > > on your patch) for review; I hope it is not mangled by Lotus Notes ...
> > >
> > > don't have time to look ATM
> >
> > Okay. When the tests succeed, I will simply mail it to the list as a
> > regular patch, for general review.
>
> I think your patch will work, but I don't like having 2 call sites
> to grab_cache_page_write_begin(mapping, index, flags) so I came
> up with this instead(untested as well):

Forgot, could you add some debug code to test if pageofs <= inode->i_size:
	ri.isize = cpu_to_je32(max((uint32_t)inode->i_size, pageofs));
      ....
      if(pageofs <= inode->i_size) {
         print debug stuff here
      }
	ri.dsize = cpu_to_je32(pageofs - inode->i_size);

 Jocke
Thomas.Betker@rohde-schwarz.com - Oct. 5, 2012, 9:17 a.m.
Hello Joakim:

> I think your patch will work, but I don't like having 2 call sites
> to grab_cache_page_write_begin(mapping, index, flags) so I came
> up with this instead(untested as well):

The tests I ran this night were successful. I have adapted my patch 
according to your suggestions and started re-testing; I won't be able to 
finish today, though, so I will post the final patch on Monday (assuming 
that it works).

Have a nice weekend,
Thomas
Joakim Tjernlund - Oct. 5, 2012, 9:21 a.m.
Thomas.Betker@rohde-schwarz.com wrote on 2012/10/05 11:17:22:
>
> Hello Joakim:
>
> > I think your patch will work, but I don't like having 2 call sites
> > to grab_cache_page_write_begin(mapping, index, flags) so I came
> > up with this instead(untested as well):
>
> The tests I ran this night were successful. I have adapted my patch
> according to your suggestions and started re-testing; I won't be able to
> finish today, though, so I will post the final patch on Monday (assuming
> that it works).

I was just about to sent the final version. There was an bug in the
error patch. Hang in and a proper patch will come shortly.

 Jocke

Patch

diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c
index db3889b..8eaeaa35 100644
--- a/fs/jffs2/file.c
+++ b/fs/jffs2/file.c
@@ -140,31 +140,33 @@  static int jffs2_write_begin(struct file *filp, struct address_space *mapping,
 	struct jffs2_inode_info *f = JFFS2_INODE_INFO(inode);
 	pgoff_t index = pos >> PAGE_CACHE_SHIFT;
 	uint32_t pageofs = index << PAGE_CACHE_SHIFT;
-	int ret = 0;
+	int ret = -ENOMEM;
+	struct jffs2_sb_info *c = JFFS2_SB_INFO(inode->i_sb);
+	struct jffs2_raw_inode ri;
+	uint32_t alloc_len;
+
+	if (pageofs > inode->i_size) {
+		ret = jffs2_reserve_space(c, sizeof(ri), &alloc_len,
+					  ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
+		if (ret)
+			return ret;
+	}

+	mutex_lock(&f->sem);
 	pg = grab_cache_page_write_begin(mapping, index, flags);
 	if (!pg)
-		return -ENOMEM;
+		goto out_unlock;
 	*pagep = pg;

 	jffs2_dbg(1, "%s()\n", __func__);

-	if (pageofs > inode->i_size) {
+	if (!ret) { /* reserve_space succeded */
 		/* Make new hole frag from old EOF to new page */
-		struct jffs2_sb_info *c = JFFS2_SB_INFO(inode->i_sb);
-		struct jffs2_raw_inode ri;
 		struct jffs2_full_dnode *fn;
-		uint32_t alloc_len;

 		jffs2_dbg(1, "Writing new hole frag 0x%x-0x%x between current EOF and new page\n",
 			  (unsigned int)inode->i_size, pageofs);

-		ret = jffs2_reserve_space(c, sizeof(ri), &alloc_len,
-					  ALLOC_NORMAL, JFFS2_SUMMARY_INODE_SIZE);
-		if (ret)
-			goto out_page;
-
-		mutex_lock(&f->sem);
 		memset(&ri, 0, sizeof(ri));

 		ri.magic = cpu_to_je16(JFFS2_MAGIC_BITMASK);
@@ -191,7 +193,6 @@  static int jffs2_write_begin(struct file *filp, struct address_space *mapping,
 		if (IS_ERR(fn)) {
 			ret = PTR_ERR(fn);
 			jffs2_complete_reservation(c);
-			mutex_unlock(&f->sem);
 			goto out_page;
 		}
 		ret = jffs2_add_full_dnode_to_inode(c, f, fn);
@@ -206,12 +207,10 @@  static int jffs2_write_begin(struct file *filp, struct address_space *mapping,
 			jffs2_mark_node_obsolete(c, fn->raw);
 			jffs2_free_full_dnode(fn);
 			jffs2_complete_reservation(c);
-			mutex_unlock(&f->sem);
 			goto out_page;
 		}
 		jffs2_complete_reservation(c);
 		inode->i_size = pageofs;
-		mutex_unlock(&f->sem);
 	}

 	/*
@@ -220,18 +219,19 @@  static int jffs2_write_begin(struct file *filp, struct address_space *mapping,
 	 * case of a short-copy.
 	 */
 	if (!PageUptodate(pg)) {
-		mutex_lock(&f->sem);
 		ret = jffs2_do_readpage_nolock(inode, pg);
-		mutex_unlock(&f->sem);
 		if (ret)
 			goto out_page;
 	}
+	mutex_unlock(&f->sem);
 	jffs2_dbg(1, "end write_begin(). pg->flags %lx\n", pg->flags);
-	return ret;
+	return 0;

 out_page:
 	unlock_page(pg);
 	page_cache_release(pg);
+out_unlock:
+	mutex_unlock(&f->sem);
 	return ret;
 }