Re: [PATCH 0/8] u-boot: fs: add generic unaligned read handling

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 3149 bytes --]

On Wed, Jun 29, 2022 at 09:40:58AM +0800, Qu Wenruo wrote:
> 
> 
> On 2022/6/28 22:17, Tom Rini wrote:
> > On Tue, Jun 28, 2022 at 03:28:00PM +0800, Qu Wenruo wrote:
> > > [BACKGROUND]
> > > Unlike FUSE/Kernel which always pass aligned read range, U-boot fs code
> > > just pass the request range to underlying fses.
> > > 
> > > Under most case, this works fine, as U-boot only really needs to read
> > > the whole file (aka, 0 for both offset and len, len will be later
> > > determined using file size).
> > > 
> > > But if some advanced user/script wants to extract kernel/initramfs from
> > > combined image, we may need to do unaligned read in that case.
> > > 
> > > [ADVANTAGE]
> > > This patchset will handle unaligned read range in _fs_read():
> > > 
> > > - Get blocksize of the underlying fs
> > > 
> > > - Read the leading block contianing the unaligned range
> > >    The full block will be stored in a local buffer, then only copy
> > >    the bytes in the unaligned range into the destination buffer.
> > > 
> > >    If the first block covers the whole range, we just call it aday.
> > > 
> > > - Read the aligned range if there is any
> > > 
> > > - Read the tailing block containing the unaligned range
> > >    And copy the covered range into the destination.
> > > 
> > > [DISADVANTAGE]
> > > There are mainly two problems:
> > > 
> > > - Extra memory allocation for every _fs_read() call
> > >    For the leading and tailing block.
> > > 
> > > - Extra path resolving
> > >    All those supported fs will have to do extra path resolving up to 2
> > >    times (one for the leading block, one for the tailing block).
> > >    This may slow down the read.
> > 
> > This conceptually seems like a good thing.  Can you please post some
> > before/after times of reading large images from the supported
> > filesystems?
> > 
> 
> One thing to mention is, this change doesn't really bother large file read.
> 
> As the patchset is splitting a large read into 3 parts:
> 
> 1) Leading block
> 2) Aligned blocks, aka the main part of a large file
> 3) Tailing block
> 
> Most time should still be spent on part 2), not much different than the
> old code. Part 1) and Part 3) are at most 2 blocks (aka, 2 * 4KiB for
> most modern large enough fses).
> 
> So I doubt it would make any difference for large file read.
> 
> 
> Furthermore, as pointed out by Huang Jianan, currently the patchset can
> not handle read on soft link correctly, thus I'd update the series to do
> the split into even less parts:
> 
> 1) Leading block
>    For the unaligned initial block
> 
> 2) Aligned blocks until the end
>    The tailing block should still starts at a block aligned position,
>    thus most filesystems is already handling them correctly.
>    (Just a min(end, blockend) is enough for most cases already).
> 
> Anyway, I'll try to craft some benchmarking for file reads using sandbox.
> But please don't expect much (or any) difference in that case.

The rework sounds good.  And doing it without any real impact to
performance either way is good.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]