Re: [PATCH 1/6] Btrfs: add extent buffer bitmap operations

[Josef Bacik <jbacik@fb.com>]

On 09/01/2015 03:01 PM, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
>
> These are going to be used for the free space tree bitmap items.
>
> Signed-off-by: Omar Sandoval <osandov@fb.com>

Can we get sanity tests for these operations so we know they are 
properly unit tested?

> ---
>   fs/btrfs/extent_io.c | 101 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   fs/btrfs/extent_io.h |   6 +++
>   2 files changed, 107 insertions(+)
>
> diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
> index 02d05817cbdf..649e3b4eeb1b 100644
> --- a/fs/btrfs/extent_io.c
> +++ b/fs/btrfs/extent_io.c
> @@ -5475,6 +5475,107 @@ void copy_extent_buffer(struct extent_buffer *dst, struct extent_buffer *src,
>   	}
>   }
>
> +/*
> + * The extent buffer bitmap operations are done with byte granularity because
> + * bitmap items are not guaranteed to be aligned to a word and therefore a
> + * single word in a bitmap may straddle two pages in the extent buffer.
> + */
> +#define BIT_BYTE(nr) ((nr) / BITS_PER_BYTE)
> +#define BYTE_MASK ((1 << BITS_PER_BYTE) - 1)
> +#define BITMAP_FIRST_BYTE_MASK(start) \
> +	((BYTE_MASK << ((start) & (BITS_PER_BYTE - 1))) & BYTE_MASK)
> +#define BITMAP_LAST_BYTE_MASK(nbits) \
> +	(BYTE_MASK >> (-(nbits) & (BITS_PER_BYTE - 1)))
> +
> +int extent_buffer_test_bit(struct extent_buffer *eb, unsigned long start,
> +			   unsigned long nr)
> +{
> +	size_t offset;
> +	char *kaddr;
> +	struct page *page;
> +	size_t byte_offset = BIT_BYTE(nr);
> +	size_t start_offset = eb->start & ((u64)PAGE_CACHE_SIZE - 1);
> +	unsigned long i = (start_offset + start + byte_offset) >> PAGE_CACHE_SHIFT;
> +
> +	offset = (start_offset + start + byte_offset) & (PAGE_CACHE_SIZE - 1);
> +	page = eb->pages[i];
> +	WARN_ON(!PageUptodate(page));
> +	kaddr = page_address(page);
> +	return 1U & (kaddr[offset] >> (nr & (BITS_PER_BYTE - 1)));
> +}
> +
> +void extent_buffer_bitmap_set(struct extent_buffer *eb, unsigned long start,
> +			      unsigned long pos, unsigned long len)
> +{
> +	size_t offset;
> +	char *kaddr;
> +	struct page *page;
> +	size_t byte_offset = BIT_BYTE(pos);
> +	size_t start_offset = eb->start & ((u64)PAGE_CACHE_SIZE - 1);
> +	unsigned long i = (start_offset + start + byte_offset) >> PAGE_CACHE_SHIFT;
> +	const unsigned int size = pos + len;
> +	int bits_to_set = BITS_PER_BYTE - (pos % BITS_PER_BYTE);
> +	unsigned int mask_to_set = BITMAP_FIRST_BYTE_MASK(pos);
> +
> +	offset = (start_offset + start + byte_offset) & (PAGE_CACHE_SIZE - 1);
> +	page = eb->pages[i];
> +	WARN_ON(!PageUptodate(page));
> +	kaddr = page_address(page);
> +
> +	while (len >= bits_to_set) {
> +		kaddr[offset] |= mask_to_set;
> +		len -= bits_to_set;
> +		bits_to_set = BITS_PER_BYTE;
> +		mask_to_set = ~0U;
> +		if (++offset >= PAGE_CACHE_SIZE && len > 0) {
> +			offset = 0;
> +			page = eb->pages[++i];
> +			WARN_ON(!PageUptodate(page));
> +			kaddr = page_address(page);
> +		}
> +	}
> +	if (len) {
> +		mask_to_set &= BITMAP_LAST_BYTE_MASK(size);
> +		kaddr[offset] |= mask_to_set;
> +	}
> +}
> +
> +void extent_buffer_bitmap_clear(struct extent_buffer *eb, unsigned long start,
> +				unsigned long pos, unsigned long len)
> +{
> +	size_t offset;
> +	char *kaddr;
> +	struct page *page;
> +	size_t byte_offset = BIT_BYTE(pos);
> +	size_t start_offset = eb->start & ((u64)PAGE_CACHE_SIZE - 1);
> +	unsigned long i = (start_offset + start + byte_offset) >> PAGE_CACHE_SHIFT;
> +	const unsigned int size = pos + len;
> +	int bits_to_clear = BITS_PER_BYTE - (pos % BITS_PER_BYTE);
> +	unsigned int mask_to_clear = BITMAP_FIRST_BYTE_MASK(pos);
> +
> +	offset = (start_offset + start + byte_offset) & (PAGE_CACHE_SIZE - 1);
> +	page = eb->pages[i];
> +	WARN_ON(!PageUptodate(page));
> +	kaddr = page_address(page);
> +

Abstract this offset finding logic to a helper function and then comment 
the hell out of it, I now have a migraine trying to figure out what is 
going on.  Thanks,

Josef