Message-ID: <20091214150453.GD4867@thunk.org>
Date:	Mon, 14 Dec 2009 10:04:53 -0500
From:	tytso@....edu
To:	Surbhi Palande <surbhi.palande@...onical.com>
Cc:	linux-ext4@...r.kernel.org, sandeen@...hat.com, fmayhar@...gle.com
Subject: Re: [PATCH v2] replaced BUG() with return -EIO from
 ext4_ext_get_blocks

Surbhi,

Thanks for the patch, and since I believe this is your first patch
submission, welcome to the ext4 developer's community!

I made a few stylistic changes to your patch before adding it to the
ext4 patch queue.  I removed the comment "We don't want to panic..."
since the explanation is not going to help someone reading the code in
the future (comments referring to non-existent code, such as why we're
not going to call BUG_ON are best placed in the commit description,
since when someone looks at the code a year from now, they won't see
the missing BUG_ON :-).

(BTW, the comment to which you added the "We don't want to panic.."
aside is itself kind of nasty:

      /*
       * consistent leaf must not be empty;
       * this situation is possible, though, _during_ tree modification;
       * this is why assert can't be put in ext4_ext_find_extent()
       */

This comment really should be made in the code body of
ext4_ext_find_extent(), with an comment above that function indicating
that callers who care should be making this check.  A code clean up
for another day, along with auditing all of the *other* callers of
ext4_ext_find_extent() that they are making this check when
appropriate...)

I also made some minor whitespace changes with respect to argument
alignment, and in the commit description I used the term "Kernel BZ"
and included the URL instead of using the term "upstream bug #", since
that's commonly used terminology for commits in the kernel tree.
(Upstream makes sense if the patch lives in a distro tree, but it's
less clear when it's actually in the upstream repository, since the
"upstream" developers generally don't refer to themselves or their
project in those terms.)

Thanks, regards,

						- Ted

commit 9f31d1227c3f2611405fc29e092c61a56b37da33
Author: Surbhi Palande <surbhi.palande@...onical.com>
Date:   Mon Dec 14 09:53:52 2009 -0500

    ext4: replace BUG() with return -EIO in ext4_ext_get_blocks
    
    This patch fixes the Kernel BZ #14286.  When the address of an extent
    corresponding to a valid block is corrupted, a -EIO should be reported
    instead of a BUG().  This situation should not normally not occur
    except in the case of a corrupted filesystem.  If however it does,
    then the system should not panic directly but depending on the mount
    time options appropriate action should be taken. If the mount options
    so permit, the I/O should be gracefully aborted by returning a -EIO.
    
    http://bugzilla.kernel.org/show_bug.cgi?id=14286
    
    Signed-off-by: Surbhi Palande <surbhi.palande@...onical.com>
    Signed-off-by: "Theodore Ts'o" <tytso@....edu>

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index 3a7928f..8fd6c56 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -3190,7 +3190,13 @@ int ext4_ext_get_blocks(handle_t *handle, struct inode *inode,
 	 * this situation is possible, though, _during_ tree modification;
 	 * this is why assert can't be put in ext4_ext_find_extent()
 	 */
-	BUG_ON(path[depth].p_ext == NULL && depth != 0);
+	if (path[depth].p_ext == NULL && depth != 0) {
+		ext4_error(inode->i_sb, __func__, "bad extent address "
+			   "inode: %lu, iblock: %d, depth: %d",
+			   inode->i_ino, iblock, depth);
+		err = -EIO;
+		goto out2;
+	}
 	eh = path[depth].p_hdr;
 
 	ex = path[depth].p_ext;
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists