Re: [PATCH v3 2/2] btrfs: use kvmalloc() for stripe buffer of scrub_stripe

在 2026/6/30 10:35, David Sterba 写道:
> On Mon, Jun 29, 2026 at 06:55:53PM +0930, Qu Wenruo wrote:
>> Currently we're using scrub_stripe::folios[] to store all contents of a
>> stripe.
>>
>> This means we need all the extra work to handle things like sub-page
>> cases, and also require larger folios to handle bs > ps cases.
>>
>> On the other hand, it's not hard to allocate a 64K large folio to cover
>> the full stripe, getting rid of the cross-page handling.
>>
>> Furthermore, even if that large folio allocation failed, we can still
>> use vmalloc() to allocate a virtually contiguous space and still get rid
>> of cross-page handling.
>>
>> This patch will go with kvmalloc() to allocate 64K of memory for
>> the stripe buffer, thus getting rid of all the complex cross-page
>> handling.
>>
>> The following aspects can be greatly simplified:
> 
> I think the trade off using the virtual mappings is justified, the code
> is indeed simplified.
> 
>> @@ -332,13 +333,10 @@ static void release_scrub_stripe(struct scrub_stripe *stripe)
>>   	if (!stripe)
>>   		return;
>>   
>> -	for (int i = 0; i < SCRUB_STRIPE_MAX_FOLIOS; i++) {
>> -		if (stripe->folios[i])
>> -			folio_put(stripe->folios[i]);
>> -		stripe->folios[i] = NULL;
>> -	}
>> +	kvfree(stripe->buffer);
> 
> The life time of the stripe->buffer matches the scrub life time so we
> won't be allocating and freeing the page repeatedly.
> 
>>   	kfree(stripe->sectors);
>>   	kfree(stripe->csums);
>> +	stripe->buffer = NULL;
>>   	stripe->sectors = NULL;
>>   	stripe->csums = NULL;
>>   	stripe->sctx = NULL;
> 
>> @@ -840,8 +805,10 @@ static void scrub_verify_one_sector(struct scrub_stripe *stripe, int sector_nr)
>>   		return;
>>   	}
>>   
>> -	ret = btrfs_check_block_csum(fs_info, paddr, csum_buf, sector->csum);
>> -	if (ret < 0) {
>> +	scrub_calc_vaddr_csum(fs_info,
>> +			      stripe->buffer + (sector_nr << fs_info->sectorsize_bits),
>> +			      fs_info->sectorsize, csum_buf);
>> +	if (memcmp(csum_buf, sector->csum, fs_info->csum_size)) {
> 
> Please use the if (memcmp(...) != 0) form
> 
>> @@ -891,6 +868,10 @@ static void scrub_repair_read_endio(struct btrfs_bio *bbio)
>>   
>>   	ASSERT(sector_nr < stripe->nr_sectors);
>>   
>> +	if (is_vmalloc_addr(stripe->buffer))
>> +		invalidate_kernel_vmap_range(
>> +			stripe->buffer + (sector_nr << fs_info->sectorsize_bits),
>> +			bio_size);
> 
> This is done twice so may not require a helper but it's quite important
> for the virtual mappings, so a comment making it explicit that read side
> must do the invalidation would be good.

I can refactor the code so that scrub_repair_read_endio() and 
scrub_read_endio() share a common function, only diff handling after the 
bio is put.

In that case, we can easily add a common for the shared path code.

Thanks,
Qu

> 
> Alternatively I was thinking about naming like "btrfs_invalidate_range_before_read"
> but a comment would be more clear, also we'd need to pass 2 more
> variables.
> 
> Otherwise, I went through this patch a few times and haven't spotted
> anything suspicious.
>