Re: [PATCH] erofs-utils: do not check ->idata_size for compressed files in erofs_prepare_inode_buffer()

[Gao Xiang <hsiangkao@linux.alibaba.com>]

Hi Yue,

On Tue, Aug 31, 2021 at 05:00:29PM +0800, Yue Hu wrote:
> On Thu, 17 Jun 2021 18:14:17 +0800
> Yue Hu <zbestahu@gmail.com> wrote:
> 
> > On Thu, 17 Jun 2021 17:30:26 +0800
> > Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> > 
> > > On Thu, Jun 17, 2021 at 05:27:33PM +0800, Gao Xiang wrote:  
> > > > On Thu, Jun 17, 2021 at 05:15:55PM +0800, Yue Hu wrote:    
> > > > > On Thu, 17 Jun 2021 17:04:29 +0800
> > > > > Gao Xiang <hsiangkao@linux.alibaba.com> wrote:
> > > > >     
> > > > > > Hi Yue,
> > > > > > 
> > > > > > On Thu, Jun 17, 2021 at 04:29:54PM +0800, Yue Hu wrote:    
> > > > > > > From: Yue Hu <huyue2@yulong.com>
> > > > > > > 
> > > > > > > erofs_write_compressed_file() will always set inode->idata_size = 0
> > > > > > > if succeed, that means no tail-end data for compressed files. So, no
> > > > > > > need to call erofs_prepare_tail_block() which is used to handle
> > > > > > > tail-end data for that case. Just skip it.      
> > > > > > 
> > > > > > Thanks for the patch, due to somewhat long time so I don't quite
> > > > > > remember the exact logic here now...
> > > > > > 
> > > > > > Yet from the description before, it's not strictly correct
> > > > > > since my original intention would be to support tail-packing
> > > > > > inline compressed data which is similar to uncompressed case
> > > > > > to decrease tail extent I/O and save more space.    
> > > > > 
> > > > > nice.
> > > > >     
> > > > > > 
> > > > > > BTW, if you have some interest, would you like to implement it? :)    
> > > > > 
> > > > > I don't know if i can finish it. But i would like to have a try :)    
> > > > 
> > > > My rough thought is to try to inline the last tail compresseed
> > > > extent after the on-disk compressed extents, maybe we could let
> > > > it work for non-compact (legacy) compress index format cases...    
> > > 
> > > I mean try to implement non-compact (legacy) compress index format cases
> > > first.
> 
> I'm trying to do it under 4.19 code (since i have no 5.x environment temporarily).
> 
> Now, i think mkfs should be done. But, kernel side seems not working fine(no crash,
> no decompression warning/bug). Only some files are working, others not. I'm sure i
> can catch the inline data correctly via file dump. And I'm trying debug the issue.
> Maybe i need more time to read/understand more decompression code related.
> 
> BTW, now i understand no need to go z_erofs_vle_work_xxx for inline part(cur-end)
> , just go next_part after mapping as below, am i right? 
> 

You are right. For the common cases (except for fiemap or cases to get the exact
decompressed length), we only need to calculate the start of the compression extent,
so it's transversal in the reverse order.

But really... Again, I don't suggest using 4.19 staging code for real production
or further development. The uncompressed part is considered as stable, but
compression side may not (also it was disabled by default). Please also see,
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/staging/erofs/Kconfig?h=v4.19#n86

" config EROFS_FS_ZIP
  bool "EROFS Data Compresssion Support"
  depends on EROFS_FS
  help
    Currently we support VLE Compression only.
    Play at your own risk.

    If you don't want to use compression feature, say N. "

Our original first real production codebase was between 5.2~5.3. Therefore,
I suggest using >= 5.4 LTS codebase for production. You could also find
some backport codebase on github, e.g.:
https://github.com/nolange/erofs_kernel_4_19
, which backports 5.6 erofs codebase to 4.19.

As for tail-packing inline extent feature, how about focusing on on-disk
design and mkfs/erofsfuse implementation first as PoC?

I'm afraid that if you only focus on 4.19 codebase, the format of compact
indexes will be ignored, but "compact indexes" is the default option for
erofs now since it has less metadata overhead than non-compact indexes,
so both the sequential / random read are better.

Thanks,
Gao Xiang

> Thanks.
>   
> > 
> > Ok, let me try to implement it.
> > 
> > Thanks.