[U-Boot] [PATCH V2] JFFS2: bug fix for summary support.

[Baidu Liu]

Hi,Detlev :

2011/4/19 Detlev Zundel <dzu at denx.de>:
> Hi Baidu,
>
>>  This patch fixes some issues with JFFS2 summary support in U-Boot.
>>  1/ Bug fix for summary support: we need to get the latest DIRENT.
>>  For example, if you create a file in linux jffs2 which config summary
>>  support, then you delete the file , you will not see the file  in
>>  linux jffs2. But you can also see this file in uboot after you reset
>>  the system. That is because all the nodes in jffs2 which config summary
>>  will not be marked as obsolete. The deleted file's DIRENT node will be
>>  seen in uboot. So what we need to do is to get the latest DIRENT whose
>>  ino in DIRENT is 0. Than we will not see this file in uboot which is
>>  what we want.
>>  2/ Add CONFIG_SYS_JFFS2_SORT_FRAGMENTS definition,if we config jffs2
>>  summary in uboot.
>>  3/ Avoid allocate too big memory if the biggest file in JFFS2 is too
>>  long. We only allocate one node size for pL->readbuf.
>>  For example, if the biggest file in the jffs2 is 15MB in the 16MB jffs2
>>  system. Our previous code will allocate a buffer(pL->readbuf) as 15MB
>>  length.
>>  4/ Improve error checking in the scanning.
>
> I missed saying this explicitely in my first review that we should
> strive to isolate changes into single commits.  As you have added even
> more changes into a single commit this has now become somewhat
> untolerable.  Please split into individual functional changes (git add
> -i can work wonders here), i.e. the list items in the changelog should
> become individual commits.
>
> This also includes the added WATCHDOG_RESET not mentioned in the
> changelog at all ;)
>
>> Changes for v2:
>>       - Add detail description for the patch.
>
> Are you sure you only changed the description?  The diffstat from this
> and the last time disagree - this time:
>
>> ---
>>  fs/jffs2/jffs2_1pass.c      |   60 +++++++++++++++++++++++++-----------------
>>  fs/jffs2/jffs2_nand_1pass.c |   28 +++++++++++++++-----
>>  include/jffs2/jffs2.h       |   10 +++++++
>>  3 files changed, 67 insertions(+), 31 deletions(-)
>
> Last time:
>
>>  fs/jffs2/jffs2_1pass.c      |   53 +++++++++++++++++++++++++-----------------
>>  fs/jffs2/jffs2_nand_1pass.c |   24 ++++++++++++++-----
>>  include/jffs2/jffs2.h       |   11 +++++++++
>>  3 files changed, 60 insertions(+), 28 deletions(-)
>
> Looking over the changes, I do _see_ changes in code, so you should tell
> us about them.
>
>> [...]
>
>> diff --git a/include/jffs2/jffs2.h b/include/jffs2/jffs2.h
>> index 651f94c..5b006c0 100644
>> --- a/include/jffs2/jffs2.h
>> +++ b/include/jffs2/jffs2.h
>> @@ -41,6 +41,16 @@
>>  #include <asm/types.h>
>>  #include <jffs2/load_kernel.h>
>>
>> +#ifdef CONFIG_JFFS2_SUMMARY
>> +#ifndef CONFIG_SYS_JFFS2_SORT_FRAGMENTS
>> +/*
>> +we should define CONFIG_SYS_JFFS2_SORT_FRAGMENTS,if
>> +CONFIG_JFFS2_SUMMARY is enabled.
>> +*/
>> +#define CONFIG_SYS_JFFS2_SORT_FRAGMENTS
>> +#endif
>> +#endif
>> +
>>  #define JFFS2_SUPER_MAGIC 0x72b6
>>
>>  /* Values we may expect to find in the 'magic' field */
>
> I liked the previous version better:
>
>> diff --git a/include/jffs2/jffs2.h b/include/jffs2/jffs2.h
>> index 651f94c..c01a76e 100644
>> --- a/include/jffs2/jffs2.h
>> +++ b/include/jffs2/jffs2.h
>> @@ -41,6 +41,17 @@
>>  #include <asm/types.h>
>>  #include <jffs2/load_kernel.h>
>>
>> +#ifdef CONFIG_JFFS2_SUMMARY
>> +#ifndef CONFIG_SYS_JFFS2_SORT_FRAGMENTS
>> +/*
>> + *   if we define summary in jffs2, we also need to define
>> + *   CONFIG_SYS_JFFS2_SORT_FRAGMENTS. If not, the data in latest inode may be
>> + *  overwritten by the old one.
>> +*/
>> +#error "need to define CONFIG_SYS_JFFS2_SORT_FRAGMENTS,if summary is enabled"
>> +#endif
>> +#endif
>> +
>>  #define JFFS2_SUPER_MAGIC 0x72b6
>
> Why did you change this to the worse?
>

If we use summary in uboot, we MUST also define CONFIG_SYS_JFFS2_SORT_FRAGMENTS.
The reason is :
All the inodes of a file will not marked as obsolete, if they do not
sort in the list struct b_node *, the latest data in inode may be
overwritten by the older one.