Modifying XATTRs doesnt change the ctime #6586

gaurkuma · 2017-08-31T00:15:49Z

Changing any metadata, should modify the ctime.

Description

Default Behaviour
root# stat test.log
File: `test.log'
Size: 32 Blocks: 9 IO Block: 512 regular file
Device: 18h/24d Inode: 128 Links: 1
Access: (0600/-rw-------) Uid: ( 0/ root) Gid: ( 0/ root)
Access: 2017-08-29 23:28:04.926223299 -0700
Modify: 2017-08-29 21:51:24.495046598 -0700
Change: 2017-08-29 21:51:24.495046598 -0700

root# setfattr -n user.name1 -v "val1" test.log
root# stat test.log
File: `test.log'
Size: 32 Blocks: 9 IO Block: 512 regular file
Device: 18h/24d Inode: 128 Links: 1
Access: (0600/-rw-------) Uid: ( 0/ root) Gid: ( 0/ root)
Access: 2017-08-29 23:28:04.926223299 -0700
Modify: 2017-08-29 21:51:24.495046598 -0700
Change: 2017-08-29 21:51:24.495046598 -0700

This patch updates the ctime when xattr is set. Currently the patch only takes care of the case where xattr=sa is set as the property.

Result:

root# stat test.log
File: `test.log'
Size: 64 Blocks: 8 IO Block: 4096 regular file
Device: 801h/2049d Inode: 425987 Links: 1
Access: (0600/-rw-------) Uid: ( 0/ root) Gid: ( 0/ root)
Access: 2017-08-30 16:52:05.519465323 -0700
Modify: 2017-08-30 16:52:05.519465323 -0700
Change: 2017-08-30 16:52:05.519465323 -0700

root# setfattr -n user.name1 -v "val1" test.log
root# stat test.log
File: `test.log'
Size: 64 Blocks: 8 IO Block: 4096 regular file
Device: 801h/2049d Inode: 425987 Links: 1
Access: (0600/-rw-------) Uid: ( 0/ root) Gid: ( 0/ root)
Access: 2017-08-30 16:52:05.519465323 -0700
Modify: 2017-08-30 16:52:05.519465323 -0700
Change: 2017-08-30 16:52:33.220226152 -0700

Motivation and Context

How Has This Been Tested?

Types of changes

[x ] Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Performance enhancement (non-breaking change which improves efficiency)
Code cleanup (non-breaking change which makes code smaller or more readable)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation (a change to man pages or other documentation)

Checklist:

[ x] My code follows the ZFS on Linux code style requirements.
I have updated the documentation accordingly.
[ x] I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.
All commit messages are properly formatted and contain Signed-off-by.
Change has been approved by a ZFS on Linux member.

behlendorf

Thanks! This is most of the needed fix for #3644. Here's what's still needed.

Adding code for the directory xattr case as you mentioned in the description. You'll want to add the needed code to zpl_xattr_set_dir(). While it would be ideal to get the update in the same TX the code's not really laid out to make this possible.

Instead I would suggest adding the following lines in the success path. It will then get properly written via __mark_inode_dirty()->zpl_dirty_inode()->zfs_dirty_inode().

ip->i_ctime = current_time(ip);
zfs_mark_inode_dirty(ip);

Test coverage. The ctime_001_pos.c test case should be updated to modify an xattr and verify the ctime was updated. You'll need to rename ctime_001_pos.c and then call it from a script. This way we can test both the xattr=sa and xattr=dir cases.

behlendorf · 2017-09-01T18:38:00Z

module/zfs/zfs_sa.c

-		VERIFY0(sa_update(zp->z_sa_hdl, SA_ZPL_DXATTR(zfsvfs),
-		    obj, size, tx));
+		int count = 0;
+		sa_bulk_attr_t  bulk[2];


nit: extra space before bulk[2].

behlendorf · 2017-09-01T18:41:52Z

module/zfs/zfs_sa.c

+
+		SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_DXATTR(zfsvfs),
+		    NULL, obj, size);
+		/* Any xattr change is metadata change hence update the ctime */


nit: No comment needed, it's clear we're updating the ctime due to the xattr change.

behlendorf · 2017-09-01T19:02:40Z

module/zfs/zfs_sa.c

+		SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_DXATTR(zfsvfs),
+		    NULL, obj, size);
+		/* Any xattr change is metadata change hence update the ctime */
+		zfs_tstamp_update_setup(zp, STATE_CHANGED, NULL, ctime);


style: it might be nice to move the zfs_tstamp_update_setup() call easier so all the SA_ADD_BULK_ATTR() calls are together.

behlendorf · 2017-09-01T19:06:15Z

module/zfs/zfs_sa.c

+
 		dmu_tx_commit(tx);
+		/* Update Inode ctime */
+		zfs_inode_update(zp);


There's no need to call zfs_inode_update(). The call to zfs_tstamp_update_setup() will update the ctime in the inode. We're working to retire the need for zfs_inode_update() so the fewer callers the better.

gaurkuma · 2017-09-07T19:34:55Z

@behlendorf I have updated the patch. I need some clarity regarding the test coverage. I have added setting xattr and checking if ctime changes in the ctime_001_pos.c file. But as you mentioned we need to test for both xattr=sa and dir. Is this what seems fair to do:

rename ctime_001_pos.c to ctime.c
Add a script ctime_001_pos.ksh
In the script set xattr to dir
Call ctime
Modify xattr to sa
call ctime

behlendorf · 2017-09-07T19:51:09Z

@gaurkuma yes, that sounds entirely reasonable. Thanks.

gaurkuma · 2017-09-11T16:21:07Z

@behlendorf Final Patch Updated.

behlendorf

Thanks! Looks good.

tuxoko · 2017-09-11T16:49:03Z

module/zfs/zfs_sa.c

+		SA_ADD_BULK_ATTR(bulk, count, SA_ZPL_CTIME(zfsvfs),
+		    NULL, &ctime, 16);
+		VERIFY0(sa_bulk_update(zp->z_sa_hdl, bulk, count, tx));
+		zfs_tstamp_update_setup(zp, STATE_CHANGED, NULL, ctime);


@gaurkuma you need to move this line before sa_bulk_update. Otherwise, you are using uninitialized ctime. You won't see that with stat(2), but you should be able see that with zdb.

@tuxoko Good Catch..Fixed it.

tuxoko · 2017-09-11T16:51:55Z

tests/runfiles/linux.run


 [tests/functional/ctime]
-tests = ['ctime_001_pos' ]
+tests = ['ctime' ]


I think it would be better to switch the two file name, so we get more consistent with others.

@tuxoko The reason I didnt do this way was because ctime_001_pos is a .c file and renaming it will mean changing multiple places in the Makefile. If that's ok, I will go ahead and make the change.

That's fine, please go ahead and rename it.

gaurkuma · 2017-09-12T16:09:39Z

@tuxoko Updated the patch..

behlendorf · 2017-09-12T20:58:18Z

tests/zfs-tests/tests/functional/ctime/Makefile.am

-pkgexec_PROGRAMS = ctime_001_pos
-ctime_001_pos_SOURCES = ctime_001_pos.c
+pkgexec_PROGRAMS = ctime
+ctime_001_pos_SOURCES = ctime.c


Shouldn't this now be ctime_SOURCES = ctime.c?

@behlendorf ..Done

Signed-off-by: gaurkuma <gauravk.18@gmail.com>

behlendorf · 2017-09-13T19:21:34Z

@tonyhutter this is a good candidate for 0.7, can you open a PR to port it. I expect it'll apply cleanly.

Changing any metadata, should modify the ctime. Reviewed-by: Chunwei Chen <tuxoko@gmail.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: gaurkuma <gauravk.18@gmail.com> Closes #3644 Closes #6586

Changing any metadata, should modify the ctime. Reviewed-by: Chunwei Chen <tuxoko@gmail.com> Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov> Signed-off-by: gaurkuma <gauravk.18@gmail.com> Closes openzfs#3644 Closes openzfs#6586

behlendorf requested changes Sep 1, 2017

View reviewed changes

behlendorf assigned tuxoko Sep 11, 2017

behlendorf approved these changes Sep 11, 2017

View reviewed changes

behlendorf unassigned tuxoko Sep 11, 2017

behlendorf requested a review from tuxoko September 11, 2017 16:31

tuxoko suggested changes Sep 11, 2017

View reviewed changes

tuxoko approved these changes Sep 12, 2017

View reviewed changes

behlendorf requested changes Sep 12, 2017

View reviewed changes

Modifying XATTRs doesnt change the ctime

024137b

Signed-off-by: gaurkuma <gauravk.18@gmail.com>

behlendorf approved these changes Sep 13, 2017

View reviewed changes

behlendorf merged commit 0107f69 into openzfs:master Sep 13, 2017

gaurkuma deleted the ctime branch September 13, 2017 20:42

Modifying XATTRs doesnt change the ctime #6586

Modifying XATTRs doesnt change the ctime #6586

Uh oh!

Conversation

gaurkuma commented Aug 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Motivation and Context

How Has This Been Tested?

Types of changes

Checklist:

Uh oh!

behlendorf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaurkuma commented Sep 7, 2017

Uh oh!

behlendorf commented Sep 7, 2017

Uh oh!

gaurkuma commented Sep 11, 2017

Uh oh!

behlendorf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gaurkuma commented Sep 12, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

behlendorf commented Sep 13, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

gaurkuma commented Aug 31, 2017 •

edited

Loading