Re: [PATCH 4/4] btrfs: fix repair of compressed extents

[Christoph Hellwig <hch@lst.de>]

On Thu, Jul 07, 2022 at 03:50:34PM +0300, Nikolay Borisov wrote:
>> +	btrfs_bio_for_each_sector(fs_info, bv, bbio, iter, offset) {
>> +		u64 start = bbio->file_offset + offset;
>> +
>> +		if (!status &&
>> +		    (!csum || !check_data_csum(inode, bbio, offset, bv.bv_page,
>> +					       bv.bv_offset))) {
>
> In the !csum case you'd be executing a lot of code for no gain i.e 
> clean_io_failure. Instead, factor out the !csum case as a break from the 
> btrfs_bio_for_each_sector i.e no point in running clean_io_failure for 
> every sector in this case.

We still need to call clean_io_failure in that case, as repair can
also happen when I/O failed even without checksums.  Note that this
code also is just a copy and paste from the direct I/O completion
handler, and an equivalent but more obsfucated version of same
logic also exists in the buffered I/O completion path.

(and before anyone asks, I do have a WIP patchset to consolidate the
logic, and remove the calls to clean_io_failure for non-repair bio
completions, but it will take a while to get there).

>> -	cb = kmalloc(compressed_bio_size(fs_info, compressed_len), GFP_NOFS);
>> +	cb = kmalloc(sizeof(struct compressed_bio), GFP_NOFS);
>
> nit: This change is irrelevant to this patch - indeed we don't need to 
> allocate the flex array at the end of compressed_bio for writes, as the 
> csums are being stored in the ordered extents for the bio, still this it's 
> independent of this patch.

True.