| Message ID | CACeaM8_zLWB_R3-kWWqGGk45ySbXEpb9J1f58adggoQ7Ovi-Ng@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jan 06, 2014 at 12:02:51AM +0100, Gerhard Heift wrote: > I am currently playing with snapshots and manual deduplication of > files. During these tests I noticed the change of ctime and mtime in > the snapshot after the deduplication with FILE_EXTENT_SAME. Does this > happens on purpose? Otherwise I would like to have ctime and mtime > left unmodified, because on a read only snapshot I cannot change them > back after the ioctl call. I'm not sure what's the correct behaviour wrt timestamps and extent cloning. The inode metadata are modified in some way, but the stat data and actual contents are left unchanged, so the timestamps do not reflect that something changed according to their definition (stat(2)). On the other hand, the differences can be seen in the extent listing, the physical offset of the blocks will change. I'm not aware of any tools that would become broken by breaking this assumption. Also, the (partial) cloning functionality is not implemented anywhere so we could have a look and try to stay consistent with that. My oppinion is to drop the mtime/iversion updates completely. david -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2014/1/6 David Sterba <dsterba@suse.cz>: > On Mon, Jan 06, 2014 at 12:02:51AM +0100, Gerhard Heift wrote: >> I am currently playing with snapshots and manual deduplication of >> files. During these tests I noticed the change of ctime and mtime in >> the snapshot after the deduplication with FILE_EXTENT_SAME. Does this >> happens on purpose? Otherwise I would like to have ctime and mtime >> left unmodified, because on a read only snapshot I cannot change them >> back after the ioctl call. > > I'm not sure what's the correct behaviour wrt timestamps and extent > cloning. The inode metadata are modified in some way, but the stat data > and actual contents are left unchanged, so the timestamps do not reflect > that something changed according to their definition (stat(2)). > > On the other hand, the differences can be seen in the extent listing, > the physical offset of the blocks will change. I'm not aware of any > tools that would become broken by breaking this assumption. Also, the > (partial) cloning functionality is not implemented anywhere so we could > have a look and try to stay consistent with that. > > My oppinion is to drop the mtime/iversion updates completely. In my opinion, we should never update, if we dedup content of files with FILE_EXTENT_SAME. If we clone with CLONE(_RANGE), the mtime should be updated, because its like a write operation. The semantics of ctime is not completely clear to me. It should change if the "visible" meta data of a file changes. In this cases should it be updated if write to it, because mtime changes, or only if the size of the file changes? > david Gerhard -- To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c index 9d46f60..975d207 100644 --- a/fs/btrfs/ioctl.c +++ b/fs/btrfs/ioctl.c @@ -59,7 +59,7 @@ #include "dev-replace.h" static int btrfs_clone(struct inode *src, struct inode *inode, - u64 off, u64 olen, u64 olen_aligned, u64 destoff); + u64 off, u64 olen, u64 olen_aligned, u64 destoff, int update_time); /* Mask out flags that are inappropriate for the given type of inode. */ static inline __u32 btrfs_mask_flags(umode_t mode, __u32 flags) @@ -2683,7 +2683,7 @@ static int btrfs_extent_same(struct inode *src, u64 loff, u64 len, ret = btrfs_cmp_data(src, loff, dst, dst_loff, len); if (ret == 0) - ret = btrfs_clone(src, dst, loff, len, len, dst_loff); + ret = btrfs_clone(src, dst, loff, len, len, dst_loff, /* update time */ 0); out_unlock: btrfs_double_unlock(src, loff, dst, dst_loff, len); @@ -2836,9 +2836,10 @@ out: * @olen_aligned: Block-aligned value of olen, extent_same uses * identical values here * @destoff: Offset within @inode to start clone + * @update_time: Should we update ctime and mtime of @inode? */ static int btrfs_clone(struct inode *src, struct inode *inode, - u64 off, u64 olen, u64 olen_aligned, u64 destoff) + u64 off, u64 olen, u64 olen_aligned, u64 destoff, int update_time) { struct btrfs_root *root = BTRFS_I(inode)->root; struct btrfs_path *path = NULL; @@ -3081,8 +3082,10 @@ static int btrfs_clone(struct inode *src, struct inode *inode, btrfs_mark_buffer_dirty(leaf); btrfs_release_path(path); - inode_inc_iversion(inode); - inode->i_mtime = inode->i_ctime = CURRENT_TIME; + if (update_time) { + inode_inc_iversion(inode); + inode->i_mtime = inode->i_ctime = CURRENT_TIME; + } /* * we round up to the block size at eof when @@ -3227,7 +3230,7 @@ static noinline long btrfs_ioctl_clone(struct file *file, unsigned long srcfd, lock_extent_range(src, off, len); - ret = btrfs_clone(src, inode, off, olen, len, destoff); + ret = btrfs_clone(src, inode, off, olen, len, destoff, /* update time */ 1); unlock_extent(&BTRFS_I(src)->io_tree, off, off + len - 1); out_unlock: