From patchwork Fri Oct 5 09:24:24 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joakim Tjernlund X-Patchwork-Id: 189450 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from merlin.infradead.org (unknown [IPv6:2001:4978:20e::2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 735A12C008E for ; Fri, 5 Oct 2012 19:25:35 +1000 (EST) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TK497-00032V-EA; Fri, 05 Oct 2012 09:24:37 +0000 Received: from gw1.transmode.se ([195.58.98.146]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1TK494-000324-PC for linux-mtd@lists.infradead.org; Fri, 05 Oct 2012 09:24:35 +0000 Received: from mail1.transmode.se (mail1.transmode.se [192.168.201.18]) by gw1.transmode.se (Postfix) with ESMTP id 7C98325833E; Fri, 5 Oct 2012 11:24:33 +0200 (CEST) Received: from gentoo-jocke.transmode.se ([172.20.4.10]) by mail1.transmode.se (Lotus Domino Release 8.5.3FP1) with ESMTP id 2012100511243267-164779 ; Fri, 5 Oct 2012 11:24:32 +0200 Received: from gentoo-jocke.transmode.se (localhost [127.0.0.1]) by gentoo-jocke.transmode.se (8.14.4/8.14.4) with ESMTP id q959OXnX010312; Fri, 5 Oct 2012 11:24:33 +0200 Received: (from jocke@localhost) by gentoo-jocke.transmode.se (8.14.4/8.14.4/Submit) id q959OULX010310; Fri, 5 Oct 2012 11:24:30 +0200 From: Joakim Tjernlund To: linux-mtd@lists.infradead.org, Thomas.Betker@rohde-schwarz.com, "David Woodhouse" Subject: [PATCH] jffs2: Fix lock acquisition order bug in jffs2_write_begin Date: Fri, 5 Oct 2012 11:24:24 +0200 Message-Id: <1349429064-10241-1-git-send-email-Joakim.Tjernlund@transmode.se> X-Mailer: git-send-email 1.7.8.6 X-MIMETrack: Itemize by SMTP Server on mail1/Transmode(Release 8.5.3FP1|March 07, 2012) at 05/10/2012 11:24:32, Serialize by Router on mail1/Transmode(Release 8.5.3FP1|March 07, 2012) at 05/10/2012 11:24:32, Serialize complete at 05/10/2012 11:24:32 X-TNEFEvaluated: 1 X-Spam-Note: CRM114 invocation failed X-Spam-Score: -4.0 (----) X-Spam-Report: SpamAssassin version 3.3.2 on merlin.infradead.org summary: Content analysis details: (-4.0 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 SPF_PASS SPF: sender matches SPF record -2.1 RP_MATCHES_RCVD Envelope sender domain matches handover relay domain -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] Cc: Joakim Tjernlund X-BeenThere: linux-mtd@lists.infradead.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: Linux MTD discussion mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: linux-mtd-bounces@lists.infradead.org Errors-To: linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org Thomas.Betker@rohde-schwarz.com reports: 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'. Fix this by restructuring jffs2_write_begin to take the f->sem lock before the page lock. However one must make sure not to hold f->sem when calling jffs2_reserve_space according to README.Locking Signed-off-by: Joakim Tjernlund --- This needs to be review by someone that understands the looking rules better. David? fs/jffs2/file.c | 41 +++++++++++++++++++++-------------------- 1 files changed, 21 insertions(+), 20 deletions(-) diff --git a/fs/jffs2/file.c b/fs/jffs2/file.c index db3889b..23adcdc 100644 --- a/fs/jffs2/file.c +++ b/fs/jffs2/file.c @@ -140,31 +140,35 @@ 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) { + /* reserve space must be be before locking f->sem */ + 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); @@ -190,8 +194,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); @@ -205,13 +207,10 @@ static int jffs2_write_begin(struct file *filp, struct address_space *mapping, ret); 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,20 @@ 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: + jffs2_complete_reservation(c); + mutex_unlock(&f->sem); return ret; }