Re: [PATCH v8 01/10] fs: Allow fine-grained control of folio sizes

Pankaj Raghav (Samsung)
2024-07-04T21:28:44Z

On Thu, Jul 04, 2024 at 04:20:13PM +0100, 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.
Good catch.
>
> Thanks. Pankaj, let me know if you want me to send you a patch or if
> you'll do it directly.
Yes, I will fold the changes.
>
> > > +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.
>

Something like this? (I also need to change the xfs_icache.c to
use this return value in the last patch)

diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
index 14e1415f7dcf..04916720f807 100644
--- a/include/linux/pagemap.h
+++ b/include/linux/pagemap.h
@@ -390,28 +390,27 @@ static inline void mapping_set_gfp_mask(struct address_space *m, gfp_t mask)
* Context: This should not be called while the inode is active as it
* is non-atomic.
*/
-static inline void mapping_set_folio_order_range(struct address_space *mapping,
+static inline int mapping_set_folio_order_range(struct address_space *mapping,
unsigned int min,
unsigned int max)
{
if (!IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE))
- return;
+ return -EINVAL;

- if (min > MAX_PAGECACHE_ORDER)
- min = MAX_PAGECACHE_ORDER;
- if (max > MAX_PAGECACHE_ORDER)
- max = MAX_PAGECACHE_ORDER;
+ if (min > MAX_PAGECACHE_ORDER || max > MAX_PAGECACHE_ORDER)
+ return -EINVAL;
if (max < min)
max = min;

mapping->flags = (mapping->flags & ~AS_FOLIO_ORDER_MASK) |
(min << AS_FOLIO_ORDER_MIN) | (max << AS_FOLIO_ORDER_MAX);
+ return 0;
}

-static inline void mapping_set_folio_min_order(struct address_space *mapping,
+static inline int mapping_set_folio_min_order(struct address_space *mapping,
unsigned int min)
{
- mapping_set_folio_order_range(mapping, min, MAX_PAGECACHE_ORDER);
+ return mapping_set_folio_order_range(mapping, min, MAX_PAGECACHE_ORDER);
}

@@ -428,6 +427,10 @@ static inline void mapping_set_folio_min_order(struct address_space *mapping,
*/
static inline void mapping_set_large_folios(struct address_space *mapping)
{
+ /*
+ * The return value can be safely ignored because this range
+ * will always be supported by the page cache.
+ */
mapping_set_folio_order_range(mapping, 0, MAX_PAGECACHE_ORDER);
}

--
Pankaj