Persist z_seq across znode eviction

[ixhamza]

Motivation and Context

Commit 312bdab advertises STATX_ATTR_CHANGE_MONOTONIC to knfsd and builds the NFSv4 change_cookie from (ctime.tv_sec << 32) | zp->z_seq. zp->z_seq is reset to a magic constant in zfs_znode_alloc(), so any event that drops the znode from cache (memory pressure, remount, reboot) brings the file back with the same ctime.tv_sec upper bits but a smaller z_seq in the lower bits, regressing the cookie within the same second.

NFSv4 clients that trust the monotonicity contract treat this as metadata they cannot rely on. VMware ESXi over NFSv4.1 reliably reproduces it with The file specified is not a virtual disk, causing a VM stored on the affected ZFS dataset to fail to power on.

Description

Persist zp->z_seq via a new SA attribute SA_ZPL_SEQ so it survives znode eviction. A new pflag bit ZFS_HAS_SEQ marks the file as carrying SA_ZPL_SEQ in its layout, mirroring the existing ZPL_PROJID/ZFS_PROJID pattern. The bit gates may_grow at SA tx-hold sites, choosing B_TRUE on the first add per file and B_FALSE thereafter, so steady-state operations pay no extra reservation.

A ZFS_PERSIST_SEQ() macro captures z_seq and sets the bit into the caller's bulk in one step, persisting both atomically alongside the file's other SA attributes. Every site that bumps z_seq uses it. zfs_znode_alloc() restores z_seq from SA_ZPL_SEQ when the bit is set.

No on-disk format change requiring a feature flag is needed. Older binaries preserve the new attribute and bit opaquely. The first modify by a patched binary lazily migrates each file.

How Has This Been Tested?

• Before: ESXi VM fails to power on over NFSv4.1.
• After: VM powers on successfully.
• CI Testing

Types of changes

• 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)
• Quality assurance (non-breaking change which makes the code more robust against bugs)
• Breaking change (fix or feature that would cause existing functionality to change)
• Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
• Documentation (a change to man pages or other documentation)

Checklist:

• My code follows the OpenZFS code style requirements.
• I have updated the documentation accordingly.
• I have read the contributing document.
• I have added tests to cover my changes.
• I have run the ZFS Test Suite with this change applied.
• All commit messages are properly formatted and contain Signed-off-by.

[ixhamza]

Updated per @amotin's private feedback. Rebased onto master.

[robn]

Sort of a low-form of drive-by review from me (ride-by review?).

@ixhamza ran this by me early and I said "looks plausible, and SAs have the nice property of being forward and backward compatible if assembled sensibly". I haven't looked since, but have reread through it now, and I still think that, and I trust from the review comments and the diffs that its been thought about in enough detail that I don't have to think it all through from first principles. (but I will if someone tells me to).

One thought though, have we actually tested that things still work right (fsvo) when importing a pool that has run on this change back into an older ZFS that hasn't? And/or, Linux<->FreeBSD? I mean, that's weird, and coming from cold I don't expect it'd make much different for NFS clients so long as the number doesn't go backwards. Just so long as we're not inadvertently causing a pool not to import or anything. I think its fine, but doesn't hurt to ask.

(Aside: SA API always feel so difficult...)

[ixhamza]

Thanks @robn for taking a look.

One thought though, have we actually tested that things still work right (fsvo) when importing a pool that has run on this change back into an older ZFS that hasn't? And/or, Linux<->FreeBSD? I mean, that's weird, and coming from cold I don't expect it'd make much different for NFS clients so long as the number doesn't go backwards. Just so long as we're not inadvertently causing a pool not to import or anything. I think its fine, but doesn't hurt to ask.

Linux<->FreeBSD imports cleanly both ways. Older <-> Newer ZFS imports fine too since SA_ZPL_SEQ is just an opaque SA attribute.

[ixhamza]

Pushed a follow up commit covering a few additional modification sites where z_seq wasn't being bumped, surfaced while testing more thoroughly. Since the NFSv4 change_cookie now relies entirely on z_seq, wanted to make sure every modification path advances it. Rebased onto current master.

[robn]

Bunch of comments, but all of the kind I would point at in a pair review and say "hmm, what's that about?" and you'll likely tell me and I'll learn something new (man that'd be SO MUCH FUN).

Assuming you know what you're doing and approving on that basis. This is a proper tour de force, nice work!

[robn]

My first thought was "how contended is this lock?". Is this called on the first fault for the mapping, or the first fault for each page in the mapping? The first is probably "eh", the second maybe is more of a concern?

The other thought is, is all change to z_seq everywhere protected by z_lock. Very difficult to follow unfortunately.

Both those could perhaps be obviated by making z_seq an atomic.

(to be clear: I have nothing to suggest these are problems, and don't understand all of this well enough to find that out without a lot of reading, so if you tell me its fine, then it is!)

[ixhamza]

It's indeed per page, but page_mkwrite only fires when a clean page goes dirty. Atomic made more sense, so I dropped the lock in page_mkwrite and zfs_create and switched z_seq operations to atomic.

[robn]

If this only bumps on transitions read->write, what happens on subsequent writes to the same map if GETATTR arrives between those? Or is that those are uncommitted, so not visible?

[ixhamza]

mtime only bumps when the page first goes dirty, not on later writes, and z_seq bumps on the same page_mkwrite. So the two stay in sync, the cookie matches mtime which is what NFS already relies on.

[robn]

Hmm, this feels like an actual bugfix beyond bumping z_seq, yes? Which is fine if so, but then is there anything more we need to do here? Tests, close anything in the bug tracker, etc? (I didn't look right now, but I know quirks around hole-punching come up from time to time, and I wonder...)

[ixhamza]

Yeah, it's indeed a real bugfix. Can be reproduced with the following script:

zpool create tank -O mountpoint=/mnt/tank sda
cd /mnt/tank
truncate -s 4096 f
sync
# mtime ctime (seconds)
before=$(stat -c '%Y %Z' f)
sleep 2
# extend past EOF, no keep-size 
fallocate -l 1048576 f
after=$(stat -c '%Y %Z' f)
echo "before: $before"
echo "after:  $after"

Without the change before and after stay the same, with it the mtime/ctime advance. Moved it to its own commit and added a test, fallocate_extend_timestamps.

[robn]

(It was these cases and others that made me wonder about the locking in zpl_page_mkwrite(), but like I say, didn't study them all).

[ixhamza]

All the z_seq bumps are atomic now, including these xattr/ACL ones.

[behlendorf]

It looks to me like xattr_bulk is actually slightly oversized here.

Suggested change

	sa_bulk_attr_t bulk[9], xattr_bulk[7];
	sa_bulk_attr_t bulk[9], xattr_bulk[6];

These sizes look right, but there are so many conditionals it's hard to see. To make sure there are no future overflows how about adding a few asserts either after out2:, or better yet before the relevant sa_bulk_update().

        ASSERT3S(count, <=, 9);
        ASSERT3S(xattr_count, <=, 6);