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

[Miquel Raynal]

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.
> 
> > ---
> > 
> > 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.

> > -		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.
> 
> 
> >  		else
> > -			blk_list_size = file_size / blk_size;
> > +			blk_list_size = file_size > LOG2(blk_size);  
> 
> Very bad mistake here: > should be >>.

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.

Thanks,
Miquèl