Question about the removal of __nfs_revalidate_inode() from do_setlk()

Question about the removal of __nfs_revalidate_inode() from do_setlk()

[Scott Mayhew]

Hi Trond,

Commit ca0daa2 ("NFS: Cache aggressively when file is open for writing")
removed the inode revalidation from do_setlk().  Why was that necessary?
If just that part of the commit is added back in, the client still seems
to be able to cope with out-of-order write replies.

Currently the client invalidates the data cache whenever it takes a lock
and that causes performance problems for some workloads... if a client
wants to re-read portions of a file, and no other client has modified 
that file, then why should the reads go out on the wire just because
locking is being used?

-Scott

Re: Question about the removal of __nfs_revalidate_inode() from do_setlk()

[Trond Myklebust]

T24gV2VkLCAyMDE4LTAyLTE0IGF0IDA5OjAxIC0wNTAwLCBTY290dCBNYXloZXcgd3JvdGU6DQo+
IEhpIFRyb25kLA0KPiANCj4gQ29tbWl0IGNhMGRhYTIgKCJORlM6IENhY2hlIGFnZ3Jlc3NpdmVs
eSB3aGVuIGZpbGUgaXMgb3BlbiBmb3INCj4gd3JpdGluZyIpDQo+IHJlbW92ZWQgdGhlIGlub2Rl
IHJldmFsaWRhdGlvbiBmcm9tIGRvX3NldGxrKCkuICBXaHkgd2FzIHRoYXQNCj4gbmVjZXNzYXJ5
Pw0KPiBJZiBqdXN0IHRoYXQgcGFydCBvZiB0aGUgY29tbWl0IGlzIGFkZGVkIGJhY2sgaW4sIHRo
ZSBjbGllbnQgc3RpbGwNCj4gc2VlbXMNCj4gdG8gYmUgYWJsZSB0byBjb3BlIHdpdGggb3V0LW9m
LW9yZGVyIHdyaXRlIHJlcGxpZXMuDQoNCkl0IGNhbiBjb3BlIHdpdGggb3V0IG9mIG9yZGVyIHJl
cGxpZXMsIGJ1dCBub3Qgd2l0aCBjaGFuZ2VzIGJ5IGEgc2Vjb25kDQpjbGllbnQsIHdoaWNoIGlz
IGhpZ2hseSBsaWtlbHkgd2hlbiB5b3UgYXJlIHVzaW5nIGZpbGUgbG9ja2luZy4gSW4gdGhhdA0K
Y2FzZSwgd2Ugc3RpbGwgbmVlZCB0byBpbnZhbGlkYXRlIHRoZSBkYXRhIGNhY2hlLg0KDQo+IEN1
cnJlbnRseSB0aGUgY2xpZW50IGludmFsaWRhdGVzIHRoZSBkYXRhIGNhY2hlIHdoZW5ldmVyIGl0
IHRha2VzIGENCj4gbG9jaw0KPiBhbmQgdGhhdCBjYXVzZXMgcGVyZm9ybWFuY2UgcHJvYmxlbXMg
Zm9yIHNvbWUgd29ya2xvYWRzLi4uIGlmIGEgDQoNCkV4YWN0bHkgd2hpY2ggd29ya2xvYWRzPw0K
DQo+IGNsaWVudA0KPiB3YW50cyB0byByZS1yZWFkIHBvcnRpb25zIG9mIGEgZmlsZSwgYW5kIG5v
IG90aGVyIGNsaWVudCBoYXMNCj4gbW9kaWZpZWQgDQo+IHRoYXQgZmlsZSwgdGhlbiB3aHkgc2hv
dWxkIHRoZSByZWFkcyBnbyBvdXQgb24gdGhlIHdpcmUganVzdCBiZWNhdXNlDQo+IGxvY2tpbmcg
aXMgYmVpbmcgdXNlZD8NCg0KVGhlIHBvaW50IG9mIHRoZSBwYXRjaCBpcyB0byBubyBsb25nZXIg
dHJhY2sgd2hldGhlciBvciBub3QgYW5vdGhlcg0KY2xpZW50IGhhcyBjaGFuZ2VkIHRoZSBmaWxl
IHdoaWxlIHRoZSBmaWxlIGlzIG9wZW4uIFRoYXQgdHJhY2tpbmcgd2FzDQpwcm9kdWNpbmcgd2F5
IHRvbyBtYW55IGZhbHNlIHBvc2l0aXZlcyBmb3Igbm8gZ2FpbiwgYW5kIGNhdXNpbmcgaGVhdnkN
CnNsb3dkb3ducyBmb3IgcGVyZm9ybWFuY2Ugb3B0aW1pc2VkIHdvcmtsb2FkcyBkdWUgdG8gc3B1
cmlvdXMgY2FjaGUNCmludmFsaWRhdGlvbnMuIFdvcmtsb2FkcyB0aGF0IHVzZSBsb2NraW5nIGFy
ZSBnZW5lcmFsbHkgX25vdF8NCmNvbnNpZGVyZWQgdG8gYmUgb3B0aW1pc2VkIGZvciBwZXJmb3Jt
YW5jZS4NCg0KSU9XOiBUaGUgcGF0Y2ggaXMgcmVzdG9yaW5nIHRoZSBiZWhhdmlvdXIgb2YgdGhl
IGxvY2tpbmcgY29kZSB0byB0aGUNCmhpc3RvcmljYWxseSBwcmVmZXJyZWQgb25lIGFzIGRlc2Ny
aWJlZCBpbiB0aGUgTkZTIEZBUS4NClNlZSBodHRwOi8vbmZzLnNvdXJjZWZvcmdlLm5ldC8jZmFx
X2E4DQoNCi0tIA0KVHJvbmQgTXlrbGVidXN0DQpMaW51eCBORlMgY2xpZW50IG1haW50YWluZXIs
IFByaW1hcnlEYXRhDQp0cm9uZC5teWtsZWJ1c3RAcHJpbWFyeWRhdGEuY29tDQo=

Re: Question about the removal of __nfs_revalidate_inode() from do_setlk()

[Scott Mayhew]

On Wed, 14 Feb 2018, Trond Myklebust wrote:

> On Wed, 2018-02-14 at 09:01 -0500, Scott Mayhew wrote:
> > Hi Trond,
> > 
> > Commit ca0daa2 ("NFS: Cache aggressively when file is open for
> > writing")
> > removed the inode revalidation from do_setlk().  Why was that
> > necessary?
> > If just that part of the commit is added back in, the client still
> > seems
> > to be able to cope with out-of-order write replies.
> 
> It can cope with out of order replies, but not with changes by a second
> client, which is highly likely when you are using file locking. In that
> case, we still need to invalidate the data cache.

Couldn't do_setlk incorporate a check similar to nfs_file_has_writers
and do the revalidation if there are no writers, otherwise do the
invalidation if there are writers?

> 
> > Currently the client invalidates the data cache whenever it takes a
> > lock
> > and that causes performance problems for some workloads... if a 
> 
> Exactly which workloads?

I'll need to get specifics on the actual workload, all that was attached
to the bugzilla was a script to illustrate the behavior, basically
flock/read/unlock in a loop (and looking at the support case attached
to the bug I don't really see any specifics either).

> 
> > client
> > wants to re-read portions of a file, and no other client has
> > modified 
> > that file, then why should the reads go out on the wire just because
> > locking is being used?
> 
> The point of the patch is to no longer track whether or not another
> client has changed the file while the file is open. That tracking was
> producing way too many false positives for no gain, and causing heavy
> slowdowns for performance optimised workloads due to spurious cache
> invalidations. Workloads that use locking are generally _not_
> considered to be optimised for performance.
> 
> IOW: The patch is restoring the behaviour of the locking code to the
> historically preferred one as described in the NFS FAQ.
> See http://nfs.sourceforge.net/#faq_a8

Doesn't that need to be reconciled with section 10.3.2 in RFCs 5661 &
7530?  Maybe those need to be amended?

-Scott
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, PrimaryData
> trond.myklebust@primarydata.com