Re: [PATCH v5 2/7] dax: support dirty DAX entries in radix tree

Andrew Morton
2015-12-22T22:46:12Z

On Fri, 18 Dec 2015 22:22:15 -0700 Ross Zwisler <ross.zwisler@linux.intel.com> wrote:

> Add support for tracking dirty DAX entries in the struct address_space
> radix tree. This tree is already used for dirty page writeback, and it
> already supports the use of exceptional (non struct page*) entries.
>
> In order to properly track dirty DAX pages we will insert new exceptional
> entries into the radix tree that represent dirty DAX PTE or PMD pages.
> These exceptional entries will also contain the writeback addresses for the
> PTE or PMD faults that we can use at fsync/msync time.
>
> There are currently two types of exceptional entries (shmem and shadow)
> that can be placed into the radix tree, and this adds a third. We rely on
> the fact that only one type of exceptional entry can be found in a given
> radix tree based on its usage. This happens for free with DAX vs shmem but
> we explicitly prevent shadow entries from being added to radix trees for
> DAX mappings.
>
> The only shadow entries that would be generated for DAX radix trees would
> be to track zero page mappings that were created for holes. These pages
> would receive minimal benefit from having shadow entries, and the choice
> to have only one type of exceptional entry in a given radix tree makes the
> logic simpler both in clear_exceptional_entry() and in the rest of DAX.
>
>
> ...
>
> --- a/include/linux/dax.h
> +++ b/include/linux/dax.h
> @@ -36,4 +36,9 @@ static inline bool vma_is_dax(struct vm_area_struct *vma)
> {
> return vma->vm_file && IS_DAX(vma->vm_file->f_mapping->host);
> }
> +
> +static inline bool dax_mapping(struct address_space *mapping)
> +{
> + return mapping->host && IS_DAX(mapping->host);
> +}

Can we make this evaluate to plain old "0" when CONFIG_FS_DAX=n? That
way a bunch of code in callers will fall away as well.

If the compiler has any brains then a good way to do this would be to
make IS_DAX be "0" but one would need to check that the zeroness
properly propagated out of the inline.

> #endif
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 3aa5142..b9ac534 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -433,6 +433,7 @@ struct address_space {
> /* Protected by tree_lock together with the radix tree */
> unsigned long nrpages; /* number of total pages */
> unsigned long nrshadows; /* number of shadow entries */
> + unsigned long nrdax; /* number of DAX entries */

hm, that's unfortunate - machines commonly carry tremendous numbers of
address_spaces in memory and adding pork to them is rather a big deal.
We can't avoid this somehow? Maybe share the space with nrshadows by
some means? Find some other field which is unused for dax files?

> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -11,6 +11,7 @@
> */
> #include <linux/export.h>
> #include <linux/compiler.h>
> +#include <linux/dax.h>
> #include <linux/fs.h>
> #include <linux/uaccess.h>
> #include <linux/capability.h>
> @@ -579,6 +580,12 @@ static int page_cache_tree_insert(struct address_space *mapping,
> p = radix_tree_deref_slot_protected(slot, &mapping->tree_lock);
> if (!radix_tree_exceptional_entry(p))
> return -EEXIST;
> +
> + if (dax_mapping(mapping)) {
> + WARN_ON(1);
> + return -EINVAL;
> + }

this:

--- a/mm/filemap.c~dax-support-dirty-dax-entries-in-radix-tree-fix
+++ a/mm/filemap.c
@@ -581,10 +581,8 @@ static int page_cache_tree_insert(struct
if (!radix_tree_exceptional_entry(p))
return -EEXIST;

- if (dax_mapping(mapping)) {
- WARN_ON(1);
+ if (WARN_ON(dax_mapping(mapping)))
return -EINVAL;
- }

if (shadowp)
*shadowp = p;