[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 1/2] vfs: update overlay inode times on write

[Vivek Goyal]

On Tue, Jan 02, 2018 at 02:26:48PM +0200, Amir Goldstein wrote:
> 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);
> +

Hi Amir,

If we update overlay inode mtime, ctime in write path, then do we still
need this call in relatime_need_update() path? I mean what other path
is there where real inode mtime/ctime will be out of sync with overlay
inode mtime/ctime.

Vivek

>  	/*
>  	 * 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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-unionfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Amir Goldstein]

On Tue, Jan 2, 2018 at 6:51 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> On Tue, Jan 02, 2018 at 02:26:48PM +0200, Amir Goldstein wrote:
>> 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);
>> +
>
> Hi Amir,
>
> If we update overlay inode mtime, ctime in write path, then do we still
> need this call in relatime_need_update() path? I mean what other path
> is there where real inode mtime/ctime will be out of sync with overlay
> inode mtime/ctime.
>

Specifically, in the mentioned nfstest_posix test, i_mtime was not
updated on write of regular file and i_mtime of parent was not updated on link.
I guess there are more cases where i_mtime is updated not in the
write() path. I can think of punch hole for regular files and even file system
specific ioctls, which overlayfs is not aware of.
To handle those cases, I added the 2nd patch, which is "mtime reader
oriented", just like the relatime_need_update() case.

The problem is that AFAIK there is no simple place where all
i_mtime updates can be intercepted. Perhaps the recent work by Jeff
to sort out i_version readers/writers will provide a better place
to intercept all mtime updates?? At least for underlying filesystems that
support i_version, we could update overlay inode times only if i_version
was updated, but I don't even know what would be the best implementation
choice for relatime_need_update() in that case.

Amir.

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

[Jeff Layton]

On Tue, 2018-01-02 at 14:26 +0200, Amir Goldstein wrote:
> 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);

This code all seems to be called from the relatime handling codepath,
but the problem statement is all about the mtime.

Is there a reason for that, or would this be better done in (e.g.)
ovl_update_time? Is this code trying to delay updating times until
something is trying to access it?
--
Jeff Layton <jlayton@kernel.org>

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

[Amir Goldstein]

On Tue, Jan 2, 2018 at 8:48 PM, Jeff Layton <jlayton@kernel.org> wrote:
> On Tue, 2018-01-02 at 14:26 +0200, Amir Goldstein wrote:
>> 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);
>
> This code all seems to be called from the relatime handling codepath,
> but the problem statement is all about the mtime.

See, currently update_ovl_inode_times() purpose is to update overlay
inode before the mtime reader relatime_need_update() access i_mtime.
Any other access to i_mtime (e.g. lease_get_mtime()) gets a non-update
version of i_mtime, because nobody bothers to call update_ovl_inode_times().

>
> Is there a reason for that, or would this be better done in (e.g.)
> ovl_update_time? Is this code trying to delay updating times until
> something is trying to access it?

There is no delaying requirement. The problem statement is that with
overlayfs there are 2 inodes in play, The overlay inode and the real underlying
inode. After an overlay regular file is open, file_inode(file) points
to the underlying real inode,
so write() only knowns about the real underlying inode and i_mtime is typically
only ever updated on real underlying inode. ovl_update_time() is thus
never called
on the write() path.

locks_inode(file) := file->f_path.dentry->d_inode is the overlay
inode, which is the
one used throughout locks.c, so the patch above calls  update_ovl_inode_times()
from file_update_time() with the overlay inode to copy i_mtime from
real to overlay
inode.

Patch 2/2, which seems like the better option to me, calls
update_ovl_inode_times()
from the i_mtime reader lease_get_mtime() before i_mtime access.

Is this making more sense to you?

Amir.

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

[Dave Chinner]

On Tue, Jan 02, 2018 at 07:27:41PM +0200, Amir Goldstein wrote:
> On Tue, Jan 2, 2018 at 6:51 PM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > On Tue, Jan 02, 2018 at 02:26:48PM +0200, Amir Goldstein wrote:
> >> 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);
> >> +
> >
> > Hi Amir,
> >
> > If we update overlay inode mtime, ctime in write path, then do we still
> > need this call in relatime_need_update() path? I mean what other path
> > is there where real inode mtime/ctime will be out of sync with overlay
> > inode mtime/ctime.
> >
> 
> Specifically, in the mentioned nfstest_posix test, i_mtime was not
> updated on write of regular file and i_mtime of parent was not updated on link.
> I guess there are more cases where i_mtime is updated not in the
> write() path. I can think of punch hole for regular files and even file system
> specific ioctls, which overlayfs is not aware of.
> To handle those cases, I added the 2nd patch, which is "mtime reader
> oriented", just like the relatime_need_update() case.
> 
> The problem is that AFAIK there is no simple place where all
> i_mtime updates can be intercepted. Perhaps the recent work by Jeff
> to sort out i_version readers/writers will provide a better place
> to intercept all mtime updates?? At least for underlying filesystems that
> support i_version, we could update overlay inode times only if i_version
> was updated, but I don't even know what would be the best implementation
> choice for relatime_need_update() in that case.

You might be able to intercept m/ctime changes at notify_change(),
but you are not going to be able to catch all timestamp changes
because many of them are deep inside filesystem specific
implementations of various functions. e.g. see all the internal XFS
calls to xfs_trans_ichgtime() to change inode timestamps during
various modifications because they are conditional on the
modifications those operations make to the filesystem and hence they
can't be specified/directed by the VFS....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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.

[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

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.