diff mbox

JFFS2 deadlock, kernel 3.4.11

Message ID OF930A0EE5.DA214D86-ONC1257A8B.00673BC9-C1257A8B.0067E92C@transmode.se
State New, archived
Headers show

Commit Message

Joakim Tjernlund Oct. 2, 2012, 6:54 p.m. UTC 
linux-mtd-bounces@lists.infradead.org wrote on 2012/10/02 20:04:36:

> From: Joakim Tjernlund <joakim.tjernlund@transmode.se>
> To:
> Cc: linux-mtd@lists.infradead.org, Thomas.Betker@rohde-schwarz.com
> Date: 2012/10/02 20:08
> Subject: Re: JFFS2 deadlock, kernel 3.4.11
> Sent by: linux-mtd-bounces@lists.infradead.org
>
> > >
> > > Hello all,
> > >
> > > I have encountered multiple times a deadlock between two JFFS2 threads:
> >
> > [SNIP]
> >
> > >
> > > The target system is an SoC with a dual ARMv7 (Cortex-A9), and we are
> > > running the long-term 3.4.11 kernel (whose fs/jffs2/ seems to be pretty
> > > close to the latest mainline kernel). The deadlock occurred when using scp
> > > to copy files from a host system to the target system.
> > >
> > > The GC thread hangs in lock_page(page), the write thread hangs in the
> > > first mutex_lock(&f->sem). The cause seems to be an AB-BA deadlock:
> > >
> > > jffs2_garbage_collect_live
> > >     mutex_lock(&f->sem)                         (A)
> > >     jffs2_garbage_collect_dnode [inlined]
> > >         jffs2_gc_fetch_page
> > >             read_cache_page_async
> > >                 do_read_cache_page
> > >                     lock_page(page) [inlined]
> > >                         __lock_page             (B) ***
> > >
> > > jffs2_write_begin
> > >     grab_cache_page_write_begin
> > >         find_lock_page
> > >             lock_page(page)                     (B)
> > >     mutex_lock(&f->sem)                         (A) ***
> > >
> > > I have manually analyzed the stacks and confirmed that both threads sit on
> > > the theme 'struct page'.
> > >
> >
> > hmm, not something I have seen but your analysis seems spot on. With any luck
> > you only need to move the mutex_lock in the write begin before lock_page. I
> > am only guessing now though.
>
> I had a look at jffs2_write_begin() and it looks fishy:
> It can write a hole frag sucessfully but still fail in:
>    if (!PageUptodate(pg)) {
>       mutex_lock(&f->sem);
>       ret = jffs2_do_readpage_nolock(inode, pg);
>       mutex_unlock(&f->sem);
>       if (ret)
>          goto out_page;
>    }
> which seems a bit strange.
>
> Further up we have this:
>       ri.isize = cpu_to_je32(max((uint32_t)inode->i_size, pageofs));
>       ...
>       ri.dsize = cpu_to_je32(pageofs - inode->i_size);
> Why max(..) when pageofs  must be > inode->i_size for ri.dsize
> to make sense?

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?

Comments

Thomas.Betker@rohde-schwarz.com Oct. 4, 2012, 2:20 p.m. UTC | #1 
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
Joakim Tjernlund Oct. 4, 2012, 4:15 p.m. UTC | #2 
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]
Thomas.Betker@rohde-schwarz.com Oct. 4, 2012, 4:51 p.m. UTC | #3 
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 mbox

Patch

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;
 }