Re: [PATCH] Stop searching for free slots in an inode chunk when there are none

[Dave Chinner <david@fromorbit.com>]

On Fri, Aug 04, 2017 at 05:36:01AM -0400, Carlos Eduardo Maiolino wrote:
> One more thing.
> 
> 
> 
> ----- Original Message -----
> > From: "Carlos Eduardo Maiolino" <cmaiolin@redhat.com>
> > To: "Dave Chinner" <david@fromorbit.com>
> > Cc: linux-xfs@vger.kernel.org
> > Sent: Friday, August 4, 2017 10:55:26 AM
> > Subject: Re: [PATCH] Stop searching for free slots in an inode chunk when there are none
> > 
> > Hi Dave.
> > 
> > 
> > > > 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.
> > > 
> > > That doesn't sound quite right. The initial scan and the restart
> > > loop are both limited to scanning search_distance records - we never
> > > search the entire tree except when it's really small (i..e less than
> > > 10-20 records (640-1280 inodes) depending on balance). If the
> > > pagino record to end of btree distance in both directions is shorter
> > > than the search distance for a given loop (i.e. less than 10 records
> > > from pagino to end-of-btree) then that is the only time a corrupted
> > > agi->freecount can cause this problem.
> > > 
> > 
> > I agree with you, but still, we are feasible to have this corruption
> > happening,
> > and I've seen reports of users hitting it.
> > 
> > 
> > > IOWs, on production systems where there's more than a few hundred
> > > inodes (i.e. the vast majority of installations) a corrupted
> > > agi->freecount won't lead to a endless loop because search_distance
> > > will terminate the retry loop and we'll allocate a new inode.
> > > 
> > > To tell the truth, I'd much rather we just use the search distance
> > > to prevent endless looping than add a second method of limiting
> > > the search loop. i.e. don't reset search_distance when we restart
> > > the search loop at pagino.  That means even for small trees (<
> > > search_distance * 2 records) we'll retry when we get to the end of
> > > tree, but we'll still break out of the loop and allocate new inodes
> > > as soon as we hit the search distance limit.
> > > 
> > 
> 
> Here, you are assuming we enter into the 
> 
> while (!doneleft || !doneright) { }
> 
> on every interaction, so it will be able to decrease the searchdistance or you
> mean by moving the --searchdistance somewhere else?
> 
> In very small trees we don't even enter the while loop (both doneleft and doneright are 1),
> so searchdistance isn't decremented at all, resetting it or not will not make any difference
> in this case.

Seems like a minor issue - the first search step left+right is
outside the while loop, and we don't account for that. So change
where the search distance check to take that into account:

	while (--searchdistance > 0 && (!doneleft || !doneright)) {
		.....
	}

	if (searchdistance <= 0) {
		/* save current chunk indexes */
		....
		goto newino;
	}

	/* restart at pagino */
	.....
	goto restart_pagno;


-Dave.
-- 
Dave Chinner
david@fromorbit.com