[PATCH v2] xfs: update ctime and mtime on clone destinatation inodes

[PATCH v2] xfs: update ctime and mtime on clone destinatation inodes

[Christoph Hellwig]

We're changing both metadata and data, so we need to update the
timestamps for clone operations.  Dedupe on the other hand does
not change file data, and only changes invisible metadata so the
timestamps should not be updated.

This follows existing btrfs behavior.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 fs/xfs/xfs_reflink.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
index d5a2cf2b469b..199ce0100bc6 100644
--- a/fs/xfs/xfs_reflink.c
+++ b/fs/xfs/xfs_reflink.c
@@ -961,13 +961,15 @@ STATIC int
 xfs_reflink_update_dest(
 	struct xfs_inode	*dest,
 	xfs_off_t		newlen,
-	xfs_extlen_t		cowextsize)
+	xfs_extlen_t		cowextsize,
+	bool			is_dedupe)
 {
 	struct xfs_mount	*mp = dest->i_mount;
 	struct xfs_trans	*tp;
 	int			error;
 
-	if (newlen <= i_size_read(VFS_I(dest)) && cowextsize == 0)
+	if (is_dedupe &&
+	    newlen <= i_size_read(VFS_I(dest)) && cowextsize == 0 && is_dedupe)
 		return 0;
 
 	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
@@ -988,6 +990,10 @@ xfs_reflink_update_dest(
 		dest->i_d.di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
 	}
 
+	if (!is_dedupe) {
+		xfs_trans_ichgtime(tp, dest,
+				   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+	}
 	xfs_trans_log_inode(tp, dest, XFS_ILOG_CORE);
 
 	error = xfs_trans_commit(tp);
@@ -1301,7 +1307,8 @@ xfs_reflink_remap_range(
 	    !(dest->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE))
 		cowextsize = src->i_d.di_cowextsize;
 
-	ret = xfs_reflink_update_dest(dest, pos_out + len, cowextsize);
+	ret = xfs_reflink_update_dest(dest, pos_out + len, cowextsize,
+			is_dedupe);
 
 out_unlock:
 	xfs_iunlock(src, XFS_MMAPLOCK_EXCL);
-- 
2.11.0

Re: [PATCH v2] xfs: update ctime and mtime on clone destinatation inodes

[Darrick J. Wong]

[cc amir since he piped up about the v1 patch]

On Fri, Feb 03, 2017 at 10:57:15AM +0100, Christoph Hellwig wrote:
> We're changing both metadata and data, so we need to update the
> timestamps for clone operations.  Dedupe on the other hand does
> not change file data, and only changes invisible metadata so the
> timestamps should not be updated.
> 
> This follows existing btrfs behavior.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  fs/xfs/xfs_reflink.c | 13 ++++++++++---
>  1 file changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> index d5a2cf2b469b..199ce0100bc6 100644
> --- a/fs/xfs/xfs_reflink.c
> +++ b/fs/xfs/xfs_reflink.c
> @@ -961,13 +961,15 @@ STATIC int
>  xfs_reflink_update_dest(
>  	struct xfs_inode	*dest,
>  	xfs_off_t		newlen,
> -	xfs_extlen_t		cowextsize)
> +	xfs_extlen_t		cowextsize,
> +	bool			is_dedupe)
>  {
>  	struct xfs_mount	*mp = dest->i_mount;
>  	struct xfs_trans	*tp;
>  	int			error;
>  
> -	if (newlen <= i_size_read(VFS_I(dest)) && cowextsize == 0)
> +	if (is_dedupe &&
> +	    newlen <= i_size_read(VFS_I(dest)) && cowextsize == 0 && is_dedupe)
>  		return 0;

Redundant is_dedupe tests here.

I also wonder if we really /want/ to emulate the existing btrfs
behavior, which seems to be:

reflink: update mtime & ctime
dedupe: do not update mtime or ctime

In particular, dedupe changes the inode metadata, which should qualify
for a ctime update, right?

--D

>  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
> @@ -988,6 +990,10 @@ xfs_reflink_update_dest(
>  		dest->i_d.di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
>  	}
>  
> +	if (!is_dedupe) {
> +		xfs_trans_ichgtime(tp, dest,
> +				   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> +	}
>  	xfs_trans_log_inode(tp, dest, XFS_ILOG_CORE);
>  
>  	error = xfs_trans_commit(tp);
> @@ -1301,7 +1307,8 @@ xfs_reflink_remap_range(
>  	    !(dest->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE))
>  		cowextsize = src->i_d.di_cowextsize;
>  
> -	ret = xfs_reflink_update_dest(dest, pos_out + len, cowextsize);
> +	ret = xfs_reflink_update_dest(dest, pos_out + len, cowextsize,
> +			is_dedupe);
>  
>  out_unlock:
>  	xfs_iunlock(src, XFS_MMAPLOCK_EXCL);
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs: update ctime and mtime on clone destinatation inodes

[Darrick J. Wong]

On Fri, Feb 03, 2017 at 09:45:41AM -0800, Darrick J. Wong wrote:
> [cc amir since he piped up about the v1 patch]
> 
> On Fri, Feb 03, 2017 at 10:57:15AM +0100, Christoph Hellwig wrote:
> > We're changing both metadata and data, so we need to update the
> > timestamps for clone operations.  Dedupe on the other hand does
> > not change file data, and only changes invisible metadata so the
> > timestamps should not be updated.
> > 
> > This follows existing btrfs behavior.
> > 
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
> > ---
> >  fs/xfs/xfs_reflink.c | 13 ++++++++++---
> >  1 file changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
> > index d5a2cf2b469b..199ce0100bc6 100644
> > --- a/fs/xfs/xfs_reflink.c
> > +++ b/fs/xfs/xfs_reflink.c
> > @@ -961,13 +961,15 @@ STATIC int
> >  xfs_reflink_update_dest(
> >  	struct xfs_inode	*dest,
> >  	xfs_off_t		newlen,
> > -	xfs_extlen_t		cowextsize)
> > +	xfs_extlen_t		cowextsize,
> > +	bool			is_dedupe)
> >  {
> >  	struct xfs_mount	*mp = dest->i_mount;
> >  	struct xfs_trans	*tp;
> >  	int			error;
> >  
> > -	if (newlen <= i_size_read(VFS_I(dest)) && cowextsize == 0)
> > +	if (is_dedupe &&
> > +	    newlen <= i_size_read(VFS_I(dest)) && cowextsize == 0 && is_dedupe)
> >  		return 0;
> 
> Redundant is_dedupe tests here.
> 
> I also wonder if we really /want/ to emulate the existing btrfs
> behavior, which seems to be:
> 
> reflink: update mtime & ctime
> dedupe: do not update mtime or ctime
> 
> In particular, dedupe changes the inode metadata, which should qualify
> for a ctime update, right?

OTOH, /me fishes out the discussion between mfasheh & zygo where they
discuss changing btrfs to avoid [cm]time updates on dedupe:

https://patchwork.kernel.org/patch/6683161/

So... <shrug> I'm willing to preserve the 'dedupe doesn't update
timestamps' behavior even though we're definitely changing di_nextents
(and possibly di_cowextsize).

(Though, you may note, I didn't write any code to update the timestamps
in the original patches, so clearly I'm not all that passionate either
way. :P)

--D

> 
> --D
> 
> >  	error = xfs_trans_alloc(mp, &M_RES(mp)->tr_ichange, 0, 0, 0, &tp);
> > @@ -988,6 +990,10 @@ xfs_reflink_update_dest(
> >  		dest->i_d.di_flags2 |= XFS_DIFLAG2_COWEXTSIZE;
> >  	}
> >  
> > +	if (!is_dedupe) {
> > +		xfs_trans_ichgtime(tp, dest,
> > +				   XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> > +	}
> >  	xfs_trans_log_inode(tp, dest, XFS_ILOG_CORE);
> >  
> >  	error = xfs_trans_commit(tp);
> > @@ -1301,7 +1307,8 @@ xfs_reflink_remap_range(
> >  	    !(dest->i_d.di_flags2 & XFS_DIFLAG2_COWEXTSIZE))
> >  		cowextsize = src->i_d.di_cowextsize;
> >  
> > -	ret = xfs_reflink_update_dest(dest, pos_out + len, cowextsize);
> > +	ret = xfs_reflink_update_dest(dest, pos_out + len, cowextsize,
> > +			is_dedupe);
> >  
> >  out_unlock:
> >  	xfs_iunlock(src, XFS_MMAPLOCK_EXCL);
> > -- 
> > 2.11.0
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] xfs: update ctime and mtime on clone destinatation inodes

[Amir Goldstein]

On Fri, Feb 3, 2017 at 8:08 PM, Darrick J. Wong <darrick.wong@oracle.com> wrote:
> On Fri, Feb 03, 2017 at 09:45:41AM -0800, Darrick J. Wong wrote:
>> [cc amir since he piped up about the v1 patch]
>>
>> On Fri, Feb 03, 2017 at 10:57:15AM +0100, Christoph Hellwig wrote:
>> > We're changing both metadata and data, so we need to update the
>> > timestamps for clone operations.  Dedupe on the other hand does
>> > not change file data, and only changes invisible metadata so the
>> > timestamps should not be updated.
>> >
>> > This follows existing btrfs behavior.
>> >
>> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>> > ---
>> >  fs/xfs/xfs_reflink.c | 13 ++++++++++---
>> >  1 file changed, 10 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/fs/xfs/xfs_reflink.c b/fs/xfs/xfs_reflink.c
>> > index d5a2cf2b469b..199ce0100bc6 100644
>> > --- a/fs/xfs/xfs_reflink.c
>> > +++ b/fs/xfs/xfs_reflink.c
>> > @@ -961,13 +961,15 @@ STATIC int
>> >  xfs_reflink_update_dest(
>> >     struct xfs_inode        *dest,
>> >     xfs_off_t               newlen,
>> > -   xfs_extlen_t            cowextsize)
>> > +   xfs_extlen_t            cowextsize,
>> > +   bool                    is_dedupe)
>> >  {
>> >     struct xfs_mount        *mp = dest->i_mount;
>> >     struct xfs_trans        *tp;
>> >     int                     error;
>> >
>> > -   if (newlen <= i_size_read(VFS_I(dest)) && cowextsize == 0)
>> > +   if (is_dedupe &&
>> > +       newlen <= i_size_read(VFS_I(dest)) && cowextsize == 0 && is_dedupe)
>> >             return 0;
>>
>> Redundant is_dedupe tests here.
>>
>> I also wonder if we really /want/ to emulate the existing btrfs
>> behavior, which seems to be:
>>
>> reflink: update mtime & ctime
>> dedupe: do not update mtime or ctime
>>
>> In particular, dedupe changes the inode metadata, which should qualify
>> for a ctime update, right?
>
> OTOH, /me fishes out the discussion between mfasheh & zygo where they
> discuss changing btrfs to avoid [cm]time updates on dedupe:
>
> https://patchwork.kernel.org/patch/6683161/
>
> So... <shrug> I'm willing to preserve the 'dedupe doesn't update
> timestamps' behavior even though we're definitely changing di_nextents
> (and possibly di_cowextsize).
>

I wasn't writing code, or talking at all, when ctime was conceived,
but I believe di_nextents and di_cowextsize are not what ctime
is about. To my understanding, because user can change mtime,
ctime keeps a more reliable record about last time inode "metadata"
was changed, where "metadata" is the rest of the records visible in stat().
Since dedup does not change any of that POSIX metadata -
no ctime change seems reasonable to me.

> (Though, you may note, I didn't write any code to update the timestamps
> in the original patches, so clearly I'm not all that passionate either
> way. :P)
>

Re: [PATCH v2] xfs: update ctime and mtime on clone destinatation inodes

[Christoph Hellwig]

On Fri, Feb 03, 2017 at 09:45:41AM -0800, Darrick J. Wong wrote:
> Redundant is_dedupe tests here.
> 
> I also wonder if we really /want/ to emulate the existing btrfs
> behavior, which seems to be:
> 
> reflink: update mtime & ctime
> dedupe: do not update mtime or ctime
> 
> In particular, dedupe changes the inode metadata, which should qualify
> for a ctime update, right?

That depends on your metadata definition.  For Posix it's basically just
about stat data, and that doesn't change with dedupe.