Date: Tue, 17 Sep 2019 02:10:36 +0000 From: Rick Macklem <rmacklem@uoguelph.ca> To: Konstantin Belousov <kostikbel@gmail.com> Cc: "peterj@freebsd.org" <peterj@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org> Subject: Re: Re; svn commit: r352393 - head/sys/fs/nfsclient Message-ID: <YTXPR0101MB2189BE1B135E716C00F78813DD8F0@YTXPR0101MB2189.CANPRD01.PROD.OUTLOOK.COM> In-Reply-To: <20190916161001.GV2559@kib.kiev.ua> References: <YTXPR0101MB21890232925E49EDEAF10772DD8C0@YTXPR0101MB2189.CANPRD01.PROD.OUTLOOK.COM>, <20190916161001.GV2559@kib.kiev.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
Konstantin Belousov wrote:=0A=
>On Mon, Sep 16, 2019 at 03:27:02PM +0000, Rick Macklem wrote:=0A=
>> Hi Kostik,=0A=
>>=0A=
>> I'm afraid there was a reason that only certain cases (where the file wa=
s=0A=
>> being shrunk) did the vnode_pager_setsize() call after the NFS node=0A=
>> lock is released.=0A=
>>=0A=
>> See the commit log message for r252528.=0A=
>>=0A=
>> If vnode_pager_setsize() now acquires a sleep lock for the case where th=
e=0A=
>> size is increasing, it sounds like the NFS node lock will have to become=
a=0A=
>> sleep lock instead of a mutex.=0A=
>> (If needed, I can get working on this in a day or two.)=0A=
>I suspect that the reason for the bug mentioned in r252528 is the fact=0A=
>that NFS calls vnode_pager_setsize() without owning the vnode lock. AFAIR=
=0A=
>this happens when iod thread performs RPCs. If you look at the start of=
=0A=
>the vnode_pager_setsize() function, you would note a commented out assert=
=0A=
>that the vnode is exclusively locked.=0A=
>=0A=
>Indeed, changing node lock to sleepable lock is a lot of work. But may=0A=
>be we should get rid of this mutex at all ? If you state that we should=
=0A=
>change it to some sleepable lock, then vnode lock should already serve=0A=
>the purpose.=0A=
>=0A=
>The only issue I see is that we would need to add missed vnode locks=0A=
>acquires in iod, and upgrade vnode lock shared to exclusive as needed.=0A=
>An ugly intermediate solution might be to make NFS vnode locks always=0A=
>exclusive.=0A=
Well, the reason that the readahead/writebehind RPCs done by the iod thread=
s=0A=
haven't acquired the vnode lock was to allow them to occur concurrently.=0A=
--> Now that there are LK_SHARED vnode locks, those could be used for=0A=
the readaheads.=0A=
--> For writebehinds, which is where the file's size changes, holding an=0A=
exclusive vnode lock for the entire RPC would serialize them and that=
=0A=
could be a large performance hit.=0A=
--> However, holding an exclusive vnode lock for short periods (but n=
ot=0A=
while the RPC is being done on the server) should be ok.=0A=
The NFS node mutex may still be needed to serialize use of fiel=
ds=0A=
in the NFS part of the vnode, but if the exclusive vnode lock c=
an be=0A=
held for the code section in question and the other places wher=
e=0A=
the n_size field is used (instead of depending on the NFS node =
mutex),=0A=
that should work, I think.=0A=
--> I'll take a look at the code tomorrow.=0A=
=0A=
rick=0A=
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?YTXPR0101MB2189BE1B135E716C00F78813DD8F0>