On Thu, Feb 16, 2017 at 10:10:35PM +0100, Christoph Hellwig wrote:
> In various places we currently assert that xfs_bmap_btalloc allocates
> from the same as the firstblock value passed in, unless it's either
> NULLAGNO or the dop_low flag is set. But the reflink code does not
> fully follow this convention as it passes in firstblock purely as
> a hint for the allocator without actually having previous allocations
Are you referring to BMAPI_REMAP? In that case *firstblock isn't a
hint, it's the new physical location of the extent.
> in the transaction, and without having a minleft check on the current
> AG, leading to the assert firing on a very full and heavily used
> file system. As even the reflink code only allocates from equal or
> higher AGs for now we can simply the check to always allow for equal
> or higher AGs.
>
> Note that we need to eventually split the two meanings of the firstblock
> value. At that point we can also allow the reflink code to allocate
> from any AG instead of limiting it in any way.
...but you're correct that we probably ought to disambiguate the two.
For now,
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
By the way, was it your intent to push this series into 4.11?
--D
>
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> ---
> fs/xfs/libxfs/xfs_bmap.c | 22 ++++++----------------
> 1 file changed, 6 insertions(+), 16 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> index bfc00de5c6f1..2e79f4cc9a8e 100644
> --- a/fs/xfs/libxfs/xfs_bmap.c
> +++ b/fs/xfs/libxfs/xfs_bmap.c
> @@ -804,9 +804,7 @@ xfs_bmap_extents_to_btree(
> */
> ASSERT(args.fsbno != NULLFSBLOCK);
> ASSERT(*firstblock == NULLFSBLOCK ||
> - args.agno == XFS_FSB_TO_AGNO(mp, *firstblock) ||
> - (dfops->dop_low &&
> - args.agno > XFS_FSB_TO_AGNO(mp, *firstblock)));
> + args.agno >= XFS_FSB_TO_AGNO(mp, *firstblock));
> *firstblock = cur->bc_private.b.firstblock = args.fsbno;
> cur->bc_private.b.allocated++;
> ip->i_d.di_nblocks++;
> @@ -3822,17 +3820,13 @@ xfs_bmap_btalloc(
> * the first block that was allocated.
> */
> ASSERT(*ap->firstblock == NULLFSBLOCK ||
> - XFS_FSB_TO_AGNO(mp, *ap->firstblock) ==
> - XFS_FSB_TO_AGNO(mp, args.fsbno) ||
> - (ap->dfops->dop_low &&
> - XFS_FSB_TO_AGNO(mp, *ap->firstblock) <
> - XFS_FSB_TO_AGNO(mp, args.fsbno)));
> + XFS_FSB_TO_AGNO(mp, *ap->firstblock) <=
> + XFS_FSB_TO_AGNO(mp, args.fsbno));
>
> ap->blkno = args.fsbno;
> if (*ap->firstblock == NULLFSBLOCK)
> *ap->firstblock = args.fsbno;
> - ASSERT(nullfb || fb_agno == args.agno ||
> - (ap->dfops->dop_low && fb_agno < args.agno));
> + ASSERT(nullfb || fb_agno <= args.agno);
> ap->length = args.len;
> if (!(ap->flags & XFS_BMAPI_COWFORK))
> ap->ip->i_d.di_nblocks += args.len;
> @@ -4746,13 +4740,9 @@ xfs_bmapi_write(
> if (bma.cur) {
> if (!error) {
> ASSERT(*firstblock == NULLFSBLOCK ||
> - XFS_FSB_TO_AGNO(mp, *firstblock) ==
> + XFS_FSB_TO_AGNO(mp, *firstblock) <=
> XFS_FSB_TO_AGNO(mp,
> - bma.cur->bc_private.b.firstblock) ||
> - (dfops->dop_low &&
> - XFS_FSB_TO_AGNO(mp, *firstblock) <
> - XFS_FSB_TO_AGNO(mp,
> - bma.cur->bc_private.b.firstblock)));
> + bma.cur->bc_private.b.firstblock));
> *firstblock = bma.cur->bc_private.b.firstblock;
> }
> xfs_btree_del_cursor(bma.cur,
> --
> 2.11.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
