[PATCH] [JFFS2] Fix free space leaking

[PATCH] [JFFS2] Fix free space leaking

[Damir Shayhutdinov]

This patch is addressed to fix very-long-standing problem in JFFS2,
first described in 2004:
http://lists.infradead.org/pipermail/linux-mtd/2004-March/009456.html

jffs2_link_node_ref() decreases c->free_size by
c->cleanmarker_size but the clean marker space can't
be accounted as free space! So we just compensate
the difference.

Signed-off-by: Alexander Yurchenko <grange@tecon.ru>
Signed-off-by: Damir Shayhutdinov <damir@tecon.ru>
---
 fs/jffs2/erase.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
index a1db918..c574fa3 100644
--- a/fs/jffs2/erase.c
+++ b/fs/jffs2/erase.c
@@ -454,6 +454,17 @@ static void jffs2_mark_erased_block(struct jffs2_sb_info *c, struct jffs2_eraseb
 		jeb->free_size = c->sector_size;
 		/* FIXME Special case for cleanmarker in empty block */
 		jffs2_link_node_ref(c, jeb, jeb->offset | REF_NORMAL, c->cleanmarker_size, NULL);
+		/*
+		 * XXX: I'm not sure this is correct but it prevents
+		 * c->free_size from slow leaking under a frequent file
+		 * overwriting.
+		 * jffs2_link_node_ref() decreases c->free_size by
+		 * c->cleanmarker_size but the clean marker space can't
+		 * be accounted as free space! So we just compensate
+		 * the difference.
+		 */
+		c->free_size += c->cleanmarker_size;
+		c->used_size -= c->cleanmarker_size;
 	}
 
 	down(&c->erase_free_sem);
-- 
1.5.3.4.GIT

Re: [PATCH] [JFFS2] Fix free space leaking

[Joakim Tjernlund]

On Tue, 2007-11-13 at 13:23 +0300, Damir Shayhutdinov wrote:
> This patch is addressed to fix very-long-standing problem in JFFS2,
> first described in 2004:
> http://lists.infradead.org/pipermail/linux-mtd/2004-March/009456.html
> 
> jffs2_link_node_ref() decreases c->free_size by
> c->cleanmarker_size but the clean marker space can't
> be accounted as free space! So we just compensate
> the difference.
> 
> Signed-off-by: Alexander Yurchenko <grange@tecon.ru>
> Signed-off-by: Damir Shayhutdinov <damir@tecon.ru>
> ---
>  fs/jffs2/erase.c |   11 +++++++++++
>  1 files changed, 11 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
> index a1db918..c574fa3 100644
> --- a/fs/jffs2/erase.c
> +++ b/fs/jffs2/erase.c
> @@ -454,6 +454,17 @@ static void jffs2_mark_erased_block(struct jffs2_sb_info *c, struct jffs2_eraseb
>  		jeb->free_size = c->sector_size;
>  		/* FIXME Special case for cleanmarker in empty block */
>  		jffs2_link_node_ref(c, jeb, jeb->offset | REF_NORMAL, c->cleanmarker_size, NULL);
> +		/*
> +		 * XXX: I'm not sure this is correct but it prevents
> +		 * c->free_size from slow leaking under a frequent file
> +		 * overwriting.
> +		 * jffs2_link_node_ref() decreases c->free_size by
> +		 * c->cleanmarker_size but the clean marker space can't
> +		 * be accounted as free space! So we just compensate
> +		 * the difference.
> +		 */
> +		c->free_size += c->cleanmarker_size;
> +		c->used_size -= c->cleanmarker_size;
>  	}
>  
>  	down(&c->erase_free_sem);

What happened to this patch? Just forgotten or was it rejected?

   Jocke

Re: [PATCH] [JFFS2] Fix free space leaking

[Damir Shayhutdinov]

> What happened to this patch? Just forgotten or was it rejected?
Just forgotten, it seems. Anyway, I tested it in Linux 2.6.22 and it works.

Re: [PATCH] [JFFS2] Fix free space leaking

[Jörn Engel]

On Wed, 6 February 2008 11:20:19 +0100, Joakim Tjernlund wrote:
> On Tue, 2007-11-13 at 13:23 +0300, Damir Shayhutdinov wrote:
> > This patch is addressed to fix very-long-standing problem in JFFS2,
> > first described in 2004:
> > http://lists.infradead.org/pipermail/linux-mtd/2004-March/009456.html
> > 
> > jffs2_link_node_ref() decreases c->free_size by
> > c->cleanmarker_size but the clean marker space can't
> > be accounted as free space! So we just compensate
> > the difference.
> > 
> > Signed-off-by: Alexander Yurchenko <grange@tecon.ru>
> > Signed-off-by: Damir Shayhutdinov <damir@tecon.ru>
> > ---
> >  fs/jffs2/erase.c |   11 +++++++++++
> >  1 files changed, 11 insertions(+), 0 deletions(-)
> > 
> > diff --git a/fs/jffs2/erase.c b/fs/jffs2/erase.c
> > index a1db918..c574fa3 100644
> > --- a/fs/jffs2/erase.c
> > +++ b/fs/jffs2/erase.c
> > @@ -454,6 +454,17 @@ static void jffs2_mark_erased_block(struct jffs2_sb_info *c, struct jffs2_eraseb
> >  		jeb->free_size = c->sector_size;
> >  		/* FIXME Special case for cleanmarker in empty block */
> >  		jffs2_link_node_ref(c, jeb, jeb->offset | REF_NORMAL, c->cleanmarker_size, NULL);
> > +		/*
> > +		 * XXX: I'm not sure this is correct but it prevents
> > +		 * c->free_size from slow leaking under a frequent file
> > +		 * overwriting.
> > +		 * jffs2_link_node_ref() decreases c->free_size by
> > +		 * c->cleanmarker_size but the clean marker space can't
> > +		 * be accounted as free space! So we just compensate
> > +		 * the difference.
> > +		 */
> > +		c->free_size += c->cleanmarker_size;
> > +		c->used_size -= c->cleanmarker_size;
> >  	}
> >  
> >  	down(&c->erase_free_sem);
> 
> What happened to this patch? Just forgotten or was it rejected?

No idea, but it might help to add dwmw2 to Cc:.

Jörn

-- 
Joern's library part 4:
http://www.paulgraham.com/spam.html

Re: [PATCH] [JFFS2] Fix free space leaking

[David Woodhouse]

On Thu, 2008-02-21 at 12:11 +0100, Jörn Engel wrote:
> No idea, but it might help to add dwmw2 to Cc:.

dwmw2 moderately confused by it. I don't see why it's correct to account
for the space as 'free' when it isn't really. If it's causing a
persistent leak, shouldn't we deal with that when the block is erased
instead?

-- 
dwmw2

Re: [PATCH] [JFFS2] Fix free space leaking

[Martin Creutziger]

On Thu, 2008-02-21 at 20:39 +0900, an unknown sender wrote:
> On Thu, 2008-02-21 at 12:11 +0100, J?rn Engel wrote:
> > No idea, but it might help to add dwmw2 to Cc:.
> 
> dwmw2 moderately confused by it. I don't see why it's correct to account
> for the space as 'free' when it isn't really. If it's causing a
> persistent leak, shouldn't we deal with that when the block is erased
> instead?

Hi, 

I am currently testing the patch on 2.6.24.2 (as a result of
http://lists.infradead.org/pipermail/linux-mtd/2008-March/021024.html),
and so far it seems to at least cure the symptoms, as in: The partition
does not "fill up" any more so far. 
But: For later production use I would certainly prefer a patch that also
dwmw2 agrees being a cure to the _cause_, not just the symptoms.
Any suggestions?

Greetings . . . Martin



DISCLAIMER:
Unless indicated otherwise, the information contained in this message is privileged and confidential, and is intended only for the use of the addressee(s) named above and others who have been specifically authorized to receive it. If you are not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this message and/or attachments is strictly prohibited. The company accepts no liability for any damage caused by any virus transmitted by this email. Furthermore, the company does not warrant a proper and complete transmission of this information, nor does it accept liability for any delays. If you have received this message in error, please contact the sender and delete the message. Thank you.

Re: [PATCH] [JFFS2] Fix free space leaking

[David Woodhouse]

On Thu, 2008-04-03 at 11:03 +0200, Martin Creutziger wrote:
> On Thu, 2008-02-21 at 20:39 +0900, an unknown sender wrote:
> > On Thu, 2008-02-21 at 12:11 +0100, J?rn Engel wrote:
> > > No idea, but it might help to add dwmw2 to Cc:.
> > 
> > dwmw2 moderately confused by it. I don't see why it's correct to account
> > for the space as 'free' when it isn't really. If it's causing a
> > persistent leak, shouldn't we deal with that when the block is erased
> > instead?
> 
> Hi, 
> 
> I am currently testing the patch on 2.6.24.2 (as a result of
> http://lists.infradead.org/pipermail/linux-mtd/2008-March/021024.html),
> and so far it seems to at least cure the symptoms, as in: The partition
> does not "fill up" any more so far. 
> But: For later production use I would certainly prefer a patch that also
> dwmw2 agrees being a cure to the _cause_, not just the symptoms.
> Any suggestions?

I have reproduced this locally with some extra 'paranoia' debug checks,
and I understand it now. The original patch was doing fairly much the
right thing, but ideally would have looked more like this:

--- a/fs/jffs2/erase.c
+++ b/fs/jffs2/erase.c
@@ -459,8 +459,9 @@ static void jffs2_mark_erased_block(struct jffs2_sb_info *c,
        down(&c->erase_free_sem);
        spin_lock(&c->erase_completion_lock);
        c->erasing_size -= c->sector_size;
-       c->free_size += jeb->free_size;
-       c->used_size += jeb->used_size;
+       /* We already accounted for the cleanmarker in c->{used,free}_size,
+          so just add the eraseblock size now. */
+       c->free_size += c->sector_size;
 
        jffs2_dbg_acct_sanity_check_nolock(c,jeb);
        jffs2_dbg_acct_paranoia_check_nolock(c, jeb);


In fact I've cleaned it up a little more to fix the fact that we were
calling jffs2_link_node_ref() without the required locking:
http://git.infradead.org/mtd-2.6.git?a=commitdiff;h=014b164e1392a166fe96e003d2f0e7ad2e2a0bb7

Thanks for your patience, and for showing me exactly where to look.

-- 
dwmw2