Re; svn commit: r352393 - head/sys/fs/nfsclient

[Rick Macklem]

Konstantin Belousov wrote:
>On Mon, Sep 16, 2019 at 03:27:02PM +0000, Rick Macklem wrote:
>> Hi Kostik,
>>
>> I'm afraid there was a reason that only certain cases (where the file was
>> being shrunk) did the vnode_pager_setsize() call after the NFS node
>> lock is released.
>>
>> See the commit log message for r252528.
>>
>> If vnode_pager_setsize() now acquires a sleep lock for the case where the
>> size is increasing, it sounds like the NFS node lock will have to become a
>> sleep lock instead of a mutex.
>> (If needed, I can get working on this in a day or two.)
>I suspect that the reason for the bug mentioned in r252528 is the fact
>that NFS calls vnode_pager_setsize() without owning the vnode lock.  AFAIR
>this happens when iod thread performs RPCs.  If you look at the start of
>the vnode_pager_setsize() function, you would note a commented out assert
>that the vnode is exclusively locked.
>
>Indeed, changing node lock to sleepable lock is a lot of work.  But may
>be we should get rid of this mutex at all ?  If you state that we should
>change it to some sleepable lock, then vnode lock should already serve
>the purpose.
>
>The only issue I see is that we would need to add missed vnode locks
>acquires in iod, and upgrade vnode lock shared to exclusive as needed.
>An ugly intermediate solution might be to make NFS vnode locks always
>exclusive.
Well, the reason that the readahead/writebehind RPCs done by the iod threads
haven't acquired the vnode lock was to allow them to occur concurrently.
--> Now that there are LK_SHARED vnode locks, those could be used for
      the readaheads.
--> For writebehinds, which is where the file's size changes, holding an
      exclusive vnode lock for the entire RPC would serialize them and that
      could be a large performance hit.
      --> However, holding an exclusive vnode lock for short periods (but not
             while the RPC is being done on the server) should be ok.
            The NFS node mutex may still be needed to serialize use of fields
            in the NFS part of the vnode, but if the exclusive vnode lock can be
            held for the code section in question and the other places where
            the n_size field is used (instead of depending on the NFS node mutex),
            that should work, I think.
            --> I'll take a look at the code tomorrow.

rick