| Message ID | 001201cbfca4$2fc3fa80$6401a8c0@LENOVOE5CA6843 |
|---|---|
| State | Changes Requested |
| Headers | show |
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? Cheers Detlev
Hi,Detlev : 2011/4/19 Detlev Zundel <dzu@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.
Hi Baidu, [...] >> 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. Yes, this is what you wrote in your first version and what I would like to see. My question was why you deleted the (helpful) comment in your second version... Cheers Detlev
diff --git a/fs/jffs2/jffs2_1pass.c b/fs/jffs2/jffs2_1pass.c index c4f7445..b3d94af 100644 --- a/fs/jffs2/jffs2_1pass.c +++ b/fs/jffs2/jffs2_1pass.c @@ -662,7 +662,8 @@ jffs2_free_cache(struct part_info *part) pL = (struct b_lists *)part->jffs2_priv; free_nodes(&pL->frag); free_nodes(&pL->dir); - free(pL->readbuf); + if(pL->readbuf) + free(pL->readbuf); free(pL); } } @@ -676,12 +677,18 @@ jffs_init_1pass_list(struct part_info *part) if (NULL != (part->jffs2_priv = malloc(sizeof(struct b_lists)))) { pL = (struct b_lists *)part->jffs2_priv; - memset(pL, 0, sizeof(*pL)); + + pL->readbuf = malloc(sizeof(union jffs2_node_union)); + if(!pL->readbuf) { + printf("jffs_init_1pass_list: malloc failed\n"); + return 0; + } #ifdef CONFIG_SYS_JFFS2_SORT_FRAGMENTS pL->dir.listCompare = compare_dirents; pL->frag.listCompare = compare_inodes; #endif + return 1; } return 0; } @@ -748,8 +755,8 @@ jffs2_1pass_read_inode(struct b_lists *pL, u32 inode, char *dest) if(dest) { src = ((uchar *) jNode) + sizeof(struct jffs2_raw_inode); - /* ignore data behind latest known EOF */ - if (jNode->offset > totalSize) { + /* ignore data which exceed file length */ + if (jNode->offset + jNode->dsize > totalSize) { put_fl_mem(jNode, pL->readbuf); continue; } @@ -836,9 +843,8 @@ jffs2_1pass_find_inode(struct b_lists * pL, const char *name, u32 pino) jDir = (struct jffs2_raw_dirent *) get_node_mem(b->offset, pL->readbuf); if ((pino == jDir->pino) && (len == jDir->nsize) && - (jDir->ino) && /* 0 for unlink */ (!strncmp((char *)jDir->name, name, len))) { /* a match */ - if (jDir->version < version) { + if (jDir->version < version) { /*ignore the old DIRENT*/ put_fl_mem(jDir, pL->readbuf); continue; } @@ -962,6 +968,14 @@ jffs2_1pass_list_inodes(struct b_lists * pL, u32 pino) struct jffs2_raw_inode ojNode; struct jffs2_raw_inode *jNode, *i = NULL; struct b_node *b2 = pL->frag.listHead; + + /* + we compare the DIRENT's ino with the latest DIRENT's ino + to determine whether this DIRENT is the latest one. + If the DIRENT is not the latest one,ignore it. + */ + if(jDir->ino != jffs2_1pass_find_inode(pL, jDir->name, pino)) + continue; while (b2) { jNode = (struct jffs2_raw_inode *) @@ -1448,7 +1462,6 @@ jffs2_1pass_build_lists(struct part_info * part) u32 counter4 = 0; u32 counterF = 0; u32 counterN = 0; - u32 max_totlen = 0; u32 buf_size = DEFAULT_EMPTY_SCAN_SIZE; char *buf; @@ -1458,9 +1471,16 @@ jffs2_1pass_build_lists(struct part_info * part) /* lcd_off(); */ /* if we are building a list we need to refresh the cache. */ - jffs_init_1pass_list(part); + if(! jffs_init_1pass_list(part)) + return 0; + pL = (struct b_lists *)part->jffs2_priv; buf = malloc(buf_size); + if (!buf) { + printf("jffs2_1pass_build_lists: malloc failed\n"); + return 0; + } + puts ("Scanning JFFS2 FS: "); /* start at the beginning of the partition */ @@ -1520,7 +1540,7 @@ jffs2_1pass_build_lists(struct part_info * part) ret = jffs2_sum_scan_sumnode(part, sector_ofs, sumptr, sumlen, pL); - if (buf_size && sumlen > buf_size) + if (sumlen > buf_size) free(sumptr); if (ret < 0) { free(buf); @@ -1631,6 +1651,8 @@ jffs2_1pass_build_lists(struct part_info * part) case JFFS2_NODETYPE_INODE: if (buf_ofs + buf_len < ofs + sizeof(struct jffs2_raw_inode)) { + buf_len = min_t(uint32_t, buf_size, sector_ofs + + part->sector_size - ofs); get_fl_mem((u32)part->offset + ofs, buf_len, buf); buf_ofs = ofs; @@ -1645,15 +1667,13 @@ jffs2_1pass_build_lists(struct part_info * part) jffs2_free_cache(part); return 0; } - if (max_totlen < node->totlen) - max_totlen = node->totlen; break; case JFFS2_NODETYPE_DIRENT: - if (buf_ofs + buf_len < ofs + sizeof(struct - jffs2_raw_dirent) + - ((struct - jffs2_raw_dirent *) - node)->nsize) { + if (buf_ofs + buf_len < ofs + + sizeof(struct jffs2_raw_dirent) + + ((struct jffs2_raw_dirent *)node)->nsize) { + buf_len = min_t(uint32_t, buf_size, sector_ofs + + part->sector_size - ofs); get_fl_mem((u32)part->offset + ofs, buf_len, buf); buf_ofs = ofs; @@ -1675,8 +1695,6 @@ jffs2_1pass_build_lists(struct part_info * part) jffs2_free_cache(part); return 0; } - if (max_totlen < node->totlen) - max_totlen = node->totlen; counterN++; break; case JFFS2_NODETYPE_CLEANMARKER: @@ -1708,12 +1726,6 @@ jffs2_1pass_build_lists(struct part_info * part) free(buf); putstr("\b\b done.\r\n"); /* close off the dots */ - /* We don't care if malloc failed - then each read operation will - * allocate its own buffer as necessary (NAND) or will read directly - * from flash (NOR). - */ - pL->readbuf = malloc(max_totlen); - /* turn the lcd back on. */ /* splash(); */ diff --git a/fs/jffs2/jffs2_nand_1pass.c b/fs/jffs2/jffs2_nand_1pass.c index 3982003..285c269 100644 --- a/fs/jffs2/jffs2_nand_1pass.c +++ b/fs/jffs2/jffs2_nand_1pass.c @@ -251,6 +251,7 @@ jffs_init_1pass_list(struct part_info *part) pL->dir.listCompare = compare_dirents; pL->frag.listCompare = compare_inodes; #endif + return 1; } return 0; } @@ -305,8 +306,8 @@ jffs2_1pass_read_inode(struct b_lists *pL, u32 ino, char *dest, if (dest) len += jNode->csize; nand_read(nand, jNode->offset, &len, inode); - /* ignore data behind latest known EOF */ - if (inode->offset > totalSize) + /* ignore data which exceed file length */ + if (inode->offset + inode->dsize > totalSize) continue; if (stat) { @@ -371,8 +372,9 @@ jffs2_1pass_find_inode(struct b_lists * pL, const char *name, u32 pino) /* we need to search all and return the inode with the highest version */ for (jDir = (struct b_dirent *)pL->dir.listHead; jDir; jDir = jDir->next) { - if ((pino == jDir->pino) && (jDir->ino) && /* 0 for unlink */ - (len == jDir->nsize) && (nhash == jDir->nhash)) { + if ((pino == jDir->pino) && + (len == jDir->nsize) && + (nhash == jDir->nhash)) { /* TODO: compare name */ if (jDir->version < version) continue; @@ -483,6 +485,14 @@ jffs2_1pass_list_inodes(struct b_lists * pL, u32 pino) struct b_inode *jNode = (struct b_inode *)pL->frag.listHead; struct b_inode *i = NULL; + /* + we compare the DIRENT's ino with the latest DIRENT's ino + to determine whether this DIRENT is the latest one. + If the DIRENT is not the latest one,ignore it. + */ + if(jDir.ino != jffs2_1pass_find_inode(pL, jDir->name, pino)) + continue; + while (jNode) { if (jNode->ino == jDir->ino && jNode->version >= i_version) { i_version = jNode->version; @@ -797,7 +807,9 @@ jffs2_1pass_build_lists(struct part_info * part) nand = nand_info + id->num; /* if we are building a list we need to refresh the cache. */ - jffs_init_1pass_list(part); + if(! jffs_init_1pass_list(part)) + return 0; + pL = (struct b_lists *)part->jffs2_priv; pL->partOffset = part->offset; puts ("Scanning JFFS2 FS: "); @@ -809,6 +821,8 @@ jffs2_1pass_build_lists(struct part_info * part) return 0; for (i = 0; i < nr_blocks; i++) { + WATCHDOG_RESET(); + printf("\b\b%c ", spinner[counter++ % sizeof(spinner)]); offset = part->offset + i * sectorsize; @@ -878,6 +892,7 @@ jffs2_1pass_build_lists(struct part_info * part) } } + free(buf); putstr("\b\b done.\r\n"); /* close off the dots */ #if 0 @@ -896,8 +911,7 @@ jffs2_1pass_build_lists(struct part_info * part) #endif /* give visual feedback that we are done scanning the flash */ - led_blink(0x0, 0x0, 0x1, 0x1); /* off, forever, on 100ms, off 100ms */ - free(buf); + led_blink(0x0, 0x0, 0x1, 0x1); /* off, forever, on 100ms, off 100ms */ return 1; } 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 */
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. Signed-off-by: Baidu Liu <liucai.lfn@gmail.com> --- Changes for v2: - Add detail description for the patch. --- 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(-)