DMAPI problem with the new filldir implementation

Hi Christoph and Dave,

It looks like we may have a problem caused by this patch:

http://oss.sgi.com/archives/xfs/2007-07/msg00338.html

It makes XFS test 144 to fail if the inode size is 2k. I did some investigation and I think xfs_readdir() returns incorrect next location for shortform directories. When the inode size is 2k, more dir entries fit inline in the inode and test 144 dm_get_dirattrs() starts using the shortform.

I think this code here

       if (filldir(dirent, sfep->name, sfep->namelen,
               off + xfs_dir2_data_entsize(sfep->namelen),
               ino, DT_UNKNOWN)) {

adds some offset "xfs_dir2_data_entsize(sfep->namelen)" to the next location pointer, which appears to be incorrect and causes directory entries to be skipped on the next call of dm_get_dirattrs().

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

off_t d_off; /* offset to the next dirent */

fs/xfs/dmapi/xfs_dm.c 2007-12-03 11:55:12.000000000 +1100 renamed from fs/xfs/dmapi/xfs_dm.c 2007-12-03 14:04:10.000000000 +1100 +0 -0

       error = -EFAULT;
       if (cb->lastbuf && put_user(0, &cb->lastbuf->_link))
 

and it seams to fix the problem.

Because I don't fully understand the new filldir implementation I would like to ask you if you could have a look at it and see what would be the best way to fix the problem.

(cc xfs-dev and xfs@oss)

Quoted 14 lines · Vlad Apostolov

Hi Christoph and Dave,

It looks like we may have a problem caused by this patch:

http://oss.sgi.com/archives/xfs/2007-07/msg00338.html

It makes XFS test 144 to fail if the inode size is 2k. I did some investigation and I think xfs_readdir() returns incorrect next location for shortform directories. When the inode size is 2k, more dir entries fit inline in the inode and test 144 dm_get_dirattrs() starts using the shortform.

I think this code here

Quoted 7 lines

       if (filldir(dirent, sfep->name, sfep->namelen,
               off + xfs_dir2_data_entsize(sfep->namelen),
               ino, DT_UNKNOWN)) {

adds some offset "xfs_dir2_data_entsize(sfep->namelen)" to the next location pointer, which appears to be incorrect and causes directory entries to be skipped on the next call of dm_get_dirattrs().

It's supposed to be the offset of the next dirent:

On Linux, the dirent structure is defined as follows:

struct dirent {
ino_t d_ino; /* inode number */

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

off_t d_off; /* offset to the next dirent */

unsigned short d_reclen; /* length of this record */
unsigned char d_type; /* type of file */
char d_name[256]; /* filename */
};

However, looking at the generic filldir() implementation in fs/readdir.c, I see that it:

        dirent = buf->previous;
        if (dirent) {
                if (__put_user(offset, &dirent->d_off))
                        goto efault;
        }

Puts the offset in the *previous* dirent, not the current one. That means that the three calls to XFs makes to filldir() are all incorrect; they should be passing the offset of the current object, not the next object as teh filldir code stuffs it in the previous dirent.

The reason that dmapi is failing here is that it uses teh offset as the cookie to to indicate the next inode to read. However, getdents uses the filp->f_pos:

        error = vfs_readdir(file, filldir, &buf);
        if (error < 0)
                goto out_putf;
        error = buf.error;
        lastdirent = buf.previous;
        if (lastdirent) {
                if (put_user(file->f_pos, &lastdirent->d_off))
                        error = -EFAULT;
                else
                        error = count - buf.count;
        }

and xfs_file_readdir uses that as teh offset for lookups and keeps it up to date correctly. hence the getdents code is overwritting the incorrect value XFS has been stuffing in there and hence we haven't seen this on normal syscalls.

Ok, patch attached. passes 144 on all inode sizes, otherwise mostly untested.

Quoted 67 lines · David Chinner

(cc xfs-dev and xfs@oss)

Quoted 14 lines · Vlad Apostolov

Hi Christoph and Dave,

It looks like we may have a problem caused by this patch:

http://oss.sgi.com/archives/xfs/2007-07/msg00338.html

It makes XFS test 144 to fail if the inode size is 2k. I did some investigation and I think xfs_readdir() returns incorrect next location for shortform directories. When the inode size is 2k, more dir entries fit inline in the inode and test 144 dm_get_dirattrs() starts using the shortform.

I think this code here

Quoted 7 lines

       if (filldir(dirent, sfep->name, sfep->namelen,
               off + xfs_dir2_data_entsize(sfep->namelen),
               ino, DT_UNKNOWN)) {

adds some offset "xfs_dir2_data_entsize(sfep->namelen)" to the next location pointer, which appears to be incorrect and causes directory entries to be skipped on the next call of dm_get_dirattrs().

It's supposed to be the offset of the next dirent:

On Linux, the dirent structure is defined as follows:

struct dirent {
ino_t d_ino; /* inode number */

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

Quoted 1 line

off_t d_off; /* offset to the next dirent */

unsigned short d_reclen; /* length of this record */
unsigned char d_type; /* type of file */
char d_name[256]; /* filename */
};

However, looking at the generic filldir() implementation in fs/readdir.c, I see that it:

        dirent = buf->previous;
        if (dirent) {
                if (__put_user(offset, &dirent->d_off))
                        goto efault;
        }

Puts the offset in the *previous* dirent, not the current one. That means that the three calls to XFs makes to filldir() are all incorrect; they should be passing the offset of the current object, not the next object as teh filldir code stuffs it in the previous dirent.

The reason that dmapi is failing here is that it uses teh offset as the cookie to to indicate the next inode to read. However, getdents uses the filp->f_pos:

        error = vfs_readdir(file, filldir, &buf);
        if (error < 0)
                goto out_putf;
        error = buf.error;
        lastdirent = buf.previous;
        if (lastdirent) {
                if (put_user(file->f_pos, &lastdirent->d_off))
                        error = -EFAULT;
                else
                        error = count - buf.count;
        }

Quoted 4 lines

and xfs_file_readdir uses that as teh offset for lookups and keeps it up to date correctly. hence the getdents code is overwritting the incorrect value XFS has been stuffing in there and hence we haven't seen this on normal syscalls.

Except that the hack to go back to double buffering relies on the filldir offset being pushed off to be the next dirent, and hence with the patch I sent we set filp->f_pos incorrectly in xfs_file_readdir().....

<sigh>

Quoted 2 lines

Ok, patch attached. passes 144 on all inode sizes, otherwise mostly untested.

I did say mostly untested :/

Updated patch below (which passes 006 but is still mostly untested).

Cheers,

dave.

Signature · 14 lines

Dave Chinner
Principal Engineer
SGI Australian Software Group

---
 fs/xfs/dmapi/xfs_dm.c       |    4 ----
 fs/xfs/linux-2.6/xfs_file.c |    4 ++--
 fs/xfs/xfs_dir2_block.c     |    6 ++----
 fs/xfs/xfs_dir2_leaf.c      |    2 +-
 fs/xfs/xfs_dir2_sf.c        |    9 +++------
 5 files changed, 8 insertions(+), 17 deletions(-)

Index: 2.6.x-xfs-new/fs/xfs/xfs_dir2_block.c
===================================================================

fs/xfs/xfs_dir2_block.c +2 -4

@@ -508,7 +508,7 @@ xfs_dir2_block_getdents(
 			continue;
 
 		cook = xfs_dir2_db_off_to_dataptr(mp, mp->m_dirdatablk,
-						    ptr - (char *)block);
+					    (char *)dep - (char *)block);
 		ino = be64_to_cpu(dep->inumber);
 #if XFS_BIG_INUMS
 		ino += mp->m_inoadd;

@@ -519,9 +519,7 @@ xfs_dir2_block_getdents(
 		 */
 		if (filldir(dirent, dep->name, dep->namelen, cook,
 			    ino, DT_UNKNOWN)) {
-			*offset = xfs_dir2_db_off_to_dataptr(mp,
-					mp->m_dirdatablk,
-					(char *)dep - (char *)block);
+			*offset = cook;
 			xfs_da_brelse(NULL, bp);
 			return 0;
 		}

Index: 2.6.x-xfs-new/fs/xfs/xfs_dir2_leaf.c
===================================================================

fs/xfs/xfs_dir2_leaf.c +1 -1

@@ -1091,7 +1091,7 @@ xfs_dir2_leaf_getdents(
 		 * Won't fit.  Return to caller.
 		 */
 		if (filldir(dirent, dep->name, dep->namelen,
-			    xfs_dir2_byte_to_dataptr(mp, curoff + length),
+			    xfs_dir2_byte_to_dataptr(mp, curoff),
 			    ino, DT_UNKNOWN))
 			break;
 

Index: 2.6.x-xfs-new/fs/xfs/xfs_dir2_sf.c
===================================================================

fs/xfs/xfs_dir2_sf.c +3 -6

@@ -752,7 +752,7 @@ xfs_dir2_sf_getdents(
 #if XFS_BIG_INUMS
 		ino += mp->m_inoadd;
 #endif
-		if (filldir(dirent, ".", 1, dotdot_offset, ino, DT_DIR)) {
+		if (filldir(dirent, ".", 1, dot_offset, ino, DT_DIR)) {
 			*offset = dot_offset;
 			return 0;
 		}

@@ -762,13 +762,11 @@ xfs_dir2_sf_getdents(
 	 * Put .. entry unless we're starting past it.
 	 */
 	if (*offset <= dotdot_offset) {
-		off = xfs_dir2_db_off_to_dataptr(mp, mp->m_dirdatablk,
-						  XFS_DIR2_DATA_FIRST_OFFSET);
 		ino = xfs_dir2_sf_get_inumber(sfp, &sfp->hdr.parent);
 #if XFS_BIG_INUMS
 		ino += mp->m_inoadd;
 #endif
-		if (filldir(dirent, "..", 2, off, ino, DT_DIR)) {
+		if (filldir(dirent, "..", 2, dotdot_offset, ino, DT_DIR)) {
 			*offset = dotdot_offset;
 			return 0;
 		}

@@ -793,8 +791,7 @@ xfs_dir2_sf_getdents(
 #endif
 
 		if (filldir(dirent, sfep->name, sfep->namelen,
-			    off + xfs_dir2_data_entsize(sfep->namelen),
-			    ino, DT_UNKNOWN)) {
+					    off, ino, DT_UNKNOWN)) {
 			*offset = off;
 			return 0;
 		}

Index: 2.6.x-xfs-new/fs/xfs/dmapi/xfs_dm.c
===================================================================

fs/xfs/dmapi/xfs_dm.c +0 -4

@@ -1772,7 +1772,6 @@ xfs_dm_get_dioinfo(
 
 typedef struct dm_readdir_cb {
 	xfs_mount_t		*mp;
-	xfs_off_t		lastoff;
 	char __user		*ubuf;
 	dm_stat_t __user	*lastbuf;
 	size_t			spaceleft;

@@ -1834,7 +1833,6 @@ dm_filldir(void *__buf, const char *name
 	cb->spaceleft -= statp->_link;
 	cb->nwritten += statp->_link;
 	cb->ubuf += statp->_link;
-	cb->lastoff = offset;
 
 	return 0;
 

@@ -1910,8 +1908,6 @@ xfs_dm_get_dirattrs_rvp(
 		goto out_kfree;
 	}
 
-	loc = cb->lastoff;
-
 	error = -EFAULT;
 	if (cb->lastbuf && put_user(0, &cb->lastbuf->_link))
 		goto out_kfree;

Index: 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_file.c
===================================================================

fs/xfs/linux-2.6/xfs_file.c +2 -2

@@ -357,13 +357,13 @@ xfs_file_readdir(
 
 			reclen = sizeof(struct hack_dirent) + de->namlen;
 			size -= reclen;
-			curr_offset = de->offset /* & 0x7fffffff */;
 			de = (struct hack_dirent *)((char *)de + reclen);
+			curr_offset = de->offset /* & 0x7fffffff */;
 		}
 	}
 
  done:
- 	if (!error) {
+	if (!error) {
 		if (size == 0)
 			filp->f_pos = offset & 0x7fffffff;
 		else if (de)