[PATCH U-BOOT 2/3] btrfs: btrfs_file_read: allow opportunistic read until the end

[Qu Wenruo]

On 2023/4/18 11:53, Dominique Martinet wrote:
> Qu Wenruo wrote on Tue, Apr 18, 2023 at 11:21:00AM +0800:
>>> No, was just thinking the leading part being a separate loop doesn't
>>> seem to make sense either as the code shouldn't care about sector size
>>> alignemnt but about full extents.
>>
>> The main concern related to the leading unaligned part is, we need to skip
>> something unaligned from the beginning , while all other situations never
>> need to skip such case (they at most skip the tailing part).
> 
> Ok, there is one exception for inline extents apparently.. But I'm not
> still not convinced the `aligned_start != file_offset` check is enough
> for that either; I'd say it's unlikely but the inline part can be
> compressed, so we could have a file which has > 4k (sectorsize) of
> expanded data, so a read from the 4k offset would skip the special
> handling and fail (reading the whole extent in dest)

Btrfs inline has a limit to sectorsize.

That means, inlined compressed extent can at most be 4K sized (if 4K is 
our sector size).

So that won't be a problem.

> 
> Even if that's not possible, reading just the first 10 bytes of an
> inline extent will be aligned and go through the main loop which just
> reads the whole extent, so it'll need the same handling as the regular
> btrfs_read_extent_reg handling at which point it might just as well
> handle start offset too.

If we just read 10 bytes, the aligned part should handle it well.

My real concern is what if we read 10 bytes at offset 10 bytes.

If this can be handled in the same way of aligned read (and still be 
reasonable readable), then it would be awesome.

> 
> 
> That aside taking the loop in order:
> - lookup_data_extent doesn't care (used in heading/tail)
> - skipping holes don't care as they explicitely put cursor at start of
> next extent (or bail out if nothing next)
> - inline needs fixing anyway as said above
> - PREALLOC or nothing on disk also goes straight to next and is ok
> - ah, I see what you meant now, we need to substract the current
> position within the extent to extent_num_bytes...
> That's also already a problem, though; to take the same example:
> 0                 8k           16k
> [extent1          | extent2 ... ]
> reading from 4k onwards will try to read
> min(extent_num_bytes, end-cur) = min(8k, 12k) = 8k
> from the 4k offset which goes over the end of the extent.

That's indeed a problem.

As most of the Uboot fs drivers only care read the whole file, never 
really utilize the ability to read part of the file, that path is not 
properly tested.
(Some driver, IIRC ubifs?, doesn't even allow read with non-zero offset)

Thanks,
Qu

> 
> That could actually be my resets from the previous mail.
> 
> 
> So either the first check should just lookup the extent and check that
> extent start matches current offset instead of checking for sectorsize
> alignment, or we can just fix the loop and remove the first if.
>