Re: [PATCH 1/3] filemap: Allow __filemap_get_folio to allocate large folios

[Dave Chinner <david@fromorbit.com>]

On Sat, May 20, 2023 at 05:36:01PM +0100, Matthew Wilcox (Oracle) wrote:
> Allow callers of __filemap_get_folio() to specify a preferred folio
> order in the FGP flags.  This is only honoured in the FGP_CREATE path;
> if there is already a folio in the page cache that covers the index,
> we will return it, no matter what its order is.  No create-around is
> attempted; we will only create folios which start at the specified index.
> Unmodified callers will continue to allocate order 0 folios.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>  include/linux/pagemap.h | 29 ++++++++++++++++++++++++---
>  mm/filemap.c            | 44 ++++++++++++++++++++++++++++-------------
>  mm/folio-compat.c       |  2 +-
>  mm/readahead.c          | 13 ------------
>  4 files changed, 57 insertions(+), 31 deletions(-)
> 
> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> index a56308a9d1a4..f4d05beb64eb 100644
> --- a/include/linux/pagemap.h
> +++ b/include/linux/pagemap.h
> @@ -466,6 +466,19 @@ static inline void *detach_page_private(struct page *page)
>  	return folio_detach_private(page_folio(page));
>  }
>  
> +/*
> + * There are some parts of the kernel which assume that PMD entries
> + * are exactly HPAGE_PMD_ORDER.  Those should be fixed, but until then,
> + * limit the maximum allocation order to PMD size.  I'm not aware of any
> + * assumptions about maximum order if THP are disabled, but 8 seems like
> + * a good order (that's 1MB if you're using 4kB pages)
> + */
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +#define MAX_PAGECACHE_ORDER	HPAGE_PMD_ORDER
> +#else
> +#define MAX_PAGECACHE_ORDER	8
> +#endif
> +
>  #ifdef CONFIG_NUMA
>  struct folio *filemap_alloc_folio(gfp_t gfp, unsigned int order);
>  #else
> @@ -505,14 +518,24 @@ pgoff_t page_cache_prev_miss(struct address_space *mapping,
>  #define FGP_NOWAIT		0x00000020
>  #define FGP_FOR_MMAP		0x00000040
>  #define FGP_STABLE		0x00000080
> +#define FGP_ORDER(fgp)		((fgp) >> 26)	/* top 6 bits */
>  
>  #define FGP_WRITEBEGIN		(FGP_LOCK | FGP_WRITE | FGP_CREAT | FGP_STABLE)
>  
> +static inline unsigned fgp_order(size_t size)
> +{
> +	unsigned int shift = ilog2(size);
> +
> +	if (shift <= PAGE_SHIFT)
> +		return 0;
> +	return (shift - PAGE_SHIFT) << 26;
> +}

Doesn't check for being larger than MAX_PAGECACHE_ORDER.

Also: naming. FGP_ORDER(fgp) to get the order stored in the fgp,
fgp_order(size) to get the order from the IO length.

Both are integers, the compiler is not going to tell us when we get
them the wrong way around, and it's impossible to determine which
one is right just from looking at the code.

Perhaps fgp_order_from_flags(fgp) and fgp_order_from_length(size)?

Also, why put the order in the high bits? Shifting integers up into
unaligned high bits is prone to sign extension issues and overflows.
e.g.  fgp_flags is passed around the filemap functions as a signed
integer, so using the high bit in a shifted value that is unsigned
seems like a recipe for unexpected sign extension bugs on
extraction.

Hence I'd much prefer low bits are used for this sort of integer
encoding (i.e. uses masks instead of shifts for extraction), and
that flags fields -always- use unsigned variables so high bit
usage doesn't unexpected do the wrong thing....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com