Re: [PATCH] dax: use switch statement over chained ifs

[Matthew Wilcox]

On Mon, Jan 16, 2023 at 09:07:23PM -0800, Amy Parker wrote:
> On Mon, Jan 16, 2023 at 6:41 PM Matthew Wilcox <willy@xxxxxxxxxxxxx> wrote:
> > CAUTION: This email originated from outside your organization. Exercise caution when opening attachments or clicking links, especially from unknown senders.

Muahaha.  I am evil.

> > Thanks for the patch!  Two problems.  First, your mailer seems to have
> > mangled the patch; in my tree these are tab indents, and the patch has
> > arrived with four-space indents, so it can't be applied.
> 
> Ah, gotcha. Next time I'll just use git send-email, was hoping this
> time I'd be able to use my normal mailing system directly. (Also
> hoping my mail server isn't applying anything outgoing that messes it
> up... should probably check on that)

Feel free to send me the patch again, off-list, and I can check if it
arrived correctly.

> > The second problem is that this function should simply not exist.
> > I forget how we ended up with enum page_entry_size, but elsewhere
> > we simply pass 'order' around.  So what I'd really like to see is
> > a patch series that eliminates page_entry_size everywhere.
> 
> Hmm, alright... I'm not really familiar with the enum/how it's used, I
> pretty much just added this as a cleanup. If you've got any
> information on it so I know how to actually work with it, that'd be
> great!

The intent is to describe which "layer" of the page tables we're trying
to hadle a fault for -- PTE, PMD or PUD.  But as you can see by this
pe_order() function, the rest of the kernel tends to use the order
to communicate this information, so pass in 0, PMD_ORDER or PUD_ORDER.
Also PMD_ORDER and PUD_ORDER should exist in mm.h ;-)

> > I can outline a way to do that in individual patches if that would be
> > helpful.
> 
> Alright - although, would it actually need to be individual patches?
> I'm not 100% sure whether the page_entry_size used across the kernel
> is the same enum or different enums, my guess looking at the grep
> context summary is that they are the same, but the number of usages (I
> count 18) should fit in a single patch just fine...

I'd take it step by step.  First, I'd lift pe_order() to mm.h.
Second patch, convert dax_finish_sync_fault() to take an order instead
of a pe_size, making each caller call pe_order().  And do it at
the start of each function, eg the very first line of
__xfs_filemap_fault() should be

	unsigned int order = pe_order(pe_size);

Third, convert dax_iomap_fault() to take an order instead of a pe_size.
Fourth, convert huge_fault() to take an order.  Fifth, remove the
enum and pe_order.

This makes it easier to review, as well as looking good for your
contribution stats ;-)