Fix setting birthtime in ZFS
ClosedPublic
Actions

Authored by asomers on Jan 26 2017, 9:16 PM.

Details

Reviewers

avg

Commits

rS316391: MFC r313483:
rS313483: Fix setting birthtime in ZFS

Summary

Fix setting birthtime in ZFS

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c

In zfs_freebsd_setattr, if the caller wants to set the birthtime,

	  set the bits that zfs_settattr expects

In zfs_setattr, if XAT_CREATETIME is set, set

	  xoa_createtime, expected by zfs_xvattr_set.  The two levels of
	  indirection seem excessive, but it minimizes diffs vs OpenZFS.

Remove red herring in zfs_getattr

sys/cddl/contrib/opensolaris/uts/common/sys/vnode.h
Un-booby-trap some macros

Test Plan

https://github.com/pjd/pjdfstest/pull/6

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

No Lint Coverage

Unit

No Test Coverage

Build Status

Buildable 7125
Build 7301: arc lint + arc unit

Event Timeline

asomers updated this revision to Diff 24490.Jan 26 2017, 9:16 PM

asomers retitled this revision from to Fix setting birthtime in ZFS.

asomers updated this object.

asomers edited the test plan for this revision. (Show Details)

asomers added reviewers: mav, avg.

Herald added subscribers: delphij, imp. · View Herald TranscriptJan 26 2017, 9:16 PM

Additionally, could you please formatting of the Summary? It's quite hard to read right now.
Thanks!

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c
2789	I am not very familiar with the code. Why is this block being removed from `zfs_getattr`?
3394	Even if this compiles, it looks weird.
sys/cddl/contrib/opensolaris/uts/common/sys/vnode.h
275	I would prefer `do { ... } while(0)` to avoid a redundant ';' after the macro expansion is done. But there are no local variables or local control flow, so these macros should be safe in their current form. If you want to change them anyway, please also submit the change to the upstream to avoid gratuitous code differences.

In D9353#193856, @avg wrote:

Additionally, could you please formatting of the Summary? It's quite hard to read right now.
Thanks!

I always format the summary the way that I'll format the SVN commit message. But then Phabricator thinks it's in markdown and consequently screws it up. Is there a guide to Phabricator's markdown flavor anywhere?

asomers marked 2 inline comments as done.Jan 30 2017, 3:52 PM

asomers added inline comments.

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c
2789	Because it has no effect. All it does it set xoap->xoa_createtime, but that variable never gets read after this point.
3394	I'll fix it.
sys/cddl/contrib/opensolaris/uts/common/sys/vnode.h
275	I chose to do it this way to match the style used by VN_RELE_ASYNC. Would you like me to add `do{...} while(0)` there too?

In D9353#193883, @asomers wrote:

I always format the summary the way that I'll format the SVN commit message. But then Phabricator thinks it's in markdown and consequently screws it up. Is there a guide to Phabricator's markdown flavor anywhere?

Yeah, it sucks that it is not always possible to write the summary in a way that would look nice both as a plain text and rendered by Phabricator.
There is some help available via a small book icon at the top right corner of the comment box.
In fact, here is a link: https://secure.phabricator.com/book/phabricator/article/remarkup/

avg added inline comments.Jan 31 2017, 1:16 PM

sys/cddl/contrib/opensolaris/uts/common/fs/zfs/zfs_vnops.c
2789	Thank you for the explanation.
sys/cddl/contrib/opensolaris/uts/common/sys/vnode.h
275	Oh, I didn't realize that that file used that sucky style throughout. I guess that there is no big reason to deviate from it then (unless we'd want to fix the style for the whole file).

Add some missing parents to an if statement

LGTM.
Also, I think that this obsoletes D2445, doesn't it?

This revision is now accepted and ready to land.Feb 6 2017, 11:37 AM

Thanks for the pointer, @avg. I didn't know about D2445. But now that I do, I can see that there are some elements from that CR that I ought to add to mine. I'll work on it.