From patchwork Thu Oct 4 18:39:42 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joakim Tjernlund X-Patchwork-Id: 189246 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 D4BE42C0370 for ; Fri, 5 Oct 2012 04:40:57 +1000 (EST) Received: from localhost ([::1] helo=merlin.infradead.org) by merlin.infradead.org with esmtp (Exim 4.76 #1 (Red Hat Linux)) id 1TJqKt-0002GB-2C; Thu, 04 Oct 2012 18:39:51 +0000 Received: from gw1.transmode.se ([195.58.98.146]) by merlin.infradead.org with esmtps (Exim 4.76 #1 (Red Hat Linux)) id 1TJqKq-0002Fx-1P for linux-mtd@lists.infradead.org; Thu, 04 Oct 2012 18:39:48 +0000 Received: from mail1.transmode.se (mail1.transmode.se [192.168.201.18]) by gw1.transmode.se (Postfix) with ESMTP id 6773E25813B; Thu, 4 Oct 2012 20:39:44 +0200 (CEST) In-Reply-To: References: Subject: Re: JFFS2 deadlock, kernel 3.4.11 X-KeepSent: B615AF11:05DABF7D-C1257A8D:00661C69; type=4; name=$KeepSent To: Thomas.Betker@rohde-schwarz.com X-Mailer: Lotus Notes Release 8.5.3 September 15, 2011 Message-ID: From: Joakim Tjernlund Date: Thu, 4 Oct 2012 20:39:42 +0200 X-MIMETrack: Serialize by Router on mail1/Transmode(Release 8.5.3FP1|March 07, 2012) at 04/10/2012 20:39:43 MIME-Version: 1.0 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: linux-mtd@lists.infradead.org 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: , Sender: linux-mtd-bounces@lists.infradead.org Errors-To: linux-mtd-bounces+incoming=patchwork.ozlabs.org@lists.infradead.org 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): 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; }