Re: [PATCH v5 2/3] btrfs: index free space entries on size

From: Josef Bacik <josef@toxicpanda.com>
To: Johannes Thumshirn <Johannes.Thumshirn@wdc.com>
Cc: "linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>,
	"kernel-team@fb.com" <kernel-team@fb.com>,
	Filipe Manana <fdmanana@suse.com>
Subject: Re: [PATCH v5 2/3] btrfs: index free space entries on size
Date: Fri, 19 Nov 2021 10:37:27 -0500	[thread overview]
Message-ID: <YZfEt/i0E87N+FmG@localhost.localdomain> (raw)
In-Reply-To: <PH0PR04MB7416BDFF1C613094C64689639B9C9@PH0PR04MB7416.namprd04.prod.outlook.com>

On Fri, Nov 19, 2021 at 10:52:38AM +0000, Johannes Thumshirn wrote:
> On 18/11/2021 22:33, Josef Bacik wrote:
> > +/*
> > + * This is a little subtle.  We *only* have ->max_extent_size set if we actually
> > + * searched through the bitmap and figured out the largest ->max_extent_size,
> > + * otherwise it's 0.  In the case that it's 0 we don't want to tell the
> > + * allocator the wrong thing, we want to use the actual real max_extent_size
> > + * we've found already if it's larger, or we want to use ->bytes.
> > + *
> > + * This matters because find_free_space() will skip entries who's ->bytes is
> > + * less than the required bytes.  So if we didn't search down this bitmap, we
> > + * may pick some previous entry that has a smaller ->max_extent_size than we
> > + * have.  For example, assume we have two entries, one that has
> > + * ->max_extent_size set to 4k and ->bytes set to 1M.  A second entry hasn't set
> > + * ->max_extent_size yet, has ->bytes set to 8k and it's contiguous.  We will
> > + *  call into find_free_space(), and return with max_extent_size == 4k, because
> > + *  that first bitmap entry had ->max_extent_size set, but the second one did
> > + *  not.  If instead we returned 8k we'd come in searching for 8k, and find the
> > + *  8k contiguous range.
> > + *
> > + *  Consider the other case, we have 2 8k chunks in that second entry and still
> > + *  don't have ->max_extent_size set.  We'll return 16k, and the next time the
> > + *  allocator comes in it'll fully search our second bitmap, and this time it'll
> > + *  get an uptodate value of 8k as the maximum chunk size.  Then we'll get the
> > + *  right allocation the next loop through.
> > + */
> > +static inline u64 get_max_extent_size(const struct btrfs_free_space *entry)
> > +{
> > +	if (entry->bitmap && entry->max_extent_size)
> > +		return entry->max_extent_size;
> > +	return entry->bytes;
> > +}
> 
> This part is also present in 
> [PATCH v5 1/3] btrfs: only use ->max_extent_size if it is set in the bitmap

Yeah I moved it up here, you'll see the removal lower down in the patch.
Thanks,

Josef