getdents64 problem in 2.6.23

getdents64 problem in 2.6.23

[Joakim Tjernlund]

started to use 2.6.23 in our development system a few days ago and
now I got problems when deleting files from a large directory structure.
A rm -rf does not delete all files.

Did a strace and found that some files does not appear in the
strace, the same ones that doesn't get deleted.

The files that remain are in two directories:

./doc/images:
enm_topleft.gif*
h_10g.gif*
h_cu.gif*
h_cuSfp.gif*
h_fpuym.gif*
h_mdu4T2F.gif*
h_mxp16.gif*
h_oaMon.gif*
h_oiu50.gif*
h_qmr.gif*
h_unexp_half.gif*
saveHoover.gif*
saveHooverWarn.gif*
showNormal.gif*
tab_sel.gif*
top-bar.jpg*
v_d10gbe.gif*
v_empty.gif*
v_gbemxp.gif*
v_mdu40.gif*
v_mdu4E.gif*
v_mdu4T.gif*
v_mdu8E.gif*
v_mdu8T.gif*
v_oa2Mon.gif*
v_sync.gif*
v_unexp.gif*
v_voa.gif*
wizard_logo.gif*

./lib:
liblum_mib_help_text_if.so.1@
libmgmt_mibserver_if.so.1@
libtsiif.so.1.0.0

looking at the strace a bit futher reveals:

open("cuappl02a-r10a-071024_yyy/doc/images", O_RDONLY|O_NONBLOCK|
O_LARGEFILE|O_DIRECTORY) = 5
fstat64(5, {st_mode=S_IFDIR|0755, st_size=0, ...}) = 0
fcntl64(5, F_SETFD, FD_CLOEXEC)         = 0
getdents64(5, /* 119 entries */, 4096)  = 4064
lstat64("cuappl02a-r10a-071024_yyy/doc/images/v_roadm.gif",
{st_mode=S_IFREG|0755, st_size=2717, ...}) = 0
unlink("cuappl02a-r10a-071024_yyy/doc/images/v_roadm.gif") = 0
lstat64("cuappl02a-r10a-071024_yyy/doc/images/h_du.gif",
{st_mode=S_IFREG|0755, st_size=1632, ...}) = 0
unlink("cuappl02a-r10a-071024_yyy/doc/images/h_du.gif") = 0
....
getdents64(5, /* 0 entries */, 4096)    = 0
close(5)                                = 0

Notice the last getdents64 returning 0, I think this is wrong as there
is more the 119 files in this directory.

 Jocke

RE: getdents64 problem in 2.6.23

[Joakim Tjernlund]

 

> -----Original Message-----
> From: linux-mtd-bounces@lists.infradead.org 
> [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of 
> Joakim Tjernlund
> Sent: den 24 oktober 2007 15:48
> To: Linux-MTD Mailing List
> Subject: getdents64 problem in 2.6.23
> 
> started to use 2.6.23 in our development system a few days ago and
> now I got problems when deleting files from a large directory 
> structure.
> A rm -rf does not delete all files.
> 
> Did a strace and found that some files does not appear in the
> strace, the same ones that doesn't get deleted.
> 
> The files that remain are in two directories:
> 
> ./doc/images:
> enm_topleft.gif*
> h_10g.gif*
> h_cu.gif*
> h_cuSfp.gif*
> h_fpuym.gif*
> h_mdu4T2F.gif*
> h_mxp16.gif*
> h_oaMon.gif*
> h_oiu50.gif*
> h_qmr.gif*
> h_unexp_half.gif*
> saveHoover.gif*
> saveHooverWarn.gif*
> showNormal.gif*
> tab_sel.gif*
> top-bar.jpg*
> v_d10gbe.gif*
> v_empty.gif*
> v_gbemxp.gif*
> v_mdu40.gif*
> v_mdu4E.gif*
> v_mdu4T.gif*
> v_mdu8E.gif*
> v_mdu8T.gif*
> v_oa2Mon.gif*
> v_sync.gif*
> v_unexp.gif*
> v_voa.gif*
> wizard_logo.gif*
> 
> ./lib:
> liblum_mib_help_text_if.so.1@
> libmgmt_mibserver_if.so.1@
> libtsiif.so.1.0.0
> 
> looking at the strace a bit futher reveals:
> 
> open("cuappl02a-r10a-071024_yyy/doc/images", O_RDONLY|O_NONBLOCK|
> O_LARGEFILE|O_DIRECTORY) = 5
> fstat64(5, {st_mode=S_IFDIR|0755, st_size=0, ...}) = 0
> fcntl64(5, F_SETFD, FD_CLOEXEC)         = 0
> getdents64(5, /* 119 entries */, 4096)  = 4064
> lstat64("cuappl02a-r10a-071024_yyy/doc/images/v_roadm.gif",
> {st_mode=S_IFREG|0755, st_size=2717, ...}) = 0
> unlink("cuappl02a-r10a-071024_yyy/doc/images/v_roadm.gif") = 0
> lstat64("cuappl02a-r10a-071024_yyy/doc/images/h_du.gif",
> {st_mode=S_IFREG|0755, st_size=1632, ...}) = 0
> unlink("cuappl02a-r10a-071024_yyy/doc/images/h_du.gif") = 0
> ....
> getdents64(5, /* 0 entries */, 4096)    = 0
> close(5)                                = 0
> 
> Notice the last getdents64 returning 0, I think this is wrong as there
> is more the 119 files in this directory.
> 
>  Jocke

David, could you comment? I need some advice on how to proceed.

 Jocke

Re: getdents64 problem in 2.6.23

[David Woodhouse]

On Wed, 2007-10-24 at 15:47 +0200, Joakim Tjernlund wrote:
> started to use 2.6.23 in our development system a few days ago and
> now I got problems when deleting files from a large directory
> structure. A rm -rf does not delete all files.
> 
> Did a strace and found that some files does not appear in the
> strace, the same ones that doesn't get deleted.

Following up on the IRC conversation... the problem here is the horrid
readdir() interface, and how it interacts with f_pos on the directory's
file descriptor. The file position simply represents the offset within
the linked list of dirents -- which used to be fairly safe, because when
we unlinked something, we'd write out a 'deletion dirent', which would
serve as a placeholder in the list.

Someone changed that code recently though, and those deletion dirents
are no longer written on NOR flash.... :)

The answer is probably to keep a 'deletion dirent' in the f->dents list
of jffs2_full_dirents, but with NULL for the 'raw' field. I think that
shouldn't break any other part of the code. Then you'd want to remove
those dummy dirents when the filedescriptor for the directory in
question is closed.

-- 
dwmw2

RE: getdents64 problem in 2.6.23

[Joakim Tjernlund]

 

> -----Original Message-----
> From: David Woodhouse [mailto:dwmw2@infradead.org] 
> Sent: den 26 oktober 2007 21:14
> To: joakim.tjernlund@transmode.se
> Cc: Linux-MTD Mailing List
> Subject: Re: getdents64 problem in 2.6.23
> 
> On Wed, 2007-10-24 at 15:47 +0200, Joakim Tjernlund wrote:
> > started to use 2.6.23 in our development system a few days ago and
> > now I got problems when deleting files from a large directory
> > structure. A rm -rf does not delete all files.
> > 
> > Did a strace and found that some files does not appear in the
> > strace, the same ones that doesn't get deleted.
> 
> Following up on the IRC conversation... the problem here is the horrid
> readdir() interface, and how it interacts with f_pos on the 
> directory's
> file descriptor. The file position simply represents the offset within
> the linked list of dirents -- which used to be fairly safe, 
> because when
> we unlinked something, we'd write out a 'deletion dirent', which would
> serve as a placeholder in the list.
> 
> Someone changed that code recently though, and those deletion dirents
> are no longer written on NOR flash.... :)

Yeah, I wonder who that was. Can't really recall :)

> 
> The answer is probably to keep a 'deletion dirent' in the 
> f->dents list
> of jffs2_full_dirents, but with NULL for the 'raw' field. I think that
> shouldn't break any other part of the code. Then you'd want to remove
> those dummy dirents when the filedescriptor for the directory in
> question is closed.

Is this what you mean(just a stupid hack for now)?
          //*prev = this->next;
            jffs2_mark_node_obsolete(c, (this->raw));
            this->raw = NULL;
          //jffs2_free_full_dirent(this);

Perhaps add a jffs2_add_fd_to_list(c, this, &dir_f->dents)?

How do I find where the filedescriptor is closed?

 Jocke

RE: getdents64 problem in 2.6.23

[David Woodhouse]

On Sat, 2007-10-27 at 01:03 +0200, Joakim Tjernlund wrote:
> Is this what you mean(just a stupid hack for now)?
>           //*prev = this->next;
>             jffs2_mark_node_obsolete(c, (this->raw));
>             this->raw = NULL;
>           //jffs2_free_full_dirent(this);

Yeah, something like that. And we also need to modify
jffs2_add_fd_to_list() so that if we later add a new dirent which
_replaces_ this one, it doesn't oops after calling
jffs2_mark_node_obsolete(c, fd->raw);

> Perhaps add a jffs2_add_fd_to_list(c, this, &dir_f->dents)?

Nah, no need for that. It's already _in_ the list.

> How do I find where the filedescriptor is closed?

We probably need to implement a release() operation in the
jffs2_dir_operations (top of dir.c), which will remove the fake
'deletion' dirents if !atomic_read(&inode->i_count). Or something like
that.

-- 
dwmw2

RE: getdents64 problem in 2.6.23

[Joakim Tjernlund]

On Fri, 2007-10-26 at 19:17 -0400, David Woodhouse wrote:
> On Sat, 2007-10-27 at 01:03 +0200, Joakim Tjernlund wrote:
> > Is this what you mean(just a stupid hack for now)?
> >           //*prev = this->next;
> >             jffs2_mark_node_obsolete(c, (this->raw));
> >             this->raw = NULL;
> >           //jffs2_free_full_dirent(this);
> 
> Yeah, something like that. And we also need to modify
> jffs2_add_fd_to_list() so that if we later add a new dirent which
> _replaces_ this one, it doesn't oops after calling
> jffs2_mark_node_obsolete(c, fd->raw);
> 
> > Perhaps add a jffs2_add_fd_to_list(c, this, &dir_f->dents)?
> 
> Nah, no need for that. It's already _in_ the list.
> 
> > How do I find where the filedescriptor is closed?
> 
> We probably need to implement a release() operation in the
> jffs2_dir_operations (top of dir.c), which will remove the fake
> 'deletion' dirents if !atomic_read(&inode->i_count). Or something like
> that.
> 

Next iteration. I am rather lost in the release function.
Some hints would be great.

 Jocke

diff --git a/fs/jffs2/dir.c b/fs/jffs2/dir.c
index 8353eb9..a658bcb 100644
--- a/fs/jffs2/dir.c
+++ b/fs/jffs2/dir.c
@@ -20,6 +20,7 @@
 #include "nodelist.h"
 
 static int jffs2_readdir (struct file *, void *, filldir_t);
+static int jffs2_release (struct inode *, struct file *);
 
 static int jffs2_create (struct inode *,struct dentry *,int,
 			 struct nameidata *);
@@ -39,7 +40,8 @@ const struct file_operations jffs2_dir_operations =
 	.read =		generic_read_dir,
 	.readdir =	jffs2_readdir,
 	.ioctl =	jffs2_ioctl,
-	.fsync =	jffs2_fsync
+	.fsync =	jffs2_fsync,
+	.release =	jffs2_release,
 };
 
 
@@ -174,6 +176,25 @@ static int jffs2_readdir(struct file *filp, void *dirent, filldir_t filldir)
 
 /***********************************************************************/
 
+static int jffs2_release(struct inode *dir_i, struct file *file)
+{
+	//struct jffs2_sb_info *c = JFFS2_SB_INFO(dir_i->i_sb);
+	struct jffs2_inode_info *dir_f = JFFS2_INODE_INFO(dir_i);
+	struct jffs2_full_dirent **prev = &dir_f->dents;
+
+	if (atomic_read(&dir_i->i_count))
+		return 0;
+
+	while (*prev) {
+		D1(printk(KERN_DEBUG "Releasing directory inode:%d\n", (*prev)->ino));
+
+		jffs2_free_full_dirent(*prev);
+		prev = &((*prev)->next);
+	}
+	return 0;
+}
+
+/***********************************************************************/
 
 static int jffs2_create(struct inode *dir_i, struct dentry *dentry, int mode,
 			struct nameidata *nd)
diff --git a/fs/jffs2/nodelist.c b/fs/jffs2/nodelist.c
index 4bf8608..5419023 100644
--- a/fs/jffs2/nodelist.c
+++ b/fs/jffs2/nodelist.c
@@ -34,13 +34,15 @@ void jffs2_add_fd_to_list(struct jffs2_sb_info *c, struct jffs2_full_dirent *new
 			if (new->version < (*prev)->version) {
 				dbg_dentlist("Eep! Marking new dirent node is obsolete, old is \"%s\", ino #%u\n",
 					(*prev)->name, (*prev)->ino);
-				jffs2_mark_node_obsolete(c, new->raw);
+				if (new->raw)
+					jffs2_mark_node_obsolete(c, new->raw);
 				jffs2_free_full_dirent(new);
 			} else {
 				dbg_dentlist("marking old dirent \"%s\", ino #%u bsolete\n",
 					(*prev)->name, (*prev)->ino);
 				new->next = (*prev)->next;
-				jffs2_mark_node_obsolete(c, ((*prev)->raw));
+				if ((*prev)->raw)
+					jffs2_mark_node_obsolete(c, ((*prev)->raw));
 				jffs2_free_full_dirent(*prev);
 				*prev = new;
 			}
diff --git a/fs/jffs2/write.c b/fs/jffs2/write.c
index 2f56954..11e8b5f 100644
--- a/fs/jffs2/write.c
+++ b/fs/jffs2/write.c
@@ -591,9 +591,8 @@ int jffs2_do_unlink(struct jffs2_sb_info *c, struct jffs2_inode_info *dir_f,
 				D1(printk(KERN_DEBUG "Marking old dirent node (ino #%u) @%08x obsolete\n",
 					  this->ino, ref_offset(this->raw)));
 
-				*prev = this->next;
 				jffs2_mark_node_obsolete(c, (this->raw));
-				jffs2_free_full_dirent(this);
+				this->raw = NULL;
 				break;
 			}
 			prev = &((*prev)->next);

RE: getdents64 problem in 2.6.23

[David Woodhouse]

On Sat, 2007-10-27 at 13:02 +0200, Joakim Tjernlund wrote:
> 
> +static int jffs2_release(struct inode *dir_i, struct file *file)
> +{
> +       //struct jffs2_sb_info *c = JFFS2_SB_INFO(dir_i->i_sb);
> +       struct jffs2_inode_info *dir_f = JFFS2_INODE_INFO(dir_i);
> +       struct jffs2_full_dirent **prev = &dir_f->dents;
> +
> +       if (atomic_read(&dir_i->i_count))
> +               return 0;

That's certainly what I was thinking -- but do check that it's right.
It's possible that you'll have to do refcounting some other way.

> +       while (*prev) {
> +               D1(printk(KERN_DEBUG "Releasing directory inode:%d\n", (*prev)->ino));
> +
> +               jffs2_free_full_dirent(*prev);
> +               prev = &((*prev)->next);
> +       }

That'll kill _everything_, so if the inode is subsequently reopened
before it's pruned from the icache, it'll appear empty. You were only
supposed to remove the dirents where fd->raw == NULL; the ones which
were acting as 'placeholders' to keep seeks in the directory's opened
filedescriptors working consistently.

-- 
dwmw2

RE: getdents64 problem in 2.6.23

[Joakim Tjernlund]

 

> -----Original Message-----
> From: David Woodhouse [mailto:dwmw2@infradead.org] 
> Sent: den 27 oktober 2007 14:44
> To: joakim.tjernlund@transmode.se
> Cc: 'Linux-MTD Mailing List'
> Subject: RE: getdents64 problem in 2.6.23
> 
> 
> On Sat, 2007-10-27 at 13:02 +0200, Joakim Tjernlund wrote:
> > 
> > +static int jffs2_release(struct inode *dir_i, struct file *file)
> > +{
> > +       //struct jffs2_sb_info *c = JFFS2_SB_INFO(dir_i->i_sb);
> > +       struct jffs2_inode_info *dir_f = JFFS2_INODE_INFO(dir_i);
> > +       struct jffs2_full_dirent **prev = &dir_f->dents;
> > +
> > +       if (atomic_read(&dir_i->i_count))
> > +               return 0;
> 
> That's certainly what I was thinking -- but do check that it's right.
> It's possible that you'll have to do refcounting some other way.

How do I do that? I can try booting it, but it has to wait until
I get acces to my board again, hopefully tonight.

What about locking? No need for down(&dir_f->sem)? Can I trust
that ->next ptr will be valid all the time?

 Jocke
> 
> > +       while (*prev) {
> > +               D1(printk(KERN_DEBUG "Releasing directory 
> inode:%d\n", (*prev)->ino));
> > +
> > +               jffs2_free_full_dirent(*prev);
> > +               prev = &((*prev)->next);
> > +       }
> 
> That'll kill _everything_, so if the inode is subsequently reopened
> before it's pruned from the icache, it'll appear empty. You were only
> supposed to remove the dirents where fd->raw == NULL; the ones which
> were acting as 'placeholders' to keep seeks in the directory's opened
> filedescriptors working consistently.

ehh, better add an if (!(*prev)->raw) test
before jffs2_free_full_dirent(*prev) then. Will clean it up too.

 Jocke

RE: getdents64 problem in 2.6.23

[David Woodhouse]

On Sat, 2007-10-27 at 17:01 +0200, Joakim Tjernlund wrote:
> How do I do that?

Add some debugging and check that it's happening at the times you
expect. And there's no _real_ substitute for the Feynman algorithm to
problem-solving. :)

>  I can try booting it, but it has to wait until
> I get acces to my board again, hopefully tonight.
> 
> What about locking? No need for down(&dir_f->sem)? Can I trust
> that ->next ptr will be valid all the time?

You'll definitely need locking, to protect against it being opened while
you're playing with it. I think that just locking dir_f->sem before
checking i_count probably ought to suffice.

> ehh, better add an if (!(*prev)->raw) test
> before jffs2_free_full_dirent(*prev) then. Will clean it up too.

You might try the unconventional step of _not_ using the dirent
structure after freeing it, too. And remember that if you're not freeing
the whole list, you're going to have to play with the list pointers to
keep it intact.

-- 
dwmw2

RE: getdents64 problem in 2.6.23

[Joakim Tjernlund]

 

> -----Original Message-----
> From: David Woodhouse [mailto:dwmw2@infradead.org] 
> Sent: den 27 oktober 2007 19:09
> To: Joakim Tjernlund
> Cc: 'Linux-MTD Mailing List'
> Subject: RE: getdents64 problem in 2.6.23
> 
> On Sat, 2007-10-27 at 17:01 +0200, Joakim Tjernlund wrote:
> > How do I do that?
> 
> Add some debugging and check that it's happening at the times you
> expect. And there's no _real_ substitute for the Feynman algorithm to
> problem-solving. :)
> 
> >  I can try booting it, but it has to wait until
> > I get acces to my board again, hopefully tonight.
> > 
> > What about locking? No need for down(&dir_f->sem)? Can I trust
> > that ->next ptr will be valid all the time?
> 
> You'll definitely need locking, to protect against it being 
> opened while
> you're playing with it. I think that just locking dir_f->sem before
> checking i_count probably ought to suffice.
> 
> > ehh, better add an if (!(*prev)->raw) test
> > before jffs2_free_full_dirent(*prev) then. Will clean it up too.
> 
> You might try the unconventional step of _not_ using the dirent
> structure after freeing it, too. And remember that if you're 
> not freeing
> the whole list, you're going to have to play with the list pointers to
> keep it intact.

:), I noticed that. Now I do:
  while (*prev) {
                this = *prev;
                if (!this->raw) {
                        *prev = this->next;
                        jffs2_free_full_dirent(this);
                }
                prev = &((*prev)->next);
        }

However that doesn't matter because the relese method isn't called while
doing rm!

I added som printk's and they were quiet. On the other hand doing an
ls does call the release method.

You need to come up with a better method I think :)

RE: getdents64 problem in 2.6.23

[Joakim Tjernlund]

 

> -----Original Message-----
> From: linux-mtd-bounces@lists.infradead.org 
> [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of 
> Joakim Tjernlund
> Sent: den 27 oktober 2007 19:21
> To: 'David Woodhouse'
> Cc: 'Linux-MTD Mailing List'
> Subject: RE: getdents64 problem in 2.6.23
> 
>  
> 
> > -----Original Message-----
> > From: David Woodhouse [mailto:dwmw2@infradead.org] 
> > Sent: den 27 oktober 2007 19:09
> > To: Joakim Tjernlund
> > Cc: 'Linux-MTD Mailing List'
> > Subject: RE: getdents64 problem in 2.6.23
> > 
> > On Sat, 2007-10-27 at 17:01 +0200, Joakim Tjernlund wrote:
> > > How do I do that?
> > 
> > Add some debugging and check that it's happening at the times you
> > expect. And there's no _real_ substitute for the Feynman 
> algorithm to
> > problem-solving. :)
> > 
> > >  I can try booting it, but it has to wait until
> > > I get acces to my board again, hopefully tonight.
> > > 
> > > What about locking? No need for down(&dir_f->sem)? Can I trust
> > > that ->next ptr will be valid all the time?
> > 
> > You'll definitely need locking, to protect against it being 
> > opened while
> > you're playing with it. I think that just locking dir_f->sem before
> > checking i_count probably ought to suffice.
> > 
> > > ehh, better add an if (!(*prev)->raw) test
> > > before jffs2_free_full_dirent(*prev) then. Will clean it up too.
> > 
> > You might try the unconventional step of _not_ using the dirent
> > structure after freeing it, too. And remember that if you're 
> > not freeing
> > the whole list, you're going to have to play with the list 
> pointers to
> > keep it intact.
> 
> :), I noticed that. Now I do:
>   while (*prev) {
>                 this = *prev;
>                 if (!this->raw) {
>                         *prev = this->next;
>                         jffs2_free_full_dirent(this);
>                 }
>                 prev = &((*prev)->next);
>         }
> 
> However that doesn't matter because the relese method isn't 
> called while
> doing rm!
> 
> I added som printk's and they were quiet. On the other hand doing an
> ls does call the release method.
> 
> You need to come up with a better method I think :)

After actually reading the code a bit I came up with this:

--- a/fs/jffs2/write.c
+++ b/fs/jffs2/write.c
@@ -590,10 +590,8 @@ int jffs2_do_unlink(struct jffs2_sb_info *c, struct jffs2_inode_info *dir_f,

                                D1(printk(KERN_DEBUG "Marking old dirent node (ino #%u) @%08x obsolete\n",
                                          this->ino, ref_offset(this->raw)));
-
-                               *prev = this->next;
-                               jffs2_mark_node_obsolete(c, (this->raw));
-                               jffs2_free_full_dirent(this);
+                               //jffs2_mark_node_obsolete(c, this->raw);
+                               this->ino = 0;
                                break;
                        }
                        prev = &((*prev)->next);

I works, even for big directories.

I noticed that the node is obsolteted a few lines below:
if (dead_f && dead_f->inocache) {
....
   jffs2_mark_node_obsolete(c, fd->raw);

Is these cases when dead_f && dead_f->inocache isn't true
so one neededs to uncomment the 
  //jffs2_mark_node_obsolete(c, this->raw);
line?

 Jocke

RE: getdents64 problem in 2.6.23

[David Woodhouse]

On Sat, 2007-10-27 at 22:31 +0200, Joakim Tjernlund wrote:
> 
> > -----Original Message-----
> > From: linux-mtd-bounces@lists.infradead.org 
> > [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of 
> > Joakim Tjernlund
> > Sent: den 27 oktober 2007 19:21
> > To: 'David Woodhouse'
> > Cc: 'Linux-MTD Mailing List'
> > Subject: RE: getdents64 problem in 2.6.23
> > 
> >  
> > 
> > > -----Original Message-----
> > > From: David Woodhouse [mailto:dwmw2@infradead.org] 
> > > Sent: den 27 oktober 2007 19:09
> > > To: Joakim Tjernlund
> > > Cc: 'Linux-MTD Mailing List'
> > > Subject: RE: getdents64 problem in 2.6.23
> > > 
> > > On Sat, 2007-10-27 at 17:01 +0200, Joakim Tjernlund wrote:
> > > > How do I do that?
> > > 
> > > Add some debugging and check that it's happening at the times you
> > > expect. And there's no _real_ substitute for the Feynman 
> > algorithm to
> > > problem-solving. :)
> > > 
> > > >  I can try booting it, but it has to wait until
> > > > I get acces to my board again, hopefully tonight.
> > > > 
> > > > What about locking? No need for down(&dir_f->sem)? Can I trust
> > > > that ->next ptr will be valid all the time?
> > > 
> > > You'll definitely need locking, to protect against it being 
> > > opened while
> > > you're playing with it. I think that just locking dir_f->sem before
> > > checking i_count probably ought to suffice.
> > > 
> > > > ehh, better add an if (!(*prev)->raw) test
> > > > before jffs2_free_full_dirent(*prev) then. Will clean it up too.
> > > 
> > > You might try the unconventional step of _not_ using the dirent
> > > structure after freeing it, too. And remember that if you're 
> > > not freeing
> > > the whole list, you're going to have to play with the list 
> > pointers to
> > > keep it intact.
> > 
> > :), I noticed that. Now I do:
> >   while (*prev) {
> >                 this = *prev;
> >                 if (!this->raw) {
> >                         *prev = this->next;
> >                         jffs2_free_full_dirent(this);
> >                 }
> >                 prev = &((*prev)->next);
> >         }
> > 
> > However that doesn't matter because the relese method isn't 
> > called while
> > doing rm!
> > 
> > I added som printk's and they were quiet. On the other hand doing an
> > ls does call the release method.
> > 
> > You need to come up with a better method I think :)
> 
> After actually reading the code a bit I came up with this:
> 
> --- a/fs/jffs2/write.c
> +++ b/fs/jffs2/write.c
> @@ -590,10 +590,8 @@ int jffs2_do_unlink(struct jffs2_sb_info *c, struct jffs2_inode_info *dir_f,
> 
>                                 D1(printk(KERN_DEBUG "Marking old dirent node (ino #%u) @%08x obsolete\n",
>                                           this->ino, ref_offset(this->raw)));
> -
> -                               *prev = this->next;
> -                               jffs2_mark_node_obsolete(c, (this->raw));
> -                               jffs2_free_full_dirent(this);
> +                               //jffs2_mark_node_obsolete(c, this->raw);
> +                               this->ino = 0;
>                                 break;
>                         }
>                         prev = &((*prev)->next);

I think you should set this->raw to NULL too, to avoid having stale
links to obsolete dirent nodes.

And yes, it'll work -- but you'll never be removing those 'deletion
dirents' from the lists, except when the inode in question is removed
completely from the icache. I think we should try to clean up more often
than that, which is why I suggested the code in a ->release() function.

That release() function really _ought_ to be invoked when rm(1) opens
and subsequently closes the directory.

-- 
dwmw2

Re: getdents64 problem in 2.6.23

[Jörn Engel]

On Fri, 26 October 2007 19:17:40 -0400, David Woodhouse wrote:
> 
> We probably need to implement a release() operation in the
> jffs2_dir_operations (top of dir.c), which will remove the fake
> 'deletion' dirents if !atomic_read(&inode->i_count). Or something like
> that.

I hate to spoil the fun, but this can cause problems with nfs.

It is legal for nfs to call telldir, close the directory, wait for half
a year, then open the directory, call seekdir and expect sane results.
Not pretty at all for filesystem implementors.

So either these deletion dirents need to stay around or you have to
convert the i_pos "cookie" to a hash of the filename or so or at least
explicitly document that you have broken nfs under some circumstances.

Of course any other program could do the telldir/seekdir stunt as well.
Nfs just appears to be the only one doing this madness in practice.

Jörn

-- 
Doubt is not a pleasant condition, but certainty is an absurd one.
-- Voltaire

RE: getdents64 problem in 2.6.23

[Joakim Tjernlund]

> -----Original Message-----
> From: David Woodhouse [mailto:dwmw2@infradead.org] 
> Sent: den 28 oktober 2007 00:37
> To: Joakim Tjernlund
> Cc: 'Linux-MTD Mailing List'
> Subject: RE: getdents64 problem in 2.6.23
> 
> 
> On Sat, 2007-10-27 at 22:31 +0200, Joakim Tjernlund wrote:
> > 
> > > -----Original Message-----
> > > From: linux-mtd-bounces@lists.infradead.org 
> > > [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of 
> > > Joakim Tjernlund
> > > Sent: den 27 oktober 2007 19:21
> > > To: 'David Woodhouse'
> > > Cc: 'Linux-MTD Mailing List'
> > > Subject: RE: getdents64 problem in 2.6.23
> > > 
> > >  
> > > 
> > > > -----Original Message-----
> > > > From: David Woodhouse [mailto:dwmw2@infradead.org] 
> > > > Sent: den 27 oktober 2007 19:09
> > > > To: Joakim Tjernlund
> > > > Cc: 'Linux-MTD Mailing List'
> > > > Subject: RE: getdents64 problem in 2.6.23
> > > > 
> > > > On Sat, 2007-10-27 at 17:01 +0200, Joakim Tjernlund wrote:
> > > > > How do I do that?
> > > > 
> > > > Add some debugging and check that it's happening at the 
> times you
> > > > expect. And there's no _real_ substitute for the Feynman 
> > > algorithm to
> > > > problem-solving. :)
> > > > 
> > > > >  I can try booting it, but it has to wait until
> > > > > I get acces to my board again, hopefully tonight.
> > > > > 
> > > > > What about locking? No need for down(&dir_f->sem)? Can I trust
> > > > > that ->next ptr will be valid all the time?
> > > > 
> > > > You'll definitely need locking, to protect against it being 
> > > > opened while
> > > > you're playing with it. I think that just locking 
> dir_f->sem before
> > > > checking i_count probably ought to suffice.
> > > > 
> > > > > ehh, better add an if (!(*prev)->raw) test
> > > > > before jffs2_free_full_dirent(*prev) then. Will clean 
> it up too.
> > > > 
> > > > You might try the unconventional step of _not_ using the dirent
> > > > structure after freeing it, too. And remember that if you're 
> > > > not freeing
> > > > the whole list, you're going to have to play with the list 
> > > pointers to
> > > > keep it intact.
> > > 
> > > :), I noticed that. Now I do:
> > >   while (*prev) {
> > >                 this = *prev;
> > >                 if (!this->raw) {
> > >                         *prev = this->next;
> > >                         jffs2_free_full_dirent(this);
> > >                 }
> > >                 prev = &((*prev)->next);
> > >         }
> > > 
> > > However that doesn't matter because the relese method isn't 
> > > called while
> > > doing rm!
> > > 
> > > I added som printk's and they were quiet. On the other 
> hand doing an
> > > ls does call the release method.
> > > 
> > > You need to come up with a better method I think :)
> > 
> > After actually reading the code a bit I came up with this:
> > 
> > --- a/fs/jffs2/write.c
> > +++ b/fs/jffs2/write.c
> > @@ -590,10 +590,8 @@ int jffs2_do_unlink(struct 
> jffs2_sb_info *c, struct jffs2_inode_info *dir_f,
> > 
> >                                 D1(printk(KERN_DEBUG 
> "Marking old dirent node (ino #%u) @%08x obsolete\n",
> >                                           this->ino, 
> ref_offset(this->raw)));
> > -
> > -                               *prev = this->next;
> > -                               jffs2_mark_node_obsolete(c, 
> (this->raw));
> > -                               jffs2_free_full_dirent(this);
> > +                               
> //jffs2_mark_node_obsolete(c, this->raw);
> > +                               this->ino = 0;
> >                                 break;
> >                         }
> >                         prev = &((*prev)->next);
> 
> I think you should set this->raw to NULL too, to avoid having stale
> links to obsolete dirent nodes.

What does the while (dead_f->dents) { .. } do then?
Seems like it is freeing stuff.

> 
> And yes, it'll work -- but you'll never be removing those 'deletion
> dirents' from the lists, except when the inode in question is removed
> completely from the icache. I think we should try to clean up 
> more often
> than that, which is why I suggested the code in a ->release() 
> function.
> 
> That release() function really _ought_ to be invoked when rm(1) opens
> and subsequently closes the directory.

But it doesn't so we are kind of stuck here. Even if it does
work I wolud prefer not to make the same mistake that got
here in the first place :)

I have changed the 
  //jffs2_mark_node_obsolete(c, this->raw);
to
  if (!(dead_f && dead_f->inocache))
          jffs2_mark_node_obsolete(c, this->raw);
and I wonder if I should add a
  jffs2_free_full_dirent(this);
in that if stmt too? 
I can add this->raw = NULL inside that if, but then one probably
should do that in lots of other places too?

Can't test for now as I managed to lock my board so I have
to wait until someone can reset it for me.

 Jocke

Re: getdents64 problem in 2.6.23

[David Woodhouse]

On Sun, 2007-10-28 at 01:18 +0200, Jörn Engel wrote:
> On Fri, 26 October 2007 19:17:40 -0400, David Woodhouse wrote:
> > 
> > We probably need to implement a release() operation in the
> > jffs2_dir_operations (top of dir.c), which will remove the fake
> > 'deletion' dirents if !atomic_read(&inode->i_count). Or something like
> > that.
> 
> I hate to spoil the fun, but this can cause problems with nfs.
> 
> It is legal for nfs to call telldir, close the directory, wait for half
> a year, then open the directory, call seekdir and expect sane results.
> Not pretty at all for filesystem implementors.
> 
> So either these deletion dirents need to stay around or you have to
> convert the i_pos "cookie" to a hash of the filename or so or at least
> explicitly document that you have broken nfs under some circumstances.

Well, the f->dents list is _already_ ordered by the hash of the
filename. The only reason we _don't_ use the hash as the i_pos 'cookie'
is because of the potential for hash collisions.

If we could deal with that, we could use trees for it instead of a
linked list.

-- 
dwmw2

Re: getdents64 problem in 2.6.23

[Jörn Engel]

On Sat, 27 October 2007 22:00:44 -0400, David Woodhouse wrote:
> 
> Well, the f->dents list is _already_ ordered by the hash of the
> filename. The only reason we _don't_ use the hash as the i_pos 'cookie'
> is because of the potential for hash collisions.
> 
> If we could deal with that, we could use trees for it instead of a
> linked list.

I'd say just do it.  You are already breaking nfs as-is.  The i_pos
cookie is required to be stable across reboots.  And since you no longer
store deletion dirents on the medium, the only thing you can do is try
to use relatively robust non-standard(1) behaviour.

One way of doing that is to favor returning the same dentries twice over
not returning them at all (and try to make it a rare event, of course).
So you can just use the hash (32bit, due to nfs protocol limitations)
as a cookie and in case of hash collision, return all colliding
dentries.  Drawback: endless loop when getdents only has enough room for
a single dentry.

Second variant is Neil Browns approach of using a 24bit hash and an 8bit
sequence number.  In case of a hash collision, all colliding dentries
are kept in a list and the sequence number is used just a i_pos is used
currently for the collision chain.  Since all these structure are not
stable across reboot, you would probably want to reset the sequence
number to zero whenever seekdir is called.

(1) Non-standard behaviour is nothing unusual.  Afaiks even ext3 with
dirhash is non-standard.

Jörn

-- 
"Translations are and will always be problematic. They inflict violence
upon two languages." (translation from German)

Re: getdents64 problem in 2.6.23

[Jamie Lokier]

Jörn Engel wrote:
> > We probably need to implement a release() operation in the
> > jffs2_dir_operations (top of dir.c), which will remove the fake
> > 'deletion' dirents if !atomic_read(&inode->i_count). Or something like
> > that.
> 
> I hate to spoil the fun, but this can cause problems with nfs.
> 
> It is legal for nfs to call telldir, close the directory, wait for half
> a year, then open the directory, call seekdir and expect sane results.
> Not pretty at all for filesystem implementors.
> 
> So either these deletion dirents need to stay around or you have to
> convert the i_pos "cookie" to a hash of the filename or so or at least
> explicitly document that you have broken nfs under some circumstances.
> 
> Of course any other program could do the telldir/seekdir stunt as well.
> Nfs just appears to be the only one doing this madness in practice.

Indeed.

However, I think it's fine to _overwrite_ the deletion dirents with
new ones.  getdentds() allows new names created during a directory
read to appear or not in the lsit.

Also, I think it's fine to _coalesce_ deletion dirents into one, as
long as the number of entries they represent is retained as a count.

If to, the number of deletion dirent structures could be bounded to be
no more than the number of active dirents.

In which case, perhaps the storage on flash needed for them could
simply be (at most) one integer per active dirent?  If that's
possible, it would provide the correct semantics for telldir/readdir
even across unmounts/reboots, without using much storage even in
pathological cases.

-- Jamie

Re: getdents64 problem in 2.6.23

[Jörn Engel]

On Sun, 28 October 2007 18:28:36 +0000, Jamie Lokier wrote:
> 
> However, I think it's fine to _overwrite_ the deletion dirents with
> new ones.  getdentds() allows new names created during a directory
> read to appear or not in the lsit.

Correct.  You can delete/overwrite dentries, but you may not rearrange
their order wrt. i_pos cookies.

Jörn

-- 
Admonish your friends privately, but praise them openly.
-- Publilius Syrus

RE: getdents64 problem in 2.6.23

[Joakim Tjernlund]

> -----Original Message-----
> From: Jörn Engel [mailto:joern@logfs.org] 
> Sent: den 29 oktober 2007 00:50
> To: Jamie Lokier
> Cc: Jörn Engel; David Woodhouse; 'Linux-MTD Mailing List'; 
> Joakim Tjernlund
> Subject: Re: getdents64 problem in 2.6.23
> 
> On Sun, 28 October 2007 18:28:36 +0000, Jamie Lokier wrote:
> > 
> > However, I think it's fine to _overwrite_ the deletion dirents with
> > new ones.  getdentds() allows new names created during a directory
> > read to appear or not in the lsit.
> 
> Correct.  You can delete/overwrite dentries, but you may not rearrange
> their order wrt. i_pos cookies.
> 
> Jörn

David, before rewriting getdents et. all may I ask you to apply the
patch I sent today? Then we will have something that works and go
from there.

     Jocke

RE: getdents64 problem in 2.6.23

[David Woodhouse]

On Mon, 2007-10-29 at 00:58 +0100, Joakim Tjernlund wrote:
> David, before rewriting getdents et. all may I ask you to apply the
> patch I sent today? Then we will have something that works and go
> from there.

I'm not happy about having fd->raw still pointing to nodes which may no
longer exist. Can't we set it to NULL (and cope with potential fallout)?

-- 
dwmw2

RE: getdents64 problem in 2.6.23

[Joakim Tjernlund]

On Sun, 2007-10-28 at 20:26 -0400, David Woodhouse wrote:
> On Mon, 2007-10-29 at 00:58 +0100, Joakim Tjernlund wrote:
> > David, before rewriting getdents et. all may I ask you to apply the
> > patch I sent today? Then we will have something that works and go
> > from there.
> 
> I'm not happy about having fd->raw still pointing to nodes which may no
> longer exist. Can't we set it to NULL (and cope with potential fallout)?

Thats a diffrent story. Now I fixed the bug so it behaves like the 
!mark_node_obsolete case. Walking through all ->raw references is
something I will save for a rainy dag, I need to fix a few other bugs in
our system ATM.

   Jocke

RE: getdents64 problem in 2.6.23

[Joakim Tjernlund]

On Mon, 2007-10-29 at 13:58 +0100, Joakim Tjernlund wrote:
> On Sun, 2007-10-28 at 20:26 -0400, David Woodhouse wrote:
> > On Mon, 2007-10-29 at 00:58 +0100, Joakim Tjernlund wrote:
> > > David, before rewriting getdents et. all may I ask you to apply the
> > > patch I sent today? Then we will have something that works and go
> > > from there.
> > 
> > I'm not happy about having fd->raw still pointing to nodes which may no
> > longer exist. Can't we set it to NULL (and cope with potential fallout)?
> 
> Thats a diffrent story. Now I fixed the bug so it behaves like the 
> !mark_node_obsolete case. Walking through all ->raw references is
> something I will save for a rainy dag, I need to fix a few other bugs in
> our system ATM.

ehh, my patch may be somewhat wrong, need to reflect a bit on that. I
might even get to the ->raw = NULL thing :)

RE: getdents64 problem in 2.6.23

[Joakim Tjernlund]

On Mon, 2007-10-29 at 18:30 +0100, Joakim Tjernlund wrote:
> On Mon, 2007-10-29 at 13:58 +0100, Joakim Tjernlund wrote:
> > On Sun, 2007-10-28 at 20:26 -0400, David Woodhouse wrote:
> > > On Mon, 2007-10-29 at 00:58 +0100, Joakim Tjernlund wrote:
> > > > David, before rewriting getdents et. all may I ask you to apply the
> > > > patch I sent today? Then we will have something that works and go
> > > > from there.
> > > 
> > > I'm not happy about having fd->raw still pointing to nodes which may no
> > > longer exist. Can't we set it to NULL (and cope with potential fallout)?
> > 
> > Thats a diffrent story. Now I fixed the bug so it behaves like the 
> > !mark_node_obsolete case. Walking through all ->raw references is
> > something I will save for a rainy dag, I need to fix a few other bugs in
> > our system ATM.
> 
> ehh, my patch may be somewhat wrong, need to reflect a bit on that. I
> might even get to the ->raw = NULL thing :)

ok, this should do it I hope. I also have ->raw = NULL in progess, but I
would like to know if this patch is accpetable to solve this problem.

 Jocke

>From 6fbc572bf34c7834a8a2d6b8a5f22b4f55d3bbea Mon Sep 17 00:00:00 2001
From: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
Date: Sun, 28 Oct 2007 13:31:58 +0100
Subject: [PATCH] [JFFS2] Fix getdents problem on large directories.

Another fallout from commit a491486a2087ac3dfc00efb4f838c8d684afaf54
rm -rf on a large directory needs to call getdents several times.
The second call would usally return empty, telling the user that
there are no more files to unlink, rmdir then fails since the
directory isn't empty.

Signed-off-by: Joakim Tjernlund <Joakim.Tjernlund@transmode.se>
---
 fs/jffs2/write.c |    7 ++-----
 1 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/fs/jffs2/write.c b/fs/jffs2/write.c
index 2f56954..bee27a9 100644
--- a/fs/jffs2/write.c
+++ b/fs/jffs2/write.c
@@ -590,10 +590,8 @@ int jffs2_do_unlink(struct jffs2_sb_info *c, struct jffs2_inode_info *dir_f,
 
 				D1(printk(KERN_DEBUG "Marking old dirent node (ino #%u) @%08x obsolete\n",
 					  this->ino, ref_offset(this->raw)));
-
-				*prev = this->next;
-				jffs2_mark_node_obsolete(c, (this->raw));
-				jffs2_free_full_dirent(this);
+				jffs2_mark_node_obsolete(c, this->raw);
+				this->ino = 0;
 				break;
 			}
 			prev = &((*prev)->next);
@@ -622,7 +620,6 @@ int jffs2_do_unlink(struct jffs2_sb_info *c, struct jffs2_inode_info *dir_f,
 					D1(printk(KERN_DEBUG "Removing deletion dirent for \"%s\" from dir ino #%u\n",
 						fd->name, dead_f->inocache->ino));
 				}
-				jffs2_mark_node_obsolete(c, fd->raw);
 				jffs2_free_full_dirent(fd);
 			}
 		}
-- 
1.5.3.4

RE: getdents64 problem in 2.6.23

[Joakim Tjernlund]

> -----Original Message-----
> From: linux-mtd-bounces@lists.infradead.org 
> [mailto:linux-mtd-bounces@lists.infradead.org] On Behalf Of 
> Joakim Tjernlund
> Sent: den 30 oktober 2007 00:42
> To: David Woodhouse
> Cc: 'Linux-MTD Mailing List'; 'Jörn Engel'; 'Jamie Lokier'
> Subject: RE: getdents64 problem in 2.6.23
> 
> On Mon, 2007-10-29 at 18:30 +0100, Joakim Tjernlund wrote:
> > On Mon, 2007-10-29 at 13:58 +0100, Joakim Tjernlund wrote:
> > > On Sun, 2007-10-28 at 20:26 -0400, David Woodhouse wrote:
> > > > On Mon, 2007-10-29 at 00:58 +0100, Joakim Tjernlund wrote:
> > > > > David, before rewriting getdents et. all may I ask 
> you to apply the
> > > > > patch I sent today? Then we will have something that 
> works and go
> > > > > from there.
> > > > 
> > > > I'm not happy about having fd->raw still pointing to 
> nodes which may no
> > > > longer exist. Can't we set it to NULL (and cope with 
> potential fallout)?
> > > 
> > > Thats a diffrent story. Now I fixed the bug so it behaves 
> like the 
> > > !mark_node_obsolete case. Walking through all ->raw references is
> > > something I will save for a rainy dag, I need to fix a 
> few other bugs in
> > > our system ATM.
> > 
> > ehh, my patch may be somewhat wrong, need to reflect a bit 
> on that. I
> > might even get to the ->raw = NULL thing :)
> 
> ok, this should do it I hope. I also have ->raw = NULL in 
> progess, but I
> would like to know if this patch is accpetable to solve this problem.
> 
>  Jocke

David, just sent 2 patches to the list to do your bidding :)
I am done now unless you have something more to add.

   Jocke

RE: getdents64 problem in 2.6.23

[David Woodhouse]

On Tue, 2007-10-30 at 18:42 +0100, Joakim Tjernlund wrote:
> David, just sent 2 patches to the list to do your bidding :)
> I am done now unless you have something more to add.

Two patches? I see only one, which seems like exactly what we want for
now. (Thanks)
 
-- 
dwmw2