Message ID | OF930A0EE5.DA214D86-ONC1257A8B.00673BC9-C1257A8B.0067E92C@transmode.se |
---|---|
State | New, archived |
Headers | show |
Hello Joakim, (sorry if the indentation is wrong, I have not yet found out how to do this in Lotus Notes): [Joakim Tjernlund] So maybe this will help(not even compile tested), don't know if jffs2_reserve_space() can be called with f->sem held. If this is bad, then perhaps move pg = grab_cache_page_write_begin(mapping, index, flags) to later in this function somehow? [/Joakim Tjernlund] jffs2_reserve_space() must not be called with f->sem held because it acquires c->alloc_sem. 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 ... The first tests (with proof of locking correctness enabled) were fine. The main tests will run until tomorrow evening, though; last time, it took me all day to reproduce the deadlock, and I want to be reasonably sure that it is gone -- and that no new deadlocks were introduced. Best regards, Thomas
Thomas.Betker@rohde-schwarz.com wrote on 2012/10/04 16:20:16: > > Hello Joakim, > > (sorry if the indentation is wrong, I have not yet found out how to do > this in Lotus Notes): Try Reply With Internet-style History (We use Notes here too) > > [Joakim Tjernlund] > So maybe this will help(not even compile tested), don't know if > jffs2_reserve_space() > can be called with f->sem held. > If this is bad, then perhaps move pg = > grab_cache_page_write_begin(mapping, index, flags) > to later in this function somehow? > [/Joakim Tjernlund] > > 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. > > 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 > > The first tests (with proof of locking correctness enabled) were fine. The > main tests will run until tomorrow evening, though; last time, it took me > all day to reproduce the deadlock, and I want to be reasonably sure that > it is gone -- and that no new deadlocks were introduced. > > Best regards, > Thomas > > [attachment "jffs2_write_begin.patch" deleted by Joakim Tjernlund/Transmode]
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(). jffs2_garbage_collect_live() doesn't call jffs2_reserve_space() directly. Is it called indirectly somehow? > > 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. Best regards, Thomas
diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c index db3889b..fb58622 100644 --- a/fs/jffs2/file.c +++ b/fs/jffs2/file.c @@ -142,9 +142,12 @@ static int jffs2_write_begin(struct file *filp, struct address_space *mapping, uint32_t pageofs = index << PAGE_CACHE_SHIFT; int ret = 0; + mutex_lock(&f->sem); pg = grab_cache_page_write_begin(mapping, index, flags); - if (!pg) + if (!pg) { + mutex_unlock(&f->sem); return -ENOMEM; + } *pagep = pg; jffs2_dbg(1, "%s()\n", __func__); @@ -164,7 +167,6 @@ static int jffs2_write_begin(struct file *filp, struct address_space *mapping, 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,18 @@ 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; out_page: unlock_page(pg); page_cache_release(pg); + mutex_unlock(&f->sem); return ret; }