Re: [PATCH v2 16/19] btrfs: use extent_io_tree to handle subpage extent buffer allocation

Re: [PATCH v2 16/19] btrfs: use extent_io_tree to handle subpage extent buffer allocation

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4937 bytes --]

CC: kbuild-all(a)lists.01.org
In-Reply-To: <20200915053532.63279-17-wqu@suse.com>
References: <20200915053532.63279-17-wqu@suse.com>
TO: Qu Wenruo <wqu@suse.com>
TO: linux-btrfs(a)vger.kernel.org

Hi Qu,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on v5.9-rc5]
[also build test WARNING on next-20200915]
[cannot apply to kdave/for-next btrfs/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Qu-Wenruo/btrfs-add-read-only-support-for-subpage-sector-size/20200915-133811
base:    856deb866d16e29bd65952e0289066f6078af773
:::::: branch date: 5 hours ago
:::::: commit date: 5 hours ago
config: x86_64-randconfig-s022-20200914 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.2-191-g10164920-dirty
        # save the attached .config to linux build tree
        make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=x86_64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

   fs/btrfs/extent_io.c:2327:9: sparse: sparse: incompatible types in comparison expression (different address spaces):
   fs/btrfs/extent_io.c:2327:9: sparse:    struct rcu_string [noderef] __rcu *
   fs/btrfs/extent_io.c:2327:9: sparse:    struct rcu_string *
>> fs/btrfs/extent_io.c:4960:13: sparse: sparse: context imbalance in 'detach_extent_buffer_subpage' - different lock contexts for basic block
   fs/btrfs/extent_io.c: note: in included file (through include/linux/mmzone.h, include/linux/gfp.h, include/linux/slab.h):
   include/linux/page-flags.h:182:30: sparse: sparse: context imbalance in 'btrfs_release_extent_buffer_pages' - different lock contexts for basic block

# https://github.com/0day-ci/linux/commit/e501596ccc2e9c62fe48af688b4f89376d799864
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Qu-Wenruo/btrfs-add-read-only-support-for-subpage-sector-size/20200915-133811
git checkout e501596ccc2e9c62fe48af688b4f89376d799864
vim +/detach_extent_buffer_subpage +4960 fs/btrfs/extent_io.c

db7f3436c1c186 Josef Bacik 2013-08-07  4959  
e501596ccc2e9c Qu Wenruo   2020-09-15 @4960  static void detach_extent_buffer_subpage(struct extent_buffer *eb)
e501596ccc2e9c Qu Wenruo   2020-09-15  4961  {
e501596ccc2e9c Qu Wenruo   2020-09-15  4962  	struct btrfs_fs_info *fs_info = eb->fs_info;
e501596ccc2e9c Qu Wenruo   2020-09-15  4963  	struct extent_io_tree *io_tree = info_to_btree_io_tree(fs_info);
e501596ccc2e9c Qu Wenruo   2020-09-15  4964  	struct page *page = eb->pages[0];
e501596ccc2e9c Qu Wenruo   2020-09-15  4965  	bool mapped = !test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
e501596ccc2e9c Qu Wenruo   2020-09-15  4966  	int ret;
e501596ccc2e9c Qu Wenruo   2020-09-15  4967  
e501596ccc2e9c Qu Wenruo   2020-09-15  4968  	if (!page)
e501596ccc2e9c Qu Wenruo   2020-09-15  4969  		return;
e501596ccc2e9c Qu Wenruo   2020-09-15  4970  
e501596ccc2e9c Qu Wenruo   2020-09-15  4971  	if (mapped)
e501596ccc2e9c Qu Wenruo   2020-09-15  4972  		spin_lock(&page->mapping->private_lock);
e501596ccc2e9c Qu Wenruo   2020-09-15  4973  
e501596ccc2e9c Qu Wenruo   2020-09-15  4974  	/*
e501596ccc2e9c Qu Wenruo   2020-09-15  4975  	 * Clear the EXTENT_NEW bit from io tree, to indicate that there is
e501596ccc2e9c Qu Wenruo   2020-09-15  4976  	 * no longer an extent buffer in the range.
e501596ccc2e9c Qu Wenruo   2020-09-15  4977  	 */
e501596ccc2e9c Qu Wenruo   2020-09-15  4978  	__clear_extent_bit(io_tree, eb->start, eb->start + eb->len - 1,
e501596ccc2e9c Qu Wenruo   2020-09-15  4979  			   EXTENT_NEW, 0, 0, NULL, GFP_ATOMIC, NULL);
e501596ccc2e9c Qu Wenruo   2020-09-15  4980  
e501596ccc2e9c Qu Wenruo   2020-09-15  4981  	/* Test if we still have other extent buffer in the page range */
e501596ccc2e9c Qu Wenruo   2020-09-15  4982  	ret = test_range_bit(io_tree, round_down(eb->start, PAGE_SIZE),
e501596ccc2e9c Qu Wenruo   2020-09-15  4983  			     round_down(eb->start, PAGE_SIZE) + PAGE_SIZE - 1,
e501596ccc2e9c Qu Wenruo   2020-09-15  4984  			     EXTENT_NEW, 0, NULL);
e501596ccc2e9c Qu Wenruo   2020-09-15  4985  	if (!ret)
e501596ccc2e9c Qu Wenruo   2020-09-15  4986  		detach_page_private(eb->pages[0]);
e501596ccc2e9c Qu Wenruo   2020-09-15  4987  	if (mapped)
e501596ccc2e9c Qu Wenruo   2020-09-15  4988  		spin_unlock(&page->mapping->private_lock);
e501596ccc2e9c Qu Wenruo   2020-09-15  4989  }
e501596ccc2e9c Qu Wenruo   2020-09-15  4990  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 36628 bytes --]

[PATCH v2 16/19] btrfs: use extent_io_tree to handle subpage extent buffer allocation

[Qu Wenruo]

Currently btrfs uses page::private as an indicator of who owns the
extent buffer, this method won't really work on subpage support, as one
page can contain several tree blocks (up to 16 for 4K node size and 64K
page size).

Instead, here we utilize btree extent io tree to handle them.
Now EXTENT_NEW means we have an extent buffer for the range.

This will affects the following functions:
- alloc_extent_buffer()
  Now for subpage we never use page->private to grab an existing eb.
  Instead, we rely on extra safenet in alloc_extent_buffer() to detect two
  callers on the same eb.

- btrfs_release_extent_buffer_pages()
  Now for subpage, we clear the EXTENT_NEW bit first, then check if the
  remaining range in the page has EXTENT_NEW bit.
  If not, then clear the private bit for the page.

- attach_extent_buffer_page()
  Now we set EXTENT_NEW bit for the new extent buffer to be attached,
  and set the page private, with NULL as page::private.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/btrfs_inode.h | 12 +++++++
 fs/btrfs/disk-io.c     |  7 ++++
 fs/btrfs/extent_io.c   | 79 ++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 96 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/btrfs_inode.h b/fs/btrfs/btrfs_inode.h
index c47b6c6fea9f..cff818e0c406 100644
--- a/fs/btrfs/btrfs_inode.h
+++ b/fs/btrfs/btrfs_inode.h
@@ -217,6 +217,18 @@ static inline struct btrfs_inode *BTRFS_I(const struct inode *inode)
 	return container_of(inode, struct btrfs_inode, vfs_inode);
 }
 
+static inline struct btrfs_fs_info *page_to_fs_info(struct page *page)
+{
+	ASSERT(page->mapping);
+	return BTRFS_I(page->mapping->host)->root->fs_info;
+}
+
+static inline struct extent_io_tree
+*info_to_btree_io_tree(struct btrfs_fs_info *fs_info)
+{
+	return &BTRFS_I(fs_info->btree_inode)->io_tree;
+}
+
 static inline unsigned long btrfs_inode_hash(u64 objectid,
 					     const struct btrfs_root *root)
 {
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index b526adf20f3e..2ef35eb7a6e1 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2120,6 +2120,13 @@ static void btrfs_init_btree_inode(struct btrfs_fs_info *fs_info)
 	inode->i_mapping->a_ops = &btree_aops;
 
 	RB_CLEAR_NODE(&BTRFS_I(inode)->rb_node);
+	/*
+	 * This extent io tree is subpage metadata specific.
+	 *
+	 * It uses the following bits to represent new meaning:
+	 * - EXTENT_NEW:	Has extent buffer allocated
+	 * - EXTENT_UPTODATE	Has latest metadata read from disk
+	 */
 	extent_io_tree_init(fs_info, &BTRFS_I(inode)->io_tree,
 			    IO_TREE_BTREE_IO, inode);
 	BTRFS_I(inode)->io_tree.track_uptodate = false;
diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index 16fe9f4313a1..2af6786e6ab4 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -3116,6 +3116,20 @@ static void attach_extent_buffer_page(struct extent_buffer *eb,
 	if (page->mapping)
 		assert_spin_locked(&page->mapping->private_lock);
 
+	if (eb->fs_info->sectorsize < PAGE_SIZE && page->mapping) {
+		struct extent_io_tree *io_tree =
+			info_to_btree_io_tree(eb->fs_info);
+
+		if (!PagePrivate(page))
+			attach_page_private(page, NULL);
+
+		/* EXTENT_NEW represents we have an extent buffer */
+		set_extent_bit(io_tree, eb->start, eb->start + eb->len - 1,
+				EXTENT_NEW, NULL, NULL, GFP_ATOMIC);
+		eb->pages[0] = page;
+		return;
+	}
+
 	if (!PagePrivate(page))
 		attach_page_private(page, eb);
 	else
@@ -4943,6 +4957,37 @@ int extent_buffer_under_io(const struct extent_buffer *eb)
 		test_bit(EXTENT_BUFFER_DIRTY, &eb->bflags));
 }
 
+static void detach_extent_buffer_subpage(struct extent_buffer *eb)
+{
+	struct btrfs_fs_info *fs_info = eb->fs_info;
+	struct extent_io_tree *io_tree = info_to_btree_io_tree(fs_info);
+	struct page *page = eb->pages[0];
+	bool mapped = !test_bit(EXTENT_BUFFER_UNMAPPED, &eb->bflags);
+	int ret;
+
+	if (!page)
+		return;
+
+	if (mapped)
+		spin_lock(&page->mapping->private_lock);
+
+	/*
+	 * Clear the EXTENT_NEW bit from io tree, to indicate that there is
+	 * no longer an extent buffer in the range.
+	 */
+	__clear_extent_bit(io_tree, eb->start, eb->start + eb->len - 1,
+			   EXTENT_NEW, 0, 0, NULL, GFP_ATOMIC, NULL);
+
+	/* Test if we still have other extent buffer in the page range */
+	ret = test_range_bit(io_tree, round_down(eb->start, PAGE_SIZE),
+			     round_down(eb->start, PAGE_SIZE) + PAGE_SIZE - 1,
+			     EXTENT_NEW, 0, NULL);
+	if (!ret)
+		detach_page_private(eb->pages[0]);
+	if (mapped)
+		spin_unlock(&page->mapping->private_lock);
+}
+
 /*
  * Release all pages attached to the extent buffer.
  */
@@ -4954,6 +4999,9 @@ static void btrfs_release_extent_buffer_pages(struct extent_buffer *eb)
 
 	BUG_ON(extent_buffer_under_io(eb));
 
+	if (eb->fs_info->sectorsize < PAGE_SIZE)
+		return detach_extent_buffer_subpage(eb);
+
 	num_pages = num_extent_pages(eb);
 	for (i = 0; i < num_pages; i++) {
 		struct page *page = eb->pages[i];
@@ -5248,6 +5296,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 	struct extent_buffer *exists = NULL;
 	struct page *p;
 	struct address_space *mapping = fs_info->btree_inode->i_mapping;
+	bool subpage = (fs_info->sectorsize < PAGE_SIZE);
 	int uptodate = 1;
 	int ret;
 
@@ -5280,7 +5329,7 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 		}
 
 		spin_lock(&mapping->private_lock);
-		if (PagePrivate(p)) {
+		if (PagePrivate(p) && !subpage) {
 			/*
 			 * We could have already allocated an eb for this page
 			 * and attached one so lets see if we can get a ref on
@@ -5321,8 +5370,21 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 		 * we could crash.
 		 */
 	}
-	if (uptodate)
+	if (uptodate && !subpage)
 		set_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);
+	/*
+	 * For subpage, we must check extent_io_tree to get if the eb is really
+	 * UPTODATE, as the page bit is no longer reliable as we can do subpage
+	 * read.
+	 */
+	if (subpage) {
+		struct extent_io_tree *io_tree = info_to_btree_io_tree(fs_info);
+
+		ret = test_range_bit(io_tree, eb->start, eb->start + eb->len - 1,
+				     EXTENT_UPTODATE, 1, NULL);
+		if (ret)
+			set_bit(EXTENT_BUFFER_UPTODATE, &eb->bflags);
+	}
 again:
 	ret = radix_tree_preload(GFP_NOFS);
 	if (ret) {
@@ -5361,6 +5423,19 @@ struct extent_buffer *alloc_extent_buffer(struct btrfs_fs_info *fs_info,
 		if (eb->pages[i])
 			unlock_page(eb->pages[i]);
 	}
+	/*
+	 * For subpage case, btrfs_release_extent_buffer() will clear the
+	 * EXTENT_NEW bit if there is a page, and EXTENT_NEW bit represents
+	 * we have an extent buffer in that range.
+	 *
+	 * Since we're here because we hit a race with another caller, who
+	 * succeeded in inserting the eb, we shouldn't clear that EXTENT_NEW
+	 * bit. So here we cleanup the page manually.
+	 */
+	if (subpage) {
+		put_page(eb->pages[0]);
+		eb->pages[i] = NULL;
+	}
 
 	btrfs_release_extent_buffer(eb);
 	return exists;
-- 
2.28.0