[PATCH] f2fs: remain written times to update inode during fsync

[PATCH] f2fs: remain written times to update inode during fsync

[Jaegeuk Kim]

This fixes xfstests/generic/392.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h  | 15 +++++++++++++++
 fs/f2fs/inode.c |  4 ++++
 2 files changed, 19 insertions(+)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 000f93f6767e..675c39d85111 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -664,6 +664,7 @@ struct f2fs_inode_info {
 	kprojid_t i_projid;		/* id for project quota */
 	int i_inline_xattr_size;	/* inline xattr size */
 	struct timespec i_crtime;	/* inode creation time */
+	struct timespec i_disk_time[4];	/* inode disk times */
 };
 
 static inline void get_extent_info(struct extent_info *ext,
@@ -2457,6 +2458,11 @@ static inline void clear_file(struct inode *inode, int type)
 	f2fs_mark_inode_dirty_sync(inode, true);
 }
 
+static inline bool time_equal(struct timespec a, struct timespec b)
+{
+	return (a.tv_sec == b.tv_sec) && (a.tv_nsec == b.tv_nsec);
+}
+
 static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
 {
 	bool ret;
@@ -2474,6 +2480,15 @@ static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
 			i_size_read(inode) & ~PAGE_MASK)
 		return false;
 
+	if (!time_equal(F2FS_I(inode)->i_disk_time[0], inode->i_atime))
+		return false;
+	if (!time_equal(F2FS_I(inode)->i_disk_time[1], inode->i_ctime))
+		return false;
+	if (!time_equal(F2FS_I(inode)->i_disk_time[2], inode->i_mtime))
+		return false;
+	if (!time_equal(F2FS_I(inode)->i_disk_time[3], F2FS_I(inode)->i_crtime))
+		return false;
+
 	down_read(&F2FS_I(inode)->i_sem);
 	ret = F2FS_I(inode)->last_disk_size == i_size_read(inode);
 	up_read(&F2FS_I(inode)->i_sem);
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 401f09ccce7e..70aba580f4b0 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -444,6 +444,10 @@ void update_inode(struct inode *inode, struct page *node_page)
 	if (inode->i_nlink == 0)
 		clear_inline_node(node_page);
 
+	F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
+	F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
+	F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
+	F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
 }
 
 void update_inode_page(struct inode *inode)
-- 
2.15.0.531.g2ccb3012c9-goog

Re: [PATCH] f2fs: remain written times to update inode during fsync

[Chao Yu]

Hi Jaegeuk,

On 2018/3/30 13:51, Jaegeuk Kim wrote:
> This fixes xfstests/generic/392.

Hmm... Could you please give more details about this issue and solution in
commit message, since I can catch up the solution only with the code.

Thanks,

> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/f2fs.h  | 15 +++++++++++++++
>  fs/f2fs/inode.c |  4 ++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 000f93f6767e..675c39d85111 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -664,6 +664,7 @@ struct f2fs_inode_info {
>  	kprojid_t i_projid;		/* id for project quota */
>  	int i_inline_xattr_size;	/* inline xattr size */
>  	struct timespec i_crtime;	/* inode creation time */
> +	struct timespec i_disk_time[4];	/* inode disk times */
>  };
>  
>  static inline void get_extent_info(struct extent_info *ext,
> @@ -2457,6 +2458,11 @@ static inline void clear_file(struct inode *inode, int type)
>  	f2fs_mark_inode_dirty_sync(inode, true);
>  }
>  
> +static inline bool time_equal(struct timespec a, struct timespec b)
> +{
> +	return (a.tv_sec == b.tv_sec) && (a.tv_nsec == b.tv_nsec);
> +}
> +
>  static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
>  {
>  	bool ret;
> @@ -2474,6 +2480,15 @@ static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
>  			i_size_read(inode) & ~PAGE_MASK)
>  		return false;
>  
> +	if (!time_equal(F2FS_I(inode)->i_disk_time[0], inode->i_atime))
> +		return false;
> +	if (!time_equal(F2FS_I(inode)->i_disk_time[1], inode->i_ctime))
> +		return false;
> +	if (!time_equal(F2FS_I(inode)->i_disk_time[2], inode->i_mtime))
> +		return false;
> +	if (!time_equal(F2FS_I(inode)->i_disk_time[3], F2FS_I(inode)->i_crtime))
> +		return false;
> +
>  	down_read(&F2FS_I(inode)->i_sem);
>  	ret = F2FS_I(inode)->last_disk_size == i_size_read(inode);
>  	up_read(&F2FS_I(inode)->i_sem);
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 401f09ccce7e..70aba580f4b0 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -444,6 +444,10 @@ void update_inode(struct inode *inode, struct page *node_page)
>  	if (inode->i_nlink == 0)
>  		clear_inline_node(node_page);
>  
> +	F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
> +	F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
> +	F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
> +	F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
>  }
>  
>  void update_inode_page(struct inode *inode)
> 

Re: [PATCH v2] f2fs: remain written times to update inode during fsync

[Jaegeuk Kim]

Change log from v1:
 - add more description

This fixes xfstests/generic/392.

The failure was caused by different times between 1) one marked in the last
fsync(2) call and 2) the other given by roll-forward recovery after power-cut.
The reason was that we skipped updating inode block at 1), since its i_size
was recoverable along with 4KB-aligned data writes, which was fixed by:
  "f2fs: fix a wrong condition in f2fs_skip_inode_update"

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h  | 15 +++++++++++++++
 fs/f2fs/inode.c |  4 ++++
 2 files changed, 19 insertions(+)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 000f93f6767e..675c39d85111 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -664,6 +664,7 @@ struct f2fs_inode_info {
 	kprojid_t i_projid;		/* id for project quota */
 	int i_inline_xattr_size;	/* inline xattr size */
 	struct timespec i_crtime;	/* inode creation time */
+	struct timespec i_disk_time[4];	/* inode disk times */
 };
 
 static inline void get_extent_info(struct extent_info *ext,
@@ -2457,6 +2458,11 @@ static inline void clear_file(struct inode *inode, int type)
 	f2fs_mark_inode_dirty_sync(inode, true);
 }
 
+static inline bool time_equal(struct timespec a, struct timespec b)
+{
+	return (a.tv_sec == b.tv_sec) && (a.tv_nsec == b.tv_nsec);
+}
+
 static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
 {
 	bool ret;
@@ -2474,6 +2480,15 @@ static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
 			i_size_read(inode) & ~PAGE_MASK)
 		return false;
 
+	if (!time_equal(F2FS_I(inode)->i_disk_time[0], inode->i_atime))
+		return false;
+	if (!time_equal(F2FS_I(inode)->i_disk_time[1], inode->i_ctime))
+		return false;
+	if (!time_equal(F2FS_I(inode)->i_disk_time[2], inode->i_mtime))
+		return false;
+	if (!time_equal(F2FS_I(inode)->i_disk_time[3], F2FS_I(inode)->i_crtime))
+		return false;
+
 	down_read(&F2FS_I(inode)->i_sem);
 	ret = F2FS_I(inode)->last_disk_size == i_size_read(inode);
 	up_read(&F2FS_I(inode)->i_sem);
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 401f09ccce7e..70aba580f4b0 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -444,6 +444,10 @@ void update_inode(struct inode *inode, struct page *node_page)
 	if (inode->i_nlink == 0)
 		clear_inline_node(node_page);
 
+	F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
+	F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
+	F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
+	F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
 }
 
 void update_inode_page(struct inode *inode)
-- 
2.15.0.531.g2ccb3012c9-goog

Re: [PATCH] f2fs: remain written times to update inode during fsync

[Jaegeuk Kim]

On 03/30, Chao Yu wrote:
> Hi Jaegeuk,
> 
> On 2018/3/30 13:51, Jaegeuk Kim wrote:
> > This fixes xfstests/generic/392.
> 
> Hmm... Could you please give more details about this issue and solution in
> commit message, since I can catch up the solution only with the code.

Yeah, I missed some explanation. :P

> 
> Thanks,
> 
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/f2fs.h  | 15 +++++++++++++++
> >  fs/f2fs/inode.c |  4 ++++
> >  2 files changed, 19 insertions(+)
> > 
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 000f93f6767e..675c39d85111 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -664,6 +664,7 @@ struct f2fs_inode_info {
> >  	kprojid_t i_projid;		/* id for project quota */
> >  	int i_inline_xattr_size;	/* inline xattr size */
> >  	struct timespec i_crtime;	/* inode creation time */
> > +	struct timespec i_disk_time[4];	/* inode disk times */
> >  };
> >  
> >  static inline void get_extent_info(struct extent_info *ext,
> > @@ -2457,6 +2458,11 @@ static inline void clear_file(struct inode *inode, int type)
> >  	f2fs_mark_inode_dirty_sync(inode, true);
> >  }
> >  
> > +static inline bool time_equal(struct timespec a, struct timespec b)
> > +{
> > +	return (a.tv_sec == b.tv_sec) && (a.tv_nsec == b.tv_nsec);
> > +}
> > +
> >  static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
> >  {
> >  	bool ret;
> > @@ -2474,6 +2480,15 @@ static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
> >  			i_size_read(inode) & ~PAGE_MASK)
> >  		return false;
> >  
> > +	if (!time_equal(F2FS_I(inode)->i_disk_time[0], inode->i_atime))
> > +		return false;
> > +	if (!time_equal(F2FS_I(inode)->i_disk_time[1], inode->i_ctime))
> > +		return false;
> > +	if (!time_equal(F2FS_I(inode)->i_disk_time[2], inode->i_mtime))
> > +		return false;
> > +	if (!time_equal(F2FS_I(inode)->i_disk_time[3], F2FS_I(inode)->i_crtime))
> > +		return false;
> > +
> >  	down_read(&F2FS_I(inode)->i_sem);
> >  	ret = F2FS_I(inode)->last_disk_size == i_size_read(inode);
> >  	up_read(&F2FS_I(inode)->i_sem);
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index 401f09ccce7e..70aba580f4b0 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -444,6 +444,10 @@ void update_inode(struct inode *inode, struct page *node_page)
> >  	if (inode->i_nlink == 0)
> >  		clear_inline_node(node_page);
> >  
> > +	F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
> > +	F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
> > +	F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
> > +	F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
> >  }
> >  
> >  void update_inode_page(struct inode *inode)
> > 

Re: [PATCH v2] f2fs: remain written times to update inode during fsync

[Chao Yu]

On 2018/3/31 0:30, Jaegeuk Kim wrote:
> Change log from v1:
>  - add more description
> 
> This fixes xfstests/generic/392.
> 
> The failure was caused by different times between 1) one marked in the last
> fsync(2) call and 2) the other given by roll-forward recovery after power-cut.
> The reason was that we skipped updating inode block at 1), since its i_size
> was recoverable along with 4KB-aligned data writes, which was fixed by:
>   "f2fs: fix a wrong condition in f2fs_skip_inode_update"
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> ---
>  fs/f2fs/f2fs.h  | 15 +++++++++++++++
>  fs/f2fs/inode.c |  4 ++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 000f93f6767e..675c39d85111 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -664,6 +664,7 @@ struct f2fs_inode_info {
>  	kprojid_t i_projid;		/* id for project quota */
>  	int i_inline_xattr_size;	/* inline xattr size */
>  	struct timespec i_crtime;	/* inode creation time */
> +	struct timespec i_disk_time[4];	/* inode disk times */
>  };
>  
>  static inline void get_extent_info(struct extent_info *ext,
> @@ -2457,6 +2458,11 @@ static inline void clear_file(struct inode *inode, int type)
>  	f2fs_mark_inode_dirty_sync(inode, true);
>  }
>  
> +static inline bool time_equal(struct timespec a, struct timespec b)
> +{
> +	return (a.tv_sec == b.tv_sec) && (a.tv_nsec == b.tv_nsec);
> +}

We can use existing timespec_equal in <linux/time.h> instead of defining a
duplicated one?

> +
>  static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
>  {
>  	bool ret;
> @@ -2474,6 +2480,15 @@ static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
>  			i_size_read(inode) & ~PAGE_MASK)
>  		return false;
>  
> +	if (!time_equal(F2FS_I(inode)->i_disk_time[0], inode->i_atime))
> +		return false;
> +	if (!time_equal(F2FS_I(inode)->i_disk_time[1], inode->i_ctime))
> +		return false;
> +	if (!time_equal(F2FS_I(inode)->i_disk_time[2], inode->i_mtime))
> +		return false;
> +	if (!time_equal(F2FS_I(inode)->i_disk_time[3], F2FS_I(inode)->i_crtime))
> +		return false;
> +
>  	down_read(&F2FS_I(inode)->i_sem);
>  	ret = F2FS_I(inode)->last_disk_size == i_size_read(inode);
>  	up_read(&F2FS_I(inode)->i_sem);
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 401f09ccce7e..70aba580f4b0 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -444,6 +444,10 @@ void update_inode(struct inode *inode, struct page *node_page)
>  	if (inode->i_nlink == 0)
>  		clear_inline_node(node_page);
>  
> +	F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
> +	F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
> +	F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
> +	F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;

We need initialize them in do_read_inode?

Thanks,

>  }
>  
>  void update_inode_page(struct inode *inode)
> 

Re: [f2fs-dev] [PATCH v3] f2fs: remain written times to update inode during fsync

[Jaegeuk Kim]

Change log from v2:
 - update do_read_inode

Change log from v1:
 - add more description

This fixes xfstests/generic/392.

The failure was caused by different times between 1) one marked in the last
fsync(2) call and 2) the other given by roll-forward recovery after power-cut.
The reason was that we skipped updating inode block at 1), since its i_size
was recoverable along with 4KB-aligned data writes, which was fixed by:
  "f2fs: fix a wrong condition in f2fs_skip_inode_update"

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h  | 15 +++++++++++++++
 fs/f2fs/inode.c |  8 ++++++++
 2 files changed, 23 insertions(+)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 7102d3965fea..33be61b19a5f 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -664,6 +664,7 @@ struct f2fs_inode_info {
 	kprojid_t i_projid;		/* id for project quota */
 	int i_inline_xattr_size;	/* inline xattr size */
 	struct timespec i_crtime;	/* inode creation time */
+	struct timespec i_disk_time[4];	/* inode disk times */
 };
 
 static inline void get_extent_info(struct extent_info *ext,
@@ -2457,6 +2458,11 @@ static inline void clear_file(struct inode *inode, int type)
 	f2fs_mark_inode_dirty_sync(inode, true);
 }
 
+static inline bool time_equal(struct timespec a, struct timespec b)
+{
+	return (a.tv_sec == b.tv_sec) && (a.tv_nsec == b.tv_nsec);
+}
+
 static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
 {
 	bool ret;
@@ -2474,6 +2480,15 @@ static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
 			i_size_read(inode) & ~PAGE_MASK)
 		return false;
 
+	if (!time_equal(F2FS_I(inode)->i_disk_time[0], inode->i_atime))
+		return false;
+	if (!time_equal(F2FS_I(inode)->i_disk_time[1], inode->i_ctime))
+		return false;
+	if (!time_equal(F2FS_I(inode)->i_disk_time[2], inode->i_mtime))
+		return false;
+	if (!time_equal(F2FS_I(inode)->i_disk_time[3], F2FS_I(inode)->i_crtime))
+		return false;
+
 	down_read(&F2FS_I(inode)->i_sem);
 	ret = F2FS_I(inode)->last_disk_size == i_size_read(inode);
 	up_read(&F2FS_I(inode)->i_sem);
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 401f09ccce7e..e0d9e8f27ed2 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -284,6 +284,10 @@ static int do_read_inode(struct inode *inode)
 		fi->i_crtime.tv_nsec = le32_to_cpu(ri->i_crtime_nsec);
 	}
 
+	F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
+	F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
+	F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
+	F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
 	f2fs_put_page(node_page, 1);
 
 	stat_inc_inline_xattr(inode);
@@ -444,6 +448,10 @@ void update_inode(struct inode *inode, struct page *node_page)
 	if (inode->i_nlink == 0)
 		clear_inline_node(node_page);
 
+	F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
+	F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
+	F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
+	F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
 }
 
 void update_inode_page(struct inode *inode)
-- 
2.15.0.531.g2ccb3012c9-goog

Re: [PATCH v2] f2fs: remain written times to update inode during fsync

[Jaegeuk Kim]

On 04/03, Chao Yu wrote:
> On 2018/3/31 0:30, Jaegeuk Kim wrote:
> > Change log from v1:
> >  - add more description
> > 
> > This fixes xfstests/generic/392.
> > 
> > The failure was caused by different times between 1) one marked in the last
> > fsync(2) call and 2) the other given by roll-forward recovery after power-cut.
> > The reason was that we skipped updating inode block at 1), since its i_size
> > was recoverable along with 4KB-aligned data writes, which was fixed by:
> >   "f2fs: fix a wrong condition in f2fs_skip_inode_update"
> > 
> > Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> > ---
> >  fs/f2fs/f2fs.h  | 15 +++++++++++++++
> >  fs/f2fs/inode.c |  4 ++++
> >  2 files changed, 19 insertions(+)
> > 
> > diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> > index 000f93f6767e..675c39d85111 100644
> > --- a/fs/f2fs/f2fs.h
> > +++ b/fs/f2fs/f2fs.h
> > @@ -664,6 +664,7 @@ struct f2fs_inode_info {
> >  	kprojid_t i_projid;		/* id for project quota */
> >  	int i_inline_xattr_size;	/* inline xattr size */
> >  	struct timespec i_crtime;	/* inode creation time */
> > +	struct timespec i_disk_time[4];	/* inode disk times */
> >  };
> >  
> >  static inline void get_extent_info(struct extent_info *ext,
> > @@ -2457,6 +2458,11 @@ static inline void clear_file(struct inode *inode, int type)
> >  	f2fs_mark_inode_dirty_sync(inode, true);
> >  }
> >  
> > +static inline bool time_equal(struct timespec a, struct timespec b)
> > +{
> > +	return (a.tv_sec == b.tv_sec) && (a.tv_nsec == b.tv_nsec);
> > +}
> 
> We can use existing timespec_equal in <linux/time.h> instead of defining a
> duplicated one?

It is defined with const parameters.

> 
> > +
> >  static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
> >  {
> >  	bool ret;
> > @@ -2474,6 +2480,15 @@ static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
> >  			i_size_read(inode) & ~PAGE_MASK)
> >  		return false;
> >  
> > +	if (!time_equal(F2FS_I(inode)->i_disk_time[0], inode->i_atime))
> > +		return false;
> > +	if (!time_equal(F2FS_I(inode)->i_disk_time[1], inode->i_ctime))
> > +		return false;
> > +	if (!time_equal(F2FS_I(inode)->i_disk_time[2], inode->i_mtime))
> > +		return false;
> > +	if (!time_equal(F2FS_I(inode)->i_disk_time[3], F2FS_I(inode)->i_crtime))
> > +		return false;
> > +
> >  	down_read(&F2FS_I(inode)->i_sem);
> >  	ret = F2FS_I(inode)->last_disk_size == i_size_read(inode);
> >  	up_read(&F2FS_I(inode)->i_sem);
> > diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> > index 401f09ccce7e..70aba580f4b0 100644
> > --- a/fs/f2fs/inode.c
> > +++ b/fs/f2fs/inode.c
> > @@ -444,6 +444,10 @@ void update_inode(struct inode *inode, struct page *node_page)
> >  	if (inode->i_nlink == 0)
> >  		clear_inline_node(node_page);
> >  
> > +	F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
> > +	F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
> > +	F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
> > +	F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
> 
> We need initialize them in do_read_inode?

Done.
Thanks,

> 
> Thanks,
> 
> >  }
> >  
> >  void update_inode_page(struct inode *inode)
> > 

Re: [PATCH v2] f2fs: remain written times to update inode during fsync

[Chao Yu]

On 2018/4/3 13:23, Jaegeuk Kim wrote:
> On 04/03, Chao Yu wrote:
>> On 2018/3/31 0:30, Jaegeuk Kim wrote:
>>> Change log from v1:
>>>  - add more description
>>>
>>> This fixes xfstests/generic/392.
>>>
>>> The failure was caused by different times between 1) one marked in the last
>>> fsync(2) call and 2) the other given by roll-forward recovery after power-cut.
>>> The reason was that we skipped updating inode block at 1), since its i_size
>>> was recoverable along with 4KB-aligned data writes, which was fixed by:
>>>   "f2fs: fix a wrong condition in f2fs_skip_inode_update"
>>>
>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>> ---
>>>  fs/f2fs/f2fs.h  | 15 +++++++++++++++
>>>  fs/f2fs/inode.c |  4 ++++
>>>  2 files changed, 19 insertions(+)
>>>
>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>> index 000f93f6767e..675c39d85111 100644
>>> --- a/fs/f2fs/f2fs.h
>>> +++ b/fs/f2fs/f2fs.h
>>> @@ -664,6 +664,7 @@ struct f2fs_inode_info {
>>>  	kprojid_t i_projid;		/* id for project quota */
>>>  	int i_inline_xattr_size;	/* inline xattr size */
>>>  	struct timespec i_crtime;	/* inode creation time */
>>> +	struct timespec i_disk_time[4];	/* inode disk times */
>>>  };
>>>  
>>>  static inline void get_extent_info(struct extent_info *ext,
>>> @@ -2457,6 +2458,11 @@ static inline void clear_file(struct inode *inode, int type)
>>>  	f2fs_mark_inode_dirty_sync(inode, true);
>>>  }
>>>  
>>> +static inline bool time_equal(struct timespec a, struct timespec b)
>>> +{
>>> +	return (a.tv_sec == b.tv_sec) && (a.tv_nsec == b.tv_nsec);
>>> +}
>>
>> We can use existing timespec_equal in <linux/time.h> instead of defining a
>> duplicated one?
> 
> It is defined with const parameters.

I didn't get it, const keyword can used to protect parameter not to be changed
in that function, that will be more safe, so it will be better to use that one?

Thanks,

> 
>>
>>> +
>>>  static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
>>>  {
>>>  	bool ret;
>>> @@ -2474,6 +2480,15 @@ static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
>>>  			i_size_read(inode) & ~PAGE_MASK)
>>>  		return false;
>>>  
>>> +	if (!time_equal(F2FS_I(inode)->i_disk_time[0], inode->i_atime))
>>> +		return false;
>>> +	if (!time_equal(F2FS_I(inode)->i_disk_time[1], inode->i_ctime))
>>> +		return false;
>>> +	if (!time_equal(F2FS_I(inode)->i_disk_time[2], inode->i_mtime))
>>> +		return false;
>>> +	if (!time_equal(F2FS_I(inode)->i_disk_time[3], F2FS_I(inode)->i_crtime))
>>> +		return false;
>>> +
>>>  	down_read(&F2FS_I(inode)->i_sem);
>>>  	ret = F2FS_I(inode)->last_disk_size == i_size_read(inode);
>>>  	up_read(&F2FS_I(inode)->i_sem);
>>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>>> index 401f09ccce7e..70aba580f4b0 100644
>>> --- a/fs/f2fs/inode.c
>>> +++ b/fs/f2fs/inode.c
>>> @@ -444,6 +444,10 @@ void update_inode(struct inode *inode, struct page *node_page)
>>>  	if (inode->i_nlink == 0)
>>>  		clear_inline_node(node_page);
>>>  
>>> +	F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
>>> +	F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
>>> +	F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
>>> +	F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
>>
>> We need initialize them in do_read_inode?
> 
> Done.
> Thanks,
> 
>>
>> Thanks,
>>
>>>  }
>>>  
>>>  void update_inode_page(struct inode *inode)
>>>
> 
> .
> 

Re: [PATCH v2] f2fs: remain written times to update inode during fsync

[Jaegeuk Kim]

On 04/03, Chao Yu wrote:
> On 2018/4/3 13:23, Jaegeuk Kim wrote:
> > On 04/03, Chao Yu wrote:
> >> On 2018/3/31 0:30, Jaegeuk Kim wrote:
> >>> Change log from v1:
> >>>  - add more description
> >>>
> >>> This fixes xfstests/generic/392.
> >>>
> >>> The failure was caused by different times between 1) one marked in the last
> >>> fsync(2) call and 2) the other given by roll-forward recovery after power-cut.
> >>> The reason was that we skipped updating inode block at 1), since its i_size
> >>> was recoverable along with 4KB-aligned data writes, which was fixed by:
> >>>   "f2fs: fix a wrong condition in f2fs_skip_inode_update"
> >>>
> >>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >>> ---
> >>>  fs/f2fs/f2fs.h  | 15 +++++++++++++++
> >>>  fs/f2fs/inode.c |  4 ++++
> >>>  2 files changed, 19 insertions(+)
> >>>
> >>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> >>> index 000f93f6767e..675c39d85111 100644
> >>> --- a/fs/f2fs/f2fs.h
> >>> +++ b/fs/f2fs/f2fs.h
> >>> @@ -664,6 +664,7 @@ struct f2fs_inode_info {
> >>>  	kprojid_t i_projid;		/* id for project quota */
> >>>  	int i_inline_xattr_size;	/* inline xattr size */
> >>>  	struct timespec i_crtime;	/* inode creation time */
> >>> +	struct timespec i_disk_time[4];	/* inode disk times */
> >>>  };
> >>>  
> >>>  static inline void get_extent_info(struct extent_info *ext,
> >>> @@ -2457,6 +2458,11 @@ static inline void clear_file(struct inode *inode, int type)
> >>>  	f2fs_mark_inode_dirty_sync(inode, true);
> >>>  }
> >>>  
> >>> +static inline bool time_equal(struct timespec a, struct timespec b)
> >>> +{
> >>> +	return (a.tv_sec == b.tv_sec) && (a.tv_nsec == b.tv_nsec);
> >>> +}
> >>
> >> We can use existing timespec_equal in <linux/time.h> instead of defining a
> >> duplicated one?
> > 
> > It is defined with const parameters.
> 
> I didn't get it, const keyword can used to protect parameter not to be changed
> in that function, that will be more safe, so it will be better to use that one?

Oh, I just made a mistake when testing timespec_equal().
I really need more recess time. :P

Change log from v3:
 - use timespec_equal()

This fixes xfstests/generic/392.

The failure was caused by different times between 1) one marked in the last
fsync(2) call and 2) the other given by roll-forward recovery after power-cut.
The reason was that we skipped updating inode block at 1), since its i_size
was recoverable along with 4KB-aligned data writes, which was fixed by:
  "f2fs: fix a wrong condition in f2fs_skip_inode_update"

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/f2fs.h  | 11 +++++++++++
 fs/f2fs/inode.c |  8 ++++++++
 2 files changed, 19 insertions(+)

diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 7102d3965fea..1df7f10476d6 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -664,6 +664,7 @@ struct f2fs_inode_info {
 	kprojid_t i_projid;		/* id for project quota */
 	int i_inline_xattr_size;	/* inline xattr size */
 	struct timespec i_crtime;	/* inode creation time */
+	struct timespec i_disk_time[4];	/* inode disk times */
 };
 
 static inline void get_extent_info(struct extent_info *ext,
@@ -2474,6 +2475,16 @@ static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
 			i_size_read(inode) & ~PAGE_MASK)
 		return false;
 
+	if (!timespec_equal(F2FS_I(inode)->i_disk_time, &inode->i_atime))
+		return false;
+	if (!timespec_equal(F2FS_I(inode)->i_disk_time + 1, &inode->i_ctime))
+		return false;
+	if (!timespec_equal(F2FS_I(inode)->i_disk_time + 2, &inode->i_mtime))
+		return false;
+	if (!timespec_equal(F2FS_I(inode)->i_disk_time + 3,
+						&F2FS_I(inode)->i_crtime))
+		return false;
+
 	down_read(&F2FS_I(inode)->i_sem);
 	ret = F2FS_I(inode)->last_disk_size == i_size_read(inode);
 	up_read(&F2FS_I(inode)->i_sem);
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index 401f09ccce7e..e0d9e8f27ed2 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -284,6 +284,10 @@ static int do_read_inode(struct inode *inode)
 		fi->i_crtime.tv_nsec = le32_to_cpu(ri->i_crtime_nsec);
 	}
 
+	F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
+	F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
+	F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
+	F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
 	f2fs_put_page(node_page, 1);
 
 	stat_inc_inline_xattr(inode);
@@ -444,6 +448,10 @@ void update_inode(struct inode *inode, struct page *node_page)
 	if (inode->i_nlink == 0)
 		clear_inline_node(node_page);
 
+	F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
+	F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
+	F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
+	F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
 }
 
 void update_inode_page(struct inode *inode)
-- 
2.15.0.531.g2ccb3012c9-goog

Re: [PATCH v2] f2fs: remain written times to update inode during fsync

[Chao Yu]

On 2018/4/4 0:08, Jaegeuk Kim wrote:
> On 04/03, Chao Yu wrote:
>> On 2018/4/3 13:23, Jaegeuk Kim wrote:
>>> On 04/03, Chao Yu wrote:
>>>> On 2018/3/31 0:30, Jaegeuk Kim wrote:
>>>>> Change log from v1:
>>>>>  - add more description
>>>>>
>>>>> This fixes xfstests/generic/392.
>>>>>
>>>>> The failure was caused by different times between 1) one marked in the last
>>>>> fsync(2) call and 2) the other given by roll-forward recovery after power-cut.
>>>>> The reason was that we skipped updating inode block at 1), since its i_size
>>>>> was recoverable along with 4KB-aligned data writes, which was fixed by:
>>>>>   "f2fs: fix a wrong condition in f2fs_skip_inode_update"
>>>>>
>>>>> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>>>>> ---
>>>>>  fs/f2fs/f2fs.h  | 15 +++++++++++++++
>>>>>  fs/f2fs/inode.c |  4 ++++
>>>>>  2 files changed, 19 insertions(+)
>>>>>
>>>>> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
>>>>> index 000f93f6767e..675c39d85111 100644
>>>>> --- a/fs/f2fs/f2fs.h
>>>>> +++ b/fs/f2fs/f2fs.h
>>>>> @@ -664,6 +664,7 @@ struct f2fs_inode_info {
>>>>>  	kprojid_t i_projid;		/* id for project quota */
>>>>>  	int i_inline_xattr_size;	/* inline xattr size */
>>>>>  	struct timespec i_crtime;	/* inode creation time */
>>>>> +	struct timespec i_disk_time[4];	/* inode disk times */
>>>>>  };
>>>>>  
>>>>>  static inline void get_extent_info(struct extent_info *ext,
>>>>> @@ -2457,6 +2458,11 @@ static inline void clear_file(struct inode *inode, int type)
>>>>>  	f2fs_mark_inode_dirty_sync(inode, true);
>>>>>  }
>>>>>  
>>>>> +static inline bool time_equal(struct timespec a, struct timespec b)
>>>>> +{
>>>>> +	return (a.tv_sec == b.tv_sec) && (a.tv_nsec == b.tv_nsec);
>>>>> +}
>>>>
>>>> We can use existing timespec_equal in <linux/time.h> instead of defining a
>>>> duplicated one?
>>>
>>> It is defined with const parameters.
>>
>> I didn't get it, const keyword can used to protect parameter not to be changed
>> in that function, that will be more safe, so it will be better to use that one?
> 
> Oh, I just made a mistake when testing timespec_equal().
> I really need more recess time. :P

Work is hard, right, we'd better keep having enough rest. :)

> 
> Change log from v3:
>  - use timespec_equal()
> 
> This fixes xfstests/generic/392.
> 
> The failure was caused by different times between 1) one marked in the last
> fsync(2) call and 2) the other given by roll-forward recovery after power-cut.
> The reason was that we skipped updating inode block at 1), since its i_size
> was recoverable along with 4KB-aligned data writes, which was fixed by:
>   "f2fs: fix a wrong condition in f2fs_skip_inode_update"
> 
> Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

> ---
>  fs/f2fs/f2fs.h  | 11 +++++++++++
>  fs/f2fs/inode.c |  8 ++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 7102d3965fea..1df7f10476d6 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -664,6 +664,7 @@ struct f2fs_inode_info {
>  	kprojid_t i_projid;		/* id for project quota */
>  	int i_inline_xattr_size;	/* inline xattr size */
>  	struct timespec i_crtime;	/* inode creation time */
> +	struct timespec i_disk_time[4];	/* inode disk times */
>  };
>  
>  static inline void get_extent_info(struct extent_info *ext,
> @@ -2474,6 +2475,16 @@ static inline bool f2fs_skip_inode_update(struct inode *inode, int dsync)
>  			i_size_read(inode) & ~PAGE_MASK)
>  		return false;
>  
> +	if (!timespec_equal(F2FS_I(inode)->i_disk_time, &inode->i_atime))
> +		return false;
> +	if (!timespec_equal(F2FS_I(inode)->i_disk_time + 1, &inode->i_ctime))
> +		return false;
> +	if (!timespec_equal(F2FS_I(inode)->i_disk_time + 2, &inode->i_mtime))
> +		return false;
> +	if (!timespec_equal(F2FS_I(inode)->i_disk_time + 3,
> +						&F2FS_I(inode)->i_crtime))
> +		return false;
> +
>  	down_read(&F2FS_I(inode)->i_sem);
>  	ret = F2FS_I(inode)->last_disk_size == i_size_read(inode);
>  	up_read(&F2FS_I(inode)->i_sem);
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index 401f09ccce7e..e0d9e8f27ed2 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -284,6 +284,10 @@ static int do_read_inode(struct inode *inode)
>  		fi->i_crtime.tv_nsec = le32_to_cpu(ri->i_crtime_nsec);
>  	}
>  
> +	F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
> +	F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
> +	F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
> +	F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
>  	f2fs_put_page(node_page, 1);
>  
>  	stat_inc_inline_xattr(inode);
> @@ -444,6 +448,10 @@ void update_inode(struct inode *inode, struct page *node_page)
>  	if (inode->i_nlink == 0)
>  		clear_inline_node(node_page);
>  
> +	F2FS_I(inode)->i_disk_time[0] = inode->i_atime;
> +	F2FS_I(inode)->i_disk_time[1] = inode->i_ctime;
> +	F2FS_I(inode)->i_disk_time[2] = inode->i_mtime;
> +	F2FS_I(inode)->i_disk_time[3] = F2FS_I(inode)->i_crtime;
>  }
>  
>  void update_inode_page(struct inode *inode)
>