[PATCH v2 0/4] dax: require 'struct page' and other fixups

newer

older

From Hakim AL Mady

[ndctl PATCH] ndctl: fix static...

Dan Williams

Friday, 29 September 2017 Fri, 29 Sep '17

1:21 a.m.

Changes since v1 [1]: * quiet bdev_dax_supported() in favor of error messages emitted by the caller (Jeff) * fix leftover parentheses in vma_merge (Jeff) * improve the changelog for "dax: stop using VM_MIXEDMAP for dax" [1]: https://lists.01.org/pipermail/linux-nvdimm/2017-September/012645.html --- Prompted by a recent change to add more protection around setting up 'vm_flags' for a dax vma [1], rework the implementation to remove the requirement to set VM_MIXEDMAP and VM_HUGEPAGE. VM_MIXEDMAP is used by dax to direct mm paths like vm_normal_page() that the memory page it is dealing with is not typical memory from the linear map. The get_user_pages_fast() path, since it does not resolve the vma, is already using {pte,pmd}_devmap() as a stand-in for VM_MIXEDMAP, so we use that as a VM_MIXEDMAP replacement in some locations. In the cases where there is no pte to consult we fallback to using vma_is_dax() to detect the VM_MIXEDMAP special case. This patch series passes a run of the ndctl unit test suite and the 'mmap.sh' [2] test in particular. 'mmap.sh' tries to catch dependencies on VM_MIXEDMAP and {pte,pmd}_devmap(). [1]: https://lkml.org/lkml/2017/9/25/638 [2]: https://github.com/pmem/ndctl/blob/master/test/mmap.sh --- Dan Williams (4): dax: quiet bdev_dax_supported() dax: disable filesystem dax on devices that do not map pages dax: stop using VM_MIXEDMAP for dax dax: stop using VM_HUGEPAGE for dax drivers/dax/device.c | 1 - drivers/dax/super.c | 15 +++++++++++---- fs/ext2/file.c | 1 - fs/ext4/file.c | 1 - fs/xfs/xfs_file.c | 2 -- mm/huge_memory.c | 8 ++++---- mm/ksm.c | 3 +++ mm/madvise.c | 2 +- mm/memory.c | 20 ++++++++++++++++++-- mm/migrate.c | 3 ++- mm/mlock.c | 3 ++- mm/mmap.c | 3 ++- 12 files changed, 43 insertions(+), 19 deletions(-)

Show replies by date

Dan Williams

Friday, 29 September Fri, 29 Sep

1:21 a.m.

New subject: [PATCH v2 1/4] dax: quiet bdev_dax_supported()

Leave it to the caller to decide if bdev_dax_supported() failures are errors worth emitting to the log. Reported-by: Jeff Moyer <jmoyer(a)redhat.com> Signed-off-by: Dan Williams <dan.j.williams(a)intel.com> --- drivers/dax/super.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/dax/super.c b/drivers/dax/super.c index 30d8a5aedd23..b0cc8117eebe 100644 --- a/drivers/dax/super.c +++ b/drivers/dax/super.c @@ -92,21 +92,21 @@ int __bdev_dax_supported(struct super_block *sb, int blocksize) long len; if (blocksize != PAGE_SIZE) { - pr_err("VFS (%s): error: unsupported blocksize for dax\n", + pr_debug("VFS (%s): error: unsupported blocksize for dax\n", sb->s_id); return -EINVAL; } err = bdev_dax_pgoff(bdev, 0, PAGE_SIZE, &pgoff); if (err) { - pr_err("VFS (%s): error: unaligned partition for dax\n", + pr_debug("VFS (%s): error: unaligned partition for dax\n", sb->s_id); return err; } dax_dev = dax_get_by_host(bdev->bd_disk->disk_name); if (!dax_dev) { - pr_err("VFS (%s): error: device does not support dax\n", + pr_debug("VFS (%s): error: device does not support dax\n", sb->s_id); return -EOPNOTSUPP; } @@ -118,7 +118,7 @@ int __bdev_dax_supported(struct super_block *sb, int blocksize) put_dax(dax_dev); if (len < 1) { - pr_err("VFS (%s): error: dax access failed (%ld)\n", + pr_debug("VFS (%s): error: dax access failed (%ld)\n", sb->s_id, len); return len < 0 ? len : -EIO; }

Dan Williams

1:21 a.m.

New subject: [PATCH v2 2/4] dax: disable filesystem dax on devices that do not map pages

If a dax buffer from a device that does not map pages is passed to read(2) or write(2) as a target for direct-I/O it triggers SIGBUS. If gdb attempts to examine the contents of a dax buffer from a device that does not map pages it triggers SIGBUS. If fork(2) is called on a process with a dax mapping from a device that does not map pages it triggers SIGBUS. 'struct page' is required otherwise several kernel code paths break in surprising ways. Disable filesystem-dax on devices that do not map pages. Cc: Jan Kara <jack(a)suse.cz> Cc: Jeff Moyer <jmoyer(a)redhat.com> Cc: Christoph Hellwig <hch(a)lst.de> Cc: Ross Zwisler <ross.zwisler(a)linux.intel.com> Signed-off-by: Dan Williams <dan.j.williams(a)intel.com> --- drivers/dax/super.c | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/drivers/dax/super.c b/drivers/dax/super.c index b0cc8117eebe..491d4859c644 100644 --- a/drivers/dax/super.c +++ b/drivers/dax/super.c @@ -15,6 +15,7 @@ #include <linux/mount.h> #include <linux/magic.h> #include <linux/genhd.h> +#include <linux/pfn_t.h> #include <linux/cdev.h> #include <linux/hash.h> #include <linux/slab.h> @@ -123,6 +124,12 @@ int __bdev_dax_supported(struct super_block *sb, int blocksize) return len < 0 ? len : -EIO; } + if (!pfn_t_has_page(pfn)) { + pr_debug("VFS (%s): error: dax support not enabled\n", + sb->s_id); + return -EOPNOTSUPP; + } + return 0; } EXPORT_SYMBOL_GPL(__bdev_dax_supported);

Dan Williams

1:21 a.m.

New subject: [PATCH v2 3/4] dax: stop using VM_MIXEDMAP for dax

VM_MIXEDMAP is used by dax to direct mm paths like vm_normal_page() that the memory page it is dealing with is not typical memory from the linear map. The get_user_pages_fast() path, since it does not resolve the vma, is already using {pte,pmd}_devmap() as a stand-in for VM_MIXEDMAP, so we use that as a VM_MIXEDMAP replacement in some locations. In the cases where there is no pte to consult we fallback to using vma_is_dax() to detect the VM_MIXEDMAP special case. Now that we always have pages for DAX we can stop setting VM_MIXEDMAP. This also means we no longer need to worry about safely manipulating vm_flags in a future where we support dynamically changing the dax mode of a file. Cc: Jan Kara <jack(a)suse.cz> Cc: Michal Hocko <mhocko(a)suse.com> Cc: Jeff Moyer <jmoyer(a)redhat.com> Cc: Christoph Hellwig <hch(a)lst.de> Cc: Andrew Morton <akpm(a)linux-foundation.org> Cc: Ross Zwisler <ross.zwisler(a)linux.intel.com> Cc: "Kirill A. Shutemov" <kirill.shutemov(a)linux.intel.com> Signed-off-by: Dan Williams <dan.j.williams(a)intel.com> --- drivers/dax/device.c | 2 +- fs/ext2/file.c | 1 - fs/ext4/file.c | 2 +- fs/xfs/xfs_file.c | 2 +- mm/huge_memory.c | 8 ++++---- mm/ksm.c | 3 +++ mm/madvise.c | 2 +- mm/memory.c | 20 ++++++++++++++++++-- mm/migrate.c | 3 ++- mm/mlock.c | 3 ++- mm/mmap.c | 3 ++- 11 files changed, 35 insertions(+), 14 deletions(-) diff --git a/drivers/dax/device.c b/drivers/dax/device.c index e9f3b3e4bbf4..ed79d006026e 100644 --- a/drivers/dax/device.c +++ b/drivers/dax/device.c @@ -450,7 +450,7 @@ static int dax_mmap(struct file *filp, struct vm_area_struct *vma) return rc; vma->vm_ops = &dax_vm_ops; - vma->vm_flags |= VM_MIXEDMAP | VM_HUGEPAGE; + vma->vm_flags |= VM_HUGEPAGE; return 0; } diff --git a/fs/ext2/file.c b/fs/ext2/file.c index ff3a3636a5ca..70657e8550ed 100644 --- a/fs/ext2/file.c +++ b/fs/ext2/file.c @@ -125,7 +125,6 @@ static int ext2_file_mmap(struct file *file, struct vm_area_struct *vma) file_accessed(file); vma->vm_ops = &ext2_dax_vm_ops; - vma->vm_flags |= VM_MIXEDMAP; return 0; } #else diff --git a/fs/ext4/file.c b/fs/ext4/file.c index b1da660ac3bc..0cc9d205bd96 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -352,7 +352,7 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma) file_accessed(file); if (IS_DAX(file_inode(file))) { vma->vm_ops = &ext4_dax_vm_ops; - vma->vm_flags |= VM_MIXEDMAP | VM_HUGEPAGE; + vma->vm_flags |= VM_HUGEPAGE; } else { vma->vm_ops = &ext4_file_vm_ops; } diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index ebdd0bd2b261..dece8fe937f5 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -1131,7 +1131,7 @@ xfs_file_mmap( file_accessed(filp); vma->vm_ops = &xfs_file_vm_ops; if (IS_DAX(file_inode(filp))) - vma->vm_flags |= VM_MIXEDMAP | VM_HUGEPAGE; + vma->vm_flags |= VM_HUGEPAGE; return 0; } diff --git a/mm/huge_memory.c b/mm/huge_memory.c index 269b5df58543..c69d30e27fd9 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -765,11 +765,11 @@ int vmf_insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr, * but we need to be consistent with PTEs and architectures that * can't support a 'special' bit. */ - BUG_ON(!(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP))); + BUG_ON(!((vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)) + || pfn_t_devmap(pfn))); BUG_ON((vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)) == (VM_PFNMAP|VM_MIXEDMAP)); BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags)); - BUG_ON(!pfn_t_devmap(pfn)); if (addr < vma->vm_start || addr >= vma->vm_end) return VM_FAULT_SIGBUS; @@ -824,11 +824,11 @@ int vmf_insert_pfn_pud(struct vm_area_struct *vma, unsigned long addr, * but we need to be consistent with PTEs and architectures that * can't support a 'special' bit. */ - BUG_ON(!(vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP))); + BUG_ON(!((vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)) + || pfn_t_devmap(pfn))); BUG_ON((vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)) == (VM_PFNMAP|VM_MIXEDMAP)); BUG_ON((vma->vm_flags & VM_PFNMAP) && is_cow_mapping(vma->vm_flags)); - BUG_ON(!pfn_t_devmap(pfn)); if (addr < vma->vm_start || addr >= vma->vm_end) return VM_FAULT_SIGBUS; diff --git a/mm/ksm.c b/mm/ksm.c index 15dd7415f7b3..787dfe4f3d44 100644 --- a/mm/ksm.c +++ b/mm/ksm.c @@ -2358,6 +2358,9 @@ int ksm_madvise(struct vm_area_struct *vma, unsigned long start, VM_HUGETLB | VM_MIXEDMAP)) return 0; /* just ignore the advice */ + if (vma_is_dax(vma)) + return 0; + #ifdef VM_SAO if (*vm_flags & VM_SAO) return 0; diff --git a/mm/madvise.c b/mm/madvise.c index 21261ff0466f..40344d43e565 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -95,7 +95,7 @@ static long madvise_behavior(struct vm_area_struct *vma, new_flags |= VM_DONTDUMP; break; case MADV_DODUMP: - if (new_flags & VM_SPECIAL) { + if (vma_is_dax(vma) || (new_flags & VM_SPECIAL)) { error = -EINVAL; goto out; } diff --git a/mm/memory.c b/mm/memory.c index ec4e15494901..771acaf54fe6 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -830,6 +830,8 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, return vma->vm_ops->find_special_page(vma, addr); if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) return NULL; + if (pte_devmap(pte)) + return NULL; if (is_zero_pfn(pfn)) return NULL; @@ -917,6 +919,8 @@ struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr, } } + if (pmd_devmap(pmd)) + return NULL; if (is_zero_pfn(pfn)) return NULL; if (unlikely(pfn > highest_memmap_pfn)) @@ -1227,7 +1231,7 @@ int copy_page_range(struct mm_struct *dst_mm, struct mm_struct *src_mm, * efficient than faulting. */ if (!(vma->vm_flags & (VM_HUGETLB | VM_PFNMAP | VM_MIXEDMAP)) && - !vma->anon_vma) + !vma->anon_vma && !vma_is_dax(vma)) return 0; if (is_vm_hugetlb_page(vma)) @@ -1896,12 +1900,24 @@ int vm_insert_pfn_prot(struct vm_area_struct *vma, unsigned long addr, } EXPORT_SYMBOL(vm_insert_pfn_prot); +static bool vm_mixed_ok(struct vm_area_struct *vma, pfn_t pfn) +{ + /* these checks mirror the abort conditions in vm_normal_page */ + if (vma->vm_flags & VM_MIXEDMAP) + return true; + if (pfn_t_devmap(pfn)) + return true; + if (is_zero_pfn(pfn_t_to_pfn(pfn))) + return true; + return false; +} + static int __vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr, pfn_t pfn, bool mkwrite) { pgprot_t pgprot = vma->vm_page_prot; - BUG_ON(!(vma->vm_flags & VM_MIXEDMAP)); + BUG_ON(!vm_mixed_ok(vma, pfn)); if (addr < vma->vm_start || addr >= vma->vm_end) return -EFAULT; diff --git a/mm/migrate.c b/mm/migrate.c index 6954c1435833..179a84a311f6 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -2927,7 +2927,8 @@ int migrate_vma(const struct migrate_vma_ops *ops, /* Sanity check the arguments */ start &= PAGE_MASK; end &= PAGE_MASK; - if (!vma || is_vm_hugetlb_page(vma) || (vma->vm_flags & VM_SPECIAL)) + if (!vma || is_vm_hugetlb_page(vma) || (vma->vm_flags & VM_SPECIAL) + || vma_is_dax(dma)) return -EINVAL; if (start < vma->vm_start || start >= vma->vm_end) return -EINVAL; diff --git a/mm/mlock.c b/mm/mlock.c index dfc6f1912176..4d009350893f 100644 --- a/mm/mlock.c +++ b/mm/mlock.c @@ -520,7 +520,8 @@ static int mlock_fixup(struct vm_area_struct *vma, struct vm_area_struct **prev, vm_flags_t old_flags = vma->vm_flags; if (newflags == vma->vm_flags || (vma->vm_flags & VM_SPECIAL) || - is_vm_hugetlb_page(vma) || vma == get_gate_vma(current->mm)) + is_vm_hugetlb_page(vma) || vma == get_gate_vma(current->mm) || + vma_is_dax(vma)) /* don't set VM_LOCKED or VM_LOCKONFAULT and don't count */ goto out; diff --git a/mm/mmap.c b/mm/mmap.c index 680506faceae..2f3971a051c6 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1723,7 +1723,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr, vm_stat_account(mm, vm_flags, len >> PAGE_SHIFT); if (vm_flags & VM_LOCKED) { if (!((vm_flags & VM_SPECIAL) || is_vm_hugetlb_page(vma) || - vma == get_gate_vma(current->mm))) + vma == get_gate_vma(current->mm) || + vma_is_dax(vma))) mm->locked_vm += (len >> PAGE_SHIFT); else vma->vm_flags &= VM_LOCKED_CLEAR_MASK;

Jan Kara

Tuesday, 3 October Tue, 3 Oct

8:09 a.m.

New subject: [PATCH v2 3/4] dax: stop using VM_MIXEDMAP for dax

On Thu 28-09-17 18:21:18, Dan Williams wrote:

...

Well, I somewhat dislike the vma_is_dax() checks sprinkled around. That seems rather errorprone (easy to forget about it when adding new check somewhere). Can we possibly also create a helper vma_is_special() (or some other name) which would do ((vma->vm_flags & VM_SPECIAL) || vma_is_dax(vma) || is_vm_hugetlb_page(vma)) and then use it in all those places? Honza

...

diff --git a/mm/ksm.c b/mm/ksm.c index 15dd7415f7b3..787dfe4f3d44 100644 --- a/mm/ksm.c +++ b/mm/ksm.c @@ -2358,6 +2358,9 @@ int ksm_madvise(struct vm_area_struct *vma, unsigned long start, VM_HUGETLB | VM_MIXEDMAP)) return 0; /* just ignore the advice */ + if (vma_is_dax(vma)) + return 0; + #ifdef VM_SAO if (*vm_flags & VM_SAO) return 0; diff --git a/mm/madvise.c b/mm/madvise.c index 21261ff0466f..40344d43e565 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -95,7 +95,7 @@ static long madvise_behavior(struct vm_area_struct *vma, new_flags |= VM_DONTDUMP; break; case MADV_DODUMP: - if (new_flags & VM_SPECIAL) { + if (vma_is_dax(vma) || (new_flags & VM_SPECIAL)) { error = -EINVAL; goto out; } diff --git a/mm/memory.c b/mm/memory.c index ec4e15494901..771acaf54fe6 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -830,6 +830,8 @@ struct page *_vm_normal_page(struct vm_area_struct *vma, unsigned long addr, return vma->vm_ops->find_special_page(vma, addr); if (vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP)) return NULL; + if (pte_devmap(pte)) + return NULL; if (is_zero_pfn(pfn)) return NULL; @@ -917,6 +919,8 @@ struct page *vm_normal_page_pmd(struct vm_area_struct *vma, unsigned long addr, } } + if (pmd_devmap(pmd)) + return NULL; if (is_zero_pfn(pfn)) return NULL; if (unlikely(pfn > highest_memmap_pfn)) @@ -1227,7 +1231,7 @@ int copy_page_range(struct mm_struct *dst_mm, struct mm_struct *src_mm, * efficient than faulting. */ if (!(vma->vm_flags & (VM_HUGETLB | VM_PFNMAP | VM_MIXEDMAP)) && - !vma->anon_vma) + !vma->anon_vma && !vma_is_dax(vma)) return 0; if (is_vm_hugetlb_page(vma)) @@ -1896,12 +1900,24 @@ int vm_insert_pfn_prot(struct vm_area_struct *vma, unsigned long addr, } EXPORT_SYMBOL(vm_insert_pfn_prot); +static bool vm_mixed_ok(struct vm_area_struct *vma, pfn_t pfn) +{ + /* these checks mirror the abort conditions in vm_normal_page */ + if (vma->vm_flags & VM_MIXEDMAP) + return true; + if (pfn_t_devmap(pfn)) + return true; + if (is_zero_pfn(pfn_t_to_pfn(pfn))) + return true; + return false; +} + static int __vm_insert_mixed(struct vm_area_struct *vma, unsigned long addr, pfn_t pfn, bool mkwrite) { pgprot_t pgprot = vma->vm_page_prot; - BUG_ON(!(vma->vm_flags & VM_MIXEDMAP)); + BUG_ON(!vm_mixed_ok(vma, pfn)); if (addr < vma->vm_start || addr >= vma->vm_end) return -EFAULT; diff --git a/mm/migrate.c b/mm/migrate.c index 6954c1435833..179a84a311f6 100644 --- a/mm/migrate.c +++ b/mm/migrate.c @@ -2927,7 +2927,8 @@ int migrate_vma(const struct migrate_vma_ops *ops, /* Sanity check the arguments */ start &= PAGE_MASK; end &= PAGE_MASK; - if (!vma || is_vm_hugetlb_page(vma) || (vma->vm_flags & VM_SPECIAL)) + if (!vma || is_vm_hugetlb_page(vma) || (vma->vm_flags & VM_SPECIAL) + || vma_is_dax(dma)) return -EINVAL; if (start < vma->vm_start || start >= vma->vm_end) return -EINVAL; diff --git a/mm/mlock.c b/mm/mlock.c index dfc6f1912176..4d009350893f 100644 --- a/mm/mlock.c +++ b/mm/mlock.c @@ -520,7 +520,8 @@ static int mlock_fixup(struct vm_area_struct *vma, struct vm_area_struct **prev, vm_flags_t old_flags = vma->vm_flags; if (newflags == vma->vm_flags || (vma->vm_flags & VM_SPECIAL) || - is_vm_hugetlb_page(vma) || vma == get_gate_vma(current->mm)) + is_vm_hugetlb_page(vma) || vma == get_gate_vma(current->mm) || + vma_is_dax(vma)) /* don't set VM_LOCKED or VM_LOCKONFAULT and don't count */ goto out; diff --git a/mm/mmap.c b/mm/mmap.c index 680506faceae..2f3971a051c6 100644 --- a/mm/mmap.c +++ b/mm/mmap.c @@ -1723,7 +1723,8 @@ unsigned long mmap_region(struct file *file, unsigned long addr, vm_stat_account(mm, vm_flags, len >> PAGE_SHIFT); if (vm_flags & VM_LOCKED) { if (!((vm_flags & VM_SPECIAL) || is_vm_hugetlb_page(vma) || - vma == get_gate_vma(current->mm))) + vma == get_gate_vma(current->mm) || + vma_is_dax(vma))) mm->locked_vm += (len >> PAGE_SHIFT); else vma->vm_flags &= VM_LOCKED_CLEAR_MASK;

-- Jan Kara <jack(a)suse.com> SUSE Labs, CR

Dan Williams

5:29 p.m.

New subject: [PATCH v2 3/4] dax: stop using VM_MIXEDMAP for dax

On Tue, Oct 3, 2017 at 1:09 AM, Jan Kara <jack(a)suse.cz> wrote:

...

On Thu 28-09-17 18:21:18, Dan Williams wrote: > VM_MIXEDMAP is used by dax to direct mm paths like vm_normal_page() that > the memory page it is dealing with is not typical memory from the linear > map. The get_user_pages_fast() path, since it does not resolve the vma, > is already using {pte,pmd}_devmap() as a stand-in for VM_MIXEDMAP, so we > use that as a VM_MIXEDMAP replacement in some locations. In the cases > where there is no pte to consult we fallback to using vma_is_dax() to > detect the VM_MIXEDMAP special case. Well, I somewhat dislike the vma_is_dax() checks sprinkled around. That seems rather errorprone (easy to forget about it when adding new check somewhere). Can we possibly also create a helper vma_is_special() (or some other name) which would do ((vma->vm_flags & VM_SPECIAL) || vma_is_dax(vma) || is_vm_hugetlb_page(vma)) and then use it in all those places?

Yes, I can take a look at that... I shied away from it initially since it does not appear that "vma_is_special()" paths are symmetric in terms of all the conditions they check, but perhaps I can start small with the ones that are common. I'll break that conversion out into a lead-in cleanup patch.

Dan Williams

Friday, 29 September Fri, 29 Sep

1:21 a.m.

New subject: [PATCH v2 4/4] dax: stop using VM_HUGEPAGE for dax

This flag is deprecated in favor of the vma_is_dax() check in transparent_hugepage_enabled() added in commit baabda261424 "mm: always enable thp for dax mappings" Cc: Jan Kara <jack(a)suse.cz> Cc: Jeff Moyer <jmoyer(a)redhat.com> Cc: Christoph Hellwig <hch(a)lst.de> Cc: Ross Zwisler <ross.zwisler(a)linux.intel.com> Signed-off-by: Dan Williams <dan.j.williams(a)intel.com> --- drivers/dax/device.c | 1 - fs/ext4/file.c | 1 - fs/xfs/xfs_file.c | 2 -- 3 files changed, 4 deletions(-) diff --git a/drivers/dax/device.c b/drivers/dax/device.c index ed79d006026e..74a35eb5e6d3 100644 --- a/drivers/dax/device.c +++ b/drivers/dax/device.c @@ -450,7 +450,6 @@ static int dax_mmap(struct file *filp, struct vm_area_struct *vma) return rc; vma->vm_ops = &dax_vm_ops; - vma->vm_flags |= VM_HUGEPAGE; return 0; } diff --git a/fs/ext4/file.c b/fs/ext4/file.c index 0cc9d205bd96..a54e1b4c49f9 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -352,7 +352,6 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma) file_accessed(file); if (IS_DAX(file_inode(file))) { vma->vm_ops = &ext4_dax_vm_ops; - vma->vm_flags |= VM_HUGEPAGE; } else { vma->vm_ops = &ext4_file_vm_ops; } diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index dece8fe937f5..c0e0fcbe1bd3 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -1130,8 +1130,6 @@ xfs_file_mmap( { file_accessed(filp); vma->vm_ops = &xfs_file_vm_ops; - if (IS_DAX(file_inode(filp))) - vma->vm_flags |= VM_HUGEPAGE; return 0; }

Jan Kara

Tuesday, 3 October Tue, 3 Oct

8:12 a.m.

New subject: [PATCH v2 4/4] dax: stop using VM_HUGEPAGE for dax

On Thu 28-09-17 18:21:23, Dan Williams wrote:

...

I like this! You can add: Reviewed-by: Jan Kara <jack(a)suse.cz> Honza

...

--- drivers/dax/device.c | 1 - fs/ext4/file.c | 1 - fs/xfs/xfs_file.c | 2 -- 3 files changed, 4 deletions(-) diff --git a/drivers/dax/device.c b/drivers/dax/device.c index ed79d006026e..74a35eb5e6d3 100644 --- a/drivers/dax/device.c +++ b/drivers/dax/device.c @@ -450,7 +450,6 @@ static int dax_mmap(struct file *filp, struct vm_area_struct *vma) return rc; vma->vm_ops = &dax_vm_ops; - vma->vm_flags |= VM_HUGEPAGE; return 0; } diff --git a/fs/ext4/file.c b/fs/ext4/file.c index 0cc9d205bd96..a54e1b4c49f9 100644 --- a/fs/ext4/file.c +++ b/fs/ext4/file.c @@ -352,7 +352,6 @@ static int ext4_file_mmap(struct file *file, struct vm_area_struct *vma) file_accessed(file); if (IS_DAX(file_inode(file))) { vma->vm_ops = &ext4_dax_vm_ops; - vma->vm_flags |= VM_HUGEPAGE; } else { vma->vm_ops = &ext4_file_vm_ops; } diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c index dece8fe937f5..c0e0fcbe1bd3 100644 --- a/fs/xfs/xfs_file.c +++ b/fs/xfs/xfs_file.c @@ -1130,8 +1130,6 @@ xfs_file_mmap( { file_accessed(filp); vma->vm_ops = &xfs_file_vm_ops; - if (IS_DAX(file_inode(filp))) - vma->vm_flags |= VM_HUGEPAGE; return 0; }

-- Jan Kara <jack(a)suse.com> SUSE Labs, CR

Christoph Hellwig

Sunday, 1 October Sun, 1 Oct

7:57 a.m.

While this looks like a really nice cleanup of the code and removes nasty race conditions I'd like to understand the tradeoffs. This now requires every dax device that is used with a file system to have a struct page backing, which means not only means we'd break existing setups, but also a sharp turn from previous policy. Unless I misremember it was you Intel guys that heavily pushed for the page-less version, so I'd like to understand why you've changed your mind.

Dan Williams

5:58 p.m.

On Sun, Oct 1, 2017 at 12:57 AM, Christoph Hellwig <hch(a)lst.de> wrote:

...

Sure, here's a quick recap of the story so far of how we got here: * In support of page-less I/O operations envisioned by Matthew I introduced pfn_t as a proposal for converting the block layer and other sub-systems to use pfns instead of pages [1]. You helped out on that patch set with some work on the DMA api. [2] * The DMA api conversion effort came to a halt when it came time to touch sparc paths and DaveM said [3]: "Generally speaking, I think that all actual physical memory the kernel operates on should have a struct page backing it." * ZONE_DEVICE was created to solve the DMA problem and in developing / testing that discovered plenty of proof for Dave's assertion (no fork, no ptrace, etc). We should have made the switch to require struct page at that point, but I was persuaded by the argument that changing the dax policy may break existing assumptions, and that there were larger issues to go solve at the time. What changed recently was the discussions around what the dax mount option means and the assertion that we can, in general, make some policy changes on our way to removing the "experimental" designation from filesystem-dax. It is clear that the page-less dax path remains experimental with all the way it fails in several kernel paths, and there has been no patches for several months to revive the effort. Meanwhile the page-less path continues to generate maintenance overhead. The recent gymnastics (new ->post_mmap file_operation) to make sure ->vm_flags are safely manipulated when dynamically changing the dax mode of a file was the final straw for me to pull the trigger on this series. In terms of what breaks by changing this policy it should be noted that we automatically create pages for "legacy" pmem devices, and the default for "ndctl create-namespace" is to allocate pages. I have yet to see a bug report where someone was surprised by fork failing or direct-I/O causing a SIGBUS. So, I think the defaults are working, it is unlikely that there are environments dependent on page-less behavior. That said, I now recall that dax also replaced xip for some setups. I think we have a couple options here: let embedded configurations override the page requirement since they can reasonably assert to not care about the several broken general purpose paths that need pages, or perhaps follow in the footsteps of what Nicolas is doing for cramfs where he calls dax "overkill" [4] for his use case. [1]: https://lwn.net/Articles/643998/ [2]: https://lkml.org/lkml/2015/8/12/86 [3]: https://lkml.org/lkml/2015/8/14/3 [4]: https://lwn.net/Articles/734995/

Dave Chinner

9:11 p.m.

On Sun, Oct 01, 2017 at 10:58:06AM -0700, Dan Williams wrote:

...

On Sun, Oct 1, 2017 at 12:57 AM, Christoph Hellwig <hch(a)lst.de> wrote: > While this looks like a really nice cleanup of the code and removes > nasty race conditions I'd like to understand the tradeoffs. > > This now requires every dax device that is used with a file system > to have a struct page backing, which means not only means we'd > break existing setups, but also a sharp turn from previous policy. > > Unless I misremember it was you Intel guys that heavily pushed for > the page-less version, so I'd like to understand why you've changed > your mind. Sure, here's a quick recap of the story so far of how we got here: * In support of page-less I/O operations envisioned by Matthew I introduced pfn_t as a proposal for converting the block layer and other sub-systems to use pfns instead of pages [1]. You helped out on that patch set with some work on the DMA api. [2] * The DMA api conversion effort came to a halt when it came time to touch sparc paths and DaveM said [3]: "Generally speaking, I think that all actual physical memory the kernel operates on should have a struct page backing it." * ZONE_DEVICE was created to solve the DMA problem and in developing / testing that discovered plenty of proof for Dave's assertion (no fork, no ptrace, etc). We should have made the switch to require struct page at that point, but I was persuaded by the argument that changing the dax policy may break existing assumptions, and that there were larger issues to go solve at the time. What changed recently was the discussions around what the dax mount option means and the assertion that we can, in general, make some policy changes on our way to removing the "experimental" designation from filesystem-dax. It is clear that the page-less dax path remains experimental with all the way it fails in several kernel paths, and there has been no patches for several months to revive the effort. Meanwhile the page-less path continues to generate maintenance overhead. The recent gymnastics (new ->post_mmap file_operation) to make sure ->vm_flags are safely manipulated when dynamically changing the dax mode of a file was the final straw for me to pull the trigger on this series. In terms of what breaks by changing this policy it should be noted that we automatically create pages for "legacy" pmem devices, and the default for "ndctl create-namespace" is to allocate pages. I have yet to see a bug report where someone was surprised by fork failing or direct-I/O causing a SIGBUS. So, I think the defaults are working, it is unlikely that there are environments dependent on page-less behavior.

Does this imply that the hardware vendors won't have tens of terabytes of pmem in systems in the near to medium term? That's what we were originally told to expect by 2018-19 timeframe (i.e. 5 years in), and that's kinda what we've been working towards. Indeed, supporting systems with a couple of orders of magnitude more pmem than ram was the big driver for page-less DAX mappings in the first place. i.e. it was needed to avoid the static RAM overhead of all the static struct pages for such large amounts of physical memory. If we decide that we must have struct pages for pmem, then we're essentially throwing away the ability to support the very systems the hardware vendors were telling us we needed to design the pmem infrastructure for. If that reality has changed, then I'd suggest that we need to determine what the long term replacement for pageless IO on large pmem systems will be before we throw what we have away. Cheers, Dave. -- Dave Chinner david(a)fromorbit.com

Dan Williams

9:22 p.m.

On Sun, Oct 1, 2017 at 2:11 PM, Dave Chinner <david(a)fromorbit.com> wrote:

...

On Sun, Oct 01, 2017 at 10:58:06AM -0700, Dan Williams wrote: > On Sun, Oct 1, 2017 at 12:57 AM, Christoph Hellwig <hch(a)lst.de> wrote: > > While this looks like a really nice cleanup of the code and removes > > nasty race conditions I'd like to understand the tradeoffs. > > > > This now requires every dax device that is used with a file system > > to have a struct page backing, which means not only means we'd > > break existing setups, but also a sharp turn from previous policy. > > > > Unless I misremember it was you Intel guys that heavily pushed for > > the page-less version, so I'd like to understand why you've changed > > your mind. > > Sure, here's a quick recap of the story so far of how we got here: > > * In support of page-less I/O operations envisioned by Matthew I > introduced pfn_t as a proposal for converting the block layer and > other sub-systems to use pfns instead of pages [1]. You helped out on > that patch set with some work on the DMA api. [2] > > * The DMA api conversion effort came to a halt when it came time to > touch sparc paths and DaveM said [3]: "Generally speaking, I think > that all actual physical memory the kernel operates on should have a > struct page backing it." > > * ZONE_DEVICE was created to solve the DMA problem and in developing / > testing that discovered plenty of proof for Dave's assertion (no fork, > no ptrace, etc). We should have made the switch to require struct page > at that point, but I was persuaded by the argument that changing the > dax policy may break existing assumptions, and that there were larger > issues to go solve at the time. > > What changed recently was the discussions around what the dax mount > option means and the assertion that we can, in general, make some > policy changes on our way to removing the "experimental" designation > from filesystem-dax. It is clear that the page-less dax path remains > experimental with all the way it fails in several kernel paths, and > there has been no patches for several months to revive the effort. > Meanwhile the page-less path continues to generate maintenance > overhead. The recent gymnastics (new ->post_mmap file_operation) to > make sure ->vm_flags are safely manipulated when dynamically changing > the dax mode of a file was the final straw for me to pull the trigger > on this series. > > In terms of what breaks by changing this policy it should be noted > that we automatically create pages for "legacy" pmem devices, and the > default for "ndctl create-namespace" is to allocate pages. I have yet > to see a bug report where someone was surprised by fork failing or > direct-I/O causing a SIGBUS. So, I think the defaults are working, it > is unlikely that there are environments dependent on page-less > behavior. Does this imply that the hardware vendors won't have tens of terabytes of pmem in systems in the near to medium term? That's what we were originally told to expect by 2018-19 timeframe (i.e. 5 years in), and that's kinda what we've been working towards. Indeed, supporting systems with a couple of orders of magnitude more pmem than ram was the big driver for page-less DAX mappings in the first place. i.e. it was needed to avoid the static RAM overhead of all the static struct pages for such large amounts of physical memory. If we decide that we must have struct pages for pmem, then we're essentially throwing away the ability to support the very systems the hardware vendors were telling us we needed to design the pmem infrastructure for. If that reality has changed, then I'd suggest that we need to determine what the long term replacement for pageless IO on large pmem systems will be before we throw what we have away.

No, we can support large pmem with struct page capacity reserved from pmem itself rather than ram. A 1.5% capacity tax does not appear to be prohibitive.

Dan Williams

9:23 p.m.

On Sun, Oct 1, 2017 at 2:22 PM, Dan Williams <dan.j.williams(a)intel.com> wrote:

...

On Sun, Oct 1, 2017 at 2:11 PM, Dave Chinner <david(a)fromorbit.com> wrote: > On Sun, Oct 01, 2017 at 10:58:06AM -0700, Dan Williams wrote: >> On Sun, Oct 1, 2017 at 12:57 AM, Christoph Hellwig <hch(a)lst.de> wrote: >> > While this looks like a really nice cleanup of the code and removes >> > nasty race conditions I'd like to understand the tradeoffs. >> > >> > This now requires every dax device that is used with a file system >> > to have a struct page backing, which means not only means we'd >> > break existing setups, but also a sharp turn from previous policy. >> > >> > Unless I misremember it was you Intel guys that heavily pushed for >> > the page-less version, so I'd like to understand why you've changed >> > your mind. >> >> Sure, here's a quick recap of the story so far of how we got here: >> >> * In support of page-less I/O operations envisioned by Matthew I >> introduced pfn_t as a proposal for converting the block layer and >> other sub-systems to use pfns instead of pages [1]. You helped out on >> that patch set with some work on the DMA api. [2] >> >> * The DMA api conversion effort came to a halt when it came time to >> touch sparc paths and DaveM said [3]: "Generally speaking, I think >> that all actual physical memory the kernel operates on should have a >> struct page backing it." >> >> * ZONE_DEVICE was created to solve the DMA problem and in developing / >> testing that discovered plenty of proof for Dave's assertion (no fork, >> no ptrace, etc). We should have made the switch to require struct page >> at that point, but I was persuaded by the argument that changing the >> dax policy may break existing assumptions, and that there were larger >> issues to go solve at the time. >> >> What changed recently was the discussions around what the dax mount >> option means and the assertion that we can, in general, make some >> policy changes on our way to removing the "experimental" designation >> from filesystem-dax. It is clear that the page-less dax path remains >> experimental with all the way it fails in several kernel paths, and >> there has been no patches for several months to revive the effort. >> Meanwhile the page-less path continues to generate maintenance >> overhead. The recent gymnastics (new ->post_mmap file_operation) to >> make sure ->vm_flags are safely manipulated when dynamically changing >> the dax mode of a file was the final straw for me to pull the trigger >> on this series. >> >> In terms of what breaks by changing this policy it should be noted >> that we automatically create pages for "legacy" pmem devices, and the >> default for "ndctl create-namespace" is to allocate pages. I have yet >> to see a bug report where someone was surprised by fork failing or >> direct-I/O causing a SIGBUS. So, I think the defaults are working, it >> is unlikely that there are environments dependent on page-less >> behavior. > > Does this imply that the hardware vendors won't have > tens of terabytes of pmem in systems in the near to medium term? > That's what we were originally told to expect by 2018-19 timeframe > (i.e. 5 years in), and that's kinda what we've been working towards. > Indeed, supporting systems with a couple of orders of magnitude more > pmem than ram was the big driver for page-less DAX mappings in the > first place. i.e. it was needed to avoid the static RAM overhead of > all the static struct pages for such large amounts of physical > memory. > > If we decide that we must have struct pages for pmem, then we're > essentially throwing away the ability to support the very systems > the hardware vendors were telling us we needed to design the pmem > infrastructure for. If that reality has changed, then I'd suggest > that we need to determine what the long term replacement for > pageless IO on large pmem systems will be before we throw what we > have away. No, we can support large pmem with struct page capacity reserved from pmem itself rather than ram. A 1.5% capacity tax does not appear to be prohibitive.

Dynamic allocation of struct page is another step we can take before deciding we need page-less I/O.

Dave Chinner

9:59 p.m.

On Sun, Oct 01, 2017 at 02:22:08PM -0700, Dan Williams wrote:

...

The "capacity tax" had nothing to do with it - the major problem with self hosting struct pages was that page locks can be hot and contention on them will rapidly burn through write cycles on the pmem. That's still a problem, yes? I don't want to have to ask about all the issues one by one, so I'll ask you to explain in one go: what has changed (both hardware and software!) since we last discussed these problems with self hosting and make it a viable solution? Cheers, Dave. -- Dave Chinner david(a)fromorbit.com

Dan Williams

11:15 p.m.

On Sun, Oct 1, 2017 at 2:59 PM, Dave Chinner <david(a)fromorbit.com> wrote: [..]

...

It was an early concern we had, but it has not born out in practice, and stopped worrying about it 2 years ago when the ZONE_DEVICE infrastructure was merged.

Andrew Morton

Monday, 2 October Mon, 2 Oct

10:47 p.m.

On Sun, 1 Oct 2017 16:15:06 -0700 Dan Williams <dan.j.williams(a)intel.com> wrote:

...

On Sun, Oct 1, 2017 at 2:59 PM, Dave Chinner <david(a)fromorbit.com> wrote: [..] > The "capacity tax" had nothing to do with it - the major problem > with self hosting struct pages was that page locks can be hot and > contention on them will rapidly burn through write cycles on the > pmem. That's still a problem, yes? It was an early concern we had, but it has not born out in practice, and stopped worrying about it 2 years ago when the ZONE_DEVICE infrastructure was merged.

These are weighty matters and should have been covered in the changelogs, please. Could you send along a few paragraphs which summarise these thoughts and I'll add them in?

1118

days inactive

1122

days old

linux-nvdimm@lists.01.org

Manage subscription

15 comments

5 participants

tags (0)

participants (5)

Andrew Morton
Christoph Hellwig
Dan Williams
Dave Chinner
Jan Kara

2020

2019

2018

2017

2016

2015

2014

[PATCH v2 0/4] dax: require 'struct page' and other fixups