ext4: update donor file's ctime/mtime

PATCH In review Kazuya Mio 3 participants 5 messages Oct 8, 2009 – Oct 14, 2009 ext4 file system

lore

Share & tools

lore

Kazuya Mio

author

2009-10-08 08:07 UTC #0 lore raw

PATCH 2/2 ext4: update donor file's ctime/mtime

EXT4_IOC_MOVE_EXT changes donor file data, but doesn't update ctime/mtime. This patch fixes this problem.

Signed-off-by Kazuya Mio <[email protected]>

---

 move_extent.c |   20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

fs/ext4/move_extent.c +20 -0

index e2e99fd..1cb2609 100644
@@ -1210,6 +1210,7 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
 	ext4_lblk_t block_end, seq_start, add_blocks, file_end, seq_blocks = 0;
 	ext4_lblk_t rest_blocks;
 	pgoff_t orig_page_offset = 0, seq_end_page;
+	handle_t *handle;
 	int ret1, ret2, depth, last_extent = 0;
 	int blocks_per_page = PAGE_CACHE_SIZE >> orig_inode->i_blkbits;
 	int data_offset_in_page;

@@ -1406,6 +1407,25 @@ ext4_move_extents(struct file *o_filp, struct file *d_filp,
 
 	}
 out:
+	/*
+	 * Note that ctime/mtime will not be updated if power failure is
+	 * detected during block replacing.
+	 */
+	if (*moved_len) {
+		handle = ext4_journal_start(donor_inode, 1);
+		if (IS_ERR(handle))
+			ret2 = PTR_ERR(handle);
+		else {
+			donor_inode->i_ctime = donor_inode->i_mtime =
+					ext4_current_time(donor_inode);
+			ret2 = ext4_mark_inode_dirty(handle, donor_inode);
+			ext4_journal_stop(handle);
+		}
+
+		if (!ret1)
+			ret1 = ret2;
+	}
+
 	if (orig_path) {
 		ext4_ext_drop_refs(orig_path);
 		kfree(orig_path);

Andreas Dilger

2009-10-09 17:21 UTC #1 lore raw

Quoted 2 lines · Kazuya Mio

EXT4_IOC_MOVE_EXT changes donor file data, but doesn't update ctime/mtime. This patch fixes this problem.

I would argue that just migrating the file data shouldn't update the ctime/mtime. Those are used to determine if the file has changed in some way, usually for the purpose of backup. Migrating the data does not change anything from the user-space POV and shouldn't force a new backup of the file.

Kazuya Mio

author

2009-10-14 01:55 UTC #2 lore raw

2009/10/10 2:20, Andreas Dilger wrote::

Quoted 7 lines · Kazuya Mio

Quoted 4 lines
EXT4_IOC_MOVE_EXT changes donor file data, but doesn't update ctime/mtime.
This patch fixes this problem.

Quoted 7 lines

I would argue that just migrating the file data shouldn't update the ctime/mtime.
Those are used to determine if the file has changed in some way, usually for the
purpose of backup. Migrating the data does not change anything from the user-space
POV and shouldn't force a new backup of the file.

EXT4_IOC_MOVE_EXT always changes the original actual contents of donor file if orig file and donor file aren't the same. It may be that some of user-space implementations hide such a changing. For example, e4defrag unlinks the donor file, and removes it by decreasing reference count after calling EXT4_IOC_MOVE_EXT. But from the ioctl point of view, EXT4_IOC_MOVE_EXT doesn't know whether donor file will be removed or not, so I think we should update ctime/mtime.

Am I missing something?

Regards,
Kazuya Mio

Andreas Dilger

2009-10-14 18:11 UTC #3 lore raw

Quoted 10 lines · Kazuya Mio

2009/10/10 2:20, Andreas Dilger wrote::
Quoted 8 lines · Kazuya Mio
Quoted 3 lines
EXT4_IOC_MOVE_EXT changes donor file data, but doesn't update ctime/mtime. This patch fixes this problem.
I would argue that just migrating the file data shouldn't update the
ctime/mtime. Those are used to determine if the file has changed in some way, usually for the purpose of backup. Migrating the data does not change anything from user-space POV and shouldn't force a new backup of the file.

Quoted 7 lines

EXT4_IOC_MOVE_EXT always changes the original actual contents of donor file if orig file and donor file aren't the same. It may be that some of user-space implementations hide such a changing. For example, e4defrag unlinks the donor file, and removes it by decreasing reference count after calling EXT4_IOC_MOVE_EXT. But from the ioctl point of view, EXT4_IOC_MOVE_EXT doesn't know whether donor file will be removed or not, so I think we should update ctime/mtime.

Maybe I am confused. Is EXT4_IOC_MOVE_EXT used for file defragmentation? I thought the goal of this ioctl was to copy the data from the donor inode to the target inode (using a new allocation in the target inode), and then once the whole donor file had been copied (defragmented) the target extents replace the entire donor file's extents?

In this model it would be OK to change the mtime/ctime of the _target_ file, but when these extents move back to the donor file the mtime/ctime of the donor file should not be changed, I think, so that it does not force a full backup.

If the caller has done something to change the actual data in the donor file it can always use utimes() to update the ctime/mtime, but it is not possible for userspace to revert the ctime after it has changed.

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

Greg Freemyer

2009-10-14 21:20 UTC #4 lore raw

Quoted 39 lines · Andreas Dilger

Quoted 13 lines · Kazuya Mio
2009/10/10 2:20, Andreas Dilger wrote::
Quoted 10 lines · Kazuya Mio
Quoted 3 lines
EXT4_IOC_MOVE_EXT changes donor file data, but doesn't update ctime/mtime. This patch fixes this problem.
I would argue that just migrating the file data shouldn't update the ctime/mtime. Those are used to determine if the file has changed in some way, usually for the purpose of backup. Migrating the data does not change anything from user-space POV and shouldn't force a new backup of the file.
Quoted 8 lines
EXT4_IOC_MOVE_EXT always changes the original actual contents of donor file
if orig file and donor file aren't the same. It may be that some of user-space implementations hide such a changing. For example, e4defrag unlinks the donor file, and removes it by decreasing reference count after calling EXT4_IOC_MOVE_EXT. But from the ioctl point of view, EXT4_IOC_MOVE_EXT doesn't know whether donor file will be removed or not, so I think we should update ctime/mtime.
Maybe I am confused. Is EXT4_IOC_MOVE_EXT used for file defragmentation? I thought the goal of this ioctl was to copy the data from the donor inode to the target inode (using a new allocation in the target inode), and then once the whole donor file had been copied (defragmented) the target extents replace the entire donor file's extents?
In this model it would be OK to change the mtime/ctime of the _target_ file, but when these extents move back to the donor file the mtime/ctime of the donor
file should not be changed, I think, so that it does not force a full backup.
If the caller has done something to change the actual data in the donor file it can always use utimes() to update the ctime/mtime, but it is not possible for userspace to revert the ctime after it has changed.

Quoted 5 lines

Cheers, Andreas
--
Andreas Dilger
Sr. Staff Engineer, Lustre Group
Sun Microsystems of Canada, Inc.

You have it backwards.

The target file is the fragmented file containing good data. ie. It is the target of the defragmentation process.

The donor file starts out as fallocated file with no valid data in it, but the data blocks are less fragmented than the target hopefully.

The target file ends up using the data blocks provided by the donor, but with the target files original data in them.

What the donor has at the end of the process, I don't know for sure. I assume it is a zero length file that is in need of being deleted. Having the donor's timestamps seems technically correct, but of little real world consequence. Or maybe there will eventually be a use case that reuses the donor file for some purpose.

Greg
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html