Re: [PATCH 04/19] btrfs: keep track of cleanliness of the bitmap

[Dennis Zhou <dennis@kernel.org>]

On Thu, Oct 10, 2019 at 10:16:30AM -0400, Josef Bacik wrote:
> On Mon, Oct 07, 2019 at 04:17:35PM -0400, Dennis Zhou wrote:
> > There is a cap in btrfs in the amount of free extents that a block group
> > can have. When it surpasses that threshold, future extents are placed
> > into bitmaps. Instead of keeping track of if a certain bit is trimmed or
> > not in a second bitmap, keep track of the relative state of the bitmap.
> > 
> > With async discard, trimming bitmaps becomes a more frequent operation.
> > As a trade off with simplicity, we keep track of if discarding a bitmap
> > is in progress. If we fully scan a bitmap and trim as necessary, the
> > bitmap is marked clean. This has some caveats as the min block size may
> > skip over regions deemed too small. But this should be a reasonable
> > trade off rather than keeping a second bitmap and making allocation
> > paths more complex. The downside is we may overtrim, but ideally the min
> > block size should prevent us from doing that too often and getting stuck
> > trimming
> > pathological cases.
> > 
> > BTRFS_FSC_TRIMMING_BITMAP is added to indicate a bitmap is in the
> > process of being trimmed. If additional free space is added to that
> > bitmap, the bit is cleared. A bitmap will be marked BTRFS_FSC_TRIMMED if
> > the trimming code was able to reach the end of it and the former is
> > still set.
> > 
> > Signed-off-by: Dennis Zhou <dennis@kernel.org>
> 
> I went through and looked at the end result and it appears to me that we never
> have TRIMMED and TRIMMING set at the same time.  Since these are the only two
> flags, and TRIMMING is only set on bitmaps, it makes more sense for this to be
> more like
> 
> enum btrfs_trim_state {
> 	BTRFS_TRIM_STATE_TRIMMED,
> 	BTRFS_TRIM_STATE_TRIMMING,
> 	BTRFS_TRIM_STATE_UNTRIMMED,
> };
> 
> and then just have enum btrfs_trim_state trim_state in the free space entry.
> This makes things a bit cleaner since it's really just a state indicator rather
> than a actual flags.  Thanks,
> 

That makes sense. I've gone ahead and done this for both this state and
the block group discard state. FWIW, at the time I didn't know what I
would need and flags was just easier to iterate on. I agree this is
nicer.

Thanks,
Dennis