Re: [PATCH] btrfs-progs: add a --check-bg-usage option to fsck

[Josef Bacik <josef@toxicpanda.com>]

On Fri, Aug 02, 2019 at 09:54:12PM +0800, Qu Wenruo wrote:
> 
> 
> On 2019/8/2 下午9:06, Josef Bacik wrote:
> > Sometimes when messing with the chunk allocator code we can end up
> > over-allocating chunks.  Generally speaking I'll notice this when a
> > random xfstest fails with ENOSPC when it shouldn't, but I'm super
> > worried that I won't catch a problem until somebody has a fs completely
> > filled up with empty block groups.  Add a fsck option to check for too
> > many empty block groups.  This way I can set FSCK_OPTIONS="-B" to catch
> > cases where we're too aggressive with the chunk allocator but not so
> > aggressive that it causes problems in xfstests.
> > 
> > Thankfully this doesn't trip up currently, so this will just keep me
> > from regressing us.  Thanks,
> 
> I think the empty bg check is valid.
> 
> Although I hope this check can be a warning for default check, and a new
> option to report too many empty bgs as error.
> 

I don't want to make it default for the reason you describe below.  I'm thinking
of some new log-writes test that checks an fs at a transaction commit where
we've emptied a ton of block groups but haven't removed them yet.

<snip>

> > +
> > +	if (empty_data > 1) {
> > +		ret = -EINVAL;
> > +		fprintf(stderr, "Too many empty data block groups: %d\n",
> > +			empty_data);
> > +	}
> > +	if (empty_metadata > 1) {
> > +		ret = -EINVAL;
> > +		fprintf(stderr, "Too many empty metadata block groups: %d\n",
> > +			empty_metadata);
> > +	}
> > +	if (empty_system > 1) {
> > +		ret = -EINVAL;
> > +		fprintf(stderr, "Too many empty system block groups: %d\n",
> > +			empty_system);
> > +	}
> 
> This hard coded threshold (1) is too vague and maybe too restrict.
> What will happen for things like a lot of data got removed and cleaner
> didn't get kicked in quickly enough before unmount?
> 

Which is why I ran it through xfstests first to verify this wasn't too weird.
What I'm try to capture is the case where something has really gone wrong, so
there will be tons of empty block groups, not just one.  I think generally
speaking having 1 left over makes sense for the testcases that fill and then
delete a bunch, so I don't want those to fail.  Thanks,

Josef