On 04/07/2024 16:20, Matthew Wilcox wrote: > On Thu, Jul 04, 2024 at 01:23:20PM +0100, Ryan Roberts wrote: >>> - AS_LARGE_FOLIO_SUPPORT = 6, >> >> nit: this removed enum is still referenced in a comment further down the file. > > Thanks. Pankaj, let me know if you want me to send you a patch or if > you'll do it directly. > >>> + /* Bits 16-25 are used for FOLIO_ORDER */ >>> + AS_FOLIO_ORDER_BITS = 5, >>> + AS_FOLIO_ORDER_MIN = 16, >>> + AS_FOLIO_ORDER_MAX = AS_FOLIO_ORDER_MIN + AS_FOLIO_ORDER_BITS, >> >> nit: These 3 new enums seem a bit odd. > > Yes, this is "too many helpful suggestions" syndrome. It made a lot > more sense originally. Well now you can add my "helpful" suggestion to that list :) > > https://lore.kernel.org/linux-fsdevel/ZlUQcEaP3FDXpCge@xxxxxxxxxxxxxxxxxxx/ > >>> +static inline void mapping_set_folio_order_range(struct address_space *mapping, >>> + unsigned int min, >>> + unsigned int max) >>> +{ >>> + if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) >>> + return; >>> + >>> + if (min > MAX_PAGECACHE_ORDER) >>> + min = MAX_PAGECACHE_ORDER; >>> + if (max > MAX_PAGECACHE_ORDER) >>> + max = MAX_PAGECACHE_ORDER; >>> + if (max < min) >>> + max = min; >> >> It seems strange to silently clamp these? Presumably for the bs>ps usecase, >> whatever values are passed in are a hard requirement? So wouldn't want them to >> be silently reduced. (Especially given the recent change to reduce the size of >> MAX_PAGECACHE_ORDER to less then PMD size in some cases). > > Hm, yes. We should probably make this return an errno. Including > returning an errno for !IS_ENABLED() and min > 0. Right. > >>> - if (new_order < MAX_PAGECACHE_ORDER) { >>> + if (new_order < mapping_max_folio_order(mapping)) { >>> new_order += 2; >>> - new_order = min_t(unsigned int, MAX_PAGECACHE_ORDER, new_order); >>> + new_order = min(mapping_max_folio_order(mapping), new_order); >>> new_order = min_t(unsigned int, new_order, ilog2(ra->size)); >> >> I wonder if its possible that ra->size could ever be less than >> mapping_min_folio_order()? Do you need to handle that? > > I think we take care of that in later patches? Yes I saw that once I got to it. You can ignore this. > This patch is mostly > about honouring the max properly and putting in the infrastructure for > the min, but not doing all the necessary work for min.
