dax: Fix mmap-write not updating c/mtime

From: Yigal Korman <[email protected]>

This will allow FS that uses VM_PFNMAP | VM_MIXEDMAP (no page structs) to get notified when access is a write to a read-only PFN.

This can happen if we mmap() a file then first mmap-read from it to page-in a read-only PFN, than we mmap-write to the same page.

We need this functionality to fix a DAX bug, where in the scenario above we fail to set ctime/mtime though we modified the file. An xfstest is attached to this patchset that shows the failure and the fix. (A DAX patch will follow)

This functionality is extra important for us, because upon dirtying of a pmem page we also want to RDMA the page to a remote cluster node.

We define a new pfn_mkwrite and do not reuse page_mkwrite because
1 - The name ;-)
2 - But mainly because it would take a very long and tedious
audit of all page_mkwrite functions of VM_MIXEDMAP/VM_PFNMAP users. To make sure they do not now CRASH. For example current DAX code (which this is for) would crash.
If we would want to reuse page_mkwrite, We will need to first patch all users, so to not-crash-on-no-page. Then enable this patch. But even if I did that I would not sleep so well at night. Adding a new vector is the safest thing to do, and is not that expensive. an extra pointer at a static function vector per driver. Also the new vector is better for performance, because else we Will call all current Kernel vectors, so to:
check-ha-no-page-do-nothing and return.

No need to call it from do_shared_fault because do_wp_page is called to change pte permissions anyway.

mm/memory.c +5 -0

@@ -1982,6 +1982,22 @@ static int do_page_mkwrite(struct vm_area_struct *vma, struct page *page,
 	return ret;
 }
 
+static int do_pfn_mkwrite(struct vm_area_struct *vma, unsigned long address)
+{
+	struct vm_fault vmf;
+
+	if (!vma->vm_ops || !vma->vm_ops->pfn_mkwrite)

mm/memory.c +2 -1

 /*
  * This routine handles present pages, when users try to write
  * to a shared page. It is done by copying the page to a new address

@@ -2025,8 +2041,17 @@ static int do_wp_page(struct mm_struct *mm, struct vm_area_struct *vma,
 		 * accounting on raw pfn maps.
 		 */
 		if ((vma->vm_flags & (VM_WRITE|VM_SHARED)) ==
-				     (VM_WRITE|VM_SHARED))
+				     (VM_WRITE|VM_SHARED)) {
+			pte_unmap_unlock(page_table, ptl);

From: Boaz Harrosh <[email protected]>

When freezing an FS, we must write protect all IS_DAX() inodes that have an mmap mapping on an inode. Otherwise application will be able to modify previously faulted-in file pages.

Quoted 6 lines · Boaz Harrosh

From: Boaz Harrosh <[email protected]>

When freezing an FS, we must write protect all IS_DAX() inodes that have an mmap mapping on an inode. Otherwise application will be able to modify previously faulted-in file pages.

All you need to do is lock out page faults once the page is clean; that's what the sb_start_pagefault() calls are for in the page fault path - they catch write faults and block them until the filesystem is unfrozen. Hence I'm not sure why this would be necessary if you are catching write faults in .pfn_mkwrite....

Cheers,
Dave.

Quoted 8 lines · Dave Chinner

Quoted 6 lines · Boaz Harrosh

From: Boaz Harrosh <[email protected]>

When freezing an FS, we must write protect all IS_DAX() inodes that have an mmap mapping on an inode. Otherwise application will be able to modify previously faulted-in file pages.

Quoted 6 lines

All you need to do is lock out page faults once the page is clean; that's what the sb_start_pagefault() calls are for in the page fault path - they catch write faults and block them until the filesystem is unfrozen. Hence I'm not sure why this would be necessary if you are catching write faults in .pfn_mkwrite....

Jan pointed it out and he was right I have a test for this. What happens is that since we had a mapping from before the freeze we will not have a page-fault. And the buffers will be modified.

As Jan explained in the cache path we do a writeback which turns all pages to read-only. But with dax we do not have writeback so the pages stay read-write mapped. Something needs to loop through the pages and write-protect them. I chose to unmap them because it is the much-much smaller code, and I do not care to optimize the freeze.

On Tue, Mar 24, 2015 at 08:14:59AM +0200, Boaz Harrosh wrote:

Then we have wider problem with DAX, then: sync doesn't work properly. i.e. if we still has write mapped pages, then we haven't flushed dirty cache lines on write-mapped files to the persistent domain by the time sync completes.

So, this shouldn't be some special case that only the freeze code takes into account - we need to make sure that sync (and therefore freeze) flushes all dirty cache lines and marks all mappings clean....

Cheers,
Dave.

Quoted 1 line · Dave Chinner

On Tue, Mar 24, 2015 at 08:14:59AM +0200, Boaz Harrosh wrote:

<>

Quoted 11 lines

Then we have wider problem with DAX, then: sync doesn't work properly. i.e. if we still has write mapped pages, then we haven't flushed dirty cache lines on write-mapped files to the persistent domain by the time sync completes.

So, this shouldn't be some special case that only the freeze code takes into account - we need to make sure that sync (and therefore freeze) flushes all dirty cache lines and marks all mappings clean....

This is not how I understood it and how I read the code.

The sync does happen, .fsync of the FS is called on each file just as if the user called it. If this is broken it just needs to be fixed there at the .fsync vector. POSIX mandate persistence at .fsync so at the vfs layer we rely on that.

So everything at this stage should be synced to real media.

What does not happen is writeback. since dax does not have any writeback.

And because of that nothing turned the
user mappings to read only. This is what I do here but instead of write-protecting I just unmap because it is easier for me to code it.

Quoted 3 lines · Boaz Harrosh

Quoted 1 line · Dave Chinner

On Tue, Mar 24, 2015 at 08:14:59AM +0200, Boaz Harrosh wrote:

<>

Quoted 4 lines

The sync does happen, .fsync of the FS is called on each file just as if the user called it. If this is broken it just needs to be fixed there at the .fsync vector. POSIX mandate persistence at .fsync so at the vfs layer we rely on that.

right now, the filesystems will see that there are no dirty pages on the inode, and then just sync the inode metadata. They will do nothing else as filesystems are not aware of CPU cachelines at all.

Quoted 1 line

So everything at this stage should be synced to real media.

Actually no. This is what intel are introducing new CPU instructions for - so fsync can flush the cpu caches and commit them to th persistence domain correctly.

Quoted 2 lines

What does not happen is writeback. since dax does not have any writeback.

Which is precisely the problem we need to address - we don't need writeback to a block device, but we do need the dirty CPU cachelines flushed and the mappings cleaned.

Quoted 4 lines

And because of that nothing turned the
user mappings to read only. This is what I do here but instead of write-protecting I just unmap because it is easier for me to code it.

That doesn't mean it is the correct solution.

Cheers,
Dave.

Quoted 4 lines · Dave Chinner

Quoted 3 lines · Boaz Harrosh

Quoted 1 line · Dave Chinner

On Tue, Mar 24, 2015 at 08:14:59AM +0200, Boaz Harrosh wrote:

<>

<>

Quoted 9 lines

Quoted 4 lines

The sync does happen, .fsync of the FS is called on each file just as if the user called it. If this is broken it just needs to be fixed there at the .fsync vector. POSIX mandate persistence at .fsync so at the vfs layer we rely on that.

right now, the filesystems will see that there are no dirty pages on the inode, and then just sync the inode metadata. They will do nothing else as filesystems are not aware of CPU cachelines at all.

Sigh yes. There is this bug. And I am sitting on a wide fix for this.

The strategy is. All Kernel writes are done with a new copy_user_nt. NT stands for none-temporal. This shows 20% improvements since cachelines need not be fetched when written too.

Quoted 5 lines

Quoted 4 lines

And because of that nothing turned the
user mappings to read only. This is what I do here but instead of write-protecting I just unmap because it is easier for me to code it.

Quoted 1 line

That doesn't mean it is the correct solution.

Please note that even if we properly .fsync cachlines the page-faults are orthogonal to this. There is no point in making mmapped dax pages read-only after every .fsync and pay a page-fault. We should leave them mapped has is. The only place that we need page protection is at freeze time.

Quoted 16 lines · Boaz Harrosh

Quoted 4 lines · Dave Chinner

Quoted 3 lines · Boaz Harrosh

Quoted 1 line · Dave Chinner

On Tue, Mar 24, 2015 at 08:14:59AM +0200, Boaz Harrosh wrote:

<>

<>

Quoted 9 lines

Quoted 4 lines

The sync does happen, .fsync of the FS is called on each file just as if the user called it. If this is broken it just needs to be fixed there at the .fsync vector. POSIX mandate persistence at .fsync so at the vfs layer we rely on that.

right now, the filesystems will see that there are no dirty pages on the inode, and then just sync the inode metadata. They will do nothing else as filesystems are not aware of CPU cachelines at all.

Quoted 5 lines

Sigh yes. There is this bug. And I am sitting on a wide fix for this.

The strategy is. All Kernel writes are done with a new copy_user_nt. NT stands for none-temporal. This shows 20% improvements since cachelines need not be fetched when written too.

That's unenforcable for mmap writes from userspace. And those are the writes that will trigger the dirty write mapping problem.

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

And because of that nothing turned the
user mappings to read only. This is what I do here but instead of write-protecting I just unmap because it is easier for me to code it.

Quoted 7 lines

Quoted 1 line

That doesn't mean it is the correct solution.

Please note that even if we properly .fsync cachlines the page-faults are orthogonal to this. There is no point in making mmapped dax pages read-only after every .fsync and pay a page-fault. We should leave them mapped has is. The only place that we need page protection is at freeze time.

Actually, current behaviour of filesystems is that fsync cleans all the pages in the range, and means all the mappings are marked read-only and so we get new calls into .page_mkwrite when write faults occur. We need that .page_mkwrite call to be able to a) update the mtime of the inode, and b) mark the inode "data dirty" so that fsync knows it needs to do something....

Hence I'd much prefer we start with identical behaviour to normal files, then we can optimise from a sane start point when write page faults show up as a performance problem. i.e. Correctness first, performance second.

Cheers,
Dave.

Quoted 23 lines · Dave Chinner

Quoted 21 lines · Boaz Harrosh

Quoted 4 lines · Dave Chinner

Quoted 3 lines · Boaz Harrosh

Quoted 1 line · Dave Chinner

On Tue, Mar 24, 2015 at 08:14:59AM +0200, Boaz Harrosh wrote:

<>

<>

Quoted 9 lines

Quoted 4 lines

The sync does happen, .fsync of the FS is called on each file just as if the user called it. If this is broken it just needs to be fixed there at the .fsync vector. POSIX mandate persistence at .fsync so at the vfs layer we rely on that.

right now, the filesystems will see that there are no dirty pages on the inode, and then just sync the inode metadata. They will do nothing else as filesystems are not aware of CPU cachelines at all.

Sigh yes. There is this bug. And I am sitting on a wide fix for this.

The strategy is. All Kernel writes are done with a new copy_user_nt. NT stands for none-temporal. This shows 20% improvements since cachelines need not be fetched when written too.

Quoted 3 lines

That's unenforcable for mmap writes from userspace. And those are the writes that will trigger the dirty write mapping problem.

So for them I was thinking of just doing the .fsync on every unmap (ie vm_operations_struct->close)

So now we know that only inodes that have an active vm mapping are in need of sync.

Quoted 13 lines

Quoted 5 lines

Please note that even if we properly .fsync cachlines the page-faults are orthogonal to this. There is no point in making mmapped dax pages read-only after every .fsync and pay a page-fault. We should leave them mapped has is. The only place that we need page protection is at freeze time.

Actually, current behaviour of filesystems is that fsync cleans all the pages in the range, and means all the mappings are marked read-only and so we get new calls into .page_mkwrite when write faults occur. We need that .page_mkwrite call to be able to a) update the mtime of the inode, and b) mark the inode "data dirty" so that fsync knows it needs to do something....

Quoted 4 lines

Hence I'd much prefer we start with identical behaviour to normal files, then we can optimise from a sane start point when write page faults show up as a performance problem. i.e. Correctness first, performance second.

OK, (you see when you speak slow I understand fast ;-)). I agree then I'll see if I can schedule some time for this. My boss will be very angry with me about this. But I will need help please, and some hands holding. Unless someone else volunteers to work on this ?

From: Boaz Harrosh <[email protected]>

When freezing an FS, we must write protect all IS_DAX() inodes that have an mmap mapping on an inode. Otherwise application will be able to modify previously faulted-in file pages.

I'm actually doing a full unmap_mapping_range because there is no readily available "mapping_write_protect" like functionality. I do not think it is worth it to define one just for here and just for some extra read-faults after an fs_freeze.

How hot-path is fs_freeze at all?

CC Jan Kara <[email protected]>

CC Matthew Wilcox <[email protected]>

CC Andrew Morton <[email protected]>

Signed-off-by Boaz Harrosh <[email protected]>

---
 fs/dax.c           | 30 ++++++++++++++++++++++++++++++
 fs/super.c         |  3 +++
 include/linux/fs.h |  1 +
 3 files changed, 34 insertions(+)

fs/dax.c +18 -0

index d0bd1f4..f3fc28b 100644
@@ -549,3 +549,33 @@ int dax_truncate_page(struct inode *inode, loff_t from, get_block_t get_block)
 	return dax_zero_page_range(inode, from, length, get_block);
 }
 EXPORT_SYMBOL_GPL(dax_truncate_page);
+
+/* This is meant to be called as part of freeze_super. otherwise we might
+ * Need some extra locking before calling here.
+ */
+void dax_prepare_freeze(struct super_block *sb)
+{
+	struct inode *inode;
+
+	/* TODO: each DAX fs has some private mount option to enable DAX. If
+	 * We made that option a generic MS_DAX_ENABLE super_block flag we could
+	 * Avoid the 95% extra unneeded loop-on-all-inodes every freeze.
+	 * if (!(sb->s_flags & MS_DAX_ENABLE))
+	 *	return 0;
+	 */
+
+	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
+		/* TODO: For freezing we can actually do with write-protecting
+		 * the page. But I cannot find a ready made function that does

fs/super.c +0 -0

 	/* Now wait for internal filesystem counter */
 	sb->s_writers.frozen = SB_FREEZE_FS;
 	smp_wmb();

include/linux/fs.h +1 -0

index 24af817..3b943d4 100644
@@ -2599,6 +2599,7 @@ int dax_truncate_page(struct inode *, loff_t from, get_block_t);
 int dax_fault(struct vm_area_struct *, struct vm_fault *, get_block_t);
 int dax_pfn_mkwrite(struct vm_area_struct *, struct vm_fault *);
 #define dax_mkwrite(vma, vmf, gb)	dax_fault(vma, vmf, gb)
+void dax_prepare_freeze(struct super_block *sb);
 
 #ifdef CONFIG_BLOCK
 typedef void (dio_submit_t)(int rw, struct bio *bio, struct inode *inode,

Quoted 7 lines · Boaz Harrosh

From: Boaz Harrosh <[email protected]>

When freezing an FS, we must write protect all IS_DAX() inodes that have an mmap mapping on an inode. Otherwise application will be able to modify previously faulted-in file pages.

Quoted 8 lines

I'm actually doing a full unmap_mapping_range because there is no readily available "mapping_write_protect" like functionality. I do not think it is worth it to define one just for here and just for some extra read-faults after an fs_freeze.

How hot-path is fs_freeze at all?

OK So reinspecting this was a complete raw RFC. I need to do more work on this thing

comments below ...

Quoted 26 lines

CC Jan Kara <[email protected]>

CC Matthew Wilcox <[email protected]>

CC Andrew Morton <[email protected]>

Signed-off-by Boaz Harrosh <[email protected]>

---
 fs/dax.c           | 30 ++++++++++++++++++++++++++++++
 fs/super.c         |  3 +++
 include/linux/fs.h |  1 +
 3 files changed, 34 insertions(+)

fs/dax.c +8 -0

index d0bd1f4..f3fc28b 100644
@@ -549,3 +549,33 @@ int dax_truncate_page(struct inode *inode, loff_t from, get_block_t get_block)
 	return dax_zero_page_range(inode, from, length, get_block);
 }
 EXPORT_SYMBOL_GPL(dax_truncate_page);
+
+/* This is meant to be called as part of freeze_super. otherwise we might
+ * Need some extra locking before calling here.
+ */
+void dax_prepare_freeze(struct super_block *sb)
+{
+	struct inode *inode;
+

fs/dax.c int dax_truncate_page(struct inode *inode, loff_t from, get_block_t get_block) · line 567

Quoted 8 lines

fs/dax.c +8 -0

+	/* TODO: each DAX fs has some private mount option to enable DAX. If
+	 * We made that option a generic MS_DAX_ENABLE super_block flag we could
+	 * Avoid the 95% extra unneeded loop-on-all-inodes every freeze.
+	 * if (!(sb->s_flags & MS_DAX_ENABLE))
+	 *	return 0;
+	 */
+
+	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {

Quoted 7 lines

fs/dax.c +7 -0

+		/* TODO: For freezing we can actually do with write-protecting
+		 * the page. But I cannot find a ready made function that does
+		 * that for a giving mapping (with all the proper locking).
+		 * How performance sensitive is the all sb_freeze API?
+		 * For now we can just unmap the all mapping, and pay extra
+		 * on read faults.
+		 */

fs/dax.c int dax_truncate_page(struct inode *inode, loff_t from, get_block_t get_block) · line 579

Quoted 5 lines

fs/dax.c +5 -0

+		/* NOTE: Do not unmap private COW mapped pages it will not
+		 * modify the FS.
+		 */
+		if (IS_DAX(inode))
+			unmap_mapping_range(inode->i_mapping, 0, 0, 0);

So what happens here is that we loop on all sb->s_inodes every freeze and in the not DAX case just do nothing.

It could be nice to have a flag at the sb level to tel us if we need to expect IS_DAX() inodes at all, for example when we are mounted on an harddisk it should not be set.

All of ext2/4 and now Dave's xfs have their own
XFS_MOUNT_DAX / EXT2_MOUNT_DAX / EXT4_MOUNT_DAX

Is it OK if I unify all this on sb->s_flags |= MS_MOUNT_DAX so I can check it here in Generic code? The option parsing will be done by each FS but the flag be global?

Quoted 8 lines

fs/super.c +0 -0

index 2b7dc90..9ef490c 100644
@@ -1329,6 +1329,9 @@ int freeze_super(struct super_block *sb)
 	/* All writers are done so after syncing there won't be dirty data */
 	sync_filesystem(sb);
 

fs/super.c int freeze_super(struct super_block *sb) · line 1334

Quoted 3 lines

fs/super.c +3 -0

+	/* Need to take care of DAX mmaped inodes */
+	dax_prepare_freeze(sb);
+

So if CONFIG_FS_DAX is not set this will not compile I need to define an empty one if not set

Quoted 8 lines

fs/dax.c int dax_truncate_page(struct inode *inode, loff_t from, get_block_t get_block) · line 567

Quoted 8 lines

fs/dax.c +8 -0

+	/* TODO: each DAX fs has some private mount option to enable DAX. If
+	 * We made that option a generic MS_DAX_ENABLE super_block flag we could
+	 * Avoid the 95% extra unneeded loop-on-all-inodes every freeze.
+	 * if (!(sb->s_flags & MS_DAX_ENABLE))
+	 *	return 0;
+	 */
+
+	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {

missing locking.

Quoted 7 lines

Quoted 7 lines

fs/dax.c +7 -0

+		/* TODO: For freezing we can actually do with write-protecting
+		 * the page. But I cannot find a ready made function that does
+		 * that for a giving mapping (with all the proper locking).
+		 * How performance sensitive is the all sb_freeze API?
+		 * For now we can just unmap the all mapping, and pay extra
+		 * on read faults.
+		 */

Quoted 8 lines

fs/dax.c int dax_truncate_page(struct inode *inode, loff_t from, get_block_t get_block) · line 579

Quoted 5 lines

fs/dax.c +5 -0

+		/* NOTE: Do not unmap private COW mapped pages it will not
+		 * modify the FS.
+		 */
+		if (IS_DAX(inode))
+			unmap_mapping_range(inode->i_mapping, 0, 0, 0);

So what happens here is that we loop on all sb->s_inodes every freeze and in the not DAX case just do nothing.

Which is real bad and known to be a performance issue. See Josef's recent sync scalability patchset posting that only tracks and walks dirty inodes...

Quoted 7 lines

It could be nice to have a flag at the sb level to tel us if we need to expect IS_DAX() inodes at all, for example when we are mounted on an harddisk it should not be set.

All of ext2/4 and now Dave's xfs have their own
XFS_MOUNT_DAX / EXT2_MOUNT_DAX / EXT4_MOUNT_DAX

Quoted 3 lines

Is it OK if I unify all this on sb->s_flags |= MS_MOUNT_DAX so I can check it here in Generic code? The option parsing will be done by each FS but the flag be global?

No, because as I mentioned in another thread we're going to end up with filesystems that don't have "mount wide" DAX behaviour, and we have to check every dirty inode anyway. And....

Quoted 8 lines

Quoted 8 lines

fs/super.c +0 -0

index 2b7dc90..9ef490c 100644
@@ -1329,6 +1329,9 @@ int freeze_super(struct super_block *sb)
 	/* All writers are done so after syncing there won't be dirty data */
 	sync_filesystem(sb);
 

Quoted 6 lines

fs/super.c int freeze_super(struct super_block *sb) · line 1334

Quoted 3 lines

fs/super.c +3 -0

+	/* Need to take care of DAX mmaped inodes */
+	dax_prepare_freeze(sb);
+

So if CONFIG_FS_DAX is not set this will not compile I need to define an empty one if not set

... it's the wrong approach - sync_filesystem(sb) shoul dbe handling this problem, so that sync and fsync work correctly, and then you don't care about whether DAX is supported or not...

Cheers,
Dave.

On 03/25/2015 04:26 AM, Dave Chinner wrote:
<>

Quoted 11 lines

Quoted 8 lines

fs/dax.c int dax_truncate_page(struct inode *inode, loff_t from, get_block_t get_block) · line 567

Quoted 8 lines

fs/dax.c +8 -0

+	/* TODO: each DAX fs has some private mount option to enable DAX. If
+	 * We made that option a generic MS_DAX_ENABLE super_block flag we could
+	 * Avoid the 95% extra unneeded loop-on-all-inodes every freeze.
+	 * if (!(sb->s_flags & MS_DAX_ENABLE))
+	 *	return 0;
+	 */
+
+	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {

missing locking.

I will please need help here. This is very deep inside the freeze process we area already holding bunch of locks. We know that nothing can be modified at this stage. We are completely read-only.

From fs/inode.c:

Quoted 16 lines

Quoted 15 lines

fs/dax.c int dax_truncate_page(struct inode *inode, loff_t from, get_block_t get_block) · line 579

Quoted 12 lines

fs/dax.c +12 -0

+		/* TODO: For freezing we can actually do with write-protecting
+		 * the page. But I cannot find a ready made function that does
+		 * that for a giving mapping (with all the proper locking).
+		 * How performance sensitive is the all sb_freeze API?
+		 * For now we can just unmap the all mapping, and pay extra
+		 * on read faults.
+		 */
+		/* NOTE: Do not unmap private COW mapped pages it will not
+		 * modify the FS.
+		 */
+		if (IS_DAX(inode))
+			unmap_mapping_range(inode->i_mapping, 0, 0, 0);

So what happens here is that we loop on all sb->s_inodes every freeze and in the not DAX case just do nothing.

Quoted 3 lines

Which is real bad and known to be a performance issue. See Josef's recent sync scalability patchset posting that only tracks and walks dirty inodes...

Sure but how hot is freeze? Josef's fixed the very hot sync path, but freeze happens once in a blue moon. Do we care?

Quoted 11 lines

Quoted 10 lines

It could be nice to have a flag at the sb level to tel us if we need to expect IS_DAX() inodes at all, for example when we are mounted on an harddisk it should not be set.

All of ext2/4 and now Dave's xfs have their own
XFS_MOUNT_DAX / EXT2_MOUNT_DAX / EXT4_MOUNT_DAX

Is it OK if I unify all this on sb->s_flags |= MS_MOUNT_DAX so I can check it here in Generic code? The option parsing will be done by each FS but the flag be global?

Quoted 4 lines

No, because as I mentioned in another thread we're going to end up with filesystems that don't have "mount wide" DAX behaviour, and we have to check every dirty inode anyway. And....

Sure! but let us contract with the FS, that please set the MS_MOUNT_DAX if there is any chance at all that IS_DAX() comes out true, so we loop here.

BTW: We must loop this way on every sb inode because we do not have dirty inodes. There is no "dirty"ing going on in dax, not of inodes and not of pages.

Quoted 4 lines

... it's the wrong approach - sync_filesystem(sb) shoul dbe handling this problem, so that sync and fsync work correctly, and then you don't care about whether DAX is supported or not...

sync and fsync should and will work correctly, but this does not solve our problem. because what turns pages to read-only is the writeback. And we do not have this in dax. Therefore we need to do this here as a special case.

Quoted 1 line · Boaz Harrosh

On 03/25/2015 04:26 AM, Dave Chinner wrote:

Quoted 4 lines

sync and fsync should and will work correctly, but this does not solve our problem. because what turns pages to read-only is the writeback. And we do not have this in dax. Therefore we need to do this here as a special case.

We can still use exactly the same dirty tracking as we use for data writeback. The difference is that we don't need to go through all teh page writeback; we can just flush the CPU caches and mark all the mappings clean, then clear the I_DIRTY_PAGES flag and move on to inode writeback....

Cheers,
Dave.

Quoted 2 lines · Dave Chinner

Quoted 1 line · Boaz Harrosh

On 03/25/2015 04:26 AM, Dave Chinner wrote:

<>

Quoted 11 lines

Quoted 4 lines

sync and fsync should and will work correctly, but this does not solve our problem. because what turns pages to read-only is the writeback. And we do not have this in dax. Therefore we need to do this here as a special case.

We can still use exactly the same dirty tracking as we use for data writeback. The difference is that we don't need to go through all teh page writeback; we can just flush the CPU caches and mark all the mappings clean, then clear the I_DIRTY_PAGES flag and move on to inode writeback....

I see what you mean. the sb wide sync will not step into mmaped inodes and fsync them.

If we go my way and write NT (None Temporal) style in Kernel. NT instructions exist since xeon and all the Intel iX core CPUs have them. In tests we conducted doing xeon NT-writes vs regular-writes-and-cl_flush at .fsync showed minimum of 20% improvement. That is on very large IOs. On 4k IOs it was even better.

It looks like you have a much better picture in your mind how to fit this properly at the inode-dirty picture. Can you attempt a rough draft?

If we are going the NT way. Then we can only I_DIRTY_ track the mmaped inodes. For me this is really scary because I do not want to trigger any writeback threads. If you could please draw me an outline (or write something up ;-)) it would be great.