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

[David Sterba <dsterba@suse.cz>]

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.

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.