[PATCH 07/10] btrfs: fix wrong ctime when adding link

[PATCH 07/10] btrfs: fix wrong ctime when adding link

[Miao Xie]

the ctime of file has not been updated when I create a link for it.

Steps to reproduce:
 # touch file1
 # stat -c %Z file1
 1273592239
 # link flink1 file1
 # stat -c %Z file1
 1273592239             <-- have not been updated

This patch fix this problem.

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
---
 fs/btrfs/inode.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
index a85b90c..5271887 100644
--- a/fs/btrfs/inode.c
+++ b/fs/btrfs/inode.c
@@ -4218,8 +4218,12 @@ int btrfs_add_link(struct btrfs_trans_handle *trans,
 
 		btrfs_i_size_write(parent_inode, parent_inode->i_size +
 				   name_len * 2);
-		parent_inode->i_mtime = parent_inode->i_ctime = CURRENT_TIME;
-		ret = btrfs_update_inode(trans, root, parent_inode);
+		parent_inode->i_mtime = parent_inode->i_ctime = inode->i_ctime
+				      = CURRENT_TIME;
+
+		ret = btrfs_update_inode(trans, root, inode);
+		if (!ret)
+			ret = btrfs_update_inode(trans, root, parent_inode);
 	}
 	return ret;
 }
-- 
1.6.5.2

Re: [PATCH 07/10] btrfs: fix wrong ctime when adding link

[Christoph Hellwig]

On Thu, May 20, 2010 at 03:22:30PM +0800, Miao Xie wrote:
> the ctime of file has not been updated when I create a link for it.
> 
> Steps to reproduce:
>  # touch file1
>  # stat -c %Z file1
>  1273592239
>  # link flink1 file1
>  # stat -c %Z file1
>  1273592239             <-- have not been updated
> 
> This patch fix this problem.

Care to add a test to xfstests to check for this regression?

Re: [PATCH 07/10] btrfs: fix wrong ctime when adding link

[Shi Weihua]

cc xfstests ml

at 2010-5-20 16:33, Christoph Hellwig wrote:
> On Thu, May 20, 2010 at 03:22:30PM +0800, Miao Xie wrote:
>> the ctime of file has not been updated when I create a link for it.
>>
>> Steps to reproduce:
>>  # touch file1
>>  # stat -c %Z file1
>>  1273592239
>>  # link flink1 file1
>>  # stat -c %Z file1
>>  1273592239             <-- have not been updated
>>
>> This patch fix this problem.
> 
> Care to add a test to xfstests to check for this regression?

did it. 

Signed-off-by: Shi Weihua <shiwh@cn.fujitsu.com>
---
diff -urpN xfstests.orig/229 xfstests/229
--- xfstests.orig/229	1970-01-01 08:00:00.000000000 +0800
+++ xfstests/229	2010-05-28 11:27:14.000000000 +0800
@@ -0,0 +1,73 @@
+#! /bin/bash
+# FS QA Test No. 229
+#
+# Check ctime updated or not if file linked
+# See also http://marc.info/?l=linux-btrfs&m=127434439020230&w=2
+#
+#-----------------------------------------------------------------------
+# Copyright (c) 2010 FUJITSU LIMITED. All Rights Reserved.
+#
+# This program is free software; you can redistribute it and/or
+# modify it under the terms of the GNU General Public License as
+# published by the Free Software Foundation.
+#
+# This program is distributed in the hope that it would be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write the Free Software Foundation,
+# Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+#
+#-----------------------------------------------------------------------
+#
+# creator
+owner=shiwh@cn.fujitsu.com
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+_cleanup()
+{
+	if [ -a $TEST_DIR/ouch2 ]; then
+		rm -f $TEST_DIR/ouch2
+	fi
+	if [ -a $TEST_DIR/ouch ]; then
+		rm -f $TEST_DIR/ouch
+	fi
+}
+
+here=`pwd`
+
+# get standard environment, filters and checks
+. ./common.rc
+
+# real QA test starts here
+_supported_fs generic
+# only Linux supports fallocate
+_supported_os Linux
+
+# create a file and get its ctime
+touch $TEST_DIR/ouch
+ctime=`stat -c %Z $TEST_DIR/ouch`
+sleep 1
+
+# create a link to a file and get existing file's ctime 
+link $TEST_DIR/ouch $TEST_DIR/ouch2
+ctime2=`stat -c %Z $TEST_DIR/ouch`
+
+# check ctime updated
+if [ $ctime2 -le $ctime ]; then
+	echo "ctime: $ctime -> $ctime2 "
+	echo "Fatal error: ctime not updated after link"
+	_cleanup
+	exit 1
+fi
+
+_cleanup
+
+echo "Test over."
+# success, all done
+status=0
+exit
diff -urpN xfstests.orig/229.out xfstests/229.out
--- xfstests.orig/229.out	1970-01-01 08:00:00.000000000 +0800
+++ xfstests/229.out	2010-05-28 11:27:14.000000000 +0800
@@ -0,0 +1,2 @@
+QA output created by 229
+Test over.
diff -urpN xfstests.orig/group xfstests/group
--- xfstests.orig/group	2010-05-07 23:32:13.000000000 +0800
+++ xfstests/group	2010-05-28 11:27:02.000000000 +0800
@@ -342,3 +342,4 @@ deprecated
 226 auto enospc
 227 auto fsr
 228 rw auto prealloc quick
+229 auto

Re: [PATCH 07/10] btrfs: fix wrong ctime when adding link

[Mike Fedyk]

On Thu, May 20, 2010 at 12:22 AM, Miao Xie <miaox@cn.fujitsu.com> wrote=
:
> the ctime of file has not been updated when I create a link for it.
>
> Steps to reproduce:
> =C2=A0# touch file1
> =C2=A0# stat -c %Z file1
> =C2=A01273592239
> =C2=A0# link flink1 file1
> =C2=A0# stat -c %Z file1
> =C2=A01273592239 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 <-- have n=
ot been updated
>
> This patch fix this problem.
>
> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
> ---
> =C2=A0fs/btrfs/inode.c | =C2=A0 =C2=A08 ++++++--
> =C2=A01 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
> index a85b90c..5271887 100644
> --- a/fs/btrfs/inode.c
> +++ b/fs/btrfs/inode.c
> @@ -4218,8 +4218,12 @@ int btrfs_add_link(struct btrfs_trans_handle *=
trans,
>
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0btrfs_i_size_w=
rite(parent_inode, parent_inode->i_size +
> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 name_len * 2);
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 parent_inode->i_mt=
ime =3D parent_inode->i_ctime =3D CURRENT_TIME;
> - =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ret =3D btrfs_upda=
te_inode(trans, root, parent_inode);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 parent_inode->i_mt=
ime =3D parent_inode->i_ctime =3D inode->i_ctime
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =3D CURRENT_TI=
ME;
> +
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 ret =3D btrfs_upda=
te_inode(trans, root, inode);
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 if (!ret)
> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0=
 =C2=A0 ret =3D btrfs_update_inode(trans, root, parent_inode);

You only update parent inode if write to current inode fails?

Also should you be updating the ctime of parent inode even with link
count of parent inode is not modified (btrfs always reports link count
of 1 on directories)?
--
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

Re: [PATCH 07/10] btrfs: fix wrong ctime when adding link

[Miao Xie]

on 2010-5-20 17:01, Mike Fedyk wrote:
> On Thu, May 20, 2010 at 12:22 AM, Miao Xie <miaox@cn.fujitsu.com> wrote:
>> the ctime of file has not been updated when I create a link for it.
>>
>> Steps to reproduce:
>>  # touch file1
>>  # stat -c %Z file1
>>  1273592239
>>  # link flink1 file1
>>  # stat -c %Z file1
>>  1273592239             <-- have not been updated
>>
>> This patch fix this problem.
>>
>> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
>> ---
>>  fs/btrfs/inode.c |    8 ++++++--
>>  1 files changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c
>> index a85b90c..5271887 100644
>> --- a/fs/btrfs/inode.c
>> +++ b/fs/btrfs/inode.c
>> @@ -4218,8 +4218,12 @@ int btrfs_add_link(struct btrfs_trans_handle *trans,
>>
>>                btrfs_i_size_write(parent_inode, parent_inode->i_size +
>>                                   name_len * 2);
>> -               parent_inode->i_mtime = parent_inode->i_ctime = CURRENT_TIME;
>> -               ret = btrfs_update_inode(trans, root, parent_inode);
>> +               parent_inode->i_mtime = parent_inode->i_ctime = inode->i_ctime
>> +                                     = CURRENT_TIME;
>> +
>> +               ret = btrfs_update_inode(trans, root, inode);
>> +               if (!ret)
>> +                       ret = btrfs_update_inode(trans, root, parent_inode);
> 
> You only update parent inode if write to current inode fails?

I think if write to current inode fails, updating the parent inode is unnecessary,
it is better to do rollback.

> 
> Also should you be updating the ctime of parent inode even with link
> count of parent inode is not modified (btrfs always reports link count
> of 1 on directories)?

the i_size of the parent inode is changed, so we must update the ctime of parent inode.

Thanks