Re: [PATCH 1/2 v2] AOSP: erofs-utils: pass a parameter to write tail end in block list

[Yue Hu <zbestahu@gmail.com>]

On Tue, 15 Aug 2023 14:44:23 +0800
Gao Xiang <hsiangkao@linux.alibaba.com> wrote:

> On 2023/8/15 14:45, Yue Hu wrote:
> > On Tue, 15 Aug 2023 14:18:08 +0800
> > Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> >   
> >> On 2023/8/15 14:21, Yue Hu wrote:  
> >>> On Tue, 15 Aug 2023 13:36:46 +0800
> >>> Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> >>>      
> >>>> On 2023/8/15 13:40, Yue Hu wrote:  
> >>>>> On Tue, 15 Aug 2023 12:59:56 +0800
> >>>>> Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> >>>>>         
> >>>>>> On 2023/8/15 12:55, Yue Hu wrote:  
> >>>>>>> From: Yue Hu <huyue2@coolpad.com>
> >>>>>>>
> >>>>>>> We can determine whether the tail block is the first one or not during
> >>>>>>> the writing process.  Therefore, instead of internally checking the
> >>>>>>> block number for the tail block map, just simply pass the flag.
> >>>>>>>
> >>>>>>> Also, add the missing sbi argument to macro erofs_blknr.  
> >>>>>>
> >>>>>> Could you submit a patch to fix this issue first?  
> >>>>>
> >>>>> ok, will do that.
> >>>>>         
> >>>>>>        
> >>>>>>>
> >>>>>>> Signed-off-by: Yue Hu <huyue2@coolpad.com>
> >>>>>>> ---
> >>>>>>> v2: change commit message a bit
> >>>>>>>
> >>>>>>>      include/erofs/block_list.h | 4 ++--
> >>>>>>>      lib/block_list.c           | 5 ++---
> >>>>>>>      lib/inode.c                | 9 +++++++--
> >>>>>>>      3 files changed, 11 insertions(+), 7 deletions(-)
> >>>>>>>
> >>>>>>> diff --git a/include/erofs/block_list.h b/include/erofs/block_list.h
> >>>>>>> index 78fab44..e0dced8 100644
> >>>>>>> --- a/include/erofs/block_list.h
> >>>>>>> +++ b/include/erofs/block_list.h
> >>>>>>> @@ -19,7 +19,7 @@ void erofs_droid_blocklist_fclose(void);
> >>>>>>>      void erofs_droid_blocklist_write(struct erofs_inode *inode,
> >>>>>>>      				 erofs_blk_t blk_start, erofs_blk_t nblocks);
> >>>>>>>      void erofs_droid_blocklist_write_tail_end(struct erofs_inode *inode,
> >>>>>>> -					  erofs_blk_t blkaddr);
> >>>>>>> +					  erofs_blk_t blkaddr, bool first_block);  
> >>>>>>
> >>>>>> I still have no idea why we need this, could you describe the Android
> >>>>>> block map details for discussion?  
> >>>>>
> >>>>> Android block map is just adding file blocks to a range.
> >>>>>
> >>>>> So, the tail block should be needed in this range as well.
> >>>>> I think one simple way is just appending the tail block address in it as below:
> >>>>>
> >>>>> /`file_path` `block1_address`-`blockn_address` `tail_block_address`  
> >>>>
> >>>> why `tail_block_address` needs a seperate field?  
> >>>
> >>> Well, `erofs_write_tail_end()` is a separate logic and i think appending this block is
> >>> simple enough since i don't need to consider whether the block address is contiguous
> >>> with previous one.
> >>>
> >>> And i think Android block map can handle this since i have saw below in ext4:
> >>>
> >>> /system/.../libclang_rt.ubsan_standalone-arm-android.so 51276-51309 0 51310-51403  
> >>
> >> What's the meaning of 0 here?  
> > 
> > I have not check that yet, maybe i need to find time...
> > 
> > And check this: "/.../libxxx.so 87378-88630 0 0 0 88631 0 0 88632-88637 0 0 0 88638-88663"
> > 
> > so, i guess it looks like null block.  
> 
> So here erofs_droid_blocklist_write_tail_end is not actually needed in principle?
> 0 is just used for sparse block?

We need to confirm this.

> 
> Thanks,
> Gao Xiang