diff mbox series

[RFC,3/3] ext4: prevent partial update of the extent blocks

Message ID 20210908120850.4012324-4-yi.zhang@huawei.com
State Awaiting Upstream
Headers show
Series ext4: enhance extent consistency check | expand

Commit Message

Zhang Yi Sept. 8, 2021, 12:08 p.m. UTC 
In the most error path of current extents updating operations are not
roll back partial updates properly when some bad things happens(.e.g in
ext4_ext_insert_extent()). So we may get an inconsistent extents tree
if journal has been aborted due to IO error, which may probability lead
to BUGON later when we accessing these extent entries in errors=continue
mode. This patch drop extent buffer's verify flag before updatng the
contents in ext4_ext_get_access(), and reset it after updating in
__ext4_ext_dirty(). After this patch we could force to check the extent
buffer if extents tree updating was break off, make sure the extents are
consistent.

Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
---
 fs/ext4/extents.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

Comments

Theodore Ts'o Oct. 7, 2021, 4:45 p.m. UTC | #1 
On Wed, Sep 08, 2021 at 08:08:50PM +0800, Zhang Yi wrote:
> In the most error path of current extents updating operations are not
> roll back partial updates properly when some bad things happens(.e.g in
> ext4_ext_insert_extent()). So we may get an inconsistent extents tree
> if journal has been aborted due to IO error, which may probability lead
> to BUGON later when we accessing these extent entries in errors=continue
> mode. This patch drop extent buffer's verify flag before updatng the
> contents in ext4_ext_get_access(), and reset it after updating in
> __ext4_ext_dirty(). After this patch we could force to check the extent
> buffer if extents tree updating was break off, make sure the extents are
> consistent.
> 
> Signed-off-by: Zhang Yi <yi.zhang@huawei.com>

Looks good, thanks

Reviewed-by: Theodore Ts'o <tytso@mit.edu>

						- Ted
diff mbox series

Patch

diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
index d2601194b462..9228de6950a2 100644
--- a/fs/ext4/extents.c
+++ b/fs/ext4/extents.c
@@ -136,15 +136,25 @@  int ext4_datasem_ensure_credits(handle_t *handle, struct inode *inode,
 static int ext4_ext_get_access(handle_t *handle, struct inode *inode,
 				struct ext4_ext_path *path)
 {
+	int err = 0;
+
 	if (path->p_bh) {
 		/* path points to block */
 		BUFFER_TRACE(path->p_bh, "get_write_access");
-		return ext4_journal_get_write_access(handle, inode->i_sb,
-						     path->p_bh, EXT4_JTR_NONE);
+		err = ext4_journal_get_write_access(handle, inode->i_sb,
+						    path->p_bh, EXT4_JTR_NONE);
+		/*
+		 * The extent buffer's verified bit will be set again in
+		 * __ext4_ext_dirty(). We could leave an inconsistent
+		 * buffer if the extents updating procudure break off du
+		 * to some error happens, force to check it again.
+		 */
+		if (!err)
+			clear_buffer_verified(path->p_bh);
 	}
 	/* path points to leaf/index in inode body */
 	/* we use in-core data, no need to protect them */
-	return 0;
+	return err;
 }
 
 /*
@@ -165,6 +175,9 @@  static int __ext4_ext_dirty(const char *where, unsigned int line,
 		/* path points to block */
 		err = __ext4_handle_dirty_metadata(where, line, handle,
 						   inode, path->p_bh);
+		/* Extents updating done, re-set verified flag */
+		if (!err)
+			set_buffer_verified(path->p_bh);
 	} else {
 		/* path points to leaf/index in inode body */
 		err = ext4_mark_inode_dirty(handle, inode);