From: Trond Myklebust <trondmy@primarydata.com>
To: "viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
"jlayton@redhat.com" <jlayton@redhat.com>,
"neilb@suse.com" <neilb@suse.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"mkoutny@suse.com" <mkoutny@suse.com>,
"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: Re: Do we really need d_weak_revalidate???
Date: Fri, 11 Aug 2017 05:55:46 +0000 [thread overview]
Message-ID: <1502430944.3822.1.camel@primarydata.com> (raw)
In-Reply-To: <87bmnmrai9.fsf@notabene.neil.brown.name>
T24gRnJpLCAyMDE3LTA4LTExIGF0IDE0OjMxICsxMDAwLCBOZWlsQnJvd24gd3JvdGU6DQo+IEZ1
bm55IHN0b3J5LiAgNC41IHllYXJzIGFnbyB3ZSBkaXNjYXJkZWQgdGhlIEZTX1JFVkFMX0RPVCBz
dXBlcmJsb2NrDQo+IGZsYWcgYW5kIGludHJvZHVjZWQgdGhlIGRfd2Vha19yZXZhbGlkYXRlIGRl
bnRyeSBvcGVyYXRpb24gaW5zdGVhZC4NCj4gV2UgZHVseSByZW1vdmVkIHRoZSBmbGFnIGZyb20g
TkZTIHN1cGVyYmxvY2tzIGFuZCBORlN2NCBzdXBlcmJsb2NrcywNCj4gYW5kIGFkZGVkIHRoZSBu
ZXcgZGVudHJ5IG9wZXJhdGlvbiB0byBORlMgZGVudHJpZXMgLi4uLiBidXQgbm90IHRvDQo+IE5G
U3Y0DQo+IGRlbnRyaWVzLg0KPiANCj4gQW5kIG5vYm9keSBub3RpY2VkLg0KPiANCj4gVW50aWwg
dG9kYXkuDQo+IA0KPiBBIGN1c3RvbWVyIHJlcG9ydHMgYSBzaXR1YXRpb24gd2hlcmUgbW91bnQo
Li4uLixNU19SRU1PVU5ULC4uKSBvbiBhbg0KPiBORlMNCj4gZmlsZXN5c3RlbSBoYW5ncyBiZWNh
dXNlIHRoZSBuZXR3b3JrIGhhcyBiZWVuIGRlY29uZmlndXJlZC4gIFRoaXMNCj4gbWFrZXMNCj4g
cGVyZmVjdCBzZW5zZSBhbmQgSSBzdWdnZXN0ZWQgYSBjb2RlIGNoYW5nZSB0byBmaXggdGhlIHBy
b2JsZW0uDQo+IEhvd2V2ZXIgd2hlbiBhIGNvbGxlYWd1ZSB3YXMgdHJ5aW5nIHRvIHJlcHJvZHVj
ZSB0aGUgcHJvYmxlbSB0bw0KPiB2YWxpZGF0ZQ0KPiB0aGUgZml4LCBoZSBjb3VsZG4ndC4gIFRo
ZW4gbm9yIGNvdWxkIEkuDQo+IA0KPiBUaGUgcHJvYmxlbSBpcyB0cml2aWFsbHkgcmVwcm9kdWNp
YmxlIHdpdGggTkZTdjMsIGFuZCBub3QgYXQgYWxsIHdpdGgNCj4gTkZTdjQuICBUaGUgcmVhc29u
IGlzIHRoZSBtaXNzaW5nIGRfd2Vha19yZXZhbGlkYXRlLg0KPiANCj4gV2UgY291bGQgc2ltcGx5
IGFkZCBkX3dlYWtfcmV2YWxpZGF0ZSBmb3IgTkZTdjQsIGJ1dCBnaXZlbiB0aGF0IGl0DQo+IGhh
cyBiZWVuIG1pc3NpbmcgZm9yIDQuNSB5ZWFycywgYW5kIHRoZSBvbmx5IHRpbWUgYW55b25lIG5v
dGljZWQgd2FzDQo+IHdoZW4gdGhlIG9tbWlzc2lvbiByZXN1bHRlZCBpbiBhIGJldHRlciB1c2Vy
IGV4cGVyaWVuY2UsIEkgZG8gd29uZGVyDQo+IGlmDQo+IHdlIG5lZWQgdG8uICBDYW4gd2UganVz
dCBkaXNjYXJkIGRfd2Vha19yZXZhbGlkYXRlPyAgV2hhdCBwdXJwb3NlDQo+IGRvZXMNCj4gaXQg
c2VydmU/ICBJIGNvdWxkbid0IGZpbmQgb25lLg0KPiANCj4gVGhhbmtzLA0KPiBOZWlsQnJvd24N
Cj4gDQo+IEZvciByZWZlcmVuY2UsIHNlZQ0KPiBDb21taXQ6IGVjZjNkMWYxYWE3NCAoInZmczog
a2lsbCBGU19SRVZBTF9ET1QgYnkgYWRkaW5nIGENCj4gZF93ZWFrX3JldmFsaWRhdGUgZGVudHJ5
IG9wIikNCj4gDQo+IA0KPiANCj4gVG8gcmVwcm9kdWNlIHRoZSBwcm9ibGVtIGF0IGhvbWUsIG9u
IGEgc3lzdGVtIHRoYXQgdXNlcyBzeXN0ZW1kOg0KPiAxLyBwbGFjZSAob3IgZmluZCkgYSBmaWxl
c3lzdGVtIGltYWdlIGluIGEgZmlsZSBvbiBhbiBORlMgZmlsZXN5c3RlbS4NCj4gMi8gbW91bnQg
dGhlIG5mcyBmaWxlc3lzdGVtIHdpdGggIm5vYWMiIC0gY2hvb3NlIHYzIG9yIHY0DQo+IDMvIGxv
b3AtbW91bnQgdGhlIGZpbGVzeXN0ZW0gaW1hZ2UgcmVhZC1vbmx5IHNvbWV3aGVyZQ0KPiA0LyBy
ZWJvb3QNCj4gDQo+IElmIHlvdSBjaG9vc2UgdjQsIHRoZSByZWJvb3Qgd2lsbCBzdWNjZWVkLCBw
b3NzaWJseSBhZnRlciBhIDkwc2Vjb25kDQo+IHRpbWVvdXQuDQo+IElmIHlvdSBjaG9vc2UgdjMs
IHRoZSByZWJvb3Qgd2lsbCBoYW5nIGluZGVmaW5pdGVseSBpbiBzeXN0ZW1kLQ0KPiBzaHV0ZG93
biB3aGlsZQ0KPiByZW1vdW50aW5nIHRoZSBuZnMgZmlsZXN5c3RlbSByZWFkLW9ubHkuDQo+IA0K
PiBJZiB5b3UgZG9uJ3QgdXNlICJub2FjIiBpdCBjYW4gc3RpbGwgaGFuZywgYnV0IG9ubHkgaWYg
c29tZXRoaW5nDQo+IHNsb3dzDQo+IGRvd24gdGhlIHJlYm9vdCBlbm91Z2ggdGhhdCBhdHRyaWJ1
dGVzIGhhdmUgdGltZWQgb3V0IGJ5IHRoZSB0aW1lDQo+IHRoYXQNCj4gc3lzdGVtZC1zaHV0ZG93
biBydW5zLiAgVGhpcyBoYXBwZW5zIGZvciBvdXIgY3VzdG9tZXIuDQo+IA0KPiBJZiB0aGUgbG9v
cC1tb3VudGVkIGZpbGVzeXN0ZW0gaXMgbm90IHJlYWQtb25seSwgeW91IGdldCBvdGhlcg0KPiBw
cm9ibGVtcy4NCj4gDQo+IFdlIHJlYWxseSB3YW50IHN5c3RlbWQgdG8gZmlndXJlIG91dCB0aGF0
IHRoZSBsb29wLW1vdW50IG5lZWRzIHRvIGJlDQo+IHVubW91bnRlZCBmaXJzdC4gIEkgaGF2ZSBp
ZGVhcyBjb25jZXJuaW5nIHRoYXQsIGJ1dCBpdCBpcyBtZXNzeS4gIEJ1dA0KPiB0aGF0IGlzbid0
IHRoZSBvbmx5IGJ1ZyBoZXJlLg0KDQpUaGUgbWFpbiBwdXJwb3NlIG9mIGRfd2Vha19yZXZhbGlk
YXRlKCkgd2FzIHRvIGNhdGNoIHRoZSBpc3N1ZXMgdGhhdA0KYXJpc2Ugd2hlbiBzb21lb25lIGNo
YW5nZXMgdGhlIGNvbnRlbnRzIG9mIHRoZSBjdXJyZW50IHdvcmtpbmcNCmRpcmVjdG9yeSBvciBp
dHMgcGFyZW50IG9uIHRoZSBzZXJ2ZXIuIFNpbmNlICcuJyBhbmQgJy4uJyBhcmUgdHJlYXRlZA0K
c3BlY2lhbGx5IGluIHRoZSBsb29rdXAgY29kZSwgdGhleSB3b3VsZCBub3QgYmUgcmV2YWxpZGF0
ZWQgd2l0aG91dA0Kc3BlY2lhbCB0cmVhdG1lbnQuIFRoYXQgbGVhZHMgdG8gaXNzdWVzIHdoZW4g
bG9va2luZyB1cCBmaWxlcyBhcw0KLi88ZmlsZW5hbWU+IG9yIC4uLzxmaWxlbmFtZT4sIHNpbmNl
IHRoZSBjbGllbnQgd29uJ3QgZGV0ZWN0IHRoYXQgaXRzDQpkY2FjaGUgaXMgc3RhbGUgdW50aWwg
aXQgdHJpZXMgdG8gdXNlIHRoZSBjYWNoZWQgZGVudHJ5K2lub2RlLg0KDQpUaGUgb25lIHRoaW5n
IHRoYXQgaGFzIGNoYW5nZWQgc2luY2UgaXRzIGludHJvZHVjdGlvbiBpcywgSSBiZWxpZXZlLA0K
dGhlIEVTVEFMRSBoYW5kbGluZyBpbiB0aGUgVkZTIGxheWVyLiBUaGF0IG1pZ2h0IGZpeCBhIGxv
dCBvZiB0aGUNCmRjYWNoZSBsb29rdXAgYnVncyB0aGF0IHdlcmUgcHJldmlvdXNseSBoYW5kbGVk
IGJ5IGRfd2Vha19yZXZhbGlkYXRlKCkuDQpJIGhhdmVuJ3QgZG9uZSBhbiBhdWRpdCB0byBmaWd1
cmUgb3V0IGlmIGl0IGFjdHVhbGx5IGNhbiBoYW5kbGUgYWxsIG9mDQp0aGVtLg0KDQotLSANClRy
b25kIE15a2xlYnVzdA0KTGludXggTkZTIGNsaWVudCBtYWludGFpbmVyLCBQcmltYXJ5RGF0YQ0K
dHJvbmQubXlrbGVidXN0QHByaW1hcnlkYXRhLmNvbQ0K
WARNING: multiple messages have this Message-ID (diff)
From: Trond Myklebust <trondmy@primarydata.com>
To: "viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
"jlayton@redhat.com" <jlayton@redhat.com>,
"neilb@suse.com" <neilb@suse.com>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
"mkoutny@suse.com" <mkoutny@suse.com>,
"linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
"linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: Re: Do we really need d_weak_revalidate???
Date: Fri, 11 Aug 2017 05:55:46 +0000 [thread overview]
Message-ID: <1502430944.3822.1.camel@primarydata.com> (raw)
In-Reply-To: <87bmnmrai9.fsf@notabene.neil.brown.name>
On Fri, 2017-08-11 at 14:31 +1000, NeilBrown wrote:
> Funny story. 4.5 years ago we discarded the FS_REVAL_DOT superblock
> flag and introduced the d_weak_revalidate dentry operation instead.
> We duly removed the flag from NFS superblocks and NFSv4 superblocks,
> and added the new dentry operation to NFS dentries .... but not to
> NFSv4
> dentries.
>
> And nobody noticed.
>
> Until today.
>
> A customer reports a situation where mount(....,MS_REMOUNT,..) on an
> NFS
> filesystem hangs because the network has been deconfigured. This
> makes
> perfect sense and I suggested a code change to fix the problem.
> However when a colleague was trying to reproduce the problem to
> validate
> the fix, he couldn't. Then nor could I.
>
> The problem is trivially reproducible with NFSv3, and not at all with
> NFSv4. The reason is the missing d_weak_revalidate.
>
> We could simply add d_weak_revalidate for NFSv4, but given that it
> has been missing for 4.5 years, and the only time anyone noticed was
> when the ommission resulted in a better user experience, I do wonder
> if
> we need to. Can we just discard d_weak_revalidate? What purpose
> does
> it serve? I couldn't find one.
>
> Thanks,
> NeilBrown
>
> For reference, see
> Commit: ecf3d1f1aa74 ("vfs: kill FS_REVAL_DOT by adding a
> d_weak_revalidate dentry op")
>
>
>
> To reproduce the problem at home, on a system that uses systemd:
> 1/ place (or find) a filesystem image in a file on an NFS filesystem.
> 2/ mount the nfs filesystem with "noac" - choose v3 or v4
> 3/ loop-mount the filesystem image read-only somewhere
> 4/ reboot
>
> If you choose v4, the reboot will succeed, possibly after a 90second
> timeout.
> If you choose v3, the reboot will hang indefinitely in systemd-
> shutdown while
> remounting the nfs filesystem read-only.
>
> If you don't use "noac" it can still hang, but only if something
> slows
> down the reboot enough that attributes have timed out by the time
> that
> systemd-shutdown runs. This happens for our customer.
>
> If the loop-mounted filesystem is not read-only, you get other
> problems.
>
> We really want systemd to figure out that the loop-mount needs to be
> unmounted first. I have ideas concerning that, but it is messy. But
> that isn't the only bug here.
The main purpose of d_weak_revalidate() was to catch the issues that
arise when someone changes the contents of the current working
directory or its parent on the server. Since '.' and '..' are treated
specially in the lookup code, they would not be revalidated without
special treatment. That leads to issues when looking up files as
./<filename> or ../<filename>, since the client won't detect that its
dcache is stale until it tries to use the cached dentry+inode.
The one thing that has changed since its introduction is, I believe,
the ESTALE handling in the VFS layer. That might fix a lot of the
dcache lookup bugs that were previously handled by d_weak_revalidate().
I haven't done an audit to figure out if it actually can handle all of
them.
--
Trond Myklebust
Linux NFS client maintainer, PrimaryData
trond.myklebust@primarydata.com
next prev parent reply other threads:[~2017-08-11 5:55 UTC|newest]
Thread overview: 35+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-08-11 4:31 Do we really need d_weak_revalidate??? NeilBrown
2017-08-11 5:55 ` Trond Myklebust [this message]
2017-08-11 5:55 ` Trond Myklebust
2017-08-11 11:01 ` Jeff Layton
2017-08-13 23:36 ` NeilBrown
2017-08-14 10:10 ` Jeff Layton
2017-08-16 2:43 ` NeilBrown
2017-08-16 11:34 ` Jeff Layton
2017-08-16 23:47 ` NeilBrown
2017-08-17 2:20 ` Ian Kent
2017-08-18 5:24 ` NeilBrown
2017-08-18 6:47 ` Ian Kent
2017-08-18 6:55 ` Ian Kent
2017-08-21 6:23 ` NeilBrown
2017-08-21 6:32 ` Ian Kent
2017-08-21 7:46 ` NeilBrown
2017-08-23 1:06 ` NeilBrown
2017-08-23 2:32 ` Ian Kent
2017-08-23 2:40 ` Ian Kent
2017-08-23 2:54 ` Ian Kent
2017-08-23 7:51 ` Ian Kent
2017-08-24 3:21 ` NeilBrown
2017-08-24 4:35 ` Ian Kent
2017-08-24 4:07 ` NeilBrown
2017-08-24 4:47 ` Ian Kent
2017-08-24 4:58 ` Ian Kent
2017-09-08 15:15 ` David Howells
2017-08-24 11:03 ` Michael Kerrisk (man-pages)
2017-08-25 0:05 ` Ian Kent
2017-08-25 5:32 ` [PATCH manpages] stat.2: correct AT_NO_AUTOMOUNT text and general revisions NeilBrown
2017-09-14 13:38 ` Michael Kerrisk (man-pages)
2017-09-14 22:25 ` NeilBrown
2017-09-16 13:11 ` Michael Kerrisk (man-pages)
2017-08-13 23:29 ` Do we really need d_weak_revalidate??? NeilBrown
2017-08-24 6:34 ` NeilBrown
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1502430944.3822.1.camel@primarydata.com \
--to=trondmy@primarydata.com \
--cc=jlayton@redhat.com \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-nfs@vger.kernel.org \
--cc=mkoutny@suse.com \
--cc=neilb@suse.com \
--cc=viro@zeniv.linux.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.