On Wed 14-06-17 11:22:11, Ross Zwisler wrote:
> When servicing mmap() reads from file holes the current DAX code allocates
> a page cache page of all zeroes and places the struct page pointer in the
> mapping->page_tree radix tree. This has two major drawbacks:
>
> 1) It consumes memory unnecessarily. For every 4k page that is read via a
> DAX mmap() over a hole, we allocate a new page cache page. This means that
> if you read 1GiB worth of pages, you end up using 1GiB of zeroed memory.
> This is easily visible by looking at the overall memory consumption of the
> system or by looking at /proc/[pid]/smaps:
>
> 7f62e72b3000-7f63272b3000 rw-s 00000000 103:00 12 /root/dax/data
> Size: 1048576 kB
> Rss: 1048576 kB
> Pss: 1048576 kB
> Shared_Clean: 0 kB
> Shared_Dirty: 0 kB
> Private_Clean: 1048576 kB
> Private_Dirty: 0 kB
> Referenced: 1048576 kB
> Anonymous: 0 kB
> LazyFree: 0 kB
> AnonHugePages: 0 kB
> ShmemPmdMapped: 0 kB
> Shared_Hugetlb: 0 kB
> Private_Hugetlb: 0 kB
> Swap: 0 kB
> SwapPss: 0 kB
> KernelPageSize: 4 kB
> MMUPageSize: 4 kB
> Locked: 0 kB
>
> 2) The fact that we had to check for both DAX exceptional entries and for
> page cache pages in the radix tree made the DAX code more complex.
>
> Solve these issues by following the lead of the DAX PMD code and using a
> common 4k zero page instead. As with the PMD code we will now insert a DAX
> exceptional entry into the radix tree instead of a struct page pointer
> which allows us to remove all the special casing in the DAX code.
>
> Note that we do still pretty aggressively check for regular pages in the
> DAX radix tree, especially where we take action based on the bits set in
> the page. If we ever find a regular page in our radix tree now that most
> likely means that someone besides DAX is inserting pages (which has
> happened lots of times in the past), and we want to find that out early and
> fail loudly.
>
> This solution also removes the extra memory consumption. Here is that same
> /proc/[pid]/smaps after 1GiB of reading from a hole with the new code:
>
> 7f2054a74000-7f2094a74000 rw-s 00000000 103:00 12 /root/dax/data
> Size: 1048576 kB
> Rss: 0 kB
> Pss: 0 kB
> Shared_Clean: 0 kB
> Shared_Dirty: 0 kB
> Private_Clean: 0 kB
> Private_Dirty: 0 kB
> Referenced: 0 kB
> Anonymous: 0 kB
> LazyFree: 0 kB
> AnonHugePages: 0 kB
> ShmemPmdMapped: 0 kB
> Shared_Hugetlb: 0 kB
> Private_Hugetlb: 0 kB
> Swap: 0 kB
> SwapPss: 0 kB
> KernelPageSize: 4 kB
> MMUPageSize: 4 kB
> Locked: 0 kB
>
> Overall system memory consumption is similarly improved.
>
> Another major change is that we remove dax_pfn_mkwrite() from our fault
> flow, and instead rely on the page fault itself to make the PTE dirty and
> writeable. The following description from the patch adding the
> vm_insert_mixed_mkwrite() call explains this a little more:
>
> ***
> To be able to use the common 4k zero page in DAX we need to have our PTE
> fault path look more like our PMD fault path where a PTE entry can be
> marked as dirty and writeable as it is first inserted, rather than
> waiting for a follow-up dax_pfn_mkwrite() => finish_mkwrite_fault() call.
>
> Right now we can rely on having a dax_pfn_mkwrite() call because we can
> distinguish between these two cases in do_wp_page():
>
> case 1: 4k zero page => writable DAX storage
> case 2: read-only DAX storage => writeable DAX storage
>
> This distinction is made by via vm_normal_page(). vm_normal_page()
> returns false for the common 4k zero page, though, just as it does for
> DAX ptes. Instead of special casing the DAX + 4k zero page case, we will
> simplify our DAX PTE page fault sequence so that it matches our DAX PMD
> sequence, and get rid of dax_pfn_mkwrite() completely.
>
> This means that insert_pfn() needs to follow the lead of insert_pfn_pmd()
> and allow us to pass in a 'mkwrite' flag. If 'mkwrite' is set
> insert_pfn() will do the work that was previously done by wp_page_reuse()
> as part of the dax_pfn_mkwrite() call path.
> ***
This looks generally fine. Just two small comments below.
> @@ -216,17 +217,6 @@ static void dax_unlock_mapping_entry(struct address_space *mapping,
> dax_wake_mapping_entry_waiter(mapping, index, entry, false);
> }
>
> -static void put_locked_mapping_entry(struct address_space *mapping,
> - pgoff_t index, void *entry)
> -{
> - if (!radix_tree_exceptional_entry(entry)) {
> - unlock_page(entry);
> - put_page(entry);
> - } else {
> - dax_unlock_mapping_entry(mapping, index);
> - }
> -}
> -
The naming becomes asymetric with this. So I'd prefer keeping
put_locked_mapping_entry() as a trivial wrapper around
dax_unlock_mapping_entry() unless we can craft more sensible naming / API
for entry grabbing (and that would be a separate patch anyway).
> -static int dax_load_hole(struct address_space *mapping, void **entry,
> +static int dax_load_hole(struct address_space *mapping, void *entry,
> struct vm_fault *vmf)
> {
> struct inode *inode = mapping->host;
> - struct page *page;
> - int ret;
> -
> - /* Hole page already exists? Return it... */
> - if (!radix_tree_exceptional_entry(*entry)) {
> - page = *entry;
> - goto finish_fault;
> - }
> + unsigned long vaddr = vmf->address;
> + int ret = VM_FAULT_NOPAGE;
> + struct page *zero_page;
> + void *entry2;
>
> - /* This will replace locked radix tree entry with a hole page */
> - page = find_or_create_page(mapping, vmf->pgoff,
> - vmf->gfp_mask | __GFP_ZERO);
With this gone, you can also remove the special DAX handling from
mm/filemap.c: page_cache_tree_insert() and remove from dax.h
dax_wake_mapping_entry_waiter(), dax_radix_locked_entry() and RADIX_DAX
definitions. Yay! As a separate patch please.
Honza
--
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR
