File timestamp related bugs

Here is a series of three patches that fix file timestamp related bugs:

Revert "[fs] gfs2: Only refresh newer in-memory timestamps"

It turns out that this commit didn't fix the actual bug
(https://bugzilla.redhat.com/show_bug.cgi?id=1304847), but only hid the effect of the bug. In addition, it failed to initialize the mtime and ctime for I_NEW inodes, resulting in timestamp corruption.

gfs2: Update file times after grabbing glock

This fixes the actual bug in
https://bugzilla.redhat.com/show_bug.cgi?id=1304847, and
https://bugzilla.redhat.com/show_bug.cgi?id=1374861 and
https://bugzilla.redhat.com/show_bug.cgi?id=1375682 as well.

gfs2: Initialize atime of I_NEW inodes

It turned out that the atime also isn't properly initialized for I_NEW inodes; this bug was introduced a long time ago, in
https://bugzilla.redhat.com/show_bug.cgi?id=462579.

Thanks,
Andreas

This reverts commit 9e0e3d6025feb402e8c01ecc65c00075795c8f9b.

This commit was meant to mitigate the effect of unsynchronized or badly synchronized clocks on different nodes on the mtime and ctime. However, it has turned out that the problem in this case (bz1304847) wasn't badly synchronized clocks but mtime and ctime updates that were getting lost when re-reading inodes from disk; see the next commit.

In addition, with commit 9e0e3d60, when an I_NEW in-memory inode is allocated, the atime, mtime, and ctime in the I_NEW inode are not initialized, and so they will end up with arbitrary values greater than the on-disk timestamps (bz1374861, bz1375682).

---
 fs/gfs2/inode.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

index cea85ce..f8f114f 100644
@@ -283,7 +283,7 @@ static void gfs2_set_nlink(struct inode *inode, u32 nlink)
 static int gfs2_dinode_in(struct gfs2_inode *ip, const void *buf)
 {
 	const struct gfs2_dinode *str = buf;
-	struct timespec footime;
+	struct timespec atime;
 	u16 height, depth;
 
 	if (unlikely(ip->i_no_addr != be64_to_cpu(str->di_num.no_addr)))

@@ -305,20 +305,14 @@ static int gfs2_dinode_in(struct gfs2_inode *ip, const void *buf)
 	ip->i_disksize = be64_to_cpu(str->di_size);
 	i_size_write(&ip->i_inode, ip->i_disksize);
 	gfs2_set_inode_blocks(&ip->i_inode, be64_to_cpu(str->di_blocks));
-	footime.tv_sec = be64_to_cpu(str->di_atime);
-	footime.tv_nsec = be32_to_cpu(str->di_atime_nsec);
-	if (timespec_compare(&ip->i_inode.i_atime, &footime) < 0)
-		ip->i_inode.i_atime = footime;
-
-	footime.tv_sec = be64_to_cpu(str->di_mtime);
-	footime.tv_nsec = be32_to_cpu(str->di_mtime_nsec);
-	if (timespec_compare(&ip->i_inode.i_mtime, &footime) < 0)
-		ip->i_inode.i_mtime = footime;
-
-	footime.tv_sec = be64_to_cpu(str->di_ctime);
-	footime.tv_nsec = be32_to_cpu(str->di_ctime_nsec);
-	if (timespec_compare(&ip->i_inode.i_ctime, &footime) < 0)
-		ip->i_inode.i_ctime = footime;
+	atime.tv_sec = be64_to_cpu(str->di_atime);
+	atime.tv_nsec = be32_to_cpu(str->di_atime_nsec);
+	if (timespec_compare(&ip->i_inode.i_atime, &atime) < 0)
+		ip->i_inode.i_atime = atime;
+	ip->i_inode.i_mtime.tv_sec = be64_to_cpu(str->di_mtime);
+	ip->i_inode.i_mtime.tv_nsec = be32_to_cpu(str->di_mtime_nsec);
+	ip->i_inode.i_ctime.tv_sec = be64_to_cpu(str->di_ctime);
+	ip->i_inode.i_ctime.tv_nsec = be32_to_cpu(str->di_ctime_nsec);
 
 	ip->i_goal = be64_to_cpu(str->di_goal_meta);
 	ip->i_generation = be64_to_cpu(str->di_generation);

2.7.4

On write code paths, the VFS usually takes care of updating the mtime and ctime of the inode. We need to grab the inode's glock in EX mode before the write can happen. When acquiring the glock in EX mode, the DLM may decide that the lock needs to be unlocked before being re-acquired in EX mode, which will cause the on-disk inode to be reread from disk. When that happens, the VFS's mtime and ctime update are lost. To fix that, make sure to update the mtime and ctime again once we hold the glock in EX mode.

---
 fs/gfs2/ops_address.c | 3 +++
 fs/gfs2/ops_file.c    | 1 +
 2 files changed, 4 insertions(+)

index f22ee46..7bb54ff 100644
@@ -700,6 +700,8 @@ int gfs2_write_begin(struct file *file, struct address_space *mapping,
 	error = gfs2_glock_nq(&ip->i_gh);
 	if (unlikely(error))
 		goto out_uninit;
+	file_update_time(file);
+
 	if (&ip->i_inode == sdp->sd_rindex) {
 		error = gfs2_glock_nq_init(m_ip->i_gl, LM_ST_EXCLUSIVE,
 					   GL_NOCACHE, &m_ip->i_gh);

@@ -1005,6 +1007,7 @@ static int gfs2_prepare_write(struct file *file, struct page *page,
 	error = gfs2_write_lock_start(ip, page, pos, write_len, &alloc_required);
 	if (error)
 		return error;
+	file_update_time(file);
 
 	if (gfs2_is_stuffed(ip)) {
 		if (end > sdp->sd_sb.sb_bsize - sizeof(struct gfs2_dinode)) {

index 3994484..2adefe6 100644
@@ -413,6 +413,7 @@ static int gfs2_page_mkwrite(struct vm_area_struct *vma, struct page *page)
 	ret = gfs2_glock_nq(&gh);
 	if (ret)
 		goto out;
+	file_update_time(vma->vm_file);
 
 	set_bit(GIF_SW_PAGED, &ip->i_flags);
 	ret = gfs2_write_alloc_required(ip, pos, PAGE_CACHE_SIZE, &alloc_required);

2.7.4

Fix for commit de6b6207: initialize atime of I_NEW inodes to 0 so that the timestamps read from disk will always be more recent than the initial timestamp, and the atime in the I_NEW inode will be overridden.

---
 fs/gfs2/inode.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

index f8f114f..3675a2cb 100644
@@ -191,8 +191,11 @@ struct inode *gfs2_inode_lookup(struct super_block *sb, unsigned int type,
 		}
 
 		gfs2_set_iop(inode);
-		unlock_new_inode(inode);
 
+		inode->i_atime.tv_sec = 0;
+		inode->i_atime.tv_nsec = 0;
+
+		unlock_new_inode(inode);
 	}
 
 	return inode;

2.7.4

----- Original Message -----

| Here is a series of three patches that fix file timestamp related bugs:
| 
| Revert "[fs] gfs2: Only refresh newer in-memory timestamps"
| 
|   It turns out that this commit didn't fix the actual bug
|   (https://bugzilla.redhat.com/show_bug.cgi?id=1304847), but only hid the
|   effect of the bug.  In addition, it failed to initialize the mtime and
|   ctime
|   for I_NEW inodes, resulting in timestamp corruption.
| 
| gfs2: Update file times after grabbing glock
| 
|   This fixes the actual bug in
|   https://bugzilla.redhat.com/show_bug.cgi?id=1304847, and
|   https://bugzilla.redhat.com/show_bug.cgi?id=1374861 and
|   https://bugzilla.redhat.com/show_bug.cgi?id=1375682 as well.
| 
| gfs2: Initialize atime of I_NEW inodes
| 
|   It turned out that the atime also isn't properly initialized for I_NEW
|   inodes; this bug was introduced a long time ago, in
|   https://bugzilla.redhat.com/show_bug.cgi?id=462579.
| 
| Thanks,
| Andreas

Hi Andreas,

This mailing list is normally only for upstream patches. Since the patch you're trying to revert is not in the upstream kernel, I can only assume this patch set is not for upstream. The concept looks good, but do you have patches for upstream?

Regards,

Bob Peterson
Red Hat File Systems

Right, silly me. Patch two and three is relevant to upstream as well, but not in the current form. So please forget about those patches for upstream for now.

Thanks,
Andreas