Skip to contentSkip to conversation

xfs: extent size hints can round up extents past MAXEXTLEN

PATCH Superseded Dave Chinner 2 participants 3 messages Apr 15, 2015 XFS filesystem

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Dave Chinner

author
#0 lore raw

PATCH 1/1 xfs: extent size hints can round up extents past MAXEXTLEN

From: Dave Chinner <[email protected]>

This results in BMBT corruption, as seen by this test:

# mkfs.xfs -f -d size=40051712b,agcount=4 /dev/vdc
....
# mount /dev/vdc /mnt/scratch
# xfs_io -ft -c "extsize 16m" -c "falloc 0 30g" -c "bmap -vp" /mnt/scratch/foo

which results in this failure on a debug kernel:

XFS: Assertion failed: (blockcount & xfs_mask64hi(64-BMBT_BLOCKCOUNT_BITLEN)) == 0, file: fs/xfs/libxfs/xfs_bmap_btree.c, line: 211
....

Call Trace:
 [<ffffffff814cf0ff>] xfs_bmbt_set_allf+0x8f/0x100
 [<ffffffff814cf18d>] xfs_bmbt_set_all+0x1d/0x20
 [<ffffffff814f2efe>] xfs_iext_insert+0x9e/0x120
 [<ffffffff814c7956>] ? xfs_bmap_add_extent_hole_real+0x1c6/0xc70
 [<ffffffff814c7956>] xfs_bmap_add_extent_hole_real+0x1c6/0xc70
 [<ffffffff814caaab>] xfs_bmapi_write+0x72b/0xed0
 [<ffffffff811c72ac>] ? kmem_cache_alloc+0x15c/0x170
 [<ffffffff814fe070>] xfs_alloc_file_space+0x160/0x400
 [<ffffffff81ddcc29>] ? down_write+0x29/0x60
 [<ffffffff815063eb>] xfs_file_fallocate+0x29b/0x310
 [<ffffffff811d2bc8>] ? __sb_start_write+0x58/0x120
 [<ffffffff811e3e18>] ? do_vfs_ioctl+0x318/0x570
 [<ffffffff811cd680>] vfs_fallocate+0x140/0x260
 [<ffffffff811ce6f8>] SyS_fallocate+0x48/0x80
 [<ffffffff81ddec09>] system_call_fastpath+0x12/0x17

The tracepoint that indicates the extent that triggered the assert failure is:

xfs_iext_insert: idx 0 offset 0 block 16777224 count 2097152 flag 1

Clearly indicating that the extent length is greater than MAXEXTLEN, which is 2097151. A prior trace point shows the allocation was an exact size match and that a length greater than MAXEXTLEN was asked for:

xfs_alloc_size_done: agno 1 agbno 8 minlen 2097152 maxlen 2097152

					    ^^^^^^^        ^^^^^^^

The issue is that the extent size hint alignment is rounding up the extent size past MAXEXTLEN, because xfs_bmapi_write() is not taking into account extent size hints when calculating the maximum extent length to allocate. xfs_bmapi_reserve_delalloc() is already doing this, but direct extent allocation is not.

We don't see this problem with extent size hints through the IO path because we can't do single IOs large enough to trigger MAXEXTLEN allocation. fallocate(), OTOH, is not limited in it's allocation sizes and so needs help here. The fix is simply to copy the logic from xfs_bmapi_reserve_delalloc() and apply it apropriately to xfs_bmapi_write().

I also add an ASSERT() to xfs_bmap_extsize_align() so we'll catch cases of alignment exceeding MAXEXTLEN on debug kernel machines in future.

Signed-off-by Dave Chinner <[email protected]>
---
 fs/xfs/libxfs/xfs_bmap.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)
fs/xfs/libxfs/xfs_bmap.c +26 -5
index aeffeaa..e5aa8a6 100644
@@ -3224,12 +3224,21 @@ xfs_bmap_extsize_align(
 		align_alen += temp;
 		align_off -= temp;
 	}
+
+	/* Same adjustment for the end of the requested area. */
+	temp = (align_alen % extsz);
+	if (temp)
+		align_alen += extsz - temp;
+
 	/*
-	 * Same adjustment for the end of the requested area.
+	 * we are in trouble if the caller requested an extent that will align
+	 * to something larger than the supported on disk extent size.  Assert
+	 * fail here to catch callers that make this mistake; they should always
+	 * be setting the maximum allocation length to be (MAXEXTLEN - extsz) so
+	 * we can round outwards here for alignment.
 	 */
-	if ((temp = (align_alen % extsz))) {
-		align_alen += extsz - temp;
-	}
+	ASSERT(align_alen <= MAXEXTLEN);
+
 	/*
 	 * If the previous block overlaps with this proposed allocation
 	 * then move the start forward without adjusting the length.

@@ -4287,7 +4296,19 @@ xfs_bmapi_allocate(
 					 &bma->prev);
 		}
 	} else {
-		bma->length = XFS_FILBLKS_MIN(bma->length, MAXEXTLEN);
+		/* Figure out the extent size, adjust alen */
+		xfs_extlen_t	maxlen = MAXEXTLEN;
+		xfs_extlen_t	extsz = xfs_get_extsz_hint(bma->ip);
+
+		/*
+		 * Make sure we don't exceed a single extent length when we
+		 * align the extent by reducing length we are going to allocate
+		 * by the maximum amount extent size aligment may require.
+		 */
+		if (extsz)
+			maxlen -= (2 * extsz - 1);
+
+		bma->length = XFS_FILBLKS_MIN(bma->length, maxlen);
 		if (!bma->eof)
 			bma->length = XFS_FILBLKS_MIN(bma->length,
 					bma->got.br_startoff - bma->offset);
Signature · 6 lines
2.0.0

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
#1 lore raw
Quoted 49 lines · Dave Chinner

From: Dave Chinner <[email protected]>

This results in BMBT corruption, as seen by this test:

# mkfs.xfs -f -d size=40051712b,agcount=4 /dev/vdc
....
# mount /dev/vdc /mnt/scratch
# xfs_io -ft -c "extsize 16m" -c "falloc 0 30g" -c "bmap -vp" /mnt/scratch/foo

which results in this failure on a debug kernel:

XFS: Assertion failed: (blockcount & xfs_mask64hi(64-BMBT_BLOCKCOUNT_BITLEN)) == 0, file: fs/xfs/libxfs/xfs_bmap_btree.c, line: 211
....

Call Trace:
 [<ffffffff814cf0ff>] xfs_bmbt_set_allf+0x8f/0x100
 [<ffffffff814cf18d>] xfs_bmbt_set_all+0x1d/0x20
 [<ffffffff814f2efe>] xfs_iext_insert+0x9e/0x120
 [<ffffffff814c7956>] ? xfs_bmap_add_extent_hole_real+0x1c6/0xc70
 [<ffffffff814c7956>] xfs_bmap_add_extent_hole_real+0x1c6/0xc70
 [<ffffffff814caaab>] xfs_bmapi_write+0x72b/0xed0
 [<ffffffff811c72ac>] ? kmem_cache_alloc+0x15c/0x170
 [<ffffffff814fe070>] xfs_alloc_file_space+0x160/0x400
 [<ffffffff81ddcc29>] ? down_write+0x29/0x60
 [<ffffffff815063eb>] xfs_file_fallocate+0x29b/0x310
 [<ffffffff811d2bc8>] ? __sb_start_write+0x58/0x120
 [<ffffffff811e3e18>] ? do_vfs_ioctl+0x318/0x570
 [<ffffffff811cd680>] vfs_fallocate+0x140/0x260
 [<ffffffff811ce6f8>] SyS_fallocate+0x48/0x80
 [<ffffffff81ddec09>] system_call_fastpath+0x12/0x17

The tracepoint that indicates the extent that triggered the assert failure is:

xfs_iext_insert: idx 0 offset 0 block 16777224 count 2097152 flag 1

Clearly indicating that the extent length is greater than MAXEXTLEN, which is 2097151. A prior trace point shows the allocation was an exact size match and that a length greater than MAXEXTLEN was asked for:

xfs_alloc_size_done: agno 1 agbno 8 minlen 2097152 maxlen 2097152

					    ^^^^^^^        ^^^^^^^

The issue is that the extent size hint alignment is rounding up the extent size past MAXEXTLEN, because xfs_bmapi_write() is not taking into account extent size hints when calculating the maximum extent length to allocate. xfs_bmapi_reserve_delalloc() is already doing this, but direct extent allocation is not.

Quoted 6 lines

We don't see this problem with extent size hints through the IO path because we can't do single IOs large enough to trigger MAXEXTLEN allocation. fallocate(), OTOH, is not limited in it's allocation sizes and so needs help here. The fix is simply to copy the logic from xfs_bmapi_reserve_delalloc() and apply it apropriately to xfs_bmapi_write().

Cool, thanks for sorting that out!

Quoted 25 lines

I also add an ASSERT() to xfs_bmap_extsize_align() so we'll catch cases of alignment exceeding MAXEXTLEN on debug kernel machines in future.

Signed-off-by Dave Chinner <[email protected]>
---
 fs/xfs/libxfs/xfs_bmap.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)
fs/xfs/libxfs/xfs_bmap.c +6 -1
index aeffeaa..e5aa8a6 100644
@@ -3224,12 +3224,21 @@ xfs_bmap_extsize_align(
 		align_alen += temp;
 		align_off -= temp;
 	}
+
+	/* Same adjustment for the end of the requested area. */
+	temp = (align_alen % extsz);
+	if (temp)
+		align_alen += extsz - temp;
+
 	/*
-	 * Same adjustment for the end of the requested area.

fs/xfs/libxfs/xfs_bmap.c xfs_bmap_extsize_align( · line 3240

Quoted 10 lines
fs/xfs/libxfs/xfs_bmap.c +6 -3
+	 * we are in trouble if the caller requested an extent that will align
+	 * to something larger than the supported on disk extent size.  Assert
+	 * fail here to catch callers that make this mistake; they should always
+	 * be setting the maximum allocation length to be (MAXEXTLEN - extsz) so
+	 * we can round outwards here for alignment.
 	 */
-	if ((temp = (align_alen % extsz))) {
-		align_alen += extsz - temp;
-	}
+	ASSERT(align_alen <= MAXEXTLEN);

Hm, I was going to ask if we could return EINVAL w/ a warning printk, so that this doesn't silently corrupt on non-dbug kernels, but I see callers do things like:

                error = xfs_bmap_extsize_align(mp, &ap->got, &ap->prev,
                                                align, 0, ap->eof, 0, ap->conv,
                                                &ap->offset, &ap->length);
                ASSERT(!error);

and nothing else with the error return, so... hohum. I think those callchains could be cleaned up to handle errors but ... outside the scope of this patch.

Quoted 7 lines
fs/xfs/libxfs/xfs_bmap.c +1 -0
+
 	/*
 	 * If the previous block overlaps with this proposed allocation
 	 * then move the start forward without adjusting the length.

@@ -4287,7 +4296,19 @@ xfs_bmapi_allocate(
 					 &bma->prev);
 		}

fs/xfs/libxfs/xfs_bmap.c xfs_bmapi_allocate( · line 4306

Quoted 10 lines
fs/xfs/libxfs/xfs_bmap.c +8 -1
 	} else {
-		bma->length = XFS_FILBLKS_MIN(bma->length, MAXEXTLEN);
+		/* Figure out the extent size, adjust alen */
+		xfs_extlen_t	maxlen = MAXEXTLEN;
+		xfs_extlen_t	extsz = xfs_get_extsz_hint(bma->ip);
+
+		/*
+		 * Make sure we don't exceed a single extent length when we
+		 * align the extent by reducing length we are going to allocate
+		 * by the maximum amount extent size aligment may require.

"alignment" (maybe fix the other comment too?)

Or better yet, would this be possible to factor into a helper?

/*
* Make sure we don't exceed a single extent length when we
* align the extent by reducing length we are going to
* allocate by the maximum amount extent size aligment may
* require.
*/
STATIC xfs_extlen_t
xfs_max_extent_len(
struct xfs_inode *ip)

{
        xfs_extlen_t    maxlen = MAXEXTLEN;
        xfs_extlen_t    extsz = xfs_get_extsz_hint(ip);

	/* Insert comment about math here ;) */
        if (extsz)
                maxlen -= (2 * extsz - 1);

        return maxlen;
}

...

	bma->length = XFS_FILBLKS_MIN(bma->length, xfs_max_extent_len(ip));

?

fs/xfs/libxfs/xfs_bmap.c xfs_bmapi_allocate( · line 4314

Quoted 9 lines
fs/xfs/libxfs/xfs_bmap.c +5 -0
+		 */
+		if (extsz)
+			maxlen -= (2 * extsz - 1);
+
+		bma->length = XFS_FILBLKS_MIN(bma->length, maxlen);
 		if (!bma->eof)
 			bma->length = XFS_FILBLKS_MIN(bma->length,
 					bma->got.br_startoff - bma->offset);

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs

author
#2 lore raw
Quoted 58 lines · Eric Sandeen
Quoted 49 lines · Dave Chinner

From: Dave Chinner <[email protected]>

This results in BMBT corruption, as seen by this test:

# mkfs.xfs -f -d size=40051712b,agcount=4 /dev/vdc
....
# mount /dev/vdc /mnt/scratch
# xfs_io -ft -c "extsize 16m" -c "falloc 0 30g" -c "bmap -vp" /mnt/scratch/foo

which results in this failure on a debug kernel:

XFS: Assertion failed: (blockcount & xfs_mask64hi(64-BMBT_BLOCKCOUNT_BITLEN)) == 0, file: fs/xfs/libxfs/xfs_bmap_btree.c, line: 211
....

Call Trace:
 [<ffffffff814cf0ff>] xfs_bmbt_set_allf+0x8f/0x100
 [<ffffffff814cf18d>] xfs_bmbt_set_all+0x1d/0x20
 [<ffffffff814f2efe>] xfs_iext_insert+0x9e/0x120
 [<ffffffff814c7956>] ? xfs_bmap_add_extent_hole_real+0x1c6/0xc70
 [<ffffffff814c7956>] xfs_bmap_add_extent_hole_real+0x1c6/0xc70
 [<ffffffff814caaab>] xfs_bmapi_write+0x72b/0xed0
 [<ffffffff811c72ac>] ? kmem_cache_alloc+0x15c/0x170
 [<ffffffff814fe070>] xfs_alloc_file_space+0x160/0x400
 [<ffffffff81ddcc29>] ? down_write+0x29/0x60
 [<ffffffff815063eb>] xfs_file_fallocate+0x29b/0x310
 [<ffffffff811d2bc8>] ? __sb_start_write+0x58/0x120
 [<ffffffff811e3e18>] ? do_vfs_ioctl+0x318/0x570
 [<ffffffff811cd680>] vfs_fallocate+0x140/0x260
 [<ffffffff811ce6f8>] SyS_fallocate+0x48/0x80
 [<ffffffff81ddec09>] system_call_fastpath+0x12/0x17

The tracepoint that indicates the extent that triggered the assert failure is:

xfs_iext_insert: idx 0 offset 0 block 16777224 count 2097152 flag 1

Clearly indicating that the extent length is greater than MAXEXTLEN, which is 2097151. A prior trace point shows the allocation was an exact size match and that a length greater than MAXEXTLEN was asked for:

xfs_alloc_size_done: agno 1 agbno 8 minlen 2097152 maxlen 2097152

					    ^^^^^^^        ^^^^^^^

The issue is that the extent size hint alignment is rounding up the extent size past MAXEXTLEN, because xfs_bmapi_write() is not taking into account extent size hints when calculating the maximum extent length to allocate. xfs_bmapi_reserve_delalloc() is already doing this, but direct extent allocation is not.

Quoted 6 lines

We don't see this problem with extent size hints through the IO path because we can't do single IOs large enough to trigger MAXEXTLEN allocation. fallocate(), OTOH, is not limited in it's allocation sizes and so needs help here. The fix is simply to copy the logic from xfs_bmapi_reserve_delalloc() and apply it apropriately to xfs_bmapi_write().

Cool, thanks for sorting that out!

....

Quoted 39 lines

fs/xfs/libxfs/xfs_bmap.c xfs_bmapi_allocate( · line 4306

Quoted 13 lines
fs/xfs/libxfs/xfs_bmap.c +8 -1
@@ -4287,7 +4296,19 @@ xfs_bmapi_allocate(
 					 &bma->prev);
 		}
 	} else {
-		bma->length = XFS_FILBLKS_MIN(bma->length, MAXEXTLEN);
+		/* Figure out the extent size, adjust alen */
+		xfs_extlen_t	maxlen = MAXEXTLEN;
+		xfs_extlen_t	extsz = xfs_get_extsz_hint(bma->ip);
+
+		/*
+		 * Make sure we don't exceed a single extent length when we
+		 * align the extent by reducing length we are going to allocate
+		 * by the maximum amount extent size aligment may require.

"alignment" (maybe fix the other comment too?)

Or better yet, would this be possible to factor into a helper?

/*
* Make sure we don't exceed a single extent length when we
* align the extent by reducing length we are going to
* allocate by the maximum amount extent size aligment may
* require.
*/
STATIC xfs_extlen_t
xfs_max_extent_len(
struct xfs_inode *ip)

{
        xfs_extlen_t    maxlen = MAXEXTLEN;
        xfs_extlen_t    extsz = xfs_get_extsz_hint(ip);

	/* Insert comment about math here ;) */
        if (extsz)
                maxlen -= (2 * extsz - 1);

        return maxlen;
}
Quoted 3 lines

...

	bma->length = XFS_FILBLKS_MIN(bma->length, xfs_max_extent_len(ip));

I thought about that, then just sent the working patch ;)

I'll refactor and send again.

Cheers,

Dave.

Signature · 7 lines
Dave Chinner
[email protected]

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs