Rework mtime and ctime updates on mmaped

This is like file_update_time, except that it acts on a struct inode * instead of a struct file *.

Signed-off-by Andy Lutomirski <[email protected]>

---
 fs/inode.c         | 72 ++++++++++++++++++++++++++++++++++++++++++------------
 include/linux/fs.h |  1 +
 2 files changed, 58 insertions(+), 15 deletions(-)

fs/inode.c +42 -15

index d6dfb09..bc90c12 100644
@@ -1637,6 +1637,34 @@ int file_remove_suid(struct file *file)
 }
 EXPORT_SYMBOL(file_remove_suid);
 
+/*
+ * This does the work that's common to file_update_time and
+ * inode_update_time.
+ */
+static int prepare_update_cmtime(struct inode *inode, struct timespec *now)
+{
+	int sync_it;
+
+	/* First try to exhaust all avenues to not sync */
+	if (IS_NOCMTIME(inode))
+		return 0;
+
+	*now = current_fs_time(inode->i_sb);
+	if (!timespec_equal(&inode->i_mtime, now))
+		sync_it = S_MTIME;
+
+	if (!timespec_equal(&inode->i_ctime, now))
+		sync_it |= S_CTIME;
+
+	if (IS_I_VERSION(inode))
+		sync_it |= S_VERSION;
+
+	if (!sync_it)
+		return 0;
+
+	return sync_it;
+}
+
 /**
  *	file_update_time	-	update mtime and ctime time
  *	@file: file accessed

@@ -1654,23 +1682,9 @@ int file_update_time(struct file *file)
 {
 	struct inode *inode = file_inode(file);
 	struct timespec now;
-	int sync_it = 0;
+	int sync_it = prepare_update_cmtime(inode, &now);
 	int ret;
 
-	/* First try to exhaust all avenues to not sync */
-	if (IS_NOCMTIME(inode))
-		return 0;
-
-	now = current_fs_time(inode->i_sb);
-	if (!timespec_equal(&inode->i_mtime, &now))
-		sync_it = S_MTIME;
-
-	if (!timespec_equal(&inode->i_ctime, &now))
-		sync_it |= S_CTIME;
-
-	if (IS_I_VERSION(inode))
-		sync_it |= S_VERSION;
-
 	if (!sync_it)
 		return 0;
 

@@ -1685,6 +1699,34 @@ int file_update_time(struct file *file)
 }
 EXPORT_SYMBOL(file_update_time);
 
+/**
+ *	inode_update_time_writable	-	update mtime and ctime time
+ *	@inode: inode accessed
+ *
+ *	This is like file_update_time, but it assumes the mnt is writable
+ *	and takes an inode parameter instead.  (We need to assume the mnt
+ *	was writable because inodes aren't associated with any particular
+ *	mnt.
+ */
+
+int inode_update_time_writable(struct inode *inode)
+{
+	struct timespec now;

Quoted 9 lines · Andy Lutomirski

This is like file_update_time, except that it acts on a struct inode * instead of a struct file *.

Signed-off-by Andy Lutomirski <[email protected]>

---
 fs/inode.c         | 72 ++++++++++++++++++++++++++++++++++++++++++------------
 include/linux/fs.h |  1 +
 2 files changed, 58 insertions(+), 15 deletions(-)

Quoted 9 lines

fs/inode.c +9 -0

+
+int inode_update_time_writable(struct inode *inode)
+{
+     struct timespec now;
+     int sync_it = prepare_update_cmtime(inode, &now);
+     int ret;
+
+     if (!sync_it)
+             return 0;

fs/inode.c int file_update_time(struct file *file) · line 1724

Quoted 5 lines

fs/inode.c +5 -0

+
+     /* sb_start_pagefault and update_time can both sleep. */
+     sb_start_pagefault(inode->i_sb);
+     ret = update_time(inode, &now, sync_it);
+     sb_end_pagefault(inode->i_sb);

This gets called from the writeback path - you can't use sb_start_pagefault/sb_end_pagefault in that path.

Quoted 10 lines · Dave Chinner

Quoted 9 lines · Andy Lutomirski

This is like file_update_time, except that it acts on a struct inode * instead of a struct file *.

Signed-off-by Andy Lutomirski <[email protected]>

---
 fs/inode.c         | 72 ++++++++++++++++++++++++++++++++++++++++++------------
 include/linux/fs.h |  1 +
 2 files changed, 58 insertions(+), 15 deletions(-)

[...]

Quoted 9 lines

Quoted 9 lines

fs/inode.c +9 -0

+
+int inode_update_time_writable(struct inode *inode)
+{
+     struct timespec now;
+     int sync_it = prepare_update_cmtime(inode, &now);
+     int ret;
+
+     if (!sync_it)
+             return 0;

Quoted 8 lines

fs/inode.c int file_update_time(struct file *file) · line 1724

Quoted 5 lines

fs/inode.c +5 -0

+
+     /* sb_start_pagefault and update_time can both sleep. */
+     sb_start_pagefault(inode->i_sb);
+     ret = update_time(inode, &now, sync_it);
+     sb_end_pagefault(inode->i_sb);

This gets called from the writeback path - you can't use sb_start_pagefault/sb_end_pagefault in that path.

The race I'm worried about is:

- mmap
- write to the mapping
- remount ro
- flush_cmtime -> inode_update_time_writable

This may be impossible, in which case I'm okay, but it's nice to have a sanity check. I'll see if I can figure out how to do that.

Quoted 32 lines · Andy Lutomirski

Quoted 10 lines · Dave Chinner

Quoted 9 lines · Andy Lutomirski

This is like file_update_time, except that it acts on a struct inode * instead of a struct file *.

Signed-off-by Andy Lutomirski <[email protected]>

---
 fs/inode.c         | 72 ++++++++++++++++++++++++++++++++++++++++++------------
 include/linux/fs.h |  1 +
 2 files changed, 58 insertions(+), 15 deletions(-)

[...]

Quoted 9 lines

Quoted 9 lines

fs/inode.c +9 -0

+
+int inode_update_time_writable(struct inode *inode)
+{
+     struct timespec now;
+     int sync_it = prepare_update_cmtime(inode, &now);
+     int ret;
+
+     if (!sync_it)
+             return 0;

Quoted 8 lines

fs/inode.c int file_update_time(struct file *file) · line 1724

Quoted 5 lines

fs/inode.c +5 -0

+
+     /* sb_start_pagefault and update_time can both sleep. */
+     sb_start_pagefault(inode->i_sb);
+     ret = update_time(inode, &now, sync_it);
+     sb_end_pagefault(inode->i_sb);

This gets called from the writeback path - you can't use sb_start_pagefault/sb_end_pagefault in that path.

Quoted 6 lines

The race I'm worried about is:

- mmap
- write to the mapping
- remount ro
- flush_cmtime -> inode_update_time_writable

sb_start_pagefault() is for filesystem freeze protection, not remount-ro protection. If you freeze the filesystem, then we stop writes and pagefaults by making sb_start_pagefault/sb_start_write block, and then run writeback to clean all the pages. If writeback then blocks on sb_start_pagefault(), we've got a deadlock.

Quoted 2 lines

This may be impossible, in which case I'm okay, but it's nice to have a sanity check. I'll see if I can figure out how to do that.

The process of remount-ro should flush the dirty pages - the inode and page has been marked dirty by page_mkwrite(), after all.

Quoted 46 lines · Dave Chinner

Quoted 32 lines · Andy Lutomirski

Quoted 10 lines · Dave Chinner

Quoted 9 lines · Andy Lutomirski

This is like file_update_time, except that it acts on a struct inode * instead of a struct file *.

Signed-off-by Andy Lutomirski <[email protected]>

---
 fs/inode.c         | 72 ++++++++++++++++++++++++++++++++++++++++++------------
 include/linux/fs.h |  1 +
 2 files changed, 58 insertions(+), 15 deletions(-)

[...]

Quoted 9 lines

Quoted 9 lines

fs/inode.c +9 -0

+
+int inode_update_time_writable(struct inode *inode)
+{
+     struct timespec now;
+     int sync_it = prepare_update_cmtime(inode, &now);
+     int ret;
+
+     if (!sync_it)
+             return 0;

Quoted 8 lines

fs/inode.c int file_update_time(struct file *file) · line 1724

Quoted 5 lines

fs/inode.c +5 -0

+
+     /* sb_start_pagefault and update_time can both sleep. */
+     sb_start_pagefault(inode->i_sb);
+     ret = update_time(inode, &now, sync_it);
+     sb_end_pagefault(inode->i_sb);

This gets called from the writeback path - you can't use sb_start_pagefault/sb_end_pagefault in that path.

Quoted 6 lines

The race I'm worried about is:

- mmap
- write to the mapping
- remount ro
- flush_cmtime -> inode_update_time_writable

sb_start_pagefault() is for filesystem freeze protection, not remount-ro protection. If you freeze the filesystem, then we stop writes and pagefaults by making sb_start_pagefault/sb_start_write block, and then run writeback to clean all the pages. If writeback then blocks on sb_start_pagefault(), we've got a deadlock.

Quoted 5 lines

Quoted 2 lines

This may be impossible, in which case I'm okay, but it's nice to have a sanity check. I'll see if I can figure out how to do that.

The process of remount-ro should flush the dirty pages - the inode and page has been marked dirty by page_mkwrite(), after all.

Hmm. We can land in here from writeback, in which case the time should be updated unconditionally. We can also land in here from msync(MS_ASYNC) or munmap. munmap at least shouldn't block.

The nasty case is if a page is dirtied, then the frozen level is set to SB_FREEZE_PAGEFAULT, and then userspace calls munmap or msync *before* writepages gets called. In this case, blocking until the fs is unfrozen is probably impolite, and returning without updating the time is questionable.

Removing the check entirely may add a new race, though: what if .flush_cmtime has called mapping_test_clear_cmtime but hasn't gotten to updating the time yet when freezing finishes? This could be prevented by changing generic_flush_cmtime to do __sb_start_write(sb, SB_FREEZE_FS, false) and doing nothing if the fs is already frozen.

Filesystems that defer cmtime updates should update cmtime when any of these events happen after a write via a mapping:

- The mapping is written back to disk. This happens from all kinds of places, all of which eventually call ->writepages.

- munmap is called or the mapping is removed when the process exits

- msync(MS_ASYNC) is called. Linux currently does nothing for msync(MS_ASYNC), but POSIX says that cmtime should be updated some time between an mmaped write and the subsequent msync call. MS_SYNC calls ->writepages, but MS_ASYNC needs special handling.

Filesystmes that defer cmtime updates should flush them on munmap or exit. Finding out that this happened through vm_ops is messy, so add a new address space op for this.

It's not strictly necessary to call ->flush_cmtime after ->writepages, but it simplifies the fs code. As an optional optimization, filesystems can call mapping_test_clear_cmtime themselves in ->writepages (as long as they're careful to scan all the pages first -- the cmtime bit may not be set when ->writepages is entered).

mm/page-writeback.c +1 -0

@@ -1928,6 +1928,18 @@ int do_writepages(struct address_space *mapping, struct writeback_control *wbc)
 		ret = mapping->a_ops->writepages(mapping, wbc);
 	else
 		ret = generic_writepages(mapping, wbc);
+

Quoted 17 lines · Andy Lutomirski

Filesystems that defer cmtime updates should update cmtime when any of these events happen after a write via a mapping:

- The mapping is written back to disk. This happens from all kinds of places, all of which eventually call ->writepages.

- munmap is called or the mapping is removed when the process exits

- msync(MS_ASYNC) is called. Linux currently does nothing for msync(MS_ASYNC), but POSIX says that cmtime should be updated some time between an mmaped write and the subsequent msync call. MS_SYNC calls ->writepages, but MS_ASYNC needs special handling.

Filesystmes that defer cmtime updates should flush them on munmap or exit. Finding out that this happened through vm_ops is messy, so add a new address space op for this.

Quoted 5 lines

It's not strictly necessary to call ->flush_cmtime after ->writepages, but it simplifies the fs code. As an optional optimization, filesystems can call mapping_test_clear_cmtime themselves in ->writepages (as long as they're careful to scan all the pages first -- the cmtime bit may not be set when ->writepages is entered).

.flush_cmtime is effectively a duplicate method. We already have .update_time to notify filesystems that they need to update the timestamp in the inode transactionally.

Indeed:

include/linux/fs.h struct address_space_operations { · line 360

Quoted 8 lines

include/linux/fs.h +8 -0

+     /*
+      * Userspace expects certain system calls to update cmtime if
+      * a file has been recently written using a shared vma.  In
+      * cases where cmtime may need to be updated but writepages is
+      * not called, this is called instead.  (Implementations
+      * should call mapping_test_clear_cmtime.)
+      */
+     void (*flush_cmtime)(struct address_space *);

You say it can be implemented in the ->writepage(s) method, and all filesystems provide ->writepage(s) in some form. Therefore I would have thought it be best to simply require filesystems to check that mapping flag during those methods and update the inode directly when that is set?

Indeed, the way you've set up the infrastructure, we'll have to rewrite the cmtime update code to enable writepages to update this within some other transaction. Perhaps you should just implement it that way first?

Quoted 24 lines · Dave Chinner

Quoted 22 lines · Andy Lutomirski

Filesystems that defer cmtime updates should update cmtime when any of these events happen after a write via a mapping:

- The mapping is written back to disk. This happens from all kinds of places, all of which eventually call ->writepages.

- munmap is called or the mapping is removed when the process exits

- msync(MS_ASYNC) is called. Linux currently does nothing for msync(MS_ASYNC), but POSIX says that cmtime should be updated some time between an mmaped write and the subsequent msync call. MS_SYNC calls ->writepages, but MS_ASYNC needs special handling.

Filesystmes that defer cmtime updates should flush them on munmap or exit. Finding out that this happened through vm_ops is messy, so add a new address space op for this.

It's not strictly necessary to call ->flush_cmtime after ->writepages, but it simplifies the fs code. As an optional optimization, filesystems can call mapping_test_clear_cmtime themselves in ->writepages (as long as they're careful to scan all the pages first -- the cmtime bit may not be set when ->writepages is entered).

Quoted 3 lines

.flush_cmtime is effectively a duplicate method. We already have .update_time to notify filesystems that they need to update the timestamp in the inode transactionally.

.update_time is used for the atime update as well, and it relies on the core code to update the in-memory timestamp first.

I used that
approach in v2, but it was (correctly, I think) pointed out that this was a layering violation and that core code shouldn't be mucking with the timestamps directly during writeback.

Quoted 11 lines

Indeed:

include/linux/fs.h struct address_space_operations { · line 360

Quoted 8 lines

include/linux/fs.h +8 -0

+     /*
+      * Userspace expects certain system calls to update cmtime if
+      * a file has been recently written using a shared vma.  In
+      * cases where cmtime may need to be updated but writepages is
+      * not called, this is called instead.  (Implementations
+      * should call mapping_test_clear_cmtime.)
+      */
+     void (*flush_cmtime)(struct address_space *);

Quoted 5 lines

You say it can be implemented in the ->writepage(s) method, and all filesystems provide ->writepage(s) in some form. Therefore I would have thought it be best to simply require filesystems to check that mapping flag during those methods and update the inode directly when that is set?

The problem with only doing it in ->writepages is that calling writepages from munmap and exit would probably hurt performance for no particular gain. So I need some kind of callback to say "update the time, but don't write data." The AS_CMTIME bit will still be set when the ptes are removed.

I could require ->writepages *and* ->flush_cmtime to handle the time update, but that would complicate non-transactional filesystems. Those filesystems should just flush cmtime at the end of writepages.

Quoted 4 lines

Indeed, the way you've set up the infrastructure, we'll have to rewrite the cmtime update code to enable writepages to update this within some other transaction. Perhaps you should just implement it that way first?

This is already possible although not IMO necessary for correctness. All that ext4 would need to do is to add something like:

if (mapping_test_clear_cmtime(mapping)) {
update times within current transaction
}

somewhere inside the transaction in writepages. There would probably be room for some kind of generic helper to do everything in inode_update_time_writable except for the actual mark_inode_dirty part, but this still seems nasty from a locking perspective, and I'd rather leave that optimization to an ext4 developer who wants to do it.

Quoted 25 lines · Andy Lutomirski

Quoted 24 lines · Dave Chinner

Quoted 22 lines · Andy Lutomirski

Filesystems that defer cmtime updates should update cmtime when any of these events happen after a write via a mapping:

- The mapping is written back to disk. This happens from all kinds of places, all of which eventually call ->writepages.

- munmap is called or the mapping is removed when the process exits

- msync(MS_ASYNC) is called. Linux currently does nothing for msync(MS_ASYNC), but POSIX says that cmtime should be updated some time between an mmaped write and the subsequent msync call. MS_SYNC calls ->writepages, but MS_ASYNC needs special handling.

Filesystmes that defer cmtime updates should flush them on munmap or exit. Finding out that this happened through vm_ops is messy, so add a new address space op for this.

It's not strictly necessary to call ->flush_cmtime after ->writepages, but it simplifies the fs code. As an optional optimization, filesystems can call mapping_test_clear_cmtime themselves in ->writepages (as long as they're careful to scan all the pages first -- the cmtime bit may not be set when ->writepages is entered).

Quoted 6 lines

Quoted 3 lines

.flush_cmtime is effectively a duplicate method. We already have .update_time to notify filesystems that they need to update the timestamp in the inode transactionally.

.update_time is used for the atime update as well, and it relies on the core code to update the in-memory timestamp first.

No, it doesn't. The caller of .update_time provides the timestamp to that gets written into the relevant fields of the inode.

i.e. this code:

        now = current_fs_time(inode->i_sb);
        if (inode->i_op->update_time)
                return inode->i_op->update_time(inode, &now, S_CTIME|S_MTIME);

will update *only* the ctime and mtime of the inode to have a value of @now. That's precisely what you want to do, yes?

Indeed, your .flush_cmtime function effectively does:

        flags = prepare_cmtime(inode, &now)
                { *now = current_fs_time()
                  flags = S_CTIME|S_MTIME;
                }
        update_time(inode, now, flags);
                {
                        inode->i_op->update_time(inode, now, flags)
                        or
                        mark_inode_dirty_sync()
                }

IOWs, you're adding a filesystem specific method to update a timestamp that wraps around a generic method for updating a timestamp. i.e. .flush_cmtime is not necessary - you could just call inode_update_time_writable() from generic_writepages() and it would simply just work for all filesystems....

Quoted 5 lines

I used that
approach in v2, but it was (correctly, I think) pointed out that this was a layering violation and that core code shouldn't be mucking with the timestamps directly during writeback.

If calling a generic method to update the timestamp is a layering violation, then why is replacing that with a filesystem specific method of updating a timestamp not a layering violation?

Quoted 17 lines

Quoted 16 lines

Indeed:

include/linux/fs.h struct address_space_operations { · line 360

Quoted 8 lines

include/linux/fs.h +8 -0

+     /*
+      * Userspace expects certain system calls to update cmtime if
+      * a file has been recently written using a shared vma.  In
+      * cases where cmtime may need to be updated but writepages is
+      * not called, this is called instead.  (Implementations
+      * should call mapping_test_clear_cmtime.)
+      */
+     void (*flush_cmtime)(struct address_space *);

You say it can be implemented in the ->writepage(s) method, and all filesystems provide ->writepage(s) in some form. Therefore I would have thought it be best to simply require filesystems to check that mapping flag during those methods and update the inode directly when that is set?

Quoted 5 lines

The problem with only doing it in ->writepages is that calling writepages from munmap and exit would probably hurt performance for no particular gain. So I need some kind of callback to say "update the time, but don't write data." The AS_CMTIME bit will still be set when the ptes are removed.

What's the point of updating the timestamp at unmap when it's going to be changed again when the writeback occurs?

Quoted 4 lines

I could require ->writepages *and* ->flush_cmtime to handle the time update, but that would complicate non-transactional filesystems. Those filesystems should just flush cmtime at the end of writepages.

do_writepages() is the wrong place to do such updates - we can get writeback directly through .writepage, so the time updates need to be in .writepage. That first .writepage call will clear the bit on the mapping, so it's only done on the first call to .writepage on the given mapping.

And some filesystems provide a .writepages method that doesn't call .writepage, so those filesystems will need to do the timestamp update in those methods.

Quoted 11 lines

Quoted 4 lines

Indeed, the way you've set up the infrastructure, we'll have to rewrite the cmtime update code to enable writepages to update this within some other transaction. Perhaps you should just implement it that way first?

This is already possible although not IMO necessary for correctness. All that ext4 would need to do is to add something like:

if (mapping_test_clear_cmtime(mapping)) {
update times within current transaction
}

Where does it get the timestamps from? i.e. the update could call prepare_update_cmtime() to do this, yes? And so having a wrapper that does

        if (mapping_test_clear_cmtime(mapping))
                return prepare_update_cmtime(inode);
        return 0;

would work just fine for them, yes?

Quoted 6 lines

somewhere inside the transaction in writepages. There would probably be room for some kind of generic helper to do everything in inode_update_time_writable except for the actual mark_inode_dirty part, but this still seems nasty from a locking perspective, and I'd rather leave that optimization to an ext4 developer who wants to do it.

filesystems that implement .update_time don't need to mark the struct inode dirty on timestamp updates. Similarly, filesystems that use a writepages transaction to do the update don't either....

Quoted 4 lines

Quoted 3 lines

I could require ->writepages *and* ->flush_cmtime to handle the time update, but that would complicate non-transactional filesystems. Those filesystems should just flush cmtime at the end of writepages.

Quoted 5 lines

do_writepages() is the wrong place to do such updates - we can get writeback directly through .writepage, so the time updates need to be in .writepage. That first .writepage call will clear the bit on the mapping, so it's only done on the first call to .writepage on the given mapping.

Last time I checked, all the paths that actually needed the timestamp update went through .writepages. I'll double-check.

Quoted 10 lines · Andy Lutomirski

Quoted 9 lines

Quoted 3 lines

I could require ->writepages *and* ->flush_cmtime to handle the time update, but that would complicate non-transactional filesystems. Those filesystems should just flush cmtime at the end of writepages.

do_writepages() is the wrong place to do such updates - we can get writeback directly through .writepage, so the time updates need to be in .writepage. That first .writepage call will clear the bit on the mapping, so it's only done on the first call to .writepage on the given mapping.

Quoted 2 lines

Last time I checked, all the paths that actually needed the timestamp update went through .writepages. I'll double-check.

kswapd can call just .writepage to do the writeout so timestamp update
should be handled there as well. Otherwise all pages in a mapping can be cleaned without timestamp being updated.

Which btw made me realize that even your scheme doesn't completely make sure timestamp is updated after mmap write - if you have pages 0 and 1, you write to both of them - CMTIME flag gets set. Then fsync_range(fd, 0, 4096) is called. We write the page 0, writeprotect it, update timestamps. But page 1 is still writeable so writes to it won't set CMTIME flag, neither update the timestamp... Not that I think this can be reasonably solved but it is a food for thought.

Quoted 11 lines · Jan Kara

Quoted 10 lines · Andy Lutomirski

Quoted 9 lines

Quoted 3 lines

I could require ->writepages *and* ->flush_cmtime to handle the time update, but that would complicate non-transactional filesystems. Those filesystems should just flush cmtime at the end of writepages.

do_writepages() is the wrong place to do such updates - we can get writeback directly through .writepage, so the time updates need to be in .writepage. That first .writepage call will clear the bit on the mapping, so it's only done on the first call to .writepage on the given mapping.

Quoted 5 lines

Quoted 2 lines

Last time I checked, all the paths that actually needed the timestamp update went through .writepages. I'll double-check.

kswapd can call just .writepage to do the writeout so timestamp update
should be handled there as well. Otherwise all pages in a mapping can be cleaned without timestamp being updated.

OK, I'll fix that.

Quoted 11 lines · Jan Kara

Quoted 10 lines · Andy Lutomirski

Quoted 9 lines

Quoted 3 lines

I could require ->writepages *and* ->flush_cmtime to handle the time update, but that would complicate non-transactional filesystems. Those filesystems should just flush cmtime at the end of writepages.

do_writepages() is the wrong place to do such updates - we can get writeback directly through .writepage, so the time updates need to be in .writepage. That first .writepage call will clear the bit on the mapping, so it's only done on the first call to .writepage on the given mapping.

Quoted 5 lines

Quoted 2 lines

Last time I checked, all the paths that actually needed the timestamp update went through .writepages. I'll double-check.

kswapd can call just .writepage to do the writeout so timestamp update
should be handled there as well. Otherwise all pages in a mapping can be cleaned without timestamp being updated.

OK, I'll fix that.

Quoted 8 lines

Which btw made me realize that even your scheme doesn't completely make sure timestamp is updated after mmap write - if you have pages 0 and 1, you write to both of them - CMTIME flag gets set. Then fsync_range(fd, 0, 4096) is called. We write the page 0, writeprotect it, update timestamps. But page 1 is still writeable so writes to it won't set CMTIME flag, neither update the timestamp... Not that I think this can be reasonably solved but it is a food for thought.

This should already work. AS_CMTIME is set when the pte goes from dirty to clean, not when the pte goes from wp to writable. So whenever clear_page_dirty_for_io is called on page 1, AS_CMTIME will be set and a subsequent writepages call will update the timestamp.

Quoted 29 lines · Andy Lutomirski

Quoted 11 lines · Jan Kara

Quoted 10 lines · Andy Lutomirski

Quoted 9 lines

Quoted 3 lines

I could require ->writepages *and* ->flush_cmtime to handle the time update, but that would complicate non-transactional filesystems. Those filesystems should just flush cmtime at the end of writepages.

do_writepages() is the wrong place to do such updates - we can get writeback directly through .writepage, so the time updates need to be in .writepage. That first .writepage call will clear the bit on the mapping, so it's only done on the first call to .writepage on the given mapping.

Quoted 5 lines

Quoted 2 lines

Last time I checked, all the paths that actually needed the timestamp update went through .writepages. I'll double-check.

kswapd can call just .writepage to do the writeout so timestamp update
should be handled there as well. Otherwise all pages in a mapping can be cleaned without timestamp being updated.

OK, I'll fix that.

Quoted 8 lines

Which btw made me realize that even your scheme doesn't completely make sure timestamp is updated after mmap write - if you have pages 0 and 1, you write to both of them - CMTIME flag gets set. Then fsync_range(fd, 0, 4096) is called. We write the page 0, writeprotect it, update timestamps. But page 1 is still writeable so writes to it won't set CMTIME flag, neither update the timestamp... Not that I think this can be reasonably solved but it is a food for thought.

Quoted 4 lines

This should already work. AS_CMTIME is set when the pte goes from dirty to clean, not when the pte goes from wp to writable. So whenever clear_page_dirty_for_io is called on page 1, AS_CMTIME will be set and a subsequent writepages call will update the timestamp.

Oh, I missed that - I thought you were setting AS_CMTIME during .page_mkwrite.

Setting it in clear_page_dirty_for_io() is too late for filesystems to include it in their existing transactions during .writepage, (at least for XFs and ext4) because they do their delayed allocation transactions before changing page state....

Cheers,

Dave.
--
Dave Chinner
[email protected]

Quoted 38 lines · Dave Chinner

Quoted 29 lines · Andy Lutomirski

Quoted 11 lines · Jan Kara

Quoted 10 lines · Andy Lutomirski

Quoted 9 lines

Quoted 3 lines

I could require ->writepages *and* ->flush_cmtime to handle the time update, but that would complicate non-transactional filesystems. Those filesystems should just flush cmtime at the end of writepages.

do_writepages() is the wrong place to do such updates - we can get writeback directly through .writepage, so the time updates need to be in .writepage. That first .writepage call will clear the bit on the mapping, so it's only done on the first call to .writepage on the given mapping.

Quoted 5 lines

Quoted 2 lines

Last time I checked, all the paths that actually needed the timestamp update went through .writepages. I'll double-check.

kswapd can call just .writepage to do the writeout so timestamp update
should be handled there as well. Otherwise all pages in a mapping can be cleaned without timestamp being updated.

OK, I'll fix that.

Quoted 8 lines

Which btw made me realize that even your scheme doesn't completely make sure timestamp is updated after mmap write - if you have pages 0 and 1, you write to both of them - CMTIME flag gets set. Then fsync_range(fd, 0, 4096) is called. We write the page 0, writeprotect it, update timestamps. But page 1 is still writeable so writes to it won't set CMTIME flag, neither update the timestamp... Not that I think this can be reasonably solved but it is a food for thought.

Quoted 4 lines

This should already work. AS_CMTIME is set when the pte goes from dirty to clean, not when the pte goes from wp to writable. So whenever clear_page_dirty_for_io is called on page 1, AS_CMTIME will be set and a subsequent writepages call will update the timestamp.

Oh, I missed that - I thought you were setting AS_CMTIME during .page_mkwrite.

Quoted 4 lines

Setting it in clear_page_dirty_for_io() is too late for filesystems to include it in their existing transactions during .writepage, (at least for XFs and ext4) because they do their delayed allocation transactions before changing page state....

Couldn't it go between mpage_map_and_submit_extent and ext4_journal_stop in ext4_writepages?

Quoted 44 lines · Andy Lutomirski

Quoted 42 lines · Dave Chinner

Quoted 29 lines · Andy Lutomirski

Quoted 11 lines · Jan Kara

Quoted 10 lines · Andy Lutomirski

Quoted 9 lines

Quoted 3 lines

I could require ->writepages *and* ->flush_cmtime to handle the time update, but that would complicate non-transactional filesystems. Those filesystems should just flush cmtime at the end of writepages.

do_writepages() is the wrong place to do such updates - we can get writeback directly through .writepage, so the time updates need to be in .writepage. That first .writepage call will clear the bit on the mapping, so it's only done on the first call to .writepage on the given mapping.

Quoted 5 lines

Quoted 2 lines

Last time I checked, all the paths that actually needed the timestamp update went through .writepages. I'll double-check.

kswapd can call just .writepage to do the writeout so timestamp update
should be handled there as well. Otherwise all pages in a mapping can be cleaned without timestamp being updated.

OK, I'll fix that.

Quoted 8 lines

Which btw made me realize that even your scheme doesn't completely make sure timestamp is updated after mmap write - if you have pages 0 and 1, you write to both of them - CMTIME flag gets set. Then fsync_range(fd, 0, 4096) is called. We write the page 0, writeprotect it, update timestamps. But page 1 is still writeable so writes to it won't set CMTIME flag, neither update the timestamp... Not that I think this can be reasonably solved but it is a food for thought.

Quoted 4 lines

This should already work. AS_CMTIME is set when the pte goes from dirty to clean, not when the pte goes from wp to writable. So whenever clear_page_dirty_for_io is called on page 1, AS_CMTIME will be set and a subsequent writepages call will update the timestamp.

Oh, I missed that - I thought you were setting AS_CMTIME during .page_mkwrite.

Setting it in clear_page_dirty_for_io() is too late for filesystems to include it in their existing transactions during .writepage, (at least for XFs and ext4) because they do their delayed allocation transactions before changing page state....

Quoted 2 lines

Couldn't it go between mpage_map_and_submit_extent and ext4_journal_stop in ext4_writepages?

Maybe - I'm not an ext4 expert - but even if you can make it work for ext4 in some way, that doesn't mean it is possible for any other filesystem to use the same method. You're adding code to generic, non-filesystem specific code paths and so the solutions need to be generic rather not tied to how a specific filesystem is implemented.

Cheers,

Dave.
--
Dave Chinner
[email protected]

Quoted 45 lines · Dave Chinner

Quoted 44 lines · Andy Lutomirski

Quoted 42 lines · Dave Chinner

Quoted 29 lines · Andy Lutomirski

Quoted 11 lines · Jan Kara

Quoted 10 lines · Andy Lutomirski

Quoted 9 lines

Quoted 3 lines

I could require ->writepages *and* ->flush_cmtime to handle the time update, but that would complicate non-transactional filesystems. Those filesystems should just flush cmtime at the end of writepages.

do_writepages() is the wrong place to do such updates - we can get writeback directly through .writepage, so the time updates need to be in .writepage. That first .writepage call will clear the bit on the mapping, so it's only done on the first call to .writepage on the given mapping.

Quoted 5 lines

Quoted 2 lines

Last time I checked, all the paths that actually needed the timestamp update went through .writepages. I'll double-check.

kswapd can call just .writepage to do the writeout so timestamp update
should be handled there as well. Otherwise all pages in a mapping can be cleaned without timestamp being updated.

OK, I'll fix that.

Quoted 8 lines

Which btw made me realize that even your scheme doesn't completely make sure timestamp is updated after mmap write - if you have pages 0 and 1, you write to both of them - CMTIME flag gets set. Then fsync_range(fd, 0, 4096) is called. We write the page 0, writeprotect it, update timestamps. But page 1 is still writeable so writes to it won't set CMTIME flag, neither update the timestamp... Not that I think this can be reasonably solved but it is a food for thought.

Quoted 4 lines

This should already work. AS_CMTIME is set when the pte goes from dirty to clean, not when the pte goes from wp to writable. So whenever clear_page_dirty_for_io is called on page 1, AS_CMTIME will be set and a subsequent writepages call will update the timestamp.

Oh, I missed that - I thought you were setting AS_CMTIME during .page_mkwrite.

Setting it in clear_page_dirty_for_io() is too late for filesystems to include it in their existing transactions during .writepage, (at least for XFs and ext4) because they do their delayed allocation transactions before changing page state....

Quoted 9 lines

Quoted 2 lines

Couldn't it go between mpage_map_and_submit_extent and ext4_journal_stop in ext4_writepages?

Maybe - I'm not an ext4 expert - but even if you can make it work for ext4 in some way, that doesn't mean it is possible for any other filesystem to use the same method. You're adding code to generic, non-filesystem specific code paths and so the solutions need to be generic rather not tied to how a specific filesystem is implemented.

I don't see the problem for xfs or btrfs either.

xfs uses generic_writepages, which already does the right thing. (xfs with my updated patches passes my tests.) xfs_vm_writepage calls xfs_start_page_writeback(..., 1, ...), so clear_page_dirty_for_io is called. At that point (I presume), it would still be possible to add metadata to a transaction (assuming there's a transaction open -- I don't have a clue here).

Even if this is too late, xfs_vm_writepage
could call page_mkwrite to for AS_CMTIME to be set if needed. page_mkwrite will be fast if the page isn't mmapped. What am I missing?

A fancier implementation could probably avoid an extra journal transaction by adding a mapping_test_clear_cmtime call in ext4_writepages, but this should already be a considerable improvement -- we'll start one transaction per writepages call instead of one per page.

Quoted 5 lines · Andy Lutomirski

A fancier implementation could probably avoid an extra journal transaction by adding a mapping_test_clear_cmtime call in ext4_writepages, but this should already be a considerable improvement -- we'll start one transaction per writepages call instead of one per page.

I'd like to see more than just an ext4 implementation - btrfs and XFS are the other main filesystems that should behave identically.

Also, it's worthwhile to write a generic xfstest to ensure that they all update the timestamp appropriately - if its' in xfstests, then we can basically guarantee that it won't get randomly regressed in future, and other filesystems can be easily verified as well sa their implement this.

Quoted 9 lines · Dave Chinner

Quoted 5 lines · Andy Lutomirski

A fancier implementation could probably avoid an extra journal transaction by adding a mapping_test_clear_cmtime call in ext4_writepages, but this should already be a considerable improvement -- we'll start one transaction per writepages call instead of one per page.

I'd like to see more than just an ext4 implementation - btrfs and XFS are the other main filesystems that should behave identically.

Will do.

Quoted 1 line

Quoted 6 lines

Also, it's worthwhile to write a generic xfstest to ensure that they all update the timestamp appropriately - if its' in xfstests, then we can basically guarantee that it won't get randomly regressed in future, and other filesystems can be easily verified as well sa their implement this.

Is there a guide to writing an xfstest?