Re: [PATCH v5.1] fs: Allow fine-grained control of folio sizes

["Pankaj Raghav (Samsung)" <kernel@pankajraghav.com>]

On Tue, May 28, 2024 at 09:00:00AM +1000, Dave Chinner wrote:
> On Mon, May 27, 2024 at 11:43:43PM +0100, Matthew Wilcox wrote:
> > On Mon, May 27, 2024 at 11:39:47PM +0100, Matthew Wilcox wrote:
> > > > > +	AS_FOLIO_ORDER_MIN = 16,
> > > > > +	AS_FOLIO_ORDER_MAX = 21, /* Bits 16-25 are used for FOLIO_ORDER */
> > > > >  };
> > > > >  
> > > > > +#define AS_FOLIO_ORDER_MIN_MASK 0x001f0000
> > > > > +#define AS_FOLIO_ORDER_MAX_MASK 0x03e00000
> > > > 
> > > > As you changed the mapping flag offset, these masks also needs to be
> > > > changed accordingly.
> > > 
> > > That's why I did change them?
> > 
> > How about:
> > 
> > -#define AS_FOLIO_ORDER_MIN_MASK 0x001f0000
> > -#define AS_FOLIO_ORDER_MAX_MASK 0x03e00000
> > +#define AS_FOLIO_ORDER_MIN_MASK (31 << AS_FOLIO_ORDER_MIN)
> > +#define AS_FOLIO_ORDER_MAX_MASK (31 << AS_FOLIO_ORDER_MAX)
> 
> Lots of magic numbers based on the order having only having 5 bits
> of resolution. Removing that magic looks like this:
> 
> 	AS_FOLIO_ORDER_BITS = 5,

I think this needs to be defined outside of the enum as 5 is already
taken by AS_NO_WRITEBACK_TAGS? But I like the idea of making it generic
like this.

Something like this?

#define AS_FOLIO_ORDER_BITS 5
/*
 * Bits in mapping->flags.
 */
enum mapping_flags {
	AS_EIO		= 0,	/* IO error on async write */
	AS_ENOSPC	= 1,	/* ENOSPC on async write */
	AS_MM_ALL_LOCKS	= 2,	/* under mm_take_all_locks() */
	AS_UNEVICTABLE	= 3,	/* e.g., ramdisk, SHM_LOCK */
	AS_EXITING	= 4, 	/* final truncate in progress */
	/* writeback related tags are not used */
	AS_NO_WRITEBACK_TAGS = 5,
	AS_RELEASE_ALWAYS = 6,	/* Call ->release_folio(), even if no private data */
	AS_STABLE_WRITES = 7,	/* must wait for writeback before modifying
				   folio contents */
	AS_UNMOVABLE = 8,	 /* The mapping cannot be moved, ever */
	/* Bit 16-21 are used for FOLIO_ORDER */
	AS_FOLIO_ORDER_MIN = 16,
	AS_FOLIO_ORDER_MAX = AS_FOLIO_ORDER_MIN + AS_FOLIO_ORDER_BITS, 
};

@willy: I can fold this change that Chinner is proposing if you are fine
with this.

> 	AS_FOLIO_ORDER_MIN = 16,
> 	AS_FOLIO_ORDER_MAX = AS_FOLIO_ORDER_MIN + AS_FOLIO_ORDER_BITS,
> };
> 
> #define AS_FOLIO_ORDER_MASK	((1u << AS_FOLIO_ORDER_BITS) - 1)
> #define AS_FOLIO_ORDER_MIN_MASK	(AS_FOLIO_ORDER_MASK << AS_FOLIO_ORDER_MIN)
> #define AS_FOLIO_ORDER_MAX_MASK	(AS_FOLIO_ORDER_MASK << AS_FOLIO_ORDER_MAX)
> 
> This way if we want to increase the order mask, we only need to
> change AS_FOLIO_ORDER_BITS and everything else automatically
> recalculates.
> 
> Doing this means We could also easily use the high bits of the flag
> word for the folio orders, rather than putting them in the middle of
> the flag space...
> 
> -Dave.
> -- 
> Dave Chinner
> david@fromorbit.com