Rework mtime and ctime updates on mmaped writes

Hello,

This is to apply on top of
"[PATCH v4 2/7] fs: Add inode_update_time_writable" dated Thu, 22 Aug 2013 17:03:18 -0700

Ensure prepare_update_cmtime() returns an initialized value.

While at it:
- always use sync_it var as a bitfield
- remove useless test

Signed-off-by Sylvain "ythier" Hitier <[email protected]>

---
 fs/inode.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

fs/inode.c +2 -5

index 2bbcb19..edf225c 100644
@@ -1643,7 +1643,7 @@ EXPORT_SYMBOL(file_remove_suid);
  */
 static int prepare_update_cmtime(struct inode *inode, struct timespec *now)
 {
-       int sync_it;
+       int sync_it = 0;
 
        /* First try to exhaust all avenues to not sync */
        if (IS_NOCMTIME(inode))

@@ -1651,7 +1651,7 @@ static int prepare_update_cmtime(struct inode *inode, struct timespec *now)
 
        *now = current_fs_time(inode->i_sb);
        if (!timespec_equal(&inode->i_mtime, now))
-               sync_it = S_MTIME;
+               sync_it |= S_MTIME;
 
        if (!timespec_equal(&inode->i_ctime, now))
                sync_it |= S_CTIME;

@@ -1659,9 +1659,6 @@ static int prepare_update_cmtime(struct inode *inode, struct timespec *now)
        if (IS_I_VERSION(inode))
                sync_it |= S_VERSION;
 
-       if (!sync_it)
-               return 0;
-
        return sync_it;
 }
 

Regards,
Sylvain "ythier" Hitier

--
Business is about being busy, not being rich...
Lived 777 days in a Debian package => http://en.wikipedia.org/wiki/Apt,_Vaucluse
There's THE room for ideals in this mechanical place!

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, most of which eventually call ->writepages. (The exceptions are vmscan and migration.)

- 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.

Filesystems are responsible for checking for pending deferred cmtime updates in .writepages (a helper is provided for this purpose) and for doing the actual update in .update_cmtime_deferred.

These changes have no effect by themselves; filesystems must opt in by implementing .update_cmtime_deferred and removing any file_update_time call in .page_mkwrite.

This patch does not implement the MS_ASYNC case; that's in the next patch.

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

mm/page-writeback.c +8 -0

+/**
+ * generic_update_cmtime_deferred - update cmtime after an mmapped write
+ * @mapping: The mapping
+ *
+ * This library function implements .update_cmtime_deferred.  It is unlikely
+ * that any filesystem will want to do anything here except update the time
+ * (using this helper) or nothing at all (by leaving .update_cmtime_deferred
+ * NULL).

mm/page-writeback.c +15 -0

+
+void mapping_flush_cmtime_nowb(struct address_space *mapping)
+{
+	/*
+	 * We get called from munmap and msync.  Both calls can race
+	 * with fs freezing.  If the fs is frozen after
+	 * mapping_test_clear_cmtime but before the time update, then
+	 * sync_filesystem will miss the cmtime update (because we
+	 * just cleared it) and we don't be able to write (because the
+	 * fs is frozen).  On the other hand, we can't just return if
+	 * we're in the SB_FREEZE_PAGEFAULT state because our caller
+	 * expects the timestamp to be synchronously updated.  So we
+	 * get write access without blocking, at the SB_FREEZE_FS
+	 * level.  If the fs is already fully frozen, then we already
+	 * know we have nothing to do.

Quoted 26 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, most of which eventually call ->writepages. (The exceptions are vmscan and migration.)

- 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.

Filesystems are responsible for checking for pending deferred cmtime updates in .writepages (a helper is provided for this purpose) and for doing the actual update in .update_cmtime_deferred.

These changes have no effect by themselves; filesystems must opt in by implementing .update_cmtime_deferred and removing any file_update_time call in .page_mkwrite.

This patch does not implement the MS_ASYNC case; that's in the next patch.

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

...

Quoted 8 lines

mm/page-writeback.c +8 -0

+/**
+ * generic_update_cmtime_deferred - update cmtime after an mmapped write
+ * @mapping: The mapping
+ *
+ * This library function implements .update_cmtime_deferred.  It is unlikely
+ * that any filesystem will want to do anything here except update the time
+ * (using this helper) or nothing at all (by leaving .update_cmtime_deferred
+ * NULL).

mm/page-writeback.c int generic_writepages(struct address_space *mapping, · line 1938

Quoted 10 lines

mm/page-writeback.c +10 -0

+ */
+void generic_update_cmtime_deferred(struct address_space *mapping)
+{
+     struct blk_plug plug;
+     blk_start_plug(&plug);
+     inode_update_time_writable(mapping->host);
+     blk_finish_plug(&plug);
+}
+EXPORT_SYMBOL(generic_update_cmtime_deferred);
+

You can remove the pluggin here. Inode update will likely result in a
single write so there's no point.

Quoted 4 lines

mm/page-writeback.c +0 -0

@@ -1970,6 +1988,39 @@ int write_one_page(struct page *page, int wait)
 }
 EXPORT_SYMBOL(write_one_page);
 

mm/page-writeback.c int write_one_page(struct page *page, int wait) · line 1997

Quoted 7 lines

mm/page-writeback.c +7 -0

+void mapping_flush_cmtime(struct address_space *mapping)
+{
+     if (mapping_test_clear_cmtime(mapping) &&
+         mapping->a_ops->update_cmtime_deferred)
+             mapping->a_ops->update_cmtime_deferred(mapping);
+}
+EXPORT_SYMBOL(mapping_flush_cmtime);

Hum, is there a reason for update_cmtime_deferred() operation? I can
hardly imagine anyone will want to do anything else than what inode_update_time_writable() does so why bother? You mention tmpfs & co. don't fit into your scheme well with which I agree so let's just keep file_update_time() in their page_mkwrite() operation. But I don't see a real need for avoiding the deferred cmtime logic...

Quoted 24 lines

mm/page-writeback.c +23 -0

+
+void mapping_flush_cmtime_nowb(struct address_space *mapping)
+{
+     /*
+      * We get called from munmap and msync.  Both calls can race
+      * with fs freezing.  If the fs is frozen after
+      * mapping_test_clear_cmtime but before the time update, then
+      * sync_filesystem will miss the cmtime update (because we
+      * just cleared it) and we don't be able to write (because the
+      * fs is frozen).  On the other hand, we can't just return if
+      * we're in the SB_FREEZE_PAGEFAULT state because our caller
+      * expects the timestamp to be synchronously updated.  So we
+      * get write access without blocking, at the SB_FREEZE_FS
+      * level.  If the fs is already fully frozen, then we already
+      * know we have nothing to do.
+      */
+
+     if (!mapping_test_cmtime(mapping))
+             return;  /* Optimization: nothing to do. */
+
+     if (__sb_start_write(mapping->host->i_sb, SB_FREEZE_FS, false)) {
+             mapping_flush_cmtime(mapping);
+             __sb_end_write(mapping->host->i_sb, SB_FREEZE_FS);

Review int write_one_page(struct page *page, int wait) · line 2022

Quoted 2 lines

(quoted diff) +2 -0

+     }
+}

This is wrong because SB_FREEZE_FS level is targetted for filesystem
internal use. Also it is racy. mapping_flush_cmtime() ends up calling mark_inode_dirty() and filesystems such as ext4 or xfs will start a transaction to store inode in the journal. This gets freeze protection at SB_FREEZE_FS level again. If freeze_super() sets s_writers.frozen to SB_FREEZE_FS before this second protection, things will deadlock.

Since the callers of this function hold mmap_sem, using SB_FREEZE_PAGEFAULT protection would be appropriate. Also since there are just two places that need the freeze protection I'd be inclined to open code the protection in the two places rather than hiding it in a special function.

Quoted 4 lines

Quoted 4 lines

mm/page-writeback.c +0 -0

@@ -1970,6 +1988,39 @@ int write_one_page(struct page *page, int wait)
 }
 EXPORT_SYMBOL(write_one_page);
 

Quoted 13 lines

mm/page-writeback.c int write_one_page(struct page *page, int wait) · line 1997

Quoted 7 lines

mm/page-writeback.c +7 -0

+void mapping_flush_cmtime(struct address_space *mapping)
+{
+     if (mapping_test_clear_cmtime(mapping) &&
+         mapping->a_ops->update_cmtime_deferred)
+             mapping->a_ops->update_cmtime_deferred(mapping);
+}
+EXPORT_SYMBOL(mapping_flush_cmtime);

Hum, is there a reason for update_cmtime_deferred() operation? I can
hardly imagine anyone will want to do anything else than what inode_update_time_writable() does so why bother? You mention tmpfs & co. don't fit into your scheme well with which I agree so let's just keep file_update_time() in their page_mkwrite() operation. But I don't see a real need for avoiding the deferred cmtime logic...

I think there might be odd corner cases. For example, mmap a tmpfs file, write it, and unmap it. Then, an hour later, maybe the system

will be under memory pressure and page out the file. This could trigger a surprising time update. (I'm not sure this can actually happen on tmpfs, but maybe it would on some other filesystem.)

Does this actually matter? A flag to turn the feature on or off would do the trick, but I don't think there's precedent for sticking a flag in a_ops.

Quoted 22 lines

Quoted 22 lines

mm/page-writeback.c +22 -0

+void mapping_flush_cmtime_nowb(struct address_space *mapping)
+{
+     /*
+      * We get called from munmap and msync.  Both calls can race
+      * with fs freezing.  If the fs is frozen after
+      * mapping_test_clear_cmtime but before the time update, then
+      * sync_filesystem will miss the cmtime update (because we
+      * just cleared it) and we don't be able to write (because the
+      * fs is frozen).  On the other hand, we can't just return if
+      * we're in the SB_FREEZE_PAGEFAULT state because our caller
+      * expects the timestamp to be synchronously updated.  So we
+      * get write access without blocking, at the SB_FREEZE_FS
+      * level.  If the fs is already fully frozen, then we already
+      * know we have nothing to do.
+      */
+
+     if (!mapping_test_cmtime(mapping))
+             return;  /* Optimization: nothing to do. */
+
+     if (__sb_start_write(mapping->host->i_sb, SB_FREEZE_FS, false)) {
+             mapping_flush_cmtime(mapping);
+             __sb_end_write(mapping->host->i_sb, SB_FREEZE_FS);

Quoted 8 lines

Review int write_one_page(struct page *page, int wait) · line 2021

Quoted 2 lines

(quoted diff) +2 -0

+     }
+}

This is wrong because SB_FREEZE_FS level is targetted for filesystem
internal use. Also it is racy. mapping_flush_cmtime() ends up calling mark_inode_dirty() and filesystems such as ext4 or xfs will start a transaction to store inode in the journal. This gets freeze protection at SB_FREEZE_FS level again. If freeze_super() sets s_writers.frozen to SB_FREEZE_FS before this second protection, things will deadlock.

Whoops -- I assumed that it was safe to recursively take freeze protection at the same level.

I'm worried about the following race:

Thread 1 (in munmap):
Check AS_CMTIME set
sb_start_pagefault

Thread 2 (freezing the fs):
frozen = SB_FREEZE_PAGEFAULT;
sync_filesystem()

Thread 1 is now stuck. It doesn't need to be, because sync_filesystem will flush out the cmtime write. But there doesn't seem to be a clean mechanism to wait for the freeze to finish.

Is there a clean way to avoid this? I don't want to return immediately if a freeze is in progress, because userspace expects that munmap will update cmtime synchronously.

And ugly but simple solution is:

if (!mapping_test_cmtime(mapping))
return; /* Optimization: nothing to do. */

if (__sb_start_write(mapping->host->i_sb, SB_FREEZE_FS, false)) {
    mapping_flush_cmtime(mapping);
    __sb_end_write(mapping->host->i_sb, SB_FREEZE_FS);
} else {
    /* Freeze is or was in progress.  The part of freezing from
SB_FREEZE_PAGEFAULT through sync_filesystem holds s_umount for write,
so we can wait for it to finish by taking s_umount for read. */
    down_read(&sb->s_umount);
    up_read(&sb->s_umount);
}

Quoted 18 lines · Andy Lutomirski

Quoted 4 lines

Quoted 4 lines

mm/page-writeback.c +0 -0

@@ -1970,6 +1988,39 @@ int write_one_page(struct page *page, int wait)
 }
 EXPORT_SYMBOL(write_one_page);
 

Quoted 13 lines

mm/page-writeback.c int write_one_page(struct page *page, int wait) · line 1997

Quoted 7 lines

mm/page-writeback.c +7 -0

+void mapping_flush_cmtime(struct address_space *mapping)
+{
+     if (mapping_test_clear_cmtime(mapping) &&
+         mapping->a_ops->update_cmtime_deferred)
+             mapping->a_ops->update_cmtime_deferred(mapping);
+}
+EXPORT_SYMBOL(mapping_flush_cmtime);

Hum, is there a reason for update_cmtime_deferred() operation? I can
hardly imagine anyone will want to do anything else than what inode_update_time_writable() does so why bother? You mention tmpfs & co. don't fit into your scheme well with which I agree so let's just keep file_update_time() in their page_mkwrite() operation. But I don't see a real need for avoiding the deferred cmtime logic...

Quoted 2 lines

I think there might be odd corner cases. For example, mmap a tmpfs file, write it, and unmap it. Then, an hour later, maybe the system

If you unmap it then that will handle the update. But if you won't unmap,
you'd get spurious updates of timestamps which would be strange.

Quoted 4 lines

will be under memory pressure and page out the file. This could trigger a surprising time update. (I'm not sure this can actually happen on tmpfs, but maybe it would on some other filesystem.)

Quoted 3 lines

Does this actually matter? A flag to turn the feature on or off would do the trick, but I don't think there's precedent for sticking a flag in a_ops.

Flag in a_ops is ugly. But you can have a flag in 'struct
filesystem_type' which would be reasonable.

Quoted 45 lines

Quoted 23 lines

Quoted 23 lines

mm/page-writeback.c +22 -0

+void mapping_flush_cmtime_nowb(struct address_space *mapping)
+{
+     /*
+      * We get called from munmap and msync.  Both calls can race
+      * with fs freezing.  If the fs is frozen after
+      * mapping_test_clear_cmtime but before the time update, then
+      * sync_filesystem will miss the cmtime update (because we
+      * just cleared it) and we don't be able to write (because the
+      * fs is frozen).  On the other hand, we can't just return if
+      * we're in the SB_FREEZE_PAGEFAULT state because our caller
+      * expects the timestamp to be synchronously updated.  So we
+      * get write access without blocking, at the SB_FREEZE_FS
+      * level.  If the fs is already fully frozen, then we already
+      * know we have nothing to do.
+      */
+
+     if (!mapping_test_cmtime(mapping))
+             return;  /* Optimization: nothing to do. */
+
+     if (__sb_start_write(mapping->host->i_sb, SB_FREEZE_FS, false)) {
+             mapping_flush_cmtime(mapping);
+             __sb_end_write(mapping->host->i_sb, SB_FREEZE_FS);

Quoted 8 lines

Review int write_one_page(struct page *page, int wait) · line 2021

Quoted 2 lines

(quoted diff) +2 -0

+     }
+}

This is wrong because SB_FREEZE_FS level is targetted for filesystem
internal use. Also it is racy. mapping_flush_cmtime() ends up calling mark_inode_dirty() and filesystems such as ext4 or xfs will start a transaction to store inode in the journal. This gets freeze protection at SB_FREEZE_FS level again. If freeze_super() sets s_writers.frozen to SB_FREEZE_FS before this second protection, things will deadlock.

Whoops -- I assumed that it was safe to recursively take freeze protection at the same level.

I'm worried about the following race:

Thread 1 (in munmap):
Check AS_CMTIME set
sb_start_pagefault

Thread 2 (freezing the fs):
frozen = SB_FREEZE_PAGEFAULT;
sync_filesystem()

Quoted 3 lines

Thread 1 is now stuck. It doesn't need to be, because sync_filesystem will flush out the cmtime write. But there doesn't seem to be a clean mechanism to wait for the freeze to finish.

OK, I see. Frankly, I'd rather live with msync() and munmap() blocking
while filesystem is frozen than trying to outsmart the freezing logic... If someone comes up with a usecase where it causes trouble, we can always improve the logic with some clever tricks.

Writes via mmap currently update mtime and ctime in ->page_mkwrite.

Writes via mmap currently update mtime and ctime in ->page_mkwrite. This hurts both throughput and latency. In workloads that dirty a large number of mmapped pages, ->page_mkwrite can be hot and file_update_time is slow and scales poorly. Updating timestamps can also sleep, which hurts latency for real-time workloads.

This is also a correctness issue. SuS says:

The st_ctime and st_mtime fields of a file that is mapped with MAP_SHARED and PROT_WRITE, will be marked for update at some point in the interval between a write reference to the mapped region and the next call to msync() with MS_ASYNC or MS_SYNC for that portion of the file by any process. If there is no such call, these fields may be marked for update at any time after a write reference if the underlying file is modified as a result.

Currently, if the same mmapped page is written twice, the timestamp may not be update at all after the second write, whereas SuS (and anything using timestamps to invalidate caches, backup data, etc.) would expect the timestamp to eventually be updated.

This patchset attempts to fix both issues at once. It adds a new address_space flag AS_CMTIME that is set atomically whenever the system transfers a pte dirty bit to a struct page backed by the address_space. This can happen with various locks held and when low on memory.

Later on, a_ops.update_cmtime_deferred is called to tell the FS to update cmtime due to a previous mmapped write.

The core changes have no effect on unmodified filesystems. To opt in, a filesystem should implement .update_cmtime_deferred (most likely by using generic_update_cmtime_deferred) and must call either mapping_flush_cmtime or mapping_test_clear_cmtime in .writepages. Filesystems should avoid updating timestamps in ->page_mkwrite.

The reason that this is not completely automatic is that filesystems without backing stores do not really fit in to this model. Eventually, someone can add support.

I've converted ext4, xfs, and btrfs. Converting most other filesystems should be straightforward.

I wrote an xfstest for this. ext4, xfs, and btrfs pass. It's here:

https://github.com/amluto/xfstests/commit/5fbb72ac799cc44a9c4c6d3919f00a479202c899

This series is pullable from:

https://git.kernel.org/cgit/linux/kernel/git/luto/linux.git/log/?h=mmap_mtime/patch_v4

Changes from v3:
- The new address space op is now called update_cmtime_deferred. Callers take care of protection from fs freezing and checking AS_CMTIME. I fixed a deadlock in the freezer interaction.
- Block plugs should be handled better.
- Fixed an infinite loop in msync(MS_ASYNC).
- Converted xfs and btrfs.
- Misc minor cleanups.
- Fixed a corner case: reclaim or migration could have cleaned all pages without updating cmtime.

Changes from v2:
- The core code now interacts with filesystems only through address_space ops, so there should be fewer layering issues.
- MS_ASYNC is handled correctly.

Changes from v1:
- inode_update_time_writable now locks against the fs freezer.
- Minor cleanups.
- Major changelog improvements.

Andy Lutomirski (7):
mm: Track mappings that have been written via ptes
fs: Add inode_update_time_writable
mm: Allow filesystems to defer cmtime updates
mm: Scan for dirty ptes and update cmtime on MS_ASYNC
ext4: Defer mmap cmtime updates
btrfs: Defer mmap cmtime updates
xfs: Defer mmap cmtime updates

 fs/btrfs/extent_io.c      |  1 +
 fs/btrfs/inode.c          | 32 +++++++++---------
 fs/buffer.c               |  7 ----
 fs/ext4/inode.c           | 11 +++++--
 fs/inode.c                | 64 +++++++++++++++++++++++++++---------
 fs/xfs/xfs_aops.c         |  1 +
 include/linux/fs.h        |  9 +++++
 include/linux/pagemap.h   | 22 +++++++++++++
 include/linux/writeback.h |  1 +
 mm/memory.c               |  7 +++-
 mm/migrate.c              |  2 ++
 mm/mmap.c                 |  6 +++-
 mm/msync.c                | 84 ++++++++++++++++++++++++++++++++++++++++-------
 mm/page-writeback.c       | 53 +++++++++++++++++++++++++++++-
 mm/rmap.c                 | 27 +++++++++++++--
 mm/vmscan.c               |  1 +
 16 files changed, 272 insertions(+), 56 deletions(-)

Signature · 1 line

1.8.3.1

Writes via mmap currently update mtime and ctime in ->page_mkwrite. This hurts both throughput and latency. In workloads that dirty a large number of mmapped pages, ->page_mkwrite can be hot and file_update_time is slow and scales poorly. Updating timestamps can also sleep, which hurts latency for real-time workloads.

Quoted 5 lines · Andy Lutomirski

Writes via mmap currently update mtime and ctime in ->page_mkwrite. This hurts both throughput and latency. In workloads that dirty a large number of mmapped pages, ->page_mkwrite can be hot and file_update_time is slow and scales poorly. Updating timestamps can also sleep, which hurts latency for real-time workloads.

It would help to make your case if you posted the latency comparison
before & after the patchset in this introductory email. We can then see how significant is the reduction of latency...

Honza
--
Jan Kara <[email protected]>
SUSE Labs, CR