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

[Matthew Wilcox <willy@infradead.org>]

On Sun, May 21, 2023 at 12:13:57PM +1000, Dave Chinner wrote:
> > +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.

It doesn't need to.  I check it on extraction.  We've got six bits,
so we can't overflow it.

> 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)?

Yeah, I don't like that either.  I could be talked into
fgp_set_order(size) and fgp_get_order(fgp).  Also we should type the
FGP flags like we type the GFP flags.

> 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.

As long as it's an unsigned int in the function which does the extraction,
there's no problem.  It's also kind of hard to set the top bit -- you'd
have to somehow get a 2^44 byte write into iomap.

> 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....

There are some encoding advantages to using low bits for flags.  Does
depend on the architecture; x86 is particularly prone to this kind of
thing, but ARM has various constraints on what constants it can
represent as immediates.  I've rarely had cause to care about other
architecture details, but generally low bits are better supported
as flags than high bits.