Fix setting birthtime in ZFS

[asomers]

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

[avg]

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 ↗

(On Diff #24490)

I am not very familiar with the code. Why is this block being removed from zfs_getattr?

3394 ↗

(On Diff #24490)

Even if this compiles, it looks weird.

sys/cddl/contrib/opensolaris/uts/common/sys/vnode.h

275 ↗

(On Diff #24490)

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.

[asomers]

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]

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

2789 ↗

(On Diff #24490)

Because it has no effect. All it does it set xoap->xoa_createtime, but that variable never gets read after this point.

3394 ↗

(On Diff #24490)

I'll fix it.

sys/cddl/contrib/opensolaris/uts/common/sys/vnode.h

275 ↗

(On Diff #24490)

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?

[avg]

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]

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

2789 ↗

(On Diff #24490)

Thank you for the explanation.

sys/cddl/contrib/opensolaris/uts/common/sys/vnode.h

275 ↗

(On Diff #24490)

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).

[asomers]

Add some missing parents to an if statement

[avg]

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

[asomers]

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.

[mav]

VFS is not my area of expertise.

[asomers]

It looks like the only part of D2445 that should be brought into D9353 is the 32-bit overflow check. I'll bring that in, test, and commit.