From: "Amir G." Subject: Re: [PATCH 5/8] ext4: call ext4_forget() from ext4_free_blocks() Date: Sun, 27 Jun 2010 05:40:23 +0300 Message-ID: Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: QUOTED-PRINTABLE Cc: "Aneesh Kumar K.V" , Ext4 Developers List To: Theodore Tso Return-path: Received: from mail-bw0-f46.google.com ([209.85.214.46]:44588 "EHLO mail-bw0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754056Ab0F0CkZ convert rfc822-to-8bit (ORCPT ); Sat, 26 Jun 2010 22:40:25 -0400 Received: by bwz10 with SMTP id 10so258465bwz.19 for ; Sat, 26 Jun 2010 19:40:23 -0700 (PDT) Sender: linux-ext4-owner@vger.kernel.org List-ID: Hi Ted, There is a lurking bug that needs to be fixed in ext4_free_branches(). it can cause double freeing of blocks after crash. please see bug description below. Amir. On Wed, Jun 23, 2010 at 10:01 PM, Amir G. wrote: > Hi, > > We have experienced bitmap inconsistencies after crash during file > delete under heavy load. > The crash is not file system related and I the following patch in > ext4_free_branches() fixes the recovery problem. > > If the transaction is restarted and there is a crash before the new > transaction is committed, > then after recovery, the blocks that this indirect block points to > have been freed, but the indirect block itself > has not been freed and may still point to some of the free blocks > (because of the ext4_forget()). > > So ext4_forget() should be called inside ext4_free_blocks() to avoid > this problem. > Are there any consequences to this patch that I am not aware of? > > Amir. > > Signed-off-by: Amir Goldstein > > diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c > index 42272d6..682e2fa 100644 > --- a/fs/ext4/inode.c > +++ b/fs/ext4/inode.c > @@ -4458,6 +4458,7 @@ static void ext4_free_branches(handle_t *handle= , > struct inode *inode, > =A0{ > =A0 =A0 =A0 =A0ext4_fsblk_t nr; > =A0 =A0 =A0 =A0__le32 *p; > + =A0 =A0 =A0 int =A0 =A0 flags; > > =A0 =A0 =A0 =A0if (ext4_handle_is_aborted(handle)) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0return; > @@ -4520,7 +4521,7 @@ static void ext4_free_branches(handle_t *handle= , > struct inode *inode, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * revoke records must= be emitted *before* clearing > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * this block's bit in= the bitmaps. > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 */ > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ext4_forget(handle, 1, = inode, bh, bh->b_blocknr); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 flags =3D > EXT4_FREE_BLOCKS_METADATA|EXT4_FREE_BLOCKS_FORGET; > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 * Everything below th= is this pointer has been > @@ -4546,8 +4547,7 @@ static void ext4_free_branches(handle_t *handle= , > struct inode *inode, > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 =A0blocks_for_truncate(inode)); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0} > > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ext4_free_blocks(handle= , inode, 0, nr, 1, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= =A0 =A0 =A0EXT4_FREE_BLOCKS_METADATA); > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ext4_free_blocks(handle= , inode, 0, nr, 1, flags); > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0if (parent_bh) { > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0/* > -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" i= n the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html