RCU caching regression in kernel v4.1+

RCU caching regression in kernel v4.1+

[Trond Myklebust]

Hi Al,

Please could you take a look at the bugzilla entry in
https://bugzilla.kernel.org/show_bug.cgi?id=104911 ?

It describes a NFS caching regression that appears to be caused by
commit 766c4cbfacd8634d7580bac6a1b8456e63de3e84 ("namei:
d_is_negative() should be checked before ->d_seq validation").

Shouldn't that test for 'if (negative) return -ENOENT;' happen after
the call to d_revalidate() in lookup_fast()? If not, we can end up
caching negative dentries forever, AFAICS...

Cheers
  Trond
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: RCU caching regression in kernel v4.1+

[Trond Myklebust]

On Wed, 2015-10-07 at 14:57 -0400, Trond Myklebust wrote:
> Hi Al,
> 
> Please could you take a look at the bugzilla entry in
> https://bugzilla.kernel.org/show_bug.cgi?id=104911 ?
> 
> It describes a NFS caching regression that appears to be caused by
> commit 766c4cbfacd8634d7580bac6a1b8456e63de3e84 ("namei:
> d_is_negative() should be checked before ->d_seq validation").
> 
> Shouldn't that test for 'if (negative) return -ENOENT;' happen after
> the call to d_revalidate() in lookup_fast()? If not, we can end up
> caching negative dentries forever, AFAICS...
> 
> Cheers
>   Trond

Leandro, can you please test if the following patch helps in any way?

Cheers
  Trond

8<-----------------------------------------------------------------
>From eb61ece5739bb2f3b6d03dd8ca8e335bf0d12687 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <trond.myklebust@primarydata.com>
Date: Thu, 8 Oct 2015 08:44:00 -0400
Subject: [PATCH] namei: results of d_is_negative() should be checked after
 dentry revalidation

Leandro Awa writes:
After switching to version 4.1.6, our parallelized and distributed workflows now  fail consistently with errors of the form:

T34: ./regex.c:39:22: error: config.h: No such file or directory

>From our 'git bisect' testing, the following commit appears to be
the possible cause of the behavior we've been seeing: commit 766c4cbfacd8

The issue is that revalidation may cause the dentry to be dropped in NFS
if, say, the client notes that the directory timestamps have changed.

Reported-by: Leandro Awa <lawa@nvidia.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=104911
Fixes: 766c4cbfacd8 ("namei: d_is_negative() should be checked...")
Cc: stable@vger.kernel.org # v4.1+
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/namei.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 726d211db484..33e9495a3129 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1558,8 +1558,6 @@ static int lookup_fast(struct nameidata *nd,
 		negative = d_is_negative(dentry);
 		if (read_seqcount_retry(&dentry->d_seq, seq))
 			return -ECHILD;
-		if (negative)
-			return -ENOENT;
 
 		/*
 		 * This sequence count validates that the parent had no
@@ -1580,6 +1578,12 @@ static int lookup_fast(struct nameidata *nd,
 				goto unlazy;
 			}
 		}
+		/*
+		 * Note: do negative dentry check after revalidation in
+		 * case that drops it.
+		 */
+		if (negative)
+			return -ENOENT;
 		path->mnt = mnt;
 		path->dentry = dentry;
 		if (likely(__follow_mount_rcu(nd, path, inode, seqp)))
-- 
2.4.3

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

RE: RCU caching regression in kernel v4.1+

[Leandro Awa]

Hi Trond,

Sure.   I'm running the test now.  It should be done within the next 4 hours.

Best Regards,
Leandro Awa


-----Original Message-----
From: Trond Myklebust [mailto:trond.myklebust@primarydata.com] 
Sent: Thursday, October 08, 2015 5:55 AM
To: Alexander Viro
Cc: Linux NFS Mailing List; Leandro Awa; Linux FS-devel Mailing List
Subject: Re: RCU caching regression in kernel v4.1+

On Wed, 2015-10-07 at 14:57 -0400, Trond Myklebust wrote:
> Hi Al,
> 
> Please could you take a look at the bugzilla entry in
> https://bugzilla.kernel.org/show_bug.cgi?id=104911 ?
> 
> It describes a NFS caching regression that appears to be caused by 
> commit 766c4cbfacd8634d7580bac6a1b8456e63de3e84 ("namei:
> d_is_negative() should be checked before ->d_seq validation").
> 
> Shouldn't that test for 'if (negative) return -ENOENT;' happen after 
> the call to d_revalidate() in lookup_fast()? If not, we can end up 
> caching negative dentries forever, AFAICS...
> 
> Cheers
>   Trond

Leandro, can you please test if the following patch helps in any way?

Cheers
  Trond

8<-----------------------------------------------------------------
From eb61ece5739bb2f3b6d03dd8ca8e335bf0d12687 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <trond.myklebust@primarydata.com>
Date: Thu, 8 Oct 2015 08:44:00 -0400
Subject: [PATCH] namei: results of d_is_negative() should be checked after  dentry revalidation

Leandro Awa writes:
After switching to version 4.1.6, our parallelized and distributed workflows now  fail consistently with errors of the form:

T34: ./regex.c:39:22: error: config.h: No such file or directory

From our 'git bisect' testing, the following commit appears to be the possible cause of the behavior we've been seeing: commit 766c4cbfacd8

The issue is that revalidation may cause the dentry to be dropped in NFS if, say, the client notes that the directory timestamps have changed.

Reported-by: Leandro Awa <lawa@nvidia.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=104911
Fixes: 766c4cbfacd8 ("namei: d_is_negative() should be checked...")
Cc: stable@vger.kernel.org # v4.1+
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/namei.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 726d211db484..33e9495a3129 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1558,8 +1558,6 @@ static int lookup_fast(struct nameidata *nd,
 		negative = d_is_negative(dentry);
 		if (read_seqcount_retry(&dentry->d_seq, seq))
 			return -ECHILD;
-		if (negative)
-			return -ENOENT;
 
 		/*
 		 * This sequence count validates that the parent had no @@ -1580,6 +1578,12 @@ static int lookup_fast(struct nameidata *nd,
 				goto unlazy;
 			}
 		}
+		/*
+		 * Note: do negative dentry check after revalidation in
+		 * case that drops it.
+		 */
+		if (negative)
+			return -ENOENT;
 		path->mnt = mnt;
 		path->dentry = dentry;
 		if (likely(__follow_mount_rcu(nd, path, inode, seqp)))
--
2.4.3

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




-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

RE: RCU caching regression in kernel v4.1+

[Leandro Awa]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 4128 bytes --]

Fyi     The patch definitely helped.

Thank you.

Regards,
Leandro Awa
MIS-Farm Support


-----Original Message-----
From: Leandro Awa 
Sent: Thursday, October 08, 2015 10:29 AM
To: 'Trond Myklebust'; Alexander Viro
Cc: Linux NFS Mailing List; Linux FS-devel Mailing List
Subject: RE: RCU caching regression in kernel v4.1+

Hi Trond,

Sure.   I'm running the test now.  It should be done within the next 4 hours.

Best Regards,
Leandro Awa


-----Original Message-----
From: Trond Myklebust [mailto:trond.myklebust@primarydata.com]
Sent: Thursday, October 08, 2015 5:55 AM
To: Alexander Viro
Cc: Linux NFS Mailing List; Leandro Awa; Linux FS-devel Mailing List
Subject: Re: RCU caching regression in kernel v4.1+

On Wed, 2015-10-07 at 14:57 -0400, Trond Myklebust wrote:
> Hi Al,
> 
> Please could you take a look at the bugzilla entry in
> https://bugzilla.kernel.org/show_bug.cgi?id=104911 ?
> 
> It describes a NFS caching regression that appears to be caused by 
> commit 766c4cbfacd8634d7580bac6a1b8456e63de3e84 ("namei:
> d_is_negative() should be checked before ->d_seq validation").
> 
> Shouldn't that test for 'if (negative) return -ENOENT;' happen after 
> the call to d_revalidate() in lookup_fast()? If not, we can end up 
> caching negative dentries forever, AFAICS...
> 
> Cheers
>   Trond

Leandro, can you please test if the following patch helps in any way?

Cheers
  Trond

8<-----------------------------------------------------------------
From eb61ece5739bb2f3b6d03dd8ca8e335bf0d12687 Mon Sep 17 00:00:00 2001
From: Trond Myklebust <trond.myklebust@primarydata.com>
Date: Thu, 8 Oct 2015 08:44:00 -0400
Subject: [PATCH] namei: results of d_is_negative() should be checked after  dentry revalidation

Leandro Awa writes:
After switching to version 4.1.6, our parallelized and distributed workflows now  fail consistently with errors of the form:

T34: ./regex.c:39:22: error: config.h: No such file or directory

From our 'git bisect' testing, the following commit appears to be the possible cause of the behavior we've been seeing: commit 766c4cbfacd8

The issue is that revalidation may cause the dentry to be dropped in NFS if, say, the client notes that the directory timestamps have changed.

Reported-by: Leandro Awa <lawa@nvidia.com>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=104911
Fixes: 766c4cbfacd8 ("namei: d_is_negative() should be checked...")
Cc: stable@vger.kernel.org # v4.1+
Signed-off-by: Trond Myklebust <trond.myklebust@primarydata.com>
---
 fs/namei.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 726d211db484..33e9495a3129 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1558,8 +1558,6 @@ static int lookup_fast(struct nameidata *nd,
 		negative = d_is_negative(dentry);
 		if (read_seqcount_retry(&dentry->d_seq, seq))
 			return -ECHILD;
-		if (negative)
-			return -ENOENT;
 
 		/*
 		 * This sequence count validates that the parent had no @@ -1580,6 +1578,12 @@ static int lookup_fast(struct nameidata *nd,
 				goto unlazy;
 			}
 		}
+		/*
+		 * Note: do negative dentry check after revalidation in
+		 * case that drops it.
+		 */
+		if (negative)
+			return -ENOENT;
 		path->mnt = mnt;
 		path->dentry = dentry;
 		if (likely(__follow_mount_rcu(nd, path, inode, seqp)))
--
2.4.3

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




-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±û"žØ^n‡r¡ö¦zË\x1aëh™¨è­Ú&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br	šê+€Ê+zf£¢·hšˆ§~†­†Ûiÿûàz¹\x1e®w¥¢¸?™¨è­Ú&¢)ߢ^[f

[PATCH] namei: results of d_is_negative() should be checked after dentry revalidation

[Trond Myklebust]

Leandro Awa writes:
After switching to version 4.1.6, our parallelized and distributed workflows now  fail consistently with errors of the form:

T34: ./regex.c:39:22: error: config.h: No such file or directory

>From our 'git bisect' testing, the following commit appears to be
the possible cause of the behavior we've been seeing: commit 766c4cbfacd8

The issue is that revalidation may cause the dentry to be dropped in NFS
if, say, the client notes that the directory timestamps have changed.

Reported-by: Leandro Awa <lawa-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Link: https://bugzilla.kernel.org/show_bug.cgi?id=104911
Fixes: 766c4cbfacd8 ("namei: d_is_negative() should be checked...")
Tested-by: Leandro Awa <lawa-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Cc: stable-u79uwXL29TY76Z2rM5mHXA@public.gmane.org # v4.1+
Signed-off-by: Trond Myklebust <trond.myklebust-7I+n7zu2hftEKMMhf/gKZA@public.gmane.org>
---
 fs/namei.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index 726d211db484..33e9495a3129 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1558,8 +1558,6 @@ static int lookup_fast(struct nameidata *nd,
 		negative = d_is_negative(dentry);
 		if (read_seqcount_retry(&dentry->d_seq, seq))
 			return -ECHILD;
-		if (negative)
-			return -ENOENT;
 
 		/*
 		 * This sequence count validates that the parent had no
@@ -1580,6 +1578,12 @@ static int lookup_fast(struct nameidata *nd,
 				goto unlazy;
 			}
 		}
+		/*
+		 * Note: do negative dentry check after revalidation in
+		 * case that drops it.
+		 */
+		if (negative)
+			return -ENOENT;
 		path->mnt = mnt;
 		path->dentry = dentry;
 		if (likely(__follow_mount_rcu(nd, path, inode, seqp)))
-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] namei: results of d_is_negative() should be checked after dentry revalidation

[Linus Torvalds]

On Fri, Oct 9, 2015 at 10:44 AM, Trond Myklebust
<trond.myklebust@primarydata.com> wrote:
>
> The issue is that revalidation may cause the dentry to be dropped in NFS
> if, say, the client notes that the directory timestamps have changed.

Ack.

We've had this bug before, where we returned something else than
-ENOCHLD while we were doing RCU lookups. See for example commit
97242f99a013 ("link_path_walk(): be careful when failing with
ENOTDIR").

So in general, we should always (a) either verify all sequence points
or (b) return -ENOCHLD to go into slow mode. The patch seems

However, this thing was explicitly made to be this way by commit
766c4cbfacd8 ("namei: d_is_negative() should be checked before ->d_seq
validation"), so while my gut feel is to consider this fix
ObviouslyCorrect(tm), I will delay it a bit in the hope to get an ACK
and comment from Al about the patch.

Al?

                  Linus

Re: [PATCH] namei: results of d_is_negative() should be checked after dentry revalidation

[Al Viro]

On Fri, Oct 09, 2015 at 05:19:02PM -0700, Linus Torvalds wrote:

> So in general, we should always (a) either verify all sequence points
> or (b) return -ENOCHLD to go into slow mode. The patch seems
> 
> However, this thing was explicitly made to be this way by commit
> 766c4cbfacd8 ("namei: d_is_negative() should be checked before ->d_seq
> validation"), so while my gut feel is to consider this fix
> ObviouslyCorrect(tm), I will delay it a bit in the hope to get an ACK
> and comment from Al about the patch.
> 
> Al?

Umm...  I agree that the current version is wrong and it looks like this
patch is a complete fix.  The only problem is the commit message -
what really happens is that 766c4cbfacd8 got the things subtly wrong.
We used to treat d_is_negative() after lookup_fast() as "fall with ENOENT".
That was wrong - checking ->d_flags outside of ->d_seq protection is
unreliable and failing with hard error on what should've fallen back to
non-RCU pathname resolution is a bug.

Unfortunately, we'd pulled the test too far up and ran afoul of another
kind of staleness.  Dentry might have been absolutely stable from the
RCU point of view (and we might be on UP, etc.), but stale from the
remote fs point of view.  If ->d_revalidate() returns "it's actually
stale", dentry gets thrown away and original code wouldn't even have looked
at its ->d_flags.  What we need is to check ->d_flags where 766c4cbfacd8 does
(prior to ->d_seq validation) but only use the result in cases where we
do not discard this dentry outright.

With some explanation along the lines of the above added, consider the patch
ACKed.
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] namei: results of d_is_negative() should be checked after dentry revalidation

[Al Viro]

On Sat, Oct 10, 2015 at 02:36:57AM +0100, Al Viro wrote:
> On Fri, Oct 09, 2015 at 05:19:02PM -0700, Linus Torvalds wrote:
> 
> > So in general, we should always (a) either verify all sequence points
> > or (b) return -ENOCHLD to go into slow mode. The patch seems
> > 
> > However, this thing was explicitly made to be this way by commit
> > 766c4cbfacd8 ("namei: d_is_negative() should be checked before ->d_seq
> > validation"), so while my gut feel is to consider this fix
> > ObviouslyCorrect(tm), I will delay it a bit in the hope to get an ACK
> > and comment from Al about the patch.
> > 
> > Al?
> 
> Umm...  I agree that the current version is wrong and it looks like this
> patch is a complete fix.  The only problem is the commit message -
> what really happens is that 766c4cbfacd8 got the things subtly wrong.
> We used to treat d_is_negative() after lookup_fast() as "fall with ENOENT".
> That was wrong - checking ->d_flags outside of ->d_seq protection is
> unreliable and failing with hard error on what should've fallen back to
> non-RCU pathname resolution is a bug.
> 
> Unfortunately, we'd pulled the test too far up and ran afoul of another
> kind of staleness.  Dentry might have been absolutely stable from the
> RCU point of view (and we might be on UP, etc.), but stale from the
> remote fs point of view.  If ->d_revalidate() returns "it's actually
> stale", dentry gets thrown away and original code wouldn't even have looked
> at its ->d_flags.  What we need is to check ->d_flags where 766c4cbfacd8 does
> (prior to ->d_seq validation) but only use the result in cases where we
> do not discard this dentry outright.
> 
> With some explanation along the lines of the above added, consider the patch
> ACKed.

OK, I've attemtped to add an explanation of what's going on; please, pull from
git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-linus

Shortlog:
Trond Myklebust (1):
      namei: results of d_is_negative() should be acted upon only after dentry revalidation

Diffstat:
 fs/namei.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] namei: results of d_is_negative() should be checked after dentry revalidation

[Linus Torvalds]

On Sat, Oct 10, 2015 at 10:13 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> OK, I've attemtped to add an explanation of what's going on; please, pull from
..

Heh. I just committed it myself with your emailed explanation, so ..

                  Linus