[RFC PATCH v2 1/1] Fix missing __udivdi3 in SquashFS implementation.

[Mauro Condarelli]

Ok, Thanks.
Patch is ready, I'll send it after some extended testing on my system (vocore2).

Regards
Mauro

On 10/1/20 10:56 AM, Miquel Raynal wrote:
> Hi Mauro,
>
> Mauro Condarelli <mc5686 at mclink.it> wrote on Thu, 1 Oct 2020 10:53:30
> +0200:
>
>> Correcting myself.
>> See below.
>>
>> On 10/1/20 10:41 AM, Mauro Condarelli wrote:
>>> Thanks for Your review.
>>>
>>> On 10/1/20 9:59 AM, Miquel Raynal wrote:  
>>>> Hello,
>>>>
>>>> Thomas Petazzoni <thomas.petazzoni at bootlin.com> wrote on Thu, 1 Oct
>>>> 2020 09:28:41 +0200:
>>>>  
>>>>> Hello,
>>>>>
>>>>> On Wed, 30 Sep 2020 17:45:11 +0200
>>>>> Mauro Condarelli <mc5686 at mclink.it> wrote:
>>>>>  
>>>>>> Use right shift to avoid 64-bit divisions.
>>>>>>
>>>>>> These divisions are needed to convert from file length (potentially
>>>>>> over 32-bit range) to block number, so result and remainder are
>>>>>> guaranteed to fit in 32-bit integers.
>>>>>>
>>>>>> Signed-off-by: Mauro Condarelli <mc5686 at mclink.it>    
>>>>> Perhaps the commit log should contain an example U-Boot
>>>>> configuration/platform where it fails to build. Indeed, we did test the
>>>>> SquashFS code on ARM 32-bit, and it built and worked fine.  
>>> This fails on mips32, specifically for the vocore2 board.
>>> Problem here is __udivdi3 is not defined for this architecture
>>> while it is for ARM32.
>>> My (limited) understanding suggests it should be removed for ARM
>>> since its usage has been (is being?) weeded out from both kernel
>>> and u-boot. This is not my call, though.
>>>
>>> I will add a note to v3.
>>>  
>>>>>> ---
>>>>>>
>>>>>> Changes in v2:
>>>>>> - replace division with right shift (Daniel Schwierzeck).
>>>>>> - remove vocore2-specific change (Daniel Schwierzeck).
>>>>>> - add warning to Kconfig about CONFIG_SYS_MALLOC_LEN (Tom Rini).
>>>>>>
>>>>>>  fs/squashfs/Kconfig      |  2 ++
>>>>>>  fs/squashfs/sqfs.c       | 30 +++++++++++++++---------------
>>>>>>  fs/squashfs/sqfs_inode.c |  6 +++---
>>>>>>  3 files changed, 20 insertions(+), 18 deletions(-)
>>>>>>
>>>>>> diff --git a/fs/squashfs/Kconfig b/fs/squashfs/Kconfig
>>>>>> index 54ab1618f1..7c3f83d007 100644
>>>>>> --- a/fs/squashfs/Kconfig
>>>>>> +++ b/fs/squashfs/Kconfig
>>>>>> @@ -9,3 +9,5 @@ config FS_SQUASHFS
>>>>>>  	  filesystem use, for archival use (i.e. in cases where a .tar.gz file
>>>>>>  	  may be used), and in constrained block device/memory systems (e.g.
>>>>>>  	  embedded systems) where low overhead is needed.
>>>>>> +	  WARNING: if compression is enabled SquashFS needs a large amount
>>>>>> +	  of dynamic memory; make sure CONFIG_SYS_MALLOC_LEN >= 0x4000.    
>>>>> This change is completely unrelated, and should be in a separate patch.  
>>>> I was about to tell you the same thing, this warning is useful but
>>>> should definitely lay into its own commit.  
>>> Will do in v3.
>>>
>>>  
>>>>>> -		n_blks = DIV_ROUND_UP(table_size + table_offset,
>>>>>> -				      ctxt.cur_dev->blksz);
>>>>>> +		n_blks = (table_size + table_offset + ctxt.cur_dev->blksz - 1) >>
>>>>>> +			ctxt.cur_dev->log2blksz;    
>>>>> I understand why you have to add blksz - 1 before doing the shift, but
>>>>> I find that it's overall a lot less readable/clear. Is there a way to
>>>>> do better ?
>>>>>
>>>>> We could use DIV_ROUND_UP_ULL() I guess.  
>>> I did not do this because DIV_ROUND_UP() is in a global (non-architecture-specific)
>>> header and it unconditionally uses division; I did not know how to handle this.
>>> Would a comment suffice to clarify intent? Something like:
>>>
>>> n_blks = (table_size + table_offset + ctxt.cur_dev->blksz - 1) >>
>>> 	  ctxt.cur_dev->log2blksz; /* ROUND_UP division */
>>>
>>> Note: this problem stays even if we roll-back to use do_div(); see below.  
>> actually include/linux/kernel.h defines both DIV_ROUND_DOWN_ULL()
>> and DIV_ROUND_UP_ULL() I suppose we should use those in all cases.
>>
>>
>>>>>>  		else
>>>>>> -			blk_list_size = file_size / blk_size;
>>>>>> +			blk_list_size = file_size > LOG2(blk_size);    
>>>>> Very bad mistake here: > should be >>.  
>>> Sorry, my bad.
>>> Corrected for v3.
>>>  
>>>> I personally highly dislike replacing divisions into shifts. I think
>>>> it's too much effort when trying to understand code you did not write
>>>> yourself. Is it possible to use something like do_div? plus, you can
>>>> check the remainder to be 0 in this case.  
>>> Please make up your mind about this.
>>> I personally tend to agree with Miquèl and my v1 patch used
>>> do_div() exclusively (i did not use even the lldiv() wrapper), but
>>> I will not insist either way... just let me know what's considered
>>> better.  
>> As said above: it seems using the macros is both "standard" and
>> safer than using shifts.
>> If I get a go-ahead I'll use those macros in v3.
> I think we all agree using these macros is much nicer, readable and
> probably almost as fast.
>
> Thanks,
> Miquèl
>