Re: FILE_EXTENT_SAME changes mtime and ctime

2014-01-09 Thread Gerhard Heift 
2014/1/6 David Sterba dste...@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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: FILE_EXTENT_SAME changes mtime and ctime

2014-01-06 Thread David Sterba 
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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

FILE_EXTENT_SAME changes mtime and ctime

2014-01-05 Thread Gerhard Heift 
Hello,

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 attached a very basic patch, which illustrates my idea.

Thanks,
  Gerhard
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: