Re: [PATCH v2] btrfs: Turn fs_info member buffer_radix into XArray

[Nikolay Borisov <nborisov@suse.com>]

On 21.04.22 г. 18:45 ч., Gabriel Niebler wrote:
> … named 'extent_buffers'. Also adjust all usages of this object to use the
> XArray API, which greatly simplifies the code as it takes care of locking
> and is generally easier to use and understand, providing notionally
> simpler array semantics.
> 
> Also perform some light refactoring.
> 
> Signed-off-by: Gabriel Niebler <gniebler@suse.com>
> ---
> 
> Changes from v1:
>   - Fixed first line of commit message
> 

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

One minor suggestion below though.

<snip>

> @@ -6313,10 +6306,8 @@ static int release_extent_buffer(struct extent_buffer *eb)
>   
>   			spin_unlock(&eb->refs_lock);
>   
> -			spin_lock(&fs_info->buffer_lock);
> -			radix_tree_delete(&fs_info->buffer_radix,
> -					  eb->start >> fs_info->sectorsize_bits);
> -			spin_unlock(&fs_info->buffer_lock);
> +			xa_erase(&fs_info->extent_buffers,
> +				 eb->start >> fs_info->sectorsize_bits);
>   		} else {
>   			spin_unlock(&eb->refs_lock);
>   		}
> @@ -7249,41 +7240,28 @@ void memmove_extent_buffer(const struct extent_buffer *dst,
>   	}
>   }
>   
> -#define GANG_LOOKUP_SIZE	16
>   static struct extent_buffer *get_next_extent_buffer(
>   		struct btrfs_fs_info *fs_info, struct page *page, u64 bytenr)
>   {
> -	struct extent_buffer *gang[GANG_LOOKUP_SIZE];
> +	struct extent_buffer *eb;
>   	struct extent_buffer *found = NULL;
> +	unsigned long index;
>   	u64 page_start = page_offset(page);
> -	u64 cur = page_start;
>   
>   	ASSERT(in_range(bytenr, page_start, PAGE_SIZE));
>   	lockdep_assert_held(&fs_info->buffer_lock);
>   
> -	while (cur < page_start + PAGE_SIZE) {
> -		int ret;
> -		int i;
> -
> -		ret = radix_tree_gang_lookup(&fs_info->buffer_radix,
> -				(void **)gang, cur >> fs_info->sectorsize_bits,
> -				min_t(unsigned int, GANG_LOOKUP_SIZE,
> -				      PAGE_SIZE / fs_info->nodesize));
> -		if (ret == 0)
> -			goto out;
> -		for (i = 0; i < ret; i++) {
> -			/* Already beyond page end */
> -			if (gang[i]->start >= page_start + PAGE_SIZE)
> -				goto out;
> +	xa_for_each_start(&fs_info->extent_buffers, index, eb,
> +			  page_start >> fs_info->sectorsize_bits) {
> +		if (eb->start >= page_start + PAGE_SIZE)
> +		        /* Already beyond page end */
> +			break;
> +		if (eb->start >= bytenr) {
>   			/* Found one */
> -			if (gang[i]->start >= bytenr) {
> -				found = gang[i];
> -				goto out;
> -			}
> +			found = eb;
> +			break; >   		}
> -		cur = gang[ret - 1]->start + gang[ret - 1]->len;
>   	}

nit: The body of the loop can be turned into:

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index da3d9dc186cd..7c1d5fec59dd 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -7318,16 +7318,13 @@ static struct extent_buffer *get_next_extent_buffer(

         xa_for_each_start(&fs_info->extent_buffers, index, eb,
                           page_start >> fs_info->sectorsize_bits) {
-               if (eb->start >= page_start + PAGE_SIZE)
+               if (in_range(eb->start, page_start, PAGE_SIZE))
+                       return eb;
+               else if (eb->start >= page_start + PAGE_SIZE)
                         /* Already beyond page end */
-                       break;
-               if (eb->start >= bytenr) {
-                       /* Found one */
-                       found = eb;
-                       break;
-               }
+                       return NULL;
         }
-       return found;
+       return NULL;
  }


That is use the in_range macro to detect when we have an eb between 
page_start and page_start + PAGE_SIZE in which case we can directly 
return it, and the in_range is self-documenting. And directly return 
NULL in case of eb->start going beyond the current page and in case we 
didn't find anything.  David, what do you think?

> -out:
>   	return found;
>   }
>   

<snip>