On Fri, Aug 04, 2017 at 05:36:01AM -0400, Carlos Eduardo Maiolino wrote:
> One more thing.
>
>
>
> ----- Original Message -----
> > From: "Carlos Eduardo Maiolino" <cmaiolin@xxxxxxxxxx>
> > To: "Dave Chinner" <david@xxxxxxxxxxxxx>
> > Cc: linux-xfs@xxxxxxxxxxxxxxx
> > 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@xxxxxxxxxxxxx
--
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
