Re: [PATCH 04/13] filemap: use mapping_min_order while allocating folios

From: Matthew Wilcox <willy@infradead.org>
To: "Pankaj Raghav (Samsung)" <kernel@pankajraghav.com>
Cc: linux-xfs@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org, david@fromorbit.com,
	chandan.babu@oracle.com, akpm@linux-foundation.org,
	mcgrof@kernel.org, ziy@nvidia.com, hare@suse.de,
	djwong@kernel.org, gost.dev@samsung.com, linux-mm@kvack.org,
	Pankaj Raghav <p.raghav@samsung.com>
Subject: Re: [PATCH 04/13] filemap: use mapping_min_order while allocating folios
Date: Mon, 26 Feb 2024 14:47:33 +0000	[thread overview]
Message-ID: <ZdykhSuPbe6knu89@casper.infradead.org> (raw)
In-Reply-To: <20240226094936.2677493-5-kernel@pankajraghav.com>

On Mon, Feb 26, 2024 at 10:49:27AM +0100, Pankaj Raghav (Samsung) wrote:
> Add some additional VM_BUG_ON() in page_cache_delete[batch] and
> __filemap_add_folio to catch errors where we delete or add folios that
> has order less than min_order.

I don't understand why we need these checks in the deletion path.  The
add path, yes, absolutely.  But the delete path?

> @@ -896,6 +900,8 @@ noinline int __filemap_add_folio(struct address_space *mapping,
>  			}
>  		}
>  
> +		VM_BUG_ON_FOLIO(folio_order(folio) < mapping_min_folio_order(mapping),
> +				folio);

But I don't understand why you put it here, while we're holding the
xa_lock.  That seems designed to cause maximum disruption.  Why not put
it at the beginning of the function with all the other VM_BUG_ON_FOLIO?

> @@ -1847,6 +1853,9 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index,
>  		fgf_t fgp_flags, gfp_t gfp)
>  {
>  	struct folio *folio;
> +	unsigned int min_order = mapping_min_folio_order(mapping);
> +
> +	index = mapping_align_start_index(mapping, index);

I would not do this here.

>  repeat:
>  	folio = filemap_get_entry(mapping, index);
> @@ -1886,7 +1895,7 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index,
>  		folio_wait_stable(folio);
>  no_page:
>  	if (!folio && (fgp_flags & FGP_CREAT)) {
> -		unsigned order = FGF_GET_ORDER(fgp_flags);
> +		unsigned int order = max(min_order, FGF_GET_ORDER(fgp_flags));
>  		int err;

Put it here instead.

>  		if ((fgp_flags & FGP_WRITE) && mapping_can_writeback(mapping))
> @@ -1912,8 +1921,13 @@ struct folio *__filemap_get_folio(struct address_space *mapping, pgoff_t index,
>  			gfp_t alloc_gfp = gfp;
>  
>  			err = -ENOMEM;
> +			if (order < min_order)
> +				order = min_order;
>  			if (order > 0)
>  				alloc_gfp |= __GFP_NORETRY | __GFP_NOWARN;
> +
> +			VM_BUG_ON(index & ((1UL << order) - 1));

Then you don't need this BUG_ON because it's obvious you just did it.
And the one in filemap_add_folio() would catch it anyway.