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

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

[Yue Hu]

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.

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);
 void erofs_droid_blocklist_write_extent(struct erofs_inode *inode,
 					erofs_blk_t blk_start, erofs_blk_t nblocks,
 					bool first_extent, bool last_extent);
@@ -28,7 +28,7 @@ static inline void erofs_droid_blocklist_write(struct erofs_inode *inode,
 				 erofs_blk_t blk_start, erofs_blk_t nblocks) {}
 static inline void
 erofs_droid_blocklist_write_tail_end(struct erofs_inode *inode,
-					  erofs_blk_t blkaddr) {}
+				     erofs_blk_t blkaddr, bool first_block) {}
 static inline void
 erofs_droid_blocklist_write_extent(struct erofs_inode *inode,
 				   erofs_blk_t blk_start, erofs_blk_t nblocks,
diff --git a/lib/block_list.c b/lib/block_list.c
index 896fb01..a1c719d 100644
--- a/lib/block_list.c
+++ b/lib/block_list.c
@@ -85,7 +85,7 @@ void erofs_droid_blocklist_write(struct erofs_inode *inode,
 }
 
 void erofs_droid_blocklist_write_tail_end(struct erofs_inode *inode,
-					  erofs_blk_t blkaddr)
+					  erofs_blk_t blkaddr, bool first_block)
 {
 	if (!block_list_fp || !cfg.mount_point)
 		return;
@@ -94,8 +94,7 @@ void erofs_droid_blocklist_write_tail_end(struct erofs_inode *inode,
 	if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode))
 		return;
 
-	/* XXX: another hack, which means it has been outputed before */
-	if (erofs_blknr(inode->i_size)) {
+	if (!first_block) {
 		if (blkaddr == NULL_ADDR)
 			fprintf(block_list_fp, "\n");
 		else
diff --git a/lib/inode.c b/lib/inode.c
index c4d1476..7cd3e69 100644
--- a/lib/inode.c
+++ b/lib/inode.c
@@ -734,12 +734,15 @@ static int erofs_write_tail_end(struct erofs_inode *inode)
 {
 	struct erofs_sb_info *sbi = inode->sbi;
 	struct erofs_buffer_head *bh, *ibh;
+	bool first_block;
 
 	bh = inode->bh_data;
 
 	if (!inode->idata_size)
 		goto out;
 
+	first_block = !erofs_blknr(sbi, inode->i_size);
+
 	/* have enough room to inline data */
 	if (inode->bh_inline) {
 		ibh = inode->bh_inline;
@@ -747,7 +750,8 @@ static int erofs_write_tail_end(struct erofs_inode *inode)
 		ibh->fsprivate = erofs_igrab(inode);
 		ibh->op = &erofs_write_inline_bhops;
 
-		erofs_droid_blocklist_write_tail_end(inode, NULL_ADDR);
+		erofs_droid_blocklist_write_tail_end(inode, NULL_ADDR,
+						     first_block);
 	} else {
 		int ret;
 		erofs_off_t pos, zero_pos;
@@ -801,7 +805,8 @@ static int erofs_write_tail_end(struct erofs_inode *inode)
 		free(inode->idata);
 		inode->idata = NULL;
 
-		erofs_droid_blocklist_write_tail_end(inode, erofs_blknr(sbi, pos));
+		erofs_droid_blocklist_write_tail_end(inode, erofs_blknr(sbi, pos),
+						     first_block);
 	}
 out:
 	/* now bh_data can drop directly */
-- 
2.17.1

[PATCH 2/2] AOSP: erofs-utils: add a variable to validate tail block map in block list

[Yue Hu]

From: Yue Hu <huyue2@coolpad.com>

We write the block map directly before the tail, as this applies only to
regular files.  However, the tail block map does not follow the rule.

Therefore, introduce a variable to track whether the tail block is safe
to write before its map if it exists rather than separately check the
file type when mapping.

Signed-off-by: Yue Hu <huyue2@coolpad.com>
---
 lib/block_list.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/lib/block_list.c b/lib/block_list.c
index a1c719d..e59a8d7 100644
--- a/lib/block_list.c
+++ b/lib/block_list.c
@@ -12,6 +12,7 @@
 #include "erofs/print.h"
 
 static FILE *block_list_fp;
+static bool regular_tail = false;
 
 int erofs_droid_blocklist_fopen(void)
 {
@@ -77,22 +78,23 @@ void erofs_droid_blocklist_write_extent(struct erofs_inode *inode,
 void erofs_droid_blocklist_write(struct erofs_inode *inode,
 				 erofs_blk_t blk_start, erofs_blk_t nblocks)
 {
-	if (!block_list_fp || !cfg.mount_point || !nblocks)
+	if (!block_list_fp || !cfg.mount_point)
 		return;
 
-	blocklist_write(inode->i_srcpath, blk_start, nblocks,
-			true, !inode->idata_size);
+	if (nblocks)
+		blocklist_write(inode->i_srcpath, blk_start, nblocks, true,
+				!inode->idata_size);
+	if (inode->idata_size)
+		regular_tail = true;
 }
 
 void erofs_droid_blocklist_write_tail_end(struct erofs_inode *inode,
 					  erofs_blk_t blkaddr, bool first_block)
 {
-	if (!block_list_fp || !cfg.mount_point)
+	if (!block_list_fp || !cfg.mount_point || !regular_tail)
 		return;
 
-	/* XXX: a bit hacky.. may need a better approach */
-	if (S_ISDIR(inode->i_mode) || S_ISLNK(inode->i_mode))
-		return;
+	regular_tail = false;
 
 	if (!first_block) {
 		if (blkaddr == NULL_ADDR)
-- 
2.17.1

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

[Gao Xiang]

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?

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

Thanks,
Gao Xiang

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

[Yue Hu]

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`

or

/`file_path` `tail_block_address`   //only one block

> 
> Thanks,
> Gao Xiang

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

[Gao Xiang]

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?

> 
> or
> 
> /`file_path` `tail_block_address`   //only one block

how about recording the previous inode pointer as an internal global variable?
if it's not the same inode, trigger "\n" immediately.

and generate files as:
" `block1_address`-`blockn_address`"  for many blocks;
" `block1_address`" for a single block?

Thanks,
Gao Xiang

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

[Yue Hu]

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

> 
> > 
> > or
> > 
> > /`file_path` `tail_block_address`   //only one block  
> 
> how about recording the previous inode pointer as an internal global variable?
> if it's not the same inode, trigger "\n" immediately.
> 
> and generate files as:
> " `block1_address`-`blockn_address`"  for many blocks;
> " `block1_address`" for a single block?

Let me check.

> 
> Thanks,
> Gao Xiang

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

[Gao Xiang]

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?

Thanks,
Gao Xiang

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

[Yue Hu]

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.

> 
> Thanks,
> Gao Xiang
> 

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

[Gao Xiang]

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?

Thanks,
Gao Xiang

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

[Yue Hu]

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