On Thu, Aug 24, 2017 at 03:29:42PM -0600, Ross Zwisler wrote: > > + /* DAX can shortcut the normal fault path on write faults! */ > > Does this comment still make sense for the PMD case? I kept the code as before, and also kept the comment intent as-is (although I shortend it a bit). > Here's what I *think* it > means: For DAX write faults we make the PTE writeable in the ->fault() code > and don't rely on a follow-up ->page_mkwrite() call. This happens in > do_shared_fault(), where we return before the ->page_mkwrite() call because > the DAX write fault returns VM_FAULT_NOPAGE. > > This is in contrast to the normal page fault case where the ->fault() call > ends up calling filemap_fault(), which populates the page read-only, and then > do_shared_fault() does a follow-up ->page_mkwrite() call which makes the page > writable. > > First, is the above correct? :) Yes. > If so, I think the comment doesn't make > sense for the PMD fault path because in that case we always make the PMD > writeable in the first ->huge_fault() call, as there is no follow-up > *_mkwrite()? Well, there is non-DAX huge_fault case. It still is different from the normal non-hugepage fault case, so the comment still makes some sense, but maybe I should remove the DAX. That being said if we eventually get huge page page cache support (patches are on the list) I suspect they'll work like the normal fault path, not the DAX path. > > static int > > STATIC We're phasing that out, so I should probably remove it where it's currently newly added/moved in the patch. > I see that this size check is gone from the new code, and we now rely on the > equivalent check in dax_iomap_fault(). Nice. Yes. And I actually used to mention that in the changelog, but it got lost. I'll re-add it. > I wonder if pe_size would be more readable as a string via __print_symbolic() > so we see "pe_size PMD" instead of "pe_size 1" from enum page_entry_size? Probably. I was just lazy :) If I want to go all the way I could also use a __print_flags for the FAULT_FLAG_* flags.
