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

[Dominique Martinet]

Dominique Martinet wrote on Tue, Apr 18, 2023 at 12:53:35PM +0900:
> 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)

(Wasn't able to create such a file, I assume that means the uncompressed
data must fits in a page -- if we deal with arm machines with 16k or 64k
page size that'll probably change things again but I'll just pretend
sector size will magically match in this case..)

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

Question regarding inline extent: the main loop goes straight out after
reading an inline extent, assuming it is always alone (or last) -- is
that correct?
By playing with `filefrag -v` I could sometimes see files that
temporarily have inline + a delalloc extent, but wasn't able to make a
file that kept the inline extent after appending more data, so I guess
it is also sane enough... Just making it continue and letting the loop
end seems just as simple now there is no trailing if, but if inline
extents cannot be mixed I'll be happy to keep it going out.


back to btrfs_read_extent_reg:
merging the tail back into the main loop breaks the first assert
(IS_ALIGNED(len, fs_info->sectorsize) in particular).
The old loop invocation made sure it was aligned and
read_and_truncate_page used to take care of calling it with a bigger
buffer when it was not.

I was only looking at the compressed path and that does not care about
'len' alignment because it makes an intermediate copy for decompression,
but the BTRFS_COMPRESS_NONE's read_extent_data might care?
I didn't see anything that actually requires alignment there (len in
particular should be ok, but even offset->logical seems to properly be
used as "being part of a range" in lookups so alignment doesn't actually
matter), but if this isn't tested I can understand wanting to be more
careful there.

Ok so this is rightfully less obvious than I had first assumed -- sorry
for rushing in. Let's do baby steps:
- I'll resend just the first patch shortly, it's a real fix and I'll be
using it right away.
- I'll double check 'len' doesn't need to be aligned in
btrfs_read_extent_reg and just rework the main loop to allow removing
the tail exception, that'll avoid a double read in many cases and I
think it's worth doing.
Might take a bit more time as I want to finish some other work, let's
say a few days.
- That leaves the two issues I brought up with the main loop:
 * inline case ignoring end; this is minor enough but easy to fix
 * btrfs_read_extent_reg() assuming start of extent in its length
 agument; I believe it'll be easier to stop trying to set a upper limit
 in the main loop and just have btrfs_read_extent_reg() do it itself, we
 can use its return value to know how much was actually read.
 (Probably just substract offset - key.offset to extent_num_bytes to
 have a new cap, but I still don't understand btrfs_file_extent_offset()
 in all of this as it's always 0 when I looked)
Will try to do that over the next few weeks, but if you want to look at
it feel free to do this before me.
- At this point it might be worth considering removing the initial check
as that also makes an extra small read in the unaligned case, but it's
not a bug and can wait.


What do you think?
-- 
Dominique Martinet | Asmadeus