| Message ID | 20170814105349.12391-1-cmaiolino@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
I do apologize for forgetting to update the subject before sending it, this is the 3rd version of this patch > > 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 >
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@redhat.com> Looks ok, will test... hey, what's the xfstest number? Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com> > --- > 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@vger.kernel.org > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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);
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@redhat.com> --- 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(-)