vn_delayed_setsize(): post-commit review' changes

[kib]

Summary

Rename the flag from VI_DELAYEDSSZ to VI_DELAYED_SETSIZE.
Change signature of vn_lock_delayed_setsize() to take flatten values
list instead of vop args structure.
__predict_true() for VI_DELAYED_SETSIZE not set.
Minor editings like removing tautological assert, and sorting items.

Fixes: 45117ffcd533ddf995f654db60b10899ae8370ec

[rmacklem]

sys/fs/deadfs/dead_vnops.c

58

If you are keeping the in alphabetical
order, maybe you could move the last
two entries as well?
(This is up to you.)

[kib]

More ordering for deadfs vops.

[markj]

sys/kern/vfs_vnops.c

1968–1969

Assert here that the vnode is locked?

1972

vn_lock_delayed_setsize() is only called from one place, and there we know that 1) the vnode is locked, 2) the vnode is not doomed. How is it possible for this v_op == &dead_vnodeops condition to be true?

2038–2039

This is an unlocked load so should be annotated with atomic_load_short.

sys/sys/vnode.h

1256

I think these helpers should also be called vn_delayed_setsize*

[kib]

sys/kern/vfs_vnops.c

1972

Oh, the _vn_lock() does not call vn_lock_delayed_setsize() for LK_RETRY/doomed case now. I fixed this.

[kib]

Call vn_lock_delayed_setsize() when doomed vnode was locked with LK_RETRY.
More notes handled.

[rmacklem]

sys/kern/vfs_vnops.c

2040

This is more for my education than a correction...
Why do atomic_load_short() here?

If v_iflag is being read without holding VI_LOCK(),
don't you need to handle v_iflag like v_irflag, where
atomics are used throughout?

At line#2011, you do VI_LOCK()/VI_UNLOCK().

[kib]

sys/kern/vfs_vnops.c

2040

This is more for my education than a correction...
Why do atomic_load_short() here?

If v_iflag is being read without holding VI_LOCK(),
don't you need to handle v_iflag like v_irflag, where
atomics are used throughout?

The unlocked check there is a minor optimization. It is not critical that specific exclusive lock instance see the VI_DELAYED_SETSIZE set (we might race with it being set anyway).

The idea behind using atomic access is not to make the code more correct, but only to indicate that the location read violates the expected locking protocol. It, in fact, should not change the generated code, except disallowing compiler to reuse possibly cached value that was read before (at least this is how gcc and clang interpret that, according to their docs).

At line#2011, you do VI_LOCK()/VI_UNLOCK().

This is in preparation for vn_clear_delayed_setsize_locked() later.

[rmacklem]

Thanks for the atomic_load_short explanation.

[markj]

sys/kern/vfs_vnops.c

2040

I think the __predict_true annotation should apply only to the flag check, not to the error != 0 check.

[kib]

Fine tune compiler hint