On Fri, Feb 3, 2017 at 8:08 PM, Darrick J. Wong <darrick.wong@xxxxxxxxxx> 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@xxxxxx>
>> > ---
>> > 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)
>
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
