mtd/fs/jffs2 nodemgmt.c,1.115,1.116

mtd/fs/jffs2 nodemgmt.c,1.115,1.116

[Thomas Gleixner]

> - avoid filing blocks on the clean list when they have wasted
> space

That's plain wrong. 

Wasted space was introduced to prevent endless garbage collection.
Wasted space is unreclaimable, because it is less than the minimum node
size. It's totally correct to file those blocks on the clean list,
because GC _cannot_ reclaim the wasted space. 

This patch just reinvents the endless GC loop. Please revert.

tglx

Re: mtd/fs/jffs2 nodemgmt.c,1.115,1.116

[David Woodhouse]

On Mon, 2005-01-24 at 22:58 +0100, Thomas Gleixner wrote:
> > - avoid filing blocks on the clean list when they have wasted
> > space
> 
> That's plain wrong. 

And the first commit I looked at violated the coding style.

-- 
dwmw2

Re: mtd/fs/jffs2 nodemgmt.c,1.115,1.116

[Estelle HAMMACHE]

David Woodhouse wrote:
> And the first commit I looked at violated the coding style.

I am a horrible coding style violator. I did submit
the patch to the list beforehand didn't I ? 
After googling up "linux kernel coding style" the only
problem I find is with leading braces. If there are other
uglies please tell me (unnecessary comments maybe ?). 
I'll be correcting it tonight.

Estelle

Re: mtd/fs/jffs2 nodemgmt.c,1.115,1.116

[Estelle HAMMACHE]

Estelle HAMMACHE wrote:
> 
> David Woodhouse wrote:
> > And the first commit I looked at violated the coding style.
> 
> I am a horrible coding style violator. I did submit
> the patch to the list beforehand didn't I ?
> After googling up "linux kernel coding style" the only
> problem I find is with leading braces. If there are other
> uglies please tell me (unnecessary comments maybe ?).
> I'll be correcting it tonight.
> 
> Estelle

Silly me I corrected everything but the braces. Next try will
do it. 

Estelle

Re: mtd/fs/jffs2 nodemgmt.c,1.115,1.116

[Estelle HAMMACHE]

Thomas Gleixner wrote:
> 
> > - avoid filing blocks on the clean list when they have wasted
> > space
> 
> That's plain wrong.
> 
> Wasted space was introduced to prevent endless garbage collection.
> Wasted space is unreclaimable, because it is less than the minimum node
> size. It's totally correct to file those blocks on the clean list,
> because GC _cannot_ reclaim the wasted space.

Well without this patch I got blocks on the clean list with huge
wasted space, more than the ISDIRTY calculation, when a write that
fills exactly the space left in nextblock also obsoletes nodes
in nextblock (not sure about the exact case it was some time ago).
In jffs2_mark_node_obsolete, if the node belongs to nextblock, the
space is always added to wasted_space - not dirty_space. So I don't
understand whether wasted_space is not what you claim, or it is
not well implemented.

I'm afraid my comment in the log was quite misleading: 
this patch merely prevents filing the block to the clean
list in jffs2_add_physical_node_ref. As the comment in that
function says, the block will be filed to the correct list later 
and with proper ISDIRTY check (or am I missing something ?).

Lastly, I did submit this patch beforehand to the list, even if it was
in a thread on NAND failure. I would have expected you to have
an interest in that thread anyway.

(please excuse me if I sound grumpy. Your comments are a bit
of a cold shower on my newbie enthusiasm. If the patch is really
a problem I will reverse it tonight.)

Estelle

Re: mtd/fs/jffs2 nodemgmt.c,1.115,1.116

[Thomas Gleixner]

Hi Estelle,

On Tue, 2005-01-25 at 11:13 +0100, Estelle HAMMACHE wrote:
> Thomas Gleixner wrote:
> > 
> > > - avoid filing blocks on the clean list when they have wasted
> > > space
> > 
> > That's plain wrong.
> > 
> > Wasted space was introduced to prevent endless garbage collection.
> > Wasted space is unreclaimable, because it is less than the minimum node
> > size. It's totally correct to file those blocks on the clean list,
> > because GC _cannot_ reclaim the wasted space.
> 
> Well without this patch I got blocks on the clean list with huge
> wasted space, more than the ISDIRTY calculation, when a write that
> fills exactly the space left in nextblock also obsoletes nodes
> in nextblock (not sure about the exact case it was some time ago).
> In jffs2_mark_node_obsolete, if the node belongs to nextblock, the
> space is always added to wasted_space - not dirty_space. So I don't
> understand whether wasted_space is not what you claim, or it is
> not well implemented.

Things have changed over time, so I'm not sure whether the wasted ->
dirty accounting is still correct all over the place. It was once :)

> I'm afraid my comment in the log was quite misleading: 
> this patch merely prevents filing the block to the clean
> list in jffs2_add_physical_node_ref. As the comment in that
> function says, the block will be filed to the correct list later 
> and with proper ISDIRTY check (or am I missing something ?).

Sorry, I misunderstood the comment. 

Filing the block in jffs2_add_physical_node_ref to the clean list if
appropriate is an optimization. Your check in fact disables the
optimization path as almost all blocks have wasted space. 

Please check !ISDIRTY(jeb->wasted_size) instead of !jeb->wasted_size.

> Lastly, I did submit this patch beforehand to the list, even if it was
> in a thread on NAND failure. I would have expected you to have
> an interest in that thread anyway.

I can not follow 100% of the mail threads all the time.

> (please excuse me if I sound grumpy. Your comments are a bit
> of a cold shower on my newbie enthusiasm. 

I did not intend to slow down your enthusiasm. :)

> If the patch is really a problem I will reverse it tonight.)

Fixing it is ok (see above). 

The coding style violations which dwmw2 pointed out need to be fixed
too.

tglx

Re: mtd/fs/jffs2 nodemgmt.c,1.115,1.116

[Artem B. Bityuckiy]

On Tue, 2005-01-25 at 11:58 +0100, Thomas Gleixner wrote:
> Things have changed over time, so I'm not sure whether the wasted ->
> dirty accounting is still correct all over the place. It was once :)
You know, it not completely correct. When wbuf is sync-ed, the padding
is treated as wasted irrespective of its size. But this doesn't break
JFFS2, though.

-- 
Best Regards,
Artem B. Bityuckiy,
St.-Petersburg, Russia.