diff mbox

jfs: fix readdir cookie incompatibility with NFSv4

Message ID 1376769691.2544.4.camel@deadeye.wl.decadent.org.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Ben Hutchings Aug. 17, 2013, 8:01 p.m. UTC
On Thu, 2013-08-15 at 14:26 -0700, Christian Kujau wrote:
> On Thu, 15 Aug 2013 at 15:48, Dave Kleikamp wrote:
> > This patch replaces the one I posted yesterday. I like this better since
> > it doesn't require fixing existing on-disk cookies or skipping a
> > position in the in-inode index table.
> 
> Thanks. Applied to 3.11-rc5 and tested, no more "readdir loop" messages 
> and with unique inode numbers, great!
> 
>   Tested-by: Christian Kujau <lists@nerdbynature.de>

Karl and Jonathan, could you test the attached backport to 3.2?

(Instructions for rebuilding the Debian kernel package are at:
<http://kernel-handbook.alioth.debian.org/ch-common-tasks.html#s-common-official>)

Ben.

Comments

ben Aug. 19, 2013, 7:11 p.m. UTC | #1
On 8/17/2013 2:01 PM, Ben Hutchings wrote:
> On Thu, 2013-08-15 at 14:26 -0700, Christian Kujau wrote:
>> On Thu, 15 Aug 2013 at 15:48, Dave Kleikamp wrote:
>>> This patch replaces the one I posted yesterday. I like this better since
>>> it doesn't require fixing existing on-disk cookies or skipping a
>>> position in the in-inode index table.
>>
>> Thanks. Applied to 3.11-rc5 and tested, no more "readdir loop" messages 
>> and with unique inode numbers, great!
>>
>>   Tested-by: Christian Kujau <lists@nerdbynature.de>
> 
> Karl and Jonathan, could you test the attached backport to 3.2?
> 
> (Instructions for rebuilding the Debian kernel package are at:
> <http://kernel-handbook.alioth.debian.org/ch-common-tasks.html#s-common-official>)
> 
> Ben.
> 
When I read this the other day I thought It was a great idea, but when
sleeping on it I have this gut feeling that somewhere there is a
jfs_dirent->position--; missing but I don't have enough familiarity with
the code to begin to guess where. Hopefully I am wrong.

ben
Jonathan McDowell Aug. 29, 2013, 10:48 p.m. UTC | #2
On Sat, Aug 17, 2013 at 10:01:31PM +0200, Ben Hutchings wrote:
> On Thu, 2013-08-15 at 14:26 -0700, Christian Kujau wrote:
> > On Thu, 15 Aug 2013 at 15:48, Dave Kleikamp wrote:
> > > This patch replaces the one I posted yesterday. I like this better since
> > > it doesn't require fixing existing on-disk cookies or skipping a
> > > position in the in-inode index table.
> > 
> > Thanks. Applied to 3.11-rc5 and tested, no more "readdir loop" messages 
> > and with unique inode numbers, great!
> > 
> >   Tested-by: Christian Kujau <lists@nerdbynature.de>
> 
> Karl and Jonathan, could you test the attached backport to 3.2?

I finally managed to be able to schedule some downtime yesterday for one
of the machines I've been seeing this issue with. So far since the
reboot to this kernel (3.2.46-1 + the patch) I haven't seen a recurrence
of the problem; will update if I see anything.

J.
Jonathan McDowell Sept. 5, 2013, 3:40 a.m. UTC | #3
On Thu, Aug 29, 2013 at 03:48:03PM -0700, Jonathan McDowell wrote:
> On Sat, Aug 17, 2013 at 10:01:31PM +0200, Ben Hutchings wrote:
> > On Thu, 2013-08-15 at 14:26 -0700, Christian Kujau wrote:
> > > On Thu, 15 Aug 2013 at 15:48, Dave Kleikamp wrote:
> > > > This patch replaces the one I posted yesterday. I like this
> > > > better since it doesn't require fixing existing on-disk cookies
> > > > or skipping a position in the in-inode index table.
> > > 
> > > Thanks. Applied to 3.11-rc5 and tested, no more "readdir loop"
> > > messages and with unique inode numbers, great!
> > > 
> > >   Tested-by: Christian Kujau <lists@nerdbynature.de>
> > 
> > Karl and Jonathan, could you test the attached backport to 3.2?
> 
> I finally managed to be able to schedule some downtime yesterday for
> one of the machines I've been seeing this issue with. So far since the
> reboot to this kernel (3.2.46-1 + the patch) I haven't seen a
> recurrence of the problem; will update if I see anything.

I see the patch hit 3.11, but just to confirm the 3.2 backport on top of
the Debian 3.2.46-1 kernel has seen no recurrence or issues in the past
week (with a set of JFS filesystems that are fairly extensively used
over NFS).

J.
diff mbox

Patch

Date: Thu, 15 Aug 2013 15:48:39 -0500
From: Dave Kleikamp <dave.kleikamp@oracle.com>
Subject: jfs: fix readdir cookie incompatibility with NFSv4

This patch replaces the one I posted yesterday. I like this better since
it doesn't require fixing existing on-disk cookies or skipping a
position in the in-inode index table.

NFSv4 reserves readdir cookie values 0-2 for special entries (. and ..),
but jfs allows a value of 2 for a non-special entry. This incompatibility
can result in the nfs client reporting a readdir loop.

This patch doesn't change the value stored internally, but adds one to
the value exposed to the iterate method.

Signed-off-by: Dave Kleikamp <dave.kleikamp@oracle.com>
[bwh: Backported to 3.2:
 - Adjust context
 - s/ctx->pos/filp->f_pos/]
Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 fs/jfs/jfs_dtree.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/fs/jfs/jfs_dtree.c b/fs/jfs/jfs_dtree.c
index 8743ba9..0ec767e 100644
--- a/fs/jfs/jfs_dtree.c
+++ b/fs/jfs/jfs_dtree.c
@@ -3047,6 +3047,14 @@  int jfs_readdir(struct file *file, struct dir_context *ctx)
 
 		dir_index = (u32) filp->f_pos;
 
+		/*
+		 * NFSv4 reserves cookies 1 and 2 for . and .. so we add
+		 * the value we return to the vfs is one greater than the
+		 * one we use internally.
+		 */
+		if (dir_index)
+			dir_index--;
+
 		if (dir_index > 1) {
 			struct dir_table_slot dirtab_slot;
 
@@ -3086,7 +3094,7 @@  int jfs_readdir(struct file *file, struct dir_context *ctx)
 			if (p->header.flag & BT_INTERNAL) {
 				jfs_err("jfs_readdir: bad index table");
 				DT_PUTPAGE(mp);
-				filp->f_pos = -1;
+				filp->f_pos = DIREND;
 				return 0;
 			}
 		} else {
@@ -3094,15 +3102,15 @@  int jfs_readdir(struct file *file, struct dir_context *ctx)
 				/*
 				 * self "."
 				 */
-				filp->f_pos = 0;
+				filp->f_pos = 1;
 				if (filldir(dirent, ".", 1, 0, ip->i_ino,
 					    DT_DIR))
 					return 0;
 			}
 			/*
 			 * parent ".."
 			 */
-			filp->f_pos = 1;
+			filp->f_pos = 2;
 			if (filldir(dirent, "..", 2, 1, PARENT(ip), DT_DIR))
 				return 0;
 
@@ -3123,24 +3131,25 @@  int jfs_readdir(struct file *file, struct dir_context *ctx)
 		/*
 		 * Legacy filesystem - OS/2 & Linux JFS < 0.3.6
 		 *
-		 * pn = index = 0:	First entry "."
-		 * pn = 0; index = 1:	Second entry ".."
+		 * pn = 0; index = 1:	First entry "."
+		 * pn = 0; index = 2:	Second entry ".."
 		 * pn > 0:		Real entries, pn=1 -> leftmost page
 		 * pn = index = -1:	No more entries
 		 */
 		dtpos = filp->f_pos;
-		if (dtpos == 0) {
+		if (dtpos < 2) {
 			/* build "." entry */
 
+			filp->f_pos = 1;
 			if (filldir(dirent, ".", 1, filp->f_pos, ip->i_ino,
 				    DT_DIR))
 				return 0;
-			dtoffset->index = 1;
+			dtoffset->index = 2;
 			filp->f_pos = dtpos;
 		}
 
 		if (dtoffset->pn == 0) {
-			if (dtoffset->index == 1) {
+			if (dtoffset->index == 2) {
 				/* build ".." entry */
 
 				if (filldir(dirent, "..", 2, filp->f_pos,
@@ -3233,6 +3242,12 @@  int jfs_readdir(struct file *file, struct dir_context *ctx)
 					}
 					jfs_dirent->position = unique_pos++;
 				}
+				/*
+				 * We add 1 to the index because we may
+				 * use a value of 2 internally, and NFSv4
+				 * doesn't like that.
+				 */
+				jfs_dirent->position++;
 			} else {
 				jfs_dirent->position = dtpos;
 				len = min(d_namleft, DTLHDRDATALEN_LEGACY);