On Tue, Feb 13, 2024 at 10:05:54PM +0100, Pankaj Raghav (Samsung) wrote:
> On Tue, Feb 13, 2024 at 08:34:31AM -0800, Darrick J. Wong wrote:
> > On Tue, Feb 13, 2024 at 10:37:00AM +0100, Pankaj Raghav (Samsung) wrote:
> > > From: "Matthew Wilcox (Oracle)" <[email protected]>
> > >
> > > Some filesystems want to be able to limit the maximum size of folios,
> > > and some want to be able to ensure that folios are at least a certain
> > > size. Add mapping_set_folio_orders() to allow this level of control.
> > > The max folio order parameter is ignored and it is always set to
> > > MAX_PAGECACHE_ORDER.
> >
> > Why? If MAX_PAGECACHE_ORDER is 8 and I instead pass in max==3, I'm
> > going to be surprised by my constraint being ignored. Maybe I said that
> > because I'm not prepared to handle an order-7 folio; or some customer
> > will have some weird desire to twist this knob to make their workflow
> > faster.
> >
> > --D
> Maybe I should have been explicit. We are planning to add support
> for min order in the first round, and we want to add support for max order
> once the min order support is upstreamed. It was done mainly to reduce
> the scope and testing of this series.
>
> I definitely agree there are usecases for setting the max order. It is
> also the feedback we got from LPC.
>
> So one idea would be not to expose max option until we add the support
> for max order? So filesystems can only set the min_order with the
> initial support?
Yeah, there's really no point in having an argument that's deliberately
ignored.
--D
> > > +static inline void mapping_set_folio_orders(struct address_space *mapping,
> > > + unsigned int min, unsigned int max)
> > > +{
> > > + if (min == 1)
> > > + min = 2;
> > > + if (max < min)
> > > + max = min;
> > > + if (max > MAX_PAGECACHE_ORDER)
> > > + max = MAX_PAGECACHE_ORDER;
> > > +
> > > + /*
> > > + * XXX: max is ignored as only minimum folio order is supported
> > > + * currently.
> > > + */
> > > + mapping->flags = (mapping->flags & ~AS_FOLIO_ORDER_MASK) |
> > > + (min << AS_FOLIO_ORDER_MIN) |
> > > + (MAX_PAGECACHE_ORDER << AS_FOLIO_ORDER_MAX);
> > > +}
> > > +
>
On Tue, Feb 13, 2024 at 01:29:14PM -0800, Darrick J. Wong wrote:
> On Tue, Feb 13, 2024 at 10:05:54PM +0100, Pankaj Raghav (Samsung) wrote:
> > On Tue, Feb 13, 2024 at 08:34:31AM -0800, Darrick J. Wong wrote:
> > > On Tue, Feb 13, 2024 at 10:37:00AM +0100, Pankaj Raghav (Samsung) wrote:
> > > > From: "Matthew Wilcox (Oracle)" <[email protected]>
> > > >
> > > > Some filesystems want to be able to limit the maximum size of folios,
> > > > and some want to be able to ensure that folios are at least a certain
> > > > size. Add mapping_set_folio_orders() to allow this level of control.
> > > > The max folio order parameter is ignored and it is always set to
> > > > MAX_PAGECACHE_ORDER.
> > >
> > > Why? If MAX_PAGECACHE_ORDER is 8 and I instead pass in max==3, I'm
> > > going to be surprised by my constraint being ignored. Maybe I said that
> > > because I'm not prepared to handle an order-7 folio; or some customer
> > > will have some weird desire to twist this knob to make their workflow
> > > faster.
> > >
> > > --D
> > Maybe I should have been explicit. We are planning to add support
> > for min order in the first round, and we want to add support for max order
> > once the min order support is upstreamed. It was done mainly to reduce
> > the scope and testing of this series.
> >
> > I definitely agree there are usecases for setting the max order. It is
> > also the feedback we got from LPC.
> >
> > So one idea would be not to expose max option until we add the support
> > for max order? So filesystems can only set the min_order with the
> > initial support?
>
> Yeah, there's really no point in having an argument that's deliberately
> ignored.
I favour introducing the right APIs even if they're not fully implemented.
We have no filesystems today that need this, so it doesn't need to
be implemented, but if we have to go back and add it, it's more churn
for every filesystem. I'm open to better ideas about the API; I think
for a lot of filesystems they only want to set the minimum, so maybe
introducing that API now would be a good thing.