[RFC][PATCH 0/2] Updating overlay inode i_mtime for nfsd

[RFC][PATCH 0/2] Updating overlay inode i_mtime for nfsd

[Amir Goldstein]

Jeff, Bruce,

These RFC patches fix an issue I found when running nfstest_posix
over NFS exported overlayfs.

I am posting these separately from (soon to be posted) overlayfs NFS
export support patches to get your inputs about the preferred way for
a fix.

The 1st patch is a proposal to fix overlay inode i_mtime on inode
modification. This fix is incomplete w.r.t nfstest_posix test failures.
The 2nd patch more or less reverts the 1st patch in favor of updating
overlay inode i_mtime only when nfsd would care. At least as far as
nfstest_posix is concerned this fix is complete.

I lean towards the 2nd fix, but wasn't sure, so posting both suggestions.
Another option would be to call vfs_getattr() from lease_get_mtime(),
which will get the correct i_mtime for overlayfs (upper inode i_mtime),
but wasn't sure if this is a viable option for nfsd.

Would appreciate your thoughts about the issue and about the proposed fix.

Thanks,
Amir.

Amir Goldstein (2):
  vfs: update overlay inode times on write
  vfs: update overlay inode times on lease_get_mtime()

 fs/inode.c    | 15 ++++++++++++---
 fs/internal.h |  1 +
 fs/locks.c    | 12 +++++++++++-
 3 files changed, 24 insertions(+), 4 deletions(-)

-- 
2.7.4

[RFC][PATCH 1/2] vfs: update overlay inode times on write

[Amir Goldstein]

Currently with overlayfs, the real upper inode's i_mtime is updated on
write, but not overlay vfs inode. The vfs inode's i_mtime is used by nfsd
to check if nfs client cache is stale, so updating the overlay vfs inode
i_mtime on write is required for overlayfs NFS export support.

The non uptodate mtime issue was found and verified with the
nfstest_posix test when run over NFS exported overlayfs:

 $ nfstest_posix --runtest=write
 ...
 FAIL: write - file st_mtime should be updated

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/inode.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index 03102d6ef044..a252256f4e51 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1567,8 +1567,7 @@ EXPORT_SYMBOL(bmap);
 /*
  * Update times in overlayed inode from underlying real inode
  */
-static void update_ovl_inode_times(struct dentry *dentry, struct inode *inode,
-			       bool rcu)
+static void update_ovl_d_inode_times(struct dentry *dentry, bool rcu)
 {
 	struct dentry *upperdentry;
 
@@ -1585,6 +1584,7 @@ static void update_ovl_inode_times(struct dentry *dentry, struct inode *inode,
 	 * stale mtime/ctime.
 	 */
 	if (upperdentry) {
+		struct inode *inode = d_inode(dentry);
 		struct inode *realinode = d_inode(upperdentry);
 
 		if ((!timespec_equal(&inode->i_mtime, &realinode->i_mtime) ||
@@ -1607,7 +1607,8 @@ static int relatime_need_update(const struct path *path, struct inode *inode,
 	if (!(path->mnt->mnt_flags & MNT_RELATIME))
 		return 1;
 
-	update_ovl_inode_times(path->dentry, inode, rcu);
+	update_ovl_d_inode_times(path->dentry, rcu);
+
 	/*
 	 * Is mtime younger than atime? If yes, update atime:
 	 */
@@ -1876,6 +1877,8 @@ int file_update_time(struct file *file)
 	ret = update_time(inode, &now, sync_it);
 	__mnt_drop_write_file(file);
 
+	update_ovl_d_inode_times(file->f_path.dentry, false);
+
 	return ret;
 }
 EXPORT_SYMBOL(file_update_time);
-- 
2.7.4

[RFC][PATCH 2/2] vfs: update overlay inode times on lease_get_mtime()

[Amir Goldstein]

Instead of looking for all the places that update inode's m_time,
update overlay inode i_mtime just before nfsd needs to read it.

The non uptodate mtime issue was found and verified with the
nfstest_posix test when run over NFS exported overlayfs:

 $ nfstest_posix --runtest=link,write
 ...
 FAIL: link - parent directory st_mtime should be updated
 FAIL: write - file st_mtime should be updated

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/inode.c    | 10 ++++++++--
 fs/internal.h |  1 +
 fs/locks.c    | 12 +++++++++++-
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index a252256f4e51..ceb5f5cb9c79 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1595,6 +1595,14 @@ static void update_ovl_d_inode_times(struct dentry *dentry, bool rcu)
 	}
 }
 
+void update_ovl_inode_times(struct inode *inode)
+{
+	struct dentry *alias = d_find_any_alias(inode);
+
+	if (alias)
+		update_ovl_d_inode_times(alias, false);
+}
+
 /*
  * With relative atime, only update atime if the previous atime is
  * earlier than either the ctime or mtime or if at least a day has
@@ -1877,8 +1885,6 @@ int file_update_time(struct file *file)
 	ret = update_time(inode, &now, sync_it);
 	__mnt_drop_write_file(file);
 
-	update_ovl_d_inode_times(file->f_path.dentry, false);
-
 	return ret;
 }
 EXPORT_SYMBOL(file_update_time);
diff --git a/fs/internal.h b/fs/internal.h
index df262f41a0ef..a631fd49a1ee 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -122,6 +122,7 @@ extern long prune_icache_sb(struct super_block *sb, struct shrink_control *sc);
 extern void inode_add_lru(struct inode *inode);
 extern int dentry_needs_remove_privs(struct dentry *dentry);
 
+extern void update_ovl_inode_times(struct inode *inode);
 extern bool __atime_needs_update(const struct path *, struct inode *, bool);
 static inline bool atime_needs_update_rcu(const struct path *path,
 					  struct inode *inode)
diff --git a/fs/locks.c b/fs/locks.c
index 21b4dfa289ee..e2f604839aaf 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -127,6 +127,7 @@
 #include <linux/pid_namespace.h>
 #include <linux/hashtable.h>
 #include <linux/percpu.h>
+#include "internal.h"
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/filelock.h>
@@ -1553,6 +1554,15 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
 
 EXPORT_SYMBOL(__break_lease);
 
+static struct timespec inode_mtime(struct inode *inode)
+{
+	/* TODO: use another SB_ flag and/or pass dentry to lease_get_mtime() */
+	if (unlikely(inode->i_sb->s_flags & MS_NOREMOTELOCK))
+		update_ovl_inode_times(inode);
+
+	return inode->i_mtime;
+}
+
 /**
  *	lease_get_mtime - get the last modified time of an inode
  *	@inode: the inode
@@ -1581,7 +1591,7 @@ void lease_get_mtime(struct inode *inode, struct timespec *time)
 	if (has_lease)
 		*time = current_time(inode);
 	else
-		*time = inode->i_mtime;
+		*time = inode_mtime(inode);
 }
 
 EXPORT_SYMBOL(lease_get_mtime);
-- 
2.7.4

Re: [RFC][PATCH 0/2] Updating overlay inode i_mtime for nfsd

[Amir Goldstein]

On Wed, Jan 3, 2018 at 5:11 PM, Amir Goldstein <amir73il@gmail.com> wrote:
> Jeff, Bruce,
>
> These RFC patches fix an issue I found when running nfstest_posix
> over NFS exported overlayfs.
>

Wrong version posted. Sorry for the noise.

Amir.

[RFC][PATCH 0/2] Updating overlay inode i_mtime for nfsd

[Amir Goldstein]

Jeff, Bruce,

These RFC patches fix an issue I found when running nfstest_posix
over NFS exported overlayfs.

I am posting these separately from (soon to be posted) overlayfs NFS
export support patches to get your inputs about the preferred way for
a fix.

The 1st patch is a proposal to fix overlay inode i_mtime on inode
modification. This fix is incomplete w.r.t nfstest_posix test failures.
The 2nd patch more or less reverts the 1st patch in favor of updating
overlay inode i_mtime only when nfsd would care. At least as far as
nfstest_posix is concerned this fix is complete.

I lean towards the 2nd fix, but wasn't sure, so posting both suggestions.
Another option would be to call vfs_getattr() from lease_get_mtime(),
which will get the correct i_mtime for overlayfs (upper inode i_mtime),
but wasn't sure if this is a viable option for nfsd.

Would appreciate your thoughts about the issue and about the proposed fix.

Thanks,
Amir.

Amir Goldstein (2):
  vfs: update overlay inode times on write
  vfs: update overlay inode times on lease_get_mtime()

 fs/inode.c    | 15 ++++++++++++---
 fs/internal.h |  1 +
 fs/locks.c    | 12 +++++++++++-
 3 files changed, 24 insertions(+), 4 deletions(-)

-- 
2.7.4

[RFC][PATCH 2/2] vfs: update overlay inode times on lease_get_mtime()

[Amir Goldstein]

Instead of looking for all the places that update inode's m_time,
update overlay inode i_mtime just before nfsd needs to read it.

The non uptodate mtime issue was found and verified with the
nfstest_posix test when run over NFS exported overlayfs:

 $ nfstest_posix --runtest=link,write
 ...
 FAIL: link - parent directory st_mtime should be updated
 FAIL: write - file st_mtime should be updated

Signed-off-by: Amir Goldstein <amir73il@gmail.com>
---
 fs/inode.c    | 10 ++++++++--
 fs/internal.h |  1 +
 fs/locks.c    | 12 +++++++++++-
 3 files changed, 20 insertions(+), 3 deletions(-)

diff --git a/fs/inode.c b/fs/inode.c
index a252256f4e51..ceb5f5cb9c79 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1595,6 +1595,14 @@ static void update_ovl_d_inode_times(struct dentry *dentry, bool rcu)
 	}
 }
 
+void update_ovl_inode_times(struct inode *inode)
+{
+	struct dentry *alias = d_find_any_alias(inode);
+
+	if (alias)
+		update_ovl_d_inode_times(alias, false);
+}
+
 /*
  * With relative atime, only update atime if the previous atime is
  * earlier than either the ctime or mtime or if at least a day has
@@ -1877,8 +1885,6 @@ int file_update_time(struct file *file)
 	ret = update_time(inode, &now, sync_it);
 	__mnt_drop_write_file(file);
 
-	update_ovl_d_inode_times(file->f_path.dentry, false);
-
 	return ret;
 }
 EXPORT_SYMBOL(file_update_time);
diff --git a/fs/internal.h b/fs/internal.h
index df262f41a0ef..a631fd49a1ee 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -122,6 +122,7 @@ extern long prune_icache_sb(struct super_block *sb, struct shrink_control *sc);
 extern void inode_add_lru(struct inode *inode);
 extern int dentry_needs_remove_privs(struct dentry *dentry);
 
+extern void update_ovl_inode_times(struct inode *inode);
 extern bool __atime_needs_update(const struct path *, struct inode *, bool);
 static inline bool atime_needs_update_rcu(const struct path *path,
 					  struct inode *inode)
diff --git a/fs/locks.c b/fs/locks.c
index 21b4dfa289ee..e2f604839aaf 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -127,6 +127,7 @@
 #include <linux/pid_namespace.h>
 #include <linux/hashtable.h>
 #include <linux/percpu.h>
+#include "internal.h"
 
 #define CREATE_TRACE_POINTS
 #include <trace/events/filelock.h>
@@ -1553,6 +1554,15 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
 
 EXPORT_SYMBOL(__break_lease);
 
+static struct timespec inode_mtime(struct inode *inode)
+{
+	/* TODO: use another SB_ flag and/or pass dentry to lease_get_mtime() */
+	if (unlikely(inode->i_sb->s_flags & MS_NOREMOTELOCK))
+		update_ovl_inode_times(inode);
+
+	return inode->i_mtime;
+}
+
 /**
  *	lease_get_mtime - get the last modified time of an inode
  *	@inode: the inode
@@ -1581,7 +1591,7 @@ void lease_get_mtime(struct inode *inode, struct timespec *time)
 	if (has_lease)
 		*time = current_time(inode);
 	else
-		*time = inode->i_mtime;
+		*time = inode_mtime(inode);
 }
 
 EXPORT_SYMBOL(lease_get_mtime);
-- 
2.7.4

Re: [RFC][PATCH 2/2] vfs: update overlay inode times on lease_get_mtime()

[J . Bruce Fields]

On Tue, Jan 02, 2018 at 02:26:49PM +0200, Amir Goldstein wrote:
> Instead of looking for all the places that update inode's m_time,
> update overlay inode i_mtime just before nfsd needs to read it.
> 
> The non uptodate mtime issue was found and verified with the
> nfstest_posix test when run over NFS exported overlayfs:
> 
>  $ nfstest_posix --runtest=link,write
>  ...
>  FAIL: link - parent directory st_mtime should be updated
>  FAIL: write - file st_mtime should be updated

Grepping for users of i_mtime in fs/nfsd/....

Looks like we might need something similar for fill_pre_wcc() ?  (And
maybe nfsd4_block_commit_blocks() ?)

I don't think it's necessary for do_nfsd_create(), but it should be
harmless if we want to just make it a rule that nfsd shouldn't be
accessing i_mtime directly.

--b.

> 
> Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> ---
>  fs/inode.c    | 10 ++++++++--
>  fs/internal.h |  1 +
>  fs/locks.c    | 12 +++++++++++-
>  3 files changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/inode.c b/fs/inode.c
> index a252256f4e51..ceb5f5cb9c79 100644
> --- a/fs/inode.c
> +++ b/fs/inode.c
> @@ -1595,6 +1595,14 @@ static void update_ovl_d_inode_times(struct dentry *dentry, bool rcu)
>  	}
>  }
>  
> +void update_ovl_inode_times(struct inode *inode)
> +{
> +	struct dentry *alias = d_find_any_alias(inode);
> +
> +	if (alias)
> +		update_ovl_d_inode_times(alias, false);
> +}
> +
>  /*
>   * With relative atime, only update atime if the previous atime is
>   * earlier than either the ctime or mtime or if at least a day has
> @@ -1877,8 +1885,6 @@ int file_update_time(struct file *file)
>  	ret = update_time(inode, &now, sync_it);
>  	__mnt_drop_write_file(file);
>  
> -	update_ovl_d_inode_times(file->f_path.dentry, false);
> -
>  	return ret;
>  }
>  EXPORT_SYMBOL(file_update_time);
> diff --git a/fs/internal.h b/fs/internal.h
> index df262f41a0ef..a631fd49a1ee 100644
> --- a/fs/internal.h
> +++ b/fs/internal.h
> @@ -122,6 +122,7 @@ extern long prune_icache_sb(struct super_block *sb, struct shrink_control *sc);
>  extern void inode_add_lru(struct inode *inode);
>  extern int dentry_needs_remove_privs(struct dentry *dentry);
>  
> +extern void update_ovl_inode_times(struct inode *inode);
>  extern bool __atime_needs_update(const struct path *, struct inode *, bool);
>  static inline bool atime_needs_update_rcu(const struct path *path,
>  					  struct inode *inode)
> diff --git a/fs/locks.c b/fs/locks.c
> index 21b4dfa289ee..e2f604839aaf 100644
> --- a/fs/locks.c
> +++ b/fs/locks.c
> @@ -127,6 +127,7 @@
>  #include <linux/pid_namespace.h>
>  #include <linux/hashtable.h>
>  #include <linux/percpu.h>
> +#include "internal.h"
>  
>  #define CREATE_TRACE_POINTS
>  #include <trace/events/filelock.h>
> @@ -1553,6 +1554,15 @@ int __break_lease(struct inode *inode, unsigned int mode, unsigned int type)
>  
>  EXPORT_SYMBOL(__break_lease);
>  
> +static struct timespec inode_mtime(struct inode *inode)
> +{
> +	/* TODO: use another SB_ flag and/or pass dentry to lease_get_mtime() */
> +	if (unlikely(inode->i_sb->s_flags & MS_NOREMOTELOCK))
> +		update_ovl_inode_times(inode);
> +
> +	return inode->i_mtime;
> +}
> +
>  /**
>   *	lease_get_mtime - get the last modified time of an inode
>   *	@inode: the inode
> @@ -1581,7 +1591,7 @@ void lease_get_mtime(struct inode *inode, struct timespec *time)
>  	if (has_lease)
>  		*time = current_time(inode);
>  	else
> -		*time = inode->i_mtime;
> +		*time = inode_mtime(inode);
>  }
>  
>  EXPORT_SYMBOL(lease_get_mtime);
> -- 
> 2.7.4

Re: [RFC][PATCH 2/2] vfs: update overlay inode times on lease_get_mtime()

[Amir Goldstein]

On Wed, Jan 3, 2018 at 12:54 AM, J . Bruce Fields <bfields@fieldses.org> wrote:
> On Tue, Jan 02, 2018 at 02:26:49PM +0200, Amir Goldstein wrote:
>> Instead of looking for all the places that update inode's m_time,
>> update overlay inode i_mtime just before nfsd needs to read it.
>>
>> The non uptodate mtime issue was found and verified with the
>> nfstest_posix test when run over NFS exported overlayfs:
>>
>>  $ nfstest_posix --runtest=link,write
>>  ...
>>  FAIL: link - parent directory st_mtime should be updated
>>  FAIL: write - file st_mtime should be updated
>
> Grepping for users of i_mtime in fs/nfsd/....
>
> Looks like we might need something similar for fill_pre_wcc() ?  (And
> maybe nfsd4_block_commit_blocks() ?)
>
> I don't think it's necessary for do_nfsd_create(), but it should be
> harmless if we want to just make it a rule that nfsd shouldn't be
> accessing i_mtime directly.
>

All right. I'll do that.
While at it, do you think it will be fine to call vfs_getattr() from a helper
nfsd_get_inode_mtime() instead of having to special case overlayfs?
Or is there some reason why accessing i_mtime is a requirement?

Thanks,
Amir.

Re: [RFC][PATCH 2/2] vfs: update overlay inode times on lease_get_mtime()

[Amir Goldstein]

On Wed, Jan 3, 2018 at 8:48 AM, Amir Goldstein <amir73il@gmail.com> wrote:
> On Wed, Jan 3, 2018 at 12:54 AM, J . Bruce Fields <bfields@fieldses.org> wrote:
>> On Tue, Jan 02, 2018 at 02:26:49PM +0200, Amir Goldstein wrote:
>>> Instead of looking for all the places that update inode's m_time,
>>> update overlay inode i_mtime just before nfsd needs to read it.
>>>
>>> The non uptodate mtime issue was found and verified with the
>>> nfstest_posix test when run over NFS exported overlayfs:
>>>
>>>  $ nfstest_posix --runtest=link,write
>>>  ...
>>>  FAIL: link - parent directory st_mtime should be updated
>>>  FAIL: write - file st_mtime should be updated
>>
>> Grepping for users of i_mtime in fs/nfsd/....
>>
>> Looks like we might need something similar for fill_pre_wcc() ?  (And
>> maybe nfsd4_block_commit_blocks() ?)
>>
>> I don't think it's necessary for do_nfsd_create(), but it should be
>> harmless if we want to just make it a rule that nfsd shouldn't be
>> accessing i_mtime directly.
>>
>
> All right. I'll do that.
> While at it, do you think it will be fine to call vfs_getattr() from a helper
> nfsd_get_inode_mtime() instead of having to special case overlayfs?
> Or is there some reason why accessing i_mtime is a requirement?
>

Looking closer... in the callers of lease_get_mtime() vfs_getattr() is
already called before accessing i_mtime, but stat->mtime is ignored,
so I will change lease_get_mtime() to only *update* mtime for the
has_lease case and leave it at the stat->mtime value otherwise.

Regarding fill_pre_wcc() it is quite unbalanced that fill_pre_wcc()
reads i_mtime, while fill_post_wcc() reads stat->mtime.
I will change fill_pre_wcc() to use stat->mtime as well.

Regarding nfsd4_block_commit_blocks(), there is no concern with
respect to overlayfs and I have no means to test it, so I rather leave it be.

Patches to follow...

Thanks for your inputs,
Amir.