On Mon, Aug 14, 2017 at 12:53:49PM +0200, Carlos Maiolino wrote:
> In a filesystem without finobt, the Space manager selects an AG to alloc a new
> inode, where xfs_dialloc_ag_inobt() will search the AG for the free slot chunk.
>
> When the new inode is in the samge AG as its parent, the btree will be searched
> starting on the parent's record, and then retried from the top if no slot is
> available beyond the parent's record.
>
> To exit this loop though, xfs_dialloc_ag_inobt() relies on the fact that the
> btree must have a free slot available, once its callers relied on the
> agi->freecount when deciding how/where to allocate this new inode.
>
> In the case when the agi->freecount is corrupted, showing available inodes in an
> AG, when in fact there is none, this becomes an infinite loop.
>
> Add a way to stop the loop when a free slot is not found in the btree, making
> the function to fall into the whole AG scan which will then, be able to detect
> the corruption and shut the filesystem down.
>
> As pointed by Brian, this might impact performance, giving the fact we
> don't reset the search distance anymore when we reach the end of the
> tree, giving it fewer tries before falling back to the whole AG search, but
> it will only affect searches that start within 10 records to the end of the tree.
>
> Signed-off-by: Carlos Maiolino <cmaiolino@xxxxxxxxxx>
Looks ok, will test... hey, what's the xfstest number?
Reviewed-by: Darrick J. Wong <darrick.wong@xxxxxxxxxx>
> ---
> Changelog:
>
> V3: - Move searchdistance state saving out of the while loop
> - Remove newino goto label
>
> V2: - Use searchdistance variable to stop the infinite loop
> instead of adding a new mechanism
>
> fs/xfs/libxfs/xfs_ialloc.c | 55 +++++++++++++++++++++++-----------------------
> 1 file changed, 27 insertions(+), 28 deletions(-)
>
> diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> index d41ade5d293e..0e1cc51f05a1 100644
> --- a/fs/xfs/libxfs/xfs_ialloc.c
> +++ b/fs/xfs/libxfs/xfs_ialloc.c
> @@ -1123,6 +1123,7 @@ xfs_dialloc_ag_inobt(
> int error;
> int offset;
> int i, j;
> + int searchdistance = 10;
>
> pag = xfs_perag_get(mp, agno);
>
> @@ -1149,7 +1150,6 @@ xfs_dialloc_ag_inobt(
> if (pagno == agno) {
> int doneleft; /* done, to the left */
> int doneright; /* done, to the right */
> - int searchdistance = 10;
>
> error = xfs_inobt_lookup(cur, pagino, XFS_LOOKUP_LE, &i);
> if (error)
> @@ -1210,21 +1210,9 @@ xfs_dialloc_ag_inobt(
> /*
> * Loop until we find an inode chunk with a free inode.
> */
> - while (!doneleft || !doneright) {
> + while (--searchdistance > 0 && (!doneleft || !doneright)) {
> int useleft; /* using left inode chunk this time */
>
> - if (!--searchdistance) {
> - /*
> - * Not in range - save last search
> - * location and allocate a new inode
> - */
> - xfs_btree_del_cursor(tcur, XFS_BTREE_NOERROR);
> - pag->pagl_leftrec = trec.ir_startino;
> - pag->pagl_rightrec = rec.ir_startino;
> - pag->pagl_pagino = pagino;
> - goto newino;
> - }
> -
> /* figure out the closer block if both are valid. */
> if (!doneleft && !doneright) {
> useleft = pagino -
> @@ -1268,26 +1256,37 @@ xfs_dialloc_ag_inobt(
> goto error1;
> }
>
> - /*
> - * We've reached the end of the btree. because
> - * we are only searching a small chunk of the
> - * btree each search, there is obviously free
> - * inodes closer to the parent inode than we
> - * are now. restart the search again.
> - */
> - pag->pagl_pagino = NULLAGINO;
> - pag->pagl_leftrec = NULLAGINO;
> - pag->pagl_rightrec = NULLAGINO;
> - xfs_btree_del_cursor(tcur, XFS_BTREE_NOERROR);
> - xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
> - goto restart_pagno;
> + if (searchdistance <= 0) {
> + /*
> + * Not in range - save last search
> + * location and allocate a new inode
> + */
> + xfs_btree_del_cursor(tcur, XFS_BTREE_NOERROR);
> + pag->pagl_leftrec = trec.ir_startino;
> + pag->pagl_rightrec = rec.ir_startino;
> + pag->pagl_pagino = pagino;
> +
> + } else {
> + /*
> + * We've reached the end of the btree. because
> + * we are only searching a small chunk of the
> + * btree each search, there is obviously free
> + * inodes closer to the parent inode than we
> + * are now. restart the search again.
> + */
> + pag->pagl_pagino = NULLAGINO;
> + pag->pagl_leftrec = NULLAGINO;
> + pag->pagl_rightrec = NULLAGINO;
> + xfs_btree_del_cursor(tcur, XFS_BTREE_NOERROR);
> + xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);
> + goto restart_pagno;
> + }
> }
>
> /*
> * In a different AG from the parent.
> * See if the most recently allocated block has any free.
> */
> -newino:
> if (agi->agi_newino != cpu_to_be32(NULLAGINO)) {
> error = xfs_inobt_lookup(cur, be32_to_cpu(agi->agi_newino),
> XFS_LOOKUP_EQ, &i);
> --
> 2.13.5
>
> --
> 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
