Fixes issue tytso#146

This patch modifies the logic of ext2fs_extent_set_bmap() so that:

- Handling of "only block in extent" special case is removed
  - This affects files during a shrink with resize2fs; for example, when a file
    has an extent of 100 blocks and all 100 are relocated, the current
    (unpatched) code does this:
    - (ref.: inode_scan_and_fix() -> ext2fs_block_iterate3() -> process_block()
      which remaps each block and returns BLOCK_CHANGED to
      ext2fs_block_iterate3(), which then calls ext2fs_extent_set_bmap() to
      update the extent tree).
    - A new extent is inserted for the same lblk (because the pblk has moved),
      and the original extent is pushed back/shrunk by 1 block (start pblk is
      incremented by 1, e_len reduced by 1).
    - All other blocks in the extent (except the last one) are appended to the
      newly-inserted extent, and the original extent is pushed back/shrunk
      accordingly (one block at a time).
    - When it's time to process the last remaining block, the code notices an
      extent of e_len = 1 (the last one remaining with the old pblk) and
      "optimizes" this by rewriting it in place (instead of appending it to the
      new extent).
    - The end result is that *every* file that has an extent with e_len > 1 is
      *always* split into two extents (the second one consisting of a single
      block).
  - I think the intention here was to make the code more efficient by making the
    update in place, but the way resize2fs calls this function practically
    causes the number of extents to double up during a shrink; in my testcase a
    large file with ~8300 extents before the resize ends up with ~8200 after
    resize (compared with ~15k extents with the unpatched code).

- More preference is given to growing the previous extent's end, by swapping two
  if blocks
    - This helps compacting the extents during resize2fs (swapping the blocks is
      necessary because of the removal above).
  - When an existing extent is shrunk and its e_len becomes zero, it is deleted.
    - I am not clear if an extent with length == 0 is valid; judging from
      https://www.mail-archive.com/[email protected]/msg20364.html,
      probably not.
  - Without this change, "extent.e_len += (extent.e_lblk - start)" in
    ext2fs_extent_fix_parents() may overflow and throw the end of the extent way
    outside the size of the filesystem. Check with the e2 image referenced in this bug.

In summary, this patch fixes tytso#146 and
optimizes the allocation of extents after a shrinking resize2fs.

        Signed-off-by: Andrea Biardi <[email protected]>

lib/ext2fs/extent.c

@@ -1439,21 +1439,49 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle,
 		retval = ext2fs_extent_fix_parents(handle);
 		if (retval)
 			goto done;
-	} else if ((logical == extent.e_lblk) && (extent.e_len == 1))  {
+	} else if (logical == extent.e_lblk) {
 #ifdef DEBUG
-		printf("(re/un)mapping only block in extent\n");
+		printf("(re/un)mapping first block in extent\n");
 #endif
 		if (physical) {
-			retval = ext2fs_extent_replace(handle, 0, &newextent);
-		} else {
-			retval = ext2fs_extent_delete(handle, 0);
+			if (has_prev &&
+			    (logical == (prev_extent.e_lblk +
+					 prev_extent.e_len)) &&
+			    (physical == (prev_extent.e_pblk +
+					  prev_extent.e_len)) &&
+			    (new_uninit == prev_uninit) &&
+			    ((int) prev_extent.e_len < max_len-1)) {
+				retval = ext2fs_extent_get(handle, 
+					EXT2_EXTENT_PREV_LEAF, &prev_extent);
+				if (retval)
+					goto done;
+				prev_extent.e_len++;
+				retval = ext2fs_extent_replace(handle, 0,
+							       &prev_extent);
+			} else
+				retval = ext2fs_extent_insert(handle,
+							      0, &newextent);
+			if (retval)
+				goto done;
+			retval = ext2fs_extent_fix_parents(handle);
+			if (retval)
+				goto done;
+			retval = ext2fs_extent_get(handle,
+						   EXT2_EXTENT_NEXT_LEAF,
+						   &extent);
 			if (retval)
 				goto done;
-			ec = ext2fs_extent_fix_parents(handle);
-			if (ec != EXT2_ET_NO_CURRENT_NODE)
-				retval = ec;
 		}
-
+		extent.e_pblk++;
+		extent.e_lblk++;
+		extent.e_len--;
+		if (extent.e_len)
+			retval = ext2fs_extent_replace(handle, 0, &extent);
+		else
+			retval = ext2fs_extent_delete(handle, 0);
+		if (retval)
+			goto done;
+		retval = ext2fs_extent_fix_parents(handle);
 		if (retval)
 			goto done;
 	} else if (logical == extent.e_lblk + extent.e_len - 1)  {
@@ -1500,50 +1528,10 @@ errcode_t ext2fs_extent_set_bmap(ext2_extent_handle_t handle,
 			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");
-#endif
-		if (physical) {
-			if (has_prev &&
-			    (logical == (prev_extent.e_lblk +
-					 prev_extent.e_len)) &&
-			    (physical == (prev_extent.e_pblk +
-					  prev_extent.e_len)) &&
-			    (new_uninit == prev_uninit) &&
-			    ((int) prev_extent.e_len < max_len-1)) {
-				retval = ext2fs_extent_get(handle, 
-					EXT2_EXTENT_PREV_LEAF, &prev_extent);
-				if (retval)
-					goto done;
-				prev_extent.e_len++;
-				retval = ext2fs_extent_replace(handle, 0,
-							       &prev_extent);
-			} else
-				retval = ext2fs_extent_insert(handle,
-							      0, &newextent);
-			if (retval)
-				goto done;
-			retval = ext2fs_extent_fix_parents(handle);
-			if (retval)
-				goto done;
-			retval = ext2fs_extent_get(handle,
-						   EXT2_EXTENT_NEXT_LEAF,
-						   &extent);
-			if (retval)
-				goto done;
-		}
-		extent.e_pblk++;
-		extent.e_lblk++;
-		extent.e_len--;
-		retval = ext2fs_extent_replace(handle, 0, &extent);
-		if (retval)
-			goto done;
-		retval = ext2fs_extent_fix_parents(handle);
+		if (--extent.e_len)
+			retval = ext2fs_extent_replace(handle, 0, &extent);
+		else
+			retval = ext2fs_extent_delete(handle, 0);
 		if (retval)
 			goto done;
 	} else {