fs:ext4: Fixes and code cleanup

Code responsible for handling situation when ext4 has block size of 1024B can be ordered to take less space.

This patch does that for ext4 common and write files.

Signed-off-by Lukasz Majewski <[email protected]>

---
 fs/ext4/ext4_common.c |    6 ++----
 fs/ext4/ext4_write.c  |   50 ++++++++++++++++---------------------------------
 2 files changed, 18 insertions(+), 38 deletions(-)

fs/ext4/ext4_common.c +0 -0

index 02da75c..62e2e80 100644

fs/ext4/ext4_common.c +13 -27

+                       if (fs->blksz == 1024) {
                                remainder = (*di_buffer) % blk_per_grp;
                                if (!remainder)
                                        bg_idx--;

@@ -213,10 +209,8 @@ static void delete_double_indirect_block(struct ext2_inode *inode)
 
                /* removing the parent double indirect block */
                blknr = inode->b.blocks.double_indir_block;
-               if (fs->blksz != 1024) {
-                       bg_idx = blknr / blk_per_grp;
-               } else {
-                       bg_idx = blknr / blk_per_grp;
+               bg_idx = blknr / blk_per_grp;
+               if (fs->blksz == 1024) {
                        remainder = blknr % blk_per_grp;
                        if (!remainder)
                                bg_idx--;

@@ -293,11 +287,8 @@ static void delete_triple_indirect_block(struct ext2_inode *inode)
                        for (j = 0; j < fs->blksz / sizeof(int); j++) {
                                if (*tip_buffer == 0)
                                        break;
-                               if (fs->blksz != 1024) {
-                                       bg_idx = (*tip_buffer) / blk_per_grp;
-                               } else {
-                                       bg_idx = (*tip_buffer) / blk_per_grp;
-
+                               bg_idx = (*tip_buffer) / blk_per_grp;
+                               if (fs->blksz == 1024) {
                                        remainder = (*tip_buffer) % blk_per_grp;
                                        if (!remainder)
                                                bg_idx--;

@@ -336,11 +327,8 @@ static void delete_triple_indirect_block(struct ext2_inode *inode)
                         * removing the grand parent blocks
                         * which is connected to inode
                         */
-                       if (fs->blksz != 1024) {
-                               bg_idx = (*tigp_buffer) / blk_per_grp;
-                       } else {
-                               bg_idx = (*tigp_buffer) / blk_per_grp;
-
+                       bg_idx = (*tigp_buffer) / blk_per_grp;
+                       if (fs->blksz == 1024) {
                                remainder = (*tigp_buffer) % blk_per_grp;
                                if (!remainder)
                                        bg_idx--;

@@ -371,10 +359,8 @@ static void delete_triple_indirect_block(struct ext2_inode *inode)
 
                /* removing the grand parent triple indirect block */
                blknr = inode->b.blocks.triple_indir_block;
-               if (fs->blksz != 1024) {
-                       bg_idx = blknr / blk_per_grp;
-               } else {
-                       bg_idx = blknr / blk_per_grp;
+               bg_idx = blknr / blk_per_grp;
+               if (fs->blksz == 1024) {
                        remainder = blknr % blk_per_grp;
                        if (!remainder)
                                bg_idx--;

@@ -452,10 +438,8 @@ static int ext4fs_delete_file(int inodeno)
 
                for (i = 0; i < no_blocks; i++) {
                        blknr = read_allocated_block(&(node_inode->inode), i);
-                       if (fs->blksz != 1024) {
-                               bg_idx = blknr / blk_per_grp;
-                       } else {
-                               bg_idx = blknr / blk_per_grp;
+                       bg_idx = blknr / blk_per_grp;
+                       if (fs->blksz == 1024) {
                                remainder = blknr % blk_per_grp;
                                if (!remainder)
                                        bg_idx--;

@@ -499,10 +483,8 @@ static int ext4fs_delete_file(int inodeno)
                        no_blocks++;
                for (i = 0; i < no_blocks; i++) {
                        blknr = read_allocated_block(&inode, i);
-                       if (fs->blksz != 1024) {
-                               bg_idx = blknr / blk_per_grp;
-                       } else {
-                               bg_idx = blknr / blk_per_grp;
+                       bg_idx = blknr / blk_per_grp;
+                       if (fs->blksz == 1024) {
                                remainder = blknr % blk_per_grp;
                                if (!remainder)
                                        bg_idx--;
--

1.7.10.4

Quoted 6 lines · Lukasz Majewski

Code responsible for handling situation when ext4 has block size of 1024B can be ordered to take less space.

This patch does that for ext4 common and write files.

Signed-off-by Lukasz Majewski <[email protected]>

Reviewed-by Simon Glass <[email protected]>

Quoted 10 lines

---
 fs/ext4/ext4_common.c |    6 ++----
 fs/ext4/ext4_write.c  |   50

++++++++++++++++--------------------------------- 2 files changed, 18 insertions(+), 38 deletions(-)

fs/ext4/ext4_common.c +0 -0

index 02da75c..62e2e80 100644

[snip]

Quoted 3 lines

fs/ext4/ext4_common.c +0 -0

@@ -181,10 +179,8 @@ static void
 delete_double_indirect_block(struct ext2_inode *inode) break;
 

fs/ext4/ext4_common.c static void · line 182

Quoted 6 lines

fs/ext4/ext4_common.c +1 -4

                        debug("DICB releasing %u\n", *di_buffer);
-                       if (fs->blksz != 1024) {
-                               bg_idx = (*di_buffer) / blk_per_grp;
-                       } else {
-                               bg_idx = (*di_buffer) / blk_per_grp;
+                       bg_idx = (*di_buffer) / blk_per_grp;

You don't need the brackets here (or below).

Quoted 95 lines

fs/ext4/ext4_common.c +3 -4

+                       if (fs->blksz == 1024) {
                                remainder = (*di_buffer) %
 blk_per_grp; if (!remainder)
                                        bg_idx--;

@@ -213,10 +209,8 @@ static void
 delete_double_indirect_block(struct ext2_inode *inode)
 
                /* removing the parent double indirect block */
                blknr = inode->b.blocks.double_indir_block;
-               if (fs->blksz != 1024) {
-                       bg_idx = blknr / blk_per_grp;
-               } else {
-                       bg_idx = blknr / blk_per_grp;
+               bg_idx = blknr / blk_per_grp;
+               if (fs->blksz == 1024) {
                        remainder = blknr % blk_per_grp;
                        if (!remainder)

                                bg_idx--;

fs/ext4/ext4_common.c +0 -4

@@ -293,11 +287,8 @@ static void
 delete_triple_indirect_block(struct ext2_inode *inode) for (j = 0;
 j < fs->blksz / sizeof(int); j++) { if (*tip_buffer == 0)
                                        break;
-                               if (fs->blksz != 1024) {
-                                       bg_idx = (*tip_buffer) /
 blk_per_grp;
-                               } else {
-                                       bg_idx = (*tip_buffer) /

blk_per_grp; -
+ bg_idx = (*tip_buffer) /
blk_per_grp;
+ if (fs->blksz == 1024) {
remainder = (*tip_buffer) %
blk_per_grp; if (!remainder)

                                                bg_idx--;

fs/ext4/ext4_common.c +0 -4

@@ -336,11 +327,8 @@ static void
 delete_triple_indirect_block(struct ext2_inode *inode)
                         * removing the grand parent blocks
                         * which is connected to inode
                         */
-                       if (fs->blksz != 1024) {
-                               bg_idx = (*tigp_buffer) /
 blk_per_grp;
-                       } else {
-                               bg_idx = (*tigp_buffer) /

blk_per_grp; -

fs/ext4/ext4_common.c +2 -0

+                       bg_idx = (*tigp_buffer) / blk_per_grp;
+                       if (fs->blksz == 1024) {
                                remainder = (*tigp_buffer) %

blk_per_grp; if (!remainder)

                                        bg_idx--;

fs/ext4/ext4_common.c +2 -4

@@ -371,10 +359,8 @@ static void
 delete_triple_indirect_block(struct ext2_inode *inode)
 
                /* removing the grand parent triple indirect block
 */ blknr = inode->b.blocks.triple_indir_block;
-               if (fs->blksz != 1024) {
-                       bg_idx = blknr / blk_per_grp;
-               } else {
-                       bg_idx = blknr / blk_per_grp;
+               bg_idx = blknr / blk_per_grp;
+               if (fs->blksz == 1024) {
                        remainder = blknr % blk_per_grp;
                        if (!remainder)

                                bg_idx--;

fs/ext4/ext4_common.c +2 -4

@@ -452,10 +438,8 @@ static int ext4fs_delete_file(int inodeno)
 
                for (i = 0; i < no_blocks; i++) {
                        blknr =
 read_allocated_block(&(node_inode->inode), i);
-                       if (fs->blksz != 1024) {
-                               bg_idx = blknr / blk_per_grp;
-                       } else {
-                               bg_idx = blknr / blk_per_grp;
+                       bg_idx = blknr / blk_per_grp;
+                       if (fs->blksz == 1024) {
                                remainder = blknr % blk_per_grp;
                                if (!remainder)

                                        bg_idx--;

fs/ext4/ext4_common.c +2 -5

@@ -499,10 +483,8 @@ static int ext4fs_delete_file(int inodeno)
                        no_blocks++;
                for (i = 0; i < no_blocks; i++) {
                        blknr = read_allocated_block(&inode, i);
-                       if (fs->blksz != 1024) {
-                               bg_idx = blknr / blk_per_grp;
-                       } else {
-                               bg_idx = blknr / blk_per_grp;
+                       bg_idx = blknr / blk_per_grp;
+                       if (fs->blksz == 1024) {
                                remainder = blknr % blk_per_grp;
                                if (!remainder)
                                        bg_idx--;
--

1.7.10.4

Hi Simon,

Quoted 26 lines

Quoted 6 lines · Lukasz Majewski

Code responsible for handling situation when ext4 has block size of 1024B can be ordered to take less space.

This patch does that for ext4 common and write files.

Signed-off-by Lukasz Majewski <[email protected]>

Reviewed-by Simon Glass <[email protected]>

Quoted 10 lines

---
 fs/ext4/ext4_common.c |    6 ++----
 fs/ext4/ext4_write.c  |   50

++++++++++++++++--------------------------------- 2 files changed, 18 insertions(+), 38 deletions(-)

fs/ext4/ext4_common.c +0 -0

index 02da75c..62e2e80 100644

[snip]

Quoted 3 lines

fs/ext4/ext4_common.c +0 -0

@@ -181,10 +179,8 @@ static void
 delete_double_indirect_block(struct ext2_inode *inode) break;
 

Quoted 8 lines

fs/ext4/ext4_common.c static void · line 182

Quoted 6 lines

fs/ext4/ext4_common.c +1 -4

                        debug("DICB releasing %u\n", *di_buffer);
-                       if (fs->blksz != 1024) {
-                               bg_idx = (*di_buffer) / blk_per_grp;
-                       } else {
-                               bg_idx = (*di_buffer) / blk_per_grp;
+                       bg_idx = (*di_buffer) / blk_per_grp;

You don't need the brackets here (or below).

Maybe the GIT formatting is a bit misleading, but I've double checked and it seems that those parenthesis are necessary here.

Hi Lukasz,

Quoted 39 lines · Lukasz Majewski

Hi Simon,

Quoted 36 lines

Quoted 6 lines · Lukasz Majewski

Code responsible for handling situation when ext4 has block size of 1024B can be ordered to take less space.

This patch does that for ext4 common and write files.

Signed-off-by Lukasz Majewski <[email protected]>

Reviewed-by Simon Glass <[email protected]>

Quoted 10 lines

---
 fs/ext4/ext4_common.c |    6 ++----
 fs/ext4/ext4_write.c  |   50

++++++++++++++++--------------------------------- 2 files changed, 18 insertions(+), 38 deletions(-)

fs/ext4/ext4_common.c +0 -0

index 02da75c..62e2e80 100644

[snip]

fs/ext4/ext4_common.c static void · line 184

Quoted 11 lines

fs/ext4/ext4_common.c +1 -4

@@ -181,10 +179,8 @@ static void
 delete_double_indirect_block(struct ext2_inode *inode) break;
 
                        debug("DICB releasing %u\n", *di_buffer);
-                       if (fs->blksz != 1024) {
-                               bg_idx = (*di_buffer) /
 blk_per_grp;
-                       } else {
-                               bg_idx = (*di_buffer) /
 blk_per_grp;
+                       bg_idx = (*di_buffer) / blk_per_grp;

You don't need the brackets here (or below).

Quoted 2 lines

Maybe the GIT formatting is a bit misleading, but I've double checked and it seems that those parenthesis are necessary here.

OK. What is di_buffer such that (*di_buffer) works but *di_buffer doesn't?

Regards,
Simon

Quoted 46 lines · On Mon, 5 May 2014 08:24:22 -0600 Simon Glass <[email protected]> wrote:

Hi Lukasz,

Quoted 41 lines · Lukasz Majewski

Hi Simon,

Quoted 26 lines

Quoted 6 lines · Lukasz Majewski

Code responsible for handling situation when ext4 has block size of 1024B can be ordered to take less space.

This patch does that for ext4 common and write files.

Signed-off-by Lukasz Majewski <[email protected]>

Reviewed-by Simon Glass <[email protected]>

Quoted 10 lines

---
 fs/ext4/ext4_common.c |    6 ++----
 fs/ext4/ext4_write.c  |   50

++++++++++++++++--------------------------------- 2 files changed, 18 insertions(+), 38 deletions(-)

fs/ext4/ext4_common.c +0 -0

index 02da75c..62e2e80 100644

[snip]

Quoted 3 lines

fs/ext4/ext4_common.c +0 -0

@@ -181,10 +179,8 @@ static void
 delete_double_indirect_block(struct ext2_inode *inode) break;
 

Quoted 10 lines

fs/ext4/ext4_common.c static void · line 184

Quoted 8 lines

fs/ext4/ext4_common.c +1 -4

                        debug("DICB releasing %u\n", *di_buffer);
-                       if (fs->blksz != 1024) {
-                               bg_idx = (*di_buffer) /
 blk_per_grp;
-                       } else {
-                               bg_idx = (*di_buffer) /
 blk_per_grp;
+                       bg_idx = (*di_buffer) / blk_per_grp;

You don't need the brackets here (or below).

Maybe the GIT formatting is a bit misleading, but I've double checked and it seems that those parenthesis are necessary here.

Quoted 2 lines

OK. What is di_buffer such that (*di_buffer) works but *di_buffer doesn't?

It is hard to admit :-), but I've misunderstood you. I thought that you are talking about the {} parentheses.

I will check the code tomorrow and prepare proper patch.

This bug shows up when file stored on the ext4 file system is updated.

The ext4fs_delete_file() is responsible for deleting file's (e.g. uImage) data.
However some global data (especially ext4fs_indir2_block), which is used during file deletion are left unchanged.

The ext4fs_indir2_block pointer stores reference to old ext4 double indirect allocated blocks. When it is unchanged, after file deletion, ext4fs_write_file() uses the same pointer (since it is already initialized
- i.e. not NULL) to return number of blocks to write. This trunks larger file when previous one was smaller.

Lets consider following scenario:

1. Flash target with ext4 formatted boot.img (which has uImage [*] on itself)
2. Developer wants to upload their custom uImage [**]
- When new uImage [**] is smaller than the [*] - everything works correctly - we are able to store the whole smaller file with corrupted ext4fs_indir2_block pointer
- When new uImage [**] is larger than the [*] - theCRC is corrupted, since truncation on data stored at eMMC was done.
3. When uImage CRC error appears, then reboot and THOR/DFU reflashing causes proper setting of ext4fs_indir2_block() and after that uImage[**] is successfully stored (correct uImage [*] metadata is stored at an eMMC on the first flashing).

Due to above the bug was very difficult to reproduce.

This patch sets default values for all ext4fs_indir* pointers/variables.

Signed-off-by Lukasz Majewski <[email protected]>

---
 fs/ext4/ext4_common.c |   23 ++++++++++++++---------
 fs/ext4/ext4_write.c  |    1 +
 include/ext4fs.h      |    1 +
 3 files changed, 16 insertions(+), 9 deletions(-)

fs/ext4/ext4_common.c +14 -9

index 62e2e80..d0de285 100644
@@ -1841,16 +1841,8 @@ long int read_allocated_block(struct ext2_inode *inode, int fileblock)
        return blknr;
 }
 
-void ext4fs_close(void)
+void ext4fs_reinit_global(void)
 {
-       if ((ext4fs_file != NULL) && (ext4fs_root != NULL)) {
-               ext4fs_free_node(ext4fs_file, &ext4fs_root->diropen);
-               ext4fs_file = NULL;
-       }
-       if (ext4fs_root != NULL) {
-               free(ext4fs_root);
-               ext4fs_root = NULL;
-       }
        if (ext4fs_indir1_block != NULL) {
                free(ext4fs_indir1_block);
                ext4fs_indir1_block = NULL;

@@ -1870,6 +1862,19 @@ void ext4fs_close(void)
                ext4fs_indir3_blkno = -1;
        }
 }
+void ext4fs_close(void)
+{
+       if ((ext4fs_file != NULL) && (ext4fs_root != NULL)) {
+               ext4fs_free_node(ext4fs_file, &ext4fs_root->diropen);
+               ext4fs_file = NULL;
+       }
+       if (ext4fs_root != NULL) {
+               free(ext4fs_root);
+               ext4fs_root = NULL;
+       }
+
+       ext4fs_reinit_global();
+}
 
 int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name,
                                struct ext2fs_node **fnode, int *ftype)

fs/ext4/ext4_write.c +1 -0

index 46c573b..4a5f652 100644
@@ -562,6 +562,7 @@ static int ext4fs_delete_file(int inodeno)
 
        ext4fs_update();
        ext4fs_deinit();
+       ext4fs_reinit_global();
 
        if (ext4fs_init() != 0) {
                printf("error in File System init\n");

include/ext4fs.h +0 -0

index aacb147..fbbb002 100644

include/ext4fs.h +0 -1

 int ext4fs_ls(const char *dirname);
 int ext4fs_exists(const char *filename);
 void ext4fs_free_node(struct ext2fs_node *node, struct ext2fs_node *currroot);
--

1.7.10.4

Hi Lukasz,

Quoted 29 lines · Lukasz Majewski

This bug shows up when file stored on the ext4 file system is updated.

The ext4fs_delete_file() is responsible for deleting file's (e.g. uImage) data.
However some global data (especially ext4fs_indir2_block), which is used during file deletion are left unchanged.

The ext4fs_indir2_block pointer stores reference to old ext4 double indirect allocated blocks. When it is unchanged, after file deletion, ext4fs_write_file() uses the same pointer (since it is already initialized
- i.e. not NULL) to return number of blocks to write. This trunks larger file when previous one was smaller.

Lets consider following scenario:

1. Flash target with ext4 formatted boot.img (which has uImage [*] on itself) 2. Developer wants to upload their custom uImage [**]
- When new uImage [**] is smaller than the [*] - everything
works correctly - we are able to store the whole smaller file with corrupted ext4fs_indir2_block pointer
- When new uImage [**] is larger than the [*] - theCRC is
corrupted, since truncation on data stored at eMMC was done.
3. When uImage CRC error appears, then reboot and THOR/DFU reflashing causes proper setting of ext4fs_indir2_block() and after that uImage[**] is successfully stored (correct uImage [*] metadata is stored at an eMMC on the first flashing).

Quoted 1 line

Due to above the bug was very difficult to reproduce.

I wonder if a sandbox test would be a good idea? You could fairly easy mkfs in a loopback file and write a U-Boot script to operate on it. See test/ for some examples.

Quoted 65 lines

This patch sets default values for all ext4fs_indir* pointers/variables.

Signed-off-by Lukasz Majewski <[email protected]>

---
 fs/ext4/ext4_common.c |   23 ++++++++++++++---------
 fs/ext4/ext4_write.c  |    1 +
 include/ext4fs.h      |    1 +
 3 files changed, 16 insertions(+), 9 deletions(-)

fs/ext4/ext4_common.c +1 -9

index 62e2e80..d0de285 100644
@@ -1841,16 +1841,8 @@ long int read_allocated_block(struct
 ext2_inode *inode, int fileblock) return blknr;
 }
 
-void ext4fs_close(void)
+void ext4fs_reinit_global(void)
 {
-       if ((ext4fs_file != NULL) && (ext4fs_root != NULL)) {
-               ext4fs_free_node(ext4fs_file,
 &ext4fs_root->diropen);
-               ext4fs_file = NULL;
-       }
-       if (ext4fs_root != NULL) {
-               free(ext4fs_root);
-               ext4fs_root = NULL;
-       }
        if (ext4fs_indir1_block != NULL) {
                free(ext4fs_indir1_block);

                ext4fs_indir1_block = NULL;

fs/ext4/ext4_common.c +13 -0

@@ -1870,6 +1862,19 @@ void ext4fs_close(void)
                ext4fs_indir3_blkno = -1;
        }
 }
+void ext4fs_close(void)
+{
+       if ((ext4fs_file != NULL) && (ext4fs_root != NULL)) {
+               ext4fs_free_node(ext4fs_file,
 &ext4fs_root->diropen);
+               ext4fs_file = NULL;
+       }
+       if (ext4fs_root != NULL) {
+               free(ext4fs_root);
+               ext4fs_root = NULL;
+       }
+
+       ext4fs_reinit_global();
+}
 
 int ext4fs_iterate_dir(struct ext2fs_node *dir, char *name,

struct ext2fs_node **fnode, int
*ftype) diff --git a/fs/ext4/ext4_write.c b/fs/ext4/ext4_write.c index 46c573b..4a5f652 100644

fs/ext4/ext4_write.c +1 -0

@@ -562,6 +562,7 @@ static int ext4fs_delete_file(int inodeno)
 
        ext4fs_update();
        ext4fs_deinit();
+       ext4fs_reinit_global();
 

include/ext4fs.h int ext4fs_open(const char *filename); · line 136

Quoted 11 lines

fs/ext4/ext4_write.c +0 -0

        if (ext4fs_init() != 0) {
                printf("error in File System init\n");

include/ext4fs.h +1 -0

index aacb147..fbbb002 100644
@@ -133,6 +133,7 @@ int ext4fs_open(const char *filename);
 int ext4fs_read(char *buf, unsigned len);
 int ext4fs_mount(unsigned part_length);
 void ext4fs_close(void);
+void ext4fs_reinit_global(void);

Can we have a comment as to what this function does?

Quoted 7 lines

include/ext4fs.h +0 -0

 int ext4fs_ls(const char *dirname);
 int ext4fs_exists(const char *filename);
 void ext4fs_free_node(struct ext2fs_node *node, struct ext2fs_node

*currroot); --
1.7.10.4

Regards,
Simon

Code responsible for handling situation when ext4 has block size of 1024B can be ordered to take less space.

This patch does that for ext4 common and write files.

Signed-off-by Lukasz Majewski <[email protected]>

This bug shows up when file stored on the ext4 file system is updated.

The ext4fs_delete_file() is responsible for deleting file's (e.g. uImage) data.
However some global data (especially ext4fs_indir2_block), which is used during file deletion are left unchanged.

The ext4fs_indir2_block pointer stores reference to old ext4 double indirect allocated blocks. When it is unchanged, after file deletion, ext4fs_write_file() uses the same pointer (since it is already initialized
- i.e. not NULL) to return number of blocks to write. This trunks larger file when previous one was smaller.

Lets consider following scenario:

1. Flash target with ext4 formatted boot.img (which has uImage [*] on itself)
2. Developer wants to upload their custom uImage [**]
- When new uImage [**] is smaller than the [*] - everything works correctly - we are able to store the whole smaller file with corrupted ext4fs_indir2_block pointer
- When new uImage [**] is larger than the [*] - theCRC is corrupted, since truncation on data stored at eMMC was done.
3. When uImage CRC error appears, then reboot and LTHOR/DFU reflashing causes proper setting of ext4fs_indir2_block() and after that uImage[**] is successfully stored (correct uImage [*] metadata is stored at an eMMC on the first flashing).

Due to above the bug was very difficult to reproduce.
This patch sets default values for all ext4fs_indir* pointers/variables.

Signed-off-by Lukasz Majewski <[email protected]>