fusefs: protect fufh table and cached attributes with the vnode lock

[asomers]

Summary

fusefs: protect fufh table and cached attributes with the vnode lock

• fusefs: protect cached file attributes with the vnode lock
• fusefs: ensure vnode is locked when accessing file handles table

[cem]

sys/fs/fuse/fuse_internal.c

416–420

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.

434–441

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.

464–466

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

709

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.

944

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.

1154

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

1241–1250

This stuff all needs ELOCK

sys/fs/fuse/fuse_io.c

986

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

987–988

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

sys/fs/fuse/fuse_node.h

118

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

sys/fs/fuse/fuse_vfsops.c

599

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

602–605

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

sys/fs/fuse/fuse_vnops.c

1381

ditto; the following code needs exclusive lock

1881

style: yoda

[asomers]

sys/fs/fuse/fuse_internal.c

416–420

dvp needs to be exclusively locked because of fuse_vnode_clear_attr_cache below.

709

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

944

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.

1154

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

1241–1250

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.

sys/fs/fuse/fuse_vfsops.c

599

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

602–605

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

sys/fs/fuse/fuse_vnops.c

1381

Ditto. fuse_vnode_get gives us ELOCK.

1881

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.

[cem]

sys/fs/fuse/fuse_internal.c

709

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.

944

Yes, in style(9).

I saw the TRYUPGRADE review, thanks for doing that.

1154

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

1241–1250

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.

sys/fs/fuse/fuse_node.h

111

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.

sys/fs/fuse/fuse_vfsops.c

602–605

Then we can should ASSERT_VOP_ELOCKED rather than VOP_LOCKED ...

sys/fs/fuse/fuse_vnops.c

1381

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

1881

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"); }

[asomers]

sys/fs/fuse/fuse_internal.c

464–466

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

sys/fs/fuse/fuse_node.h

111

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.

sys/fs/fuse/fuse_vnops.c

1381

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

[asomers]

• fusefs: ensure vnode is locked when accessing file handles table
• fusefs: protect cached file attributes with the vnode lock
• Respond to cem's comments.
• DO NOT MERGE. Use KTR and DEBUG_VFS_LOCKS
• style changes

[cem]

sys/amd64/conf/GENERIC

102–103

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

sys/fs/fuse/fuse_internal.c

464–466

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.

sys/fs/fuse/fuse_node.h

111

Works for me

tests/sys/fs/fusefs/read.cc

299

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

[asomers]

sys/amd64/conf/GENERIC

102–103

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.

[cem]

sys/amd64/conf/GENERIC

102–103

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

[asomers]

• Fix a git hash in a comment

[asomers]

@cem are we good here? I'd like to audit the fvdat field in a separate commit, and add DEBUG_VFS_LOCKS in a separate commit, too.

[cem]

Two small issues remaining, but otherwise I think it's in good shape

sys/fs/fuse/fuse_internal.c

944

style(9)

sys/fs/fuse/fuse_vnops.c

1381

This one was never addressed. It should be ELOCKED.

[asomers]

sys/fs/fuse/fuse_vnops.c

1381

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.

[asomers]

• style(9)

[cem]

sys/fs/fuse/fuse_vnops.c

1381

Please don't try to tell me how I think; it's boorish, and you're mistaken anyway.

I don't think we should assert ELOCKED just because we happen to have it.

I think we should assert ELOCKED because later in this codepath, unconditionally, we require ELOCKED. If we're going to assert anything, it should match our requirements. LOCKED-but-not-ELOCKED here cannot be valid, and asserting on the wrong condition is (1) most significantly, misleading to readers, (2) in general, might mean missing a lock invariant violation, and (3) a small waste of cycles, in general (sure, vnode locks in particular seem to be disabled in GENERIC).

Putting a million lock checks local to accesses doesn't make sense in C++, either.