Why make these exclusive? The cache operation locks independently and doesn't need exclusive.

I'm not sure why we invoke fuse_vnode_clear_attr_cache on the directory. Anticipating of invalidated directory mtime? Ok.

The majority of this stuff is no longer used, apparently. In particular, LK_SHARED jumps out, but doesn't mean anything.

From my quick scan, only flags, nameptr, and namelen are used.

We must have never hit this path with a DEBUG kernel? fuse_io_invalbuf asserts the elock already.

Usually these "input invariant" assertions are put at the top of the function. I don't see something messing with vp's locking before this, but may be missing it.

style(9): blah == constant, not yoda-style.

Also, I think you want LK_TRYUPGRADE rather than LK_UPGRADE | LK_NOWAIT. The latter will drop our shared lock and then give up if the xlock cannot be acquired. TRYUPGRADE will upgrade if we're the sole shared-holder, or leave our lock status unchanged otherwise.

Probably need exclusive vnode lock here. E.g., for fuse_internal_cache_attrs() later. (This matches the canonical VOP_SETATTR locking protocol.)

This stuff all needs ELOCK

It would be sort of nice if this could be invalidated atomically. (In practice, it might be enough to zero the seconds half of the bintime and force a subsecond->second read ordering ... outside the scope of this change, either way.)

In addition to the same feedback as the earlier iteration of this construct, there appears to be a spurious \.

Implied in this diff is that this entry is also vnode-locked.

We can update flag non-atomically with only a shared lock? (Probably not, I think we need ELOCK or significant rototill of flags access.)

These need ELOCK. That vnode is visible on the mount hash/queue concurrent threads before fuse_vnode_get() returns.

ditto; the following code needs exclusive lock

style: yoda

dvp needs to be exclusively locked because of fuse_vnode_clear_attr_cache below.

In this case, I put it next to the use of cached_attrs, because that's what it's protecting. I'm not actually using it as a "input invariant."

Ahh, good to know! Is that documented anywhere?

And it turns out that LK_TRYUPGRADE is broken. I'll open a separate review for that.

I meant to put that next to the use of cached_attrs. I'll move it.

The assertion within fuse_internal_cache_attrs has it covered. As for fuse_vnode_undirty_cached_timestamps, that doesn't access cached_attrs. It only accesses v_data. I haven't audited that field yet to decide what kind of locking it needs. I'll do that separately.

That's also part of v_data. I'll audit that later.

We'll have ELOCK here, because fuse_vnode_get returns an exclusively locked vnode.

Ditto. fuse_vnode_get gives us ELOCK.

There's some popular test library whose assertion messages are formatted correctly for Yoda clauses. Pytest? I can't remember which. But it got me used to writing that way.

Ok, but it is a common pattern that locks are not changed during a function, so usually lock assertions are input invariants. That is true in this function, regardless of which branches are taken afterwards. I think this one makes more sense at the top than in the middle.

Yes, in style(9).

I saw the TRYUPGRADE review, thanks for doing that.

I'd suggest leaving it where it is and just adding the letter E.

fuse_internal_cache_attrs also accesses v_data aka fvdat. Those things are currently vnode lock protected, including fuse_vnode_undirty_cached_timestamps, which non-atomically sets a flag. It needs some form of exclusive serialization against readers and the prevailing pattern for v_data/fvdat is the vnode lock.

All of these fields seem to be protected by the vnode lock. It might be better to put a struct-level comment documenting that, and then only document exceptions to the rule.

Then we can should ASSERT_VOP_ELOCKED rather than VOP_LOCKED ...

Ok, so assert ELOCKed... I don't get it.

A lot of C developers picked up yoda comparisons in the 90s before -Wparentheses (or similar) became common.

a.c:39:10: warning: using the result of an assignment as a condition without parentheses [-Wparentheses]
        if (abc = 1) { printf("\n"); }

It's because DEBUG_VFS_LOCKS isn't in GENERIC. I wasn't turning it on until recently.

Except that much of it isn't protected by the vnode lock. Maybe it should be, but it's not. So I'm auditing the fields one by one. I'll audit those fields in a separate commit, if that's ok.

The assertion concerns the access to cached_attrs. If you're referring to the fvdat access, I'll cover that in a later review.

I'm on board but it is probably an independent change

Oh, wild! Obviously out of scope for this diff but IMO that seems like something that should be enabled in our GENERIC kernels. It seems consistent with the kind of lock invariant checks we already enable and I don't think the overhead is especially bad compared with, say, WITNESS.

Works for me

I'm not seeing this hash; is it in the canonical freebsd git tree?

I'm not intending to commit this part. Phabricator doesn't show it clearly, but this is in its own commit with a big DO NOT MERGE in the commit message. But it's necessary to develop the rest of the change.

I missed the "DO NOT MERGE. Use KTR and DEBUG_VFS_LOCKS" in your earlier update, too. No worries.

style(9)

This one was never addressed. It should be ELOCKED.

We have a difference of philosophy here. You say that I should assert ELOCKED because fuse_vnode_get gives us that, and you think in terms of input invariants. I think in terms of RAII, because I'm spoiled by better languages. That's why I like to put my assertions right next to where I need the lock, and assert only what we need, not what we happen to have.