nfsclient: upgrade vnode lock in VOP_OPEN() if we need to flush buffers

[kib]

Summary

VOP_FSYNC() asserts that the vnode is exclusively locked for NFS. If we try to execute file with recently modified content, the assert is triggered.

[rmacklem]

Oops. I have always assumed calls to VOP_OPEN/VOP_CLOSE had
exclusively locked vnodes.

I think there are reasons beyond calls the ncl_vinvalbuf()
that expects this. I'll need to take a closer look, but at least
for NFSv4 VOP_OPEN(), the upgrade will always be needed,
I think?

Maybe the upgrades will need to be done at the beginning
of both calls instead of distributed through them.

[kib]

In D32999#745181, @rmacklem wrote:

Oops. I have always assumed calls to VOP_OPEN/VOP_CLOSE had
exclusively locked vnodes.

For instance, exec() does VOP_OPEN() for shared-locked vnode.
Your note about VOP_CLOSE() makes me think that the same upgrade would be needed there, for instance execve() closes vp as well, But we do flush in open, and then text counter prevents writes.

I think there are reasons beyond calls the ncl_vinvalbuf()
that expects this. I'll need to take a closer look, but at least
for NFSv4 VOP_OPEN(), the upgrade will always be needed,
I think?

Why? I do not see it as needed unconditionally for NFSv4. There are still additional checks for mtime.

Maybe the upgrades will need to be done at the beginning
of both calls instead of distributed through them.

I tried to save the upgrades. I noted this assert happens sometimes, not always, even when I did things like "configure". Just booting multiuser from nfsroot did not. So doing upgrades always would waste a lot, IMO.

[rmacklem]

I just looked and it does look like the handling of
the NFSv4 Open state is safe for a shared vnode.
There is a separate sleep lock in the openowner
that is set appropriately (exclusive or shared)
in nfscl_open(), called from nfsrpc_open().

So, I think this patch is sufficient for nfs_open().
I'll take a closer look before I click reviewed.

[rmacklem]

Looks ok, now that I am convinced the nfsrpc_open() call
is safe for a shared vnode lock.

I also looked at nfs_close() and the other stuff is safe for
a shared vnode lock.

I'd say adding the same stuff to nfs_close() before the
ncl_vinvalbuf() and ncl_flush() calls makes sense.
As you say, with a little luck, the upgrade may never
actually happen, but having the code there just in case
seems reasonable?

[kib]

In D32999#745215, @rmacklem wrote:

I'd say adding the same stuff to nfs_close() before the
ncl_vinvalbuf() and ncl_flush() calls makes sense.
As you say, with a little luck, the upgrade may never
actually happen, but having the code there just in case
seems reasonable?

It is somewhat more delicate there, we must not fail the call just due to doomed vnode, because VOP_CLOSE() is called from vgonel().
I updated the patch with some variant, which should be good enough, I believe.

[kib]

Also upgrade in VOP_CLOSE()

[rmacklem]

Looks fine to me. Btw, I did a search for any other
calls to ncl_flush()/ncl_vinvalbuf() where the vnode
might be shared locked and didn't find any.