* Re: DMAPI problem with the new filldir implementation
[not found] <475879DB.40705@sgi.com>
@ 2007-12-07 0:29 ` David Chinner
2007-12-07 1:49 ` David Chinner
0 siblings, 1 reply; 4+ messages in thread
From: David Chinner @ 2007-12-07 0:29 UTC (permalink / raw)
To: Vlad Apostolov
Cc: Christoph Hellwig, David Chinner, Mark Goodwin, xfs-dev, xfs-oss
(cc xfs-dev and xfs@oss)
On Fri, Dec 07, 2007 at 09:38:19AM +1100, Vlad Apostolov wrote:
> 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().
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 */
>>>>>> 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.
> I tried this simple patch
>
> --- a/fs/xfs/dmapi/xfs_dm.c 2007-12-03 14:04:10.000000000 +1100
> +++ b/fs/xfs/dmapi/xfs_dm.c 2007-12-03 11:55:12.000000000 +1100
> @@ -1910,7 +1910,6 @@
> goto out_kfree;
> }
>
> - loc = cb->lastoff;
Ok, which is using the offset returned by xfs_readdir (the offset of
the last dirent successfully read) instead of that used in
dm_filldir which is the offset of the next dir.
>
> 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.
Ok, patch attached. passes 144 on all inode sizes, otherwise mostly
untested.
Cheers,
Dave.
--
Dave Chinner
Principal Engineer
SGI Australian Software Group
---
fs/xfs/dmapi/xfs_dm.c | 4 ----
fs/xfs/xfs_dir2_block.c | 6 ++----
fs/xfs/xfs_dir2_leaf.c | 2 +-
fs/xfs/xfs_dir2_sf.c | 9 +++------
4 files changed, 6 insertions(+), 15 deletions(-)
Index: 2.6.x-xfs-new/fs/xfs/xfs_dir2_block.c
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_dir2_block.c 2007-08-23 23:03:09.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_dir2_block.c 2007-12-07 10:23:14.933645761 +1100
@@ -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
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_dir2_leaf.c 2007-08-24 22:24:45.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_dir2_leaf.c 2007-12-07 10:18:17.623544813 +1100
@@ -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
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_dir2_sf.c 2007-08-23 23:03:09.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_dir2_sf.c 2007-12-07 10:20:57.887115812 +1100
@@ -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
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/dmapi/xfs_dm.c 2007-12-03 17:11:46.000000000 +1100
+++ 2.6.x-xfs-new/fs/xfs/dmapi/xfs_dm.c 2007-12-07 11:25:42.678472255 +1100
@@ -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;
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: DMAPI problem with the new filldir implementation
2007-12-07 0:29 ` DMAPI problem with the new filldir implementation David Chinner
@ 2007-12-07 1:49 ` David Chinner
2007-12-10 7:04 ` [review please] " David Chinner
2007-12-10 20:48 ` Christoph Hellwig
0 siblings, 2 replies; 4+ messages in thread
From: David Chinner @ 2007-12-07 1:49 UTC (permalink / raw)
To: David Chinner
Cc: Vlad Apostolov, Christoph Hellwig, Mark Goodwin, xfs-dev, xfs-oss
On Fri, Dec 07, 2007 at 11:29:22AM +1100, David Chinner wrote:
> (cc xfs-dev and xfs@oss)
>
> On Fri, Dec 07, 2007 at 09:38:19AM +1100, Vlad Apostolov wrote:
> > 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().
>
> 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 */
> >>>>>> 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.
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>
> 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.
--
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
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_dir2_block.c 2007-08-23 23:03:09.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_dir2_block.c 2007-12-07 10:23:14.933645761 +1100
@@ -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
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_dir2_leaf.c 2007-08-24 22:24:45.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_dir2_leaf.c 2007-12-07 10:18:17.623544813 +1100
@@ -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
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/xfs_dir2_sf.c 2007-08-23 23:03:09.000000000 +1000
+++ 2.6.x-xfs-new/fs/xfs/xfs_dir2_sf.c 2007-12-07 10:20:57.887115812 +1100
@@ -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
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/dmapi/xfs_dm.c 2007-12-03 17:11:46.000000000 +1100
+++ 2.6.x-xfs-new/fs/xfs/dmapi/xfs_dm.c 2007-12-07 11:25:42.678472255 +1100
@@ -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
===================================================================
--- 2.6.x-xfs-new.orig/fs/xfs/linux-2.6/xfs_file.c 2007-12-03 18:41:26.000000000 +1100
+++ 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_file.c 2007-12-07 12:36:52.123061435 +1100
@@ -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)
^ permalink raw reply [flat|nested] 4+ messages in thread
* [review please] Re: DMAPI problem with the new filldir implementation
2007-12-07 1:49 ` David Chinner
@ 2007-12-10 7:04 ` David Chinner
2007-12-10 20:48 ` Christoph Hellwig
1 sibling, 0 replies; 4+ messages in thread
From: David Chinner @ 2007-12-10 7:04 UTC (permalink / raw)
To: David Chinner
Cc: Vlad Apostolov, Christoph Hellwig, Mark Goodwin, xfs-dev, xfs-oss
This passes xfsqa and various other handrolled tests I've thrown
at it. Can i get some eyeballs on this, please?
Cheers,
Dave.
On Fri, Dec 07, 2007 at 12:49:40PM +1100, David Chinner wrote:
> On Fri, Dec 07, 2007 at 11:29:22AM +1100, David Chinner wrote:
> > (cc xfs-dev and xfs@oss)
> >
> > On Fri, Dec 07, 2007 at 09:38:19AM +1100, Vlad Apostolov wrote:
> > > 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().
> >
> > 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 */
> > >>>>>> 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.
>
> 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>
>
> > 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.
> --
> 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
> ===================================================================
> --- 2.6.x-xfs-new.orig/fs/xfs/xfs_dir2_block.c 2007-08-23 23:03:09.000000000 +1000
> +++ 2.6.x-xfs-new/fs/xfs/xfs_dir2_block.c 2007-12-07 10:23:14.933645761 +1100
> @@ -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
> ===================================================================
> --- 2.6.x-xfs-new.orig/fs/xfs/xfs_dir2_leaf.c 2007-08-24 22:24:45.000000000 +1000
> +++ 2.6.x-xfs-new/fs/xfs/xfs_dir2_leaf.c 2007-12-07 10:18:17.623544813 +1100
> @@ -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
> ===================================================================
> --- 2.6.x-xfs-new.orig/fs/xfs/xfs_dir2_sf.c 2007-08-23 23:03:09.000000000 +1000
> +++ 2.6.x-xfs-new/fs/xfs/xfs_dir2_sf.c 2007-12-07 10:20:57.887115812 +1100
> @@ -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
> ===================================================================
> --- 2.6.x-xfs-new.orig/fs/xfs/dmapi/xfs_dm.c 2007-12-03 17:11:46.000000000 +1100
> +++ 2.6.x-xfs-new/fs/xfs/dmapi/xfs_dm.c 2007-12-07 11:25:42.678472255 +1100
> @@ -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
> ===================================================================
> --- 2.6.x-xfs-new.orig/fs/xfs/linux-2.6/xfs_file.c 2007-12-03 18:41:26.000000000 +1100
> +++ 2.6.x-xfs-new/fs/xfs/linux-2.6/xfs_file.c 2007-12-07 12:36:52.123061435 +1100
> @@ -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)
--
Dave Chinner
Principal Engineer
SGI Australian Software Group
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: DMAPI problem with the new filldir implementation
2007-12-07 1:49 ` David Chinner
2007-12-10 7:04 ` [review please] " David Chinner
@ 2007-12-10 20:48 ` Christoph Hellwig
1 sibling, 0 replies; 4+ messages in thread
From: Christoph Hellwig @ 2007-12-10 20:48 UTC (permalink / raw)
To: David Chinner
Cc: Vlad Apostolov, Christoph Hellwig, Mark Goodwin, xfs-dev, xfs-oss
Looks good to me. Might be worth to add a test that does a readdir
with a small buffer where only "." fits so we don't regress that special
case.
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2007-12-10 22:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <475879DB.40705@sgi.com>
2007-12-07 0:29 ` DMAPI problem with the new filldir implementation David Chinner
2007-12-07 1:49 ` David Chinner
2007-12-10 7:04 ` [review please] " David Chinner
2007-12-10 20:48 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox