[PATCH] ext4: fix indirect punch hole corruption

* [PATCH] ext4: fix indirect punch hole corruption
@ 2015-02-05 20:50 Omar Sandoval
  2015-02-05 20:56 ` Omar Sandoval
  2015-02-05 21:30 ` Chris J Arges
  0 siblings, 2 replies; 16+ messages in thread
From: Omar Sandoval @ 2015-02-05 20:50 UTC (permalink / raw)
  To: Theodore Ts'o, Andreas Dilger, Lukáš Czerner,
	linux-ext4, linux-kernel
  Cc: Chris J Arges, Omar Sandoval

Commit 4f579ae7de56 (ext4: fix punch hole on files with indirect
mapping) rewrote FALLOC_FL_PUNCH_HOLE for ext4 files with indirect
mapping. However, the case where the punch happens within one level of
indirection is incorrect. It assumes that the partial branches returned
from ext4_find_shared will have the same depth, but this is not
necessarily the case even when the offsets have the same depth. For
example, if the last block occurs at the beginning of an indirect group
(i.e., it has an offset of 0 at the end of the offsets array), then
ext4_find_shared will return a shallower chain. So, let's handle the
mismatch and clean up that case. Tested with generic/270, which no
longer leads to an inconsistent filesystem like before.

Signed-off-by: Omar Sandoval <osandov@osandov.com>
---
 fs/ext4/indirect.c | 61 +++++++++++++++++++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 26 deletions(-)

diff --git a/fs/ext4/indirect.c b/fs/ext4/indirect.c
index 36b3696..e04bbce 100644
--- a/fs/ext4/indirect.c
+++ b/fs/ext4/indirect.c
@@ -1434,50 +1434,59 @@ end_range:
 	 * in punch_hole so we need to point to the next element
 	 */
 	partial2->p++;
-	while ((partial > chain) || (partial2 > chain2)) {
-		/* We're at the same block, so we're almost finished */
-		if ((partial->bh && partial2->bh) &&
-		    (partial->bh->b_blocknr == partial2->bh->b_blocknr)) {
-			if ((partial > chain) && (partial2 > chain2)) {
-				ext4_free_branches(handle, inode, partial->bh,
-						   partial->p + 1,
-						   partial2->p,
-						   (chain+n-1) - partial);
-				BUFFER_TRACE(partial->bh, "call brelse");
-				brelse(partial->bh);
-				BUFFER_TRACE(partial2->bh, "call brelse");
-				brelse(partial2->bh);
-			}
+	while (partial > chain && partial2 > chain2) {
+		int depth = (chain+n-1) - partial;
+		int depth2 = (chain2+n2-1) - partial2;
+
+		if (partial->bh->b_blocknr == partial2->bh->b_blocknr) {
+			/*
+			 * We've converged on the same block. Clear the range,
+			 * then we're done.
+			 */
+			ext4_free_branches(handle, inode, partial->bh,
+					   partial->p + 1,
+					   partial2->p,
+					   (chain+n-1) - partial);
+			BUFFER_TRACE(partial->bh, "call brelse");
+			brelse(partial->bh);
+			BUFFER_TRACE(partial2->bh, "call brelse");
+			brelse(partial2->bh);
 			return 0;
 		}
+
 		/*
-		 * Clear the ends of indirect blocks on the shared branch
-		 * at the start of the range
+		 * The start and end partial branches may not be at the same
+		 * level even though the punch happened within one level. So, we
+		 * give them a chance to arrive at the same level, then walk
+		 * them in step with each other until we converge on the same
+		 * block.
 		 */
-		if (partial > chain) {
+		if (depth <= depth2) {
 			ext4_free_branches(handle, inode, partial->bh,
-				   partial->p + 1,
-				   (__le32 *)partial->bh->b_data+addr_per_block,
-				   (chain+n-1) - partial);
+					   partial->p + 1,
+					   (__le32 *)partial->bh->b_data+addr_per_block,
+					   (chain+n-1) - partial);
 			BUFFER_TRACE(partial->bh, "call brelse");
 			brelse(partial->bh);
 			partial--;
 		}
-		/*
-		 * Clear the ends of indirect blocks on the shared branch
-		 * at the end of the range
-		 */
-		if (partial2 > chain2) {
+		if (depth2 <= depth) {
 			ext4_free_branches(handle, inode, partial2->bh,
 					   (__le32 *)partial2->bh->b_data,
 					   partial2->p,
-					   (chain2+n-1) - partial2);
+					   (chain2+n2-1) - partial2);
 			BUFFER_TRACE(partial2->bh, "call brelse");
 			brelse(partial2->bh);
 			partial2--;
 		}
 	}
 
+	/*
+	 * The punch happened within the same level, so at some point we _must_
+	 * converge on an indirect block.
+	 */
+	BUG_ON(1);
+
 do_indirects:
 	/* Kill the remaining (whole) subtrees */
 	switch (offsets[0]) {
-- 
2.2.2


^ permalink raw reply related	[flat|nested] 16+ messages in thread