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

[Omar Sandoval <osandov@osandov.com>]

On Tue, Sep 01, 2015 at 03:25:54PM -0400, Josef Bacik wrote:
> 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?
> 

No problem, I'll do that.

> >---
> >  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

Will do, thanks.

-- 
Omar