On 4/14/15 7:22 PM, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
> 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().
Cool, thanks for sorting that out!
> 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 <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 31 ++++++++++++++++++++++++++-----
> 1 file changed, 26 insertions(+), 5 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index aeffeaa..e5aa8a6 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -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);
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.
> +
> /*
> * 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.
"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));
?
> + */
> + 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
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
