On Wed, Sep 27, 2017 at 8:07 AM, Jan Kara <jack@xxxxxxx> wrote: > On Wed 27-09-17 07:00:53, Dan Williams wrote: >> On Wed, Sep 27, 2017 at 4:35 AM, Jan Kara <jack@xxxxxxx> wrote: >> > On Tue 26-09-17 14:41:53, Dan Williams wrote: >> >> On Tue, Sep 26, 2017 at 2:06 PM, Ross Zwisler >> >> <ross.zwisler@xxxxxxxxxxxxxxx> wrote: >> >> > On Tue, Sep 26, 2017 at 12:19:21PM -0700, Dan Williams wrote: >> >> >> On Tue, Sep 26, 2017 at 11:57 AM, Ross Zwisler >> >> > <> >> >> >> > This decision can only be made (in this >> >> >> > proposed scheme) *after* the inode->i_mapping->i_mmap tree has been >> >> >> > populated, which means we need another call into the filesystem after this >> >> >> > insertion has happened. >> >> >> >> >> >> I get that, but it seems over-engineered and something that can also >> >> >> be safely cleaned up after the fact by the code path that is disabling >> >> >> DAX. >> >> > >> >> > I don't think you can safely clean it up after the fact because some thread >> >> > might have already called ->mmap() to set up the vma->vm_flags for their new >> >> > mapping, but they haven't added it to inode->i_mapping->i_mmap. >> >> >> >> If madvise(MADV_NOHUGEPAGE) can dynamically change vm_flags, then the >> >> DAX disable path can as well. VM_MIXEDMAP looks to be a nop for normal >> >> memory mappings. >> >> >> >> > The inode->i_mapping->i_mmap tree is the only way (that I know of at least) >> >> > that the filesystem has any idea about about the mapping. This is the method >> >> > by which we would try and clean up mapping flags, if we were to do so, and >> >> > it's the only way that the filesystem can know whether or not mappings exist. >> >> > >> >> > The only way that I could think of to make this safely work is to have the >> >> > insertion into the inode->i_mapping->i_mmap tree be our sync point. After >> >> > that the filesystem and the mapping code can communicate on the state of DAX, >> >> > but before that I think it's basically indeterminate. >> >> >> >> If we lose the race and leak VM_HUGEPAGE to a non-DAX mapping what >> >> breaks? I'd rather be in favor of not setting VM_HUGEPAGE at all in >> >> the ->mmap() handler and let the default THP policy take over. In >> >> fact, see transparent_hugepage_enabled() we already auto-enable huge >> >> page support for dax mappings regardless of VM_HUGEPAGE. >> > >> > Hum, this is an interesting option. So do you suggest that filesystems >> > supporting DAX would always setup mappings with VM_MIXEDMAP and without >> > VM_HUGEPAGE and thus we'd get rid of dependency on S_DAX flag in ->mmap? >> > That could actually work. The only possible issue I can see is that >> > VM_MIXEDMAP is still slightly different from normal page mappings and it >> > could have some performance implications - e.g. copy_page_range() does more >> > work on VM_MIXEDMAP mappings but not on normal page mappings. >> >> We can also get rid of VM_MIXEDMAP if we disable DAX in the >> !pfn_t_has_page() case. > > Yeah, although it would be a pity to require struct page just to avoid > having to set VM_MIXEDMAP flag... Yes, but the real motivation is fixing all the basic things that break without struct page, like ptrace and direct-i/o. The removal of needing to set VM_MIXEDMAP is just a nice side effect. I'll send a patch because DAX without pages has too many surprise failure cases. -- To unsubscribe from this list: send the line "unsubscribe linux-xfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
