Thanks for the review,
I'll prepare a v2 ASAP.
On 9/23/20 12:05 AM, Daniel Schwierzeck wrote:
> Am Sonntag, den 20.09.2020, 21:21 -0400 schrieb Tom Rini:
>> On Sun, Sep 20, 2020 at 06:29:01PM +0200, Mauro Condarelli wrote:
>>
>>> Signed-off-by: Mauro Condarelli <mc5...@mclink.it>
>>> ---
>>> fs/squashfs/sqfs.c | 45 +++++++++++++++++++++++++--------------
>>> fs/squashfs/sqfs_inode.c | 8 +++----
>>> include/configs/vocore2.h | 2 +-
> remove that file which is unrelated to this patch
I will as this is fixing things just for my target and that is clearly wrong.
OTOH I feel some provision should be implemented (probably at Config.in
level) to ensure SquashFS has enough malloc space for its needs.
What are the best practices to handle this?
>>> 3 files changed, 34 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/fs/squashfs/sqfs.c b/fs/squashfs/sqfs.c
>>> index 15208b4dab..b49331ce93 100644
>>> --- a/fs/squashfs/sqfs.c
>>> +++ b/fs/squashfs/sqfs.c
>>> @@ -18,6 +18,8 @@
>>> #include <string.h>
>>> #include <squashfs.h>
>>> #include <part.h>
>>> +#include <div64.h>
>>> +#include <stdio.h>
>>>
>>> #include "sqfs_decompressor.h"
>>> #include "sqfs_filesystem.h"
>>> @@ -82,13 +84,16 @@ static int sqfs_count_tokens(const char *filename)
>>> */
>>> static int sqfs_calc_n_blks(__le64 start, __le64 end, u64 *offset)
>>> {
>>> - u64 start_, table_size;
>>> + u64 start_, table_size, blks;
>>>
>>> table_size = le64_to_cpu(end) - le64_to_cpu(start);
>>> - start_ = le64_to_cpu(start) / ctxt.cur_dev->blksz;
>>> + start_ = le64_to_cpu(start);
>>> + do_div(start_, ctxt.cur_dev->blksz);
> have you tried with lldiv() which returns the 64bit result? Also it
> would be a little cleaner:
>
> start_ = lldiv(le64_to_cpu(start), ctxt.cur_dev->blksz);
I thought of that (actually my first attempt was quite similar,
but I noticed that lldiv() actually uses do_div() internally and
so I decided to go directly for the lower level (and presumably
faster) solution.
If You (or the maintainers) feel otherwise I can revert with
no problems.
>>> *offset = le64_to_cpu(start) - (start_ * ctxt.cur_dev->blksz);
>>>
>>> - return DIV_ROUND_UP(table_size + *offset, ctxt.cur_dev->blksz);
>>> + blks = table_size + *offset;
>>> + if (do_div(blks, ctxt.cur_dev->blksz)) blks++;
>>> + return blks;
> maybe define something like this and use that instead of DIV_ROUND_UP:
>
> #define lldiv_round_up(n, d) lldiv((n) + (d) - 1, (d))
Again: IMHO having a macro does not add much value here
and using lldiv() only adds a further level of call nesting, but
I'm open to switch; I would like to understand the rationale
behind these requests, though.
>>> }
>>>
>>> /*
>>> @@ -109,8 +114,8 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
>>> if (inode_fragment_index >= get_unaligned_le32(&sblk->fragments))
>>> return -EINVAL;
>>>
>>> - start = get_unaligned_le64(&sblk->fragment_table_start) /
>>> - ctxt.cur_dev->blksz;
>>> + start = get_unaligned_le64(&sblk->fragment_table_start);
>>> + do_div(start, ctxt.cur_dev->blksz);
>>> n_blks = sqfs_calc_n_blks(sblk->fragment_table_start,
>>> sblk->export_table_start,
>>> &table_offset);
>>> @@ -135,7 +140,8 @@ static int sqfs_frag_lookup(u32 inode_fragment_index,
>>> start_block = get_unaligned_le64(table + table_offset + block *
>>> sizeof(u64));
>>>
>>> - start = start_block / ctxt.cur_dev->blksz;
>>> + start = start_block;
>>> + do_div(start, ctxt.cur_dev->blksz);
>>> n_blks = sqfs_calc_n_blks(cpu_to_le64(start_block),
>>> sblk->fragment_table_start, &table_offset);
>>>
>>> @@ -641,8 +647,8 @@ static int sqfs_read_inode_table(unsigned char
>>> **inode_table)
>>>
>>> table_size = get_unaligned_le64(&sblk->directory_table_start) -
>>> get_unaligned_le64(&sblk->inode_table_start);
>>> - start = get_unaligned_le64(&sblk->inode_table_start) /
>>> - ctxt.cur_dev->blksz;
>>> + start = get_unaligned_le64(&sblk->inode_table_start);
>>> + do_div(start, ctxt.cur_dev->blksz);
>>> n_blks = sqfs_calc_n_blks(sblk->inode_table_start,
>>> sblk->directory_table_start, &table_offset);
>>>
>>> @@ -725,8 +731,8 @@ static int sqfs_read_directory_table(unsigned char
>>> **dir_table, u32 **pos_list)
>>> /* DIRECTORY TABLE */
>>> table_size = get_unaligned_le64(&sblk->fragment_table_start) -
>>> get_unaligned_le64(&sblk->directory_table_start);
>>> - start = get_unaligned_le64(&sblk->directory_table_start) /
>>> - ctxt.cur_dev->blksz;
>>> + start = get_unaligned_le64(&sblk->directory_table_start);
>>> + do_div(start, ctxt.cur_dev->blksz);
>>> n_blks = sqfs_calc_n_blks(sblk->directory_table_start,
>>> sblk->fragment_table_start, &table_offset);
>>>
>>> @@ -1158,6 +1164,7 @@ static int sqfs_get_regfile_info(struct
>>> squashfs_reg_inode *reg,
>>> fentry);
>>> if (ret < 0)
>>> return -EINVAL;
>>> +
>>> finfo->comp = true;
>>> if (fentry->size < 1 || fentry->start == 0x7FFFFFFF)
>>> return -EINVAL;
>>> @@ -1328,17 +1335,19 @@ int sqfs_read(const char *filename, void *buf,
>>> loff_t offset, loff_t len,
>>> data_offset = finfo.start;
>>> datablock = malloc(get_unaligned_le32(&sblk->block_size));
>>> if (!datablock) {
>>> + printf("Error: malloc(%u) failed.\n",
>>> get_unaligned_le32(&sblk->block_size));
>>> ret = -ENOMEM;
>>> goto free_paths;
>>> }
>>> }
>>>
>>> for (j = 0; j < datablk_count; j++) {
>>> - start = data_offset / ctxt.cur_dev->blksz;
>>> + start = data_offset;
>>> + do_div(start, ctxt.cur_dev->blksz);
>>> table_size = SQFS_BLOCK_SIZE(finfo.blk_sizes[j]);
>>> table_offset = data_offset - (start * ctxt.cur_dev->blksz);
>>> - n_blks = DIV_ROUND_UP(table_size + table_offset,
>>> - ctxt.cur_dev->blksz);
>>> + n_blks = table_size + table_offset;
>>> + if (do_div(n_blks, ctxt.cur_dev->blksz)) n_blks++;
>>>
>>> data_buffer = malloc_cache_aligned(n_blks *
>>> ctxt.cur_dev->blksz);
>>>
>>> @@ -1365,8 +1374,10 @@ int sqfs_read(const char *filename, void *buf,
>>> loff_t offset, loff_t len,
>>> dest_len = get_unaligned_le32(&sblk->block_size);
>>> ret = sqfs_decompress(&ctxt, datablock, &dest_len,
>>> data, table_size);
>>> - if (ret)
>>> + if (ret) {
>>> + printf("Errrt: block decompress failed.\n");
>>> goto free_buffer;
>>> + }
>>>
>>> memcpy(buf + offset + *actread, datablock, dest_len);
>>> *actread += dest_len;
>>> @@ -1388,10 +1399,12 @@ int sqfs_read(const char *filename, void *buf,
>>> loff_t offset, loff_t len,
>>> goto free_buffer;
>>> }
>>>
>>> - start = frag_entry.start / ctxt.cur_dev->blksz;
>>> + start = frag_entry.start;
>>> + do_div(start, ctxt.cur_dev->blksz);
>>> table_size = SQFS_BLOCK_SIZE(frag_entry.size);
>>> table_offset = frag_entry.start - (start * ctxt.cur_dev->blksz);
>>> - n_blks = DIV_ROUND_UP(table_size + table_offset, ctxt.cur_dev->blksz);
>>> + n_blks = table_size + table_offset;
>>> + if (do_div(n_blks, ctxt.cur_dev->blksz)) n_blks++;
>>>
>>> fragment = malloc_cache_aligned(n_blks * ctxt.cur_dev->blksz);
>>>
>>> diff --git a/fs/squashfs/sqfs_inode.c b/fs/squashfs/sqfs_inode.c
>>> index 1368f3063c..c330e32a0e 100644
>>> --- a/fs/squashfs/sqfs_inode.c
>>> +++ b/fs/squashfs/sqfs_inode.c
>>> @@ -11,6 +11,7 @@
>>> #include <stdio.h>
>>> #include <stdlib.h>
>>> #include <string.h>
>>> +#include <div64.h>
>>>
>>> #include "sqfs_decompressor.h"
>>> #include "sqfs_filesystem.h"
>>> @@ -67,10 +68,9 @@ int sqfs_inode_size(struct squashfs_base_inode *inode,
>>> u32 blk_size)
>>> u64 file_size = get_unaligned_le64(&lreg->file_size);
>>> unsigned int blk_list_size;
>>>
>>> - if (fragment == 0xFFFFFFFF)
>>> - blk_list_size = DIV_ROUND_UP(file_size, blk_size);
>>> - else
>>> - blk_list_size = file_size / blk_size;
>>> + if (do_div(file_size, blk_size) && (fragment == 0xFFFFFFFF))
>>> + file_size++;
>>> + blk_list_size = file_size;
>>>
>>> return sizeof(*lreg) + blk_list_size * sizeof(u32);
>>> }
>>> diff --git a/include/configs/vocore2.h b/include/configs/vocore2.h
>>> index 29a57ad233..dfdb8fcc04 100644
>>> --- a/include/configs/vocore2.h
>>> +++ b/include/configs/vocore2.h
>>> @@ -41,7 +41,7 @@
>>>
>>> /* Memory usage */
>>> #define CONFIG_SYS_MAXARGS 64
>>> -#define CONFIG_SYS_MALLOC_LEN (1024 * 1024)
>>> +#define CONFIG_SYS_MALLOC_LEN (16 * 1024 * 1024)
>>> #define CONFIG_SYS_BOOTPARAMS_LEN (128 * 1024)
>>> #define CONFIG_SYS_CBSIZE 512
>> Add in maintainers..
done ;)
I just rebased on later master (some patches did not apply cleanly).
I will check again before submitting v2.
Regards
Mauro
