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.
