From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp1040.oracle.com ([156.151.31.81]:46053 "EHLO userp1040.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752754AbdHNWsX (ORCPT ); Mon, 14 Aug 2017 18:48:23 -0400 Date: Mon, 14 Aug 2017 15:48:17 -0700 From: "Darrick J. Wong" Subject: Re: [PATCH] Stop searching for free slots in an inode chunk when there are none Message-ID: <20170814224817.GD4796@magnolia> References: <20170814105349.12391-1-cmaiolino@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170814105349.12391-1-cmaiolino@redhat.com> Sender: linux-xfs-owner@vger.kernel.org List-ID: List-Id: xfs To: Carlos Maiolino Cc: linux-xfs@vger.kernel.org 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 Looks ok, will test... hey, what's the xfstest number? Reviewed-by: Darrick J. Wong > --- > 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