resize2fs: fix ENOSPC corruption case

http://people.redhat.com/esandeen/livecd-creator-imagefile.bz2
contains an image (for now) which, when resized to 578639, corrupts the filesystem.

This is a bit crazy, I guess, because the fs currently has only 1 free block, but still, we should be graceful about the failure. Perhaps it would make sense to check the requested valuea against the minimum value resize2fs would compute for "-P" and fail (at least without a force).

But in any case, this exposed 2 bugs when moving that one block required an extent split, which is what hit the ENOSPC.

For starters, ext2fs_extent_set_bmap() in the "(re/un)mapping last block in extent" case was replacing the old extent before the new one was created; when the new extent creation failed, it left us in an inconsistent state. Simply changing the order of the two should fix this problem.

Next, ext2fs_extent_insert was calling ext2fs_extent_delete() on *any* error, including one caused by failure to allocate a new block to split the node to hold that extent ... the handle was left unchanged, and we deleted the -original- extent.

As a quick fix for this, just don't do the delete if we fail the split, though this may need to be smarter. I don't think we have terribly consistent behavior about where a handle is left on various errors.

---

Index: e2fsprogs/lib/ext2fs/extent.c
===================================================================
@@ -1068,16 +1068,17 @@ errcode_t ext2fs_extent_insert(ext2_exte
 
 	retval = ext2fs_extent_replace(handle, 0, extent);
 	if (retval)
-		goto errout;
+		goto errout_delete;
 
 	retval = update_path(handle);
 	if (retval)
-		goto errout;
+		goto errout_delete;
 
 	return 0;
 
-errout:
+errout_delete:
 	ext2fs_extent_delete(handle, 0);
+errout:
 	return retval;
 }
 

@@ -1239,16 +1240,17 @@ again:
 #ifdef DEBUG
 		printf("(re/un)mapping last block in extent\n");
 #endif
-		extent.e_len--;
-		retval = ext2fs_extent_replace(handle, 0, &extent);
-		if (retval)
-			goto done;
+		/* Make sure insert works before replacing old extent */
 		if (physical) {
 			retval = ext2fs_extent_insert(handle,
 					EXT2_EXTENT_INSERT_AFTER, &newextent);
 			if (retval)
 				goto done;
 		}
+		extent.e_len--;
+		retval = ext2fs_extent_replace(handle, 0, &extent);
+		if (retval)
+			goto done;
 	} else if (logical == extent.e_lblk) {
 #ifdef DEBUG
 		printf("(re/un)mapping first block in extent\n");

heh, well, maybe not:

[root@inode e2fsprogs]# dumpe2fs -h /mnt/test/livecd-creator-imagefile | grep "Block count"
dumpe2fs 1.41.5 (23-Apr-2009)
Block count: 578640
[root@inode e2fsprogs]# resize/resize2fs.static -P /mnt/test/livecd-creator-imagefile
resize2fs 1.41.5 (23-Apr-2009)
Estimated minimum size of the filesystem: 9171138

The estimated minimum size is 16x the current size.

So much for that plan, at least without a bit more work.

-Eric

I guess there are other earlier direct returns, so sure, that'd be fine.

yeah, it probably needs more careful inspection. Who wrote that thing anyway ... ;)

No, but I should.

yep, this likely needs more work to document the semantics, make sure they're consistent, and check all those error cases.

For F11 I will likely put in the later 2 patches, to fix up the minimum size reporting and then enforce that as the minimum size, so we shouldn't(tm) get into these error paths. Seems like the safer last-minute change.

-Eric

The first half of this patch I think is quite safe, and after stepping through tst_etent, I'm pretty sure the second half of this patch would be pretty easy to validate. We probably don't want to enter any of these error paths for now, though, since it's not clear we've fied all of them, particularly in the split_node code.

- Ted

http://people.redhat.com/esandeen/livecd-creator-imagefile.bz2
contains an image (for now) which, when resized to 578639, corrupts the filesystem.

This is a bit crazy, I guess, because the fs currently has only 1 free block, but still, we should be graceful about the failure. Perhaps it would make sense to check the requested valuea against the minimum value resize2fs would compute for "-P" and fail (at least without a force).

But in any case, this exposed 2 bugs when moving that one block required an extent split, which is what hit the ENOSPC.

For starters, ext2fs_extent_set_bmap() in the "(re/un)mapping last block in extent" case was replacing the old extent before the new one was created; when the new extent creation failed, it left us in an inconsistent state. Simply changing the order of the two should fix this problem.

Next, ext2fs_extent_insert was calling ext2fs_extent_delete() on *any* error, including one caused by failure to allocate a new block to split the node to hold that extent ... the handle was left unchanged, and we deleted the -original- extent.

As a quick fix for this, just don't do the delete if we fail the split, though this may need to be smarter. I don't think we have terribly consistent behavior about where a handle is left on various errors.

v2: lose the goto change complexity

---

index 143929e..ddb2d2a 100644
@@ -1040,7 +1040,7 @@ errcode_t ext2fs_extent_insert(ext2_extent_handle_t handle, int flags,
 #endif
 			retval = extent_node_split(handle);
 			if (retval)
-				goto errout;
+				return retval;
 			path = handle->path + handle->level;
 		}
 	}

@@ -1239,16 +1239,17 @@ again:
 #ifdef DEBUG
 		printf("(re/un)mapping last block in extent\n");
 #endif
-		extent.e_len--;
-		retval = ext2fs_extent_replace(handle, 0, &extent);
-		if (retval)
-			goto done;
+		/* Make sure insert works before replacing old extent */
 		if (physical) {
 			retval = ext2fs_extent_insert(handle,
 					EXT2_EXTENT_INSERT_AFTER, &newextent);
 			if (retval)
 				goto done;
 		}
+		extent.e_len--;
+		retval = ext2fs_extent_replace(handle, 0, &extent);
+		if (retval)
+			goto done;
 	} else if (logical == extent.e_lblk) {
 #ifdef DEBUG
 		printf("(re/un)mapping first block in extent\n");

Applied, thanks.

- Ted