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@xxxxxxxxxxxxx
