From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Stephen C. Tweedie" Subject: Re: [RFC] Bug in journal_commit_transaction? Date: Thu, 20 Apr 2006 12:00:37 -0400 Message-ID: <1145548837.3084.73.camel@orbit.scot.redhat.com> References: <20060411133512.GE21928@atrey.karlin.mff.cuni.cz> <20060411182731.07541de2.akpm@osdl.org> <20060415210629.GA29171@atrey.karlin.mff.cuni.cz> <20060420084753.GA24993@atrey.karlin.mff.cuni.cz> Mime-Version: 1.0 Content-Type: text/plain Content-Transfer-Encoding: 7bit Cc: Andrew Morton , linux-fsdevel@vger.kernel.org, Stephen Tweedie Return-path: Received: from mx1.redhat.com ([66.187.233.31]:58569 "EHLO mx1.redhat.com") by vger.kernel.org with ESMTP id S1751076AbWDTQA6 (ORCPT ); Thu, 20 Apr 2006 12:00:58 -0400 To: Jan Kara In-Reply-To: <20060420084753.GA24993@atrey.karlin.mff.cuni.cz> Sender: linux-fsdevel-owner@vger.kernel.org List-Id: linux-fsdevel.vger.kernel.org Hi, On Thu, 2006-04-20 at 10:47 +0200, Jan Kara wrote: > I've got no answer but I still think my argument was sound ;) Below > is a patch also with a verbose comment what it fixes and how. Looks good to me, but it complicates the logic for this case and removes some useful debug checks for conditions which are almost always true. Wouldn't it be better and safer just to make those almost-always true conditions always true? We can do that pretty simply, just by processing the t_forget list in two passes, always doing the bitmaps last. (Though this will need care, as the list is potentially moving under our feet...) Actually, scratch that, because journal_unmap_buffer() can return a buffer to the committing transaction's forget list at any time, so we really cannot guarantee to do all non-bitmaps first: new non-bitmaps might arrive after we've started doing the bitmaps. Ugh. > diff -rupX /home/jack/.kerndiffexclude linux-2.6.16/fs/jbd/commit.c linux-2.6.16-1-realloc_freed_fix/fs/jbd/commit.c > --- linux-2.6.16/fs/jbd/commit.c 2006-01-15 00:20:12.000000000 +0100 > +++ linux-2.6.16-1-realloc_freed_fix/fs/jbd/commit.c 2006-04-20 10:32:15.000000000 +0200 > @@ -790,11 +790,22 @@ restart_loop: > + JBUFFER_TRACE(jh, "refile or unfile freed buffer"); > + __journal_refile_buffer(jh); > + if (!jh->b_transaction) { > + jbd_unlock_bh_state(bh); > + /* needs a brelse */ > + journal_remove_journal_head(bh); > + release_buffer_page(bh); > + } else > + jbd_unlock_bh_state(bh); Looks good to me. ACK. --Stephen