[PATCH 0/2] btrfs: fixes for subpage which also affect read-only mount

[PATCH 0/2] btrfs: fixes for subpage which also affect read-only mount

[Qu Wenruo]

During the fstests run for btrfs subpage read-write support, generic/475
crashes the system with a very high chance.

It turns out the cause is also affecting btrfs subpage read-only mount
so it's worthy a quick fix.

Also the crash call site shows a new rabbit hole of hard coded
PAGE_SHIFT in readahead.
This reada problem does not only greatly slow down my test on my ARM
board, but also affects read-only mount.

So this patchset is here to address the problems, and hope these fixes
can fit into current merge window.

Qu Wenruo (2):
  btrfs: fix wild pointer access during metadata read failure for
    subpage
  btrfs: make reada to be subpage compatible

 fs/btrfs/extent_io.c | 31 ++++++++++++++++++++++++++++++-
 fs/btrfs/reada.c     | 35 ++++++++++++++++++-----------------
 2 files changed, 48 insertions(+), 18 deletions(-)

-- 
2.30.1

[PATCH 1/2] btrfs: fix wild pointer access during metadata read failure for subpage

[Qu Wenruo]

[BUG]
When running fstests for btrfs subpage read-write test, it has a very
high chance to crash at generic/475 with the following crash:

 BTRFS warning (device dm-8): direct IO failed ino 510 rw 1,34817 sector 0xcdf0 len 94208 err no 10
 Unable to handle kernel paging request at virtual address ffff80001157e7c0
 CPU: 2 PID: 687125 Comm: kworker/u12:4 Tainted: G        WC        5.12.0-rc2-custom+ #5
 Hardware name: Khadas VIM3 (DT)
 Workqueue: btrfs-endio-meta btrfs_work_helper [btrfs]
 pc : queued_spin_lock_slowpath+0x1a0/0x390
 lr : do_raw_spin_lock+0xc4/0x11c
 Call trace:
  queued_spin_lock_slowpath+0x1a0/0x390
  _raw_spin_lock+0x68/0x84
  btree_readahead_hook+0x38/0xc0 [btrfs]
  end_bio_extent_readpage+0x504/0x5f4 [btrfs]
  bio_endio+0x170/0x1a4
  end_workqueue_fn+0x3c/0x60 [btrfs]
  btrfs_work_helper+0x1b0/0x1b4 [btrfs]
  process_one_work+0x22c/0x430
  worker_thread+0x70/0x3a0
  kthread+0x13c/0x140
  ret_from_fork+0x10/0x30
 Code: 910020e0 8b0200c2 f861d884 aa0203e1 (f8246827)

[CAUSE]
In end_bio_extent_readpage(), if we hit an error during read, we will
handle the error differently for data and metadata.
For data we queue a repair, while for metadata, we record the error and
let the caller to choose what to do.

But the code is still using page->private to grab extent buffer, which
no longer points to extent buffer for subpage metadata pages.

Thus this wild pointer access leads to above crash.

[FIX]
Introduce a helper, find_extent_buffer_readpage(), to grab extent
buffer.

The difference against find_extent_buffer_nospinlock() is:
- Also handles regular sectorsize == PAGE_SIZE case
- No extent buffer refs increase/decrease
  As extent buffer under IO must has non-zero refs.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_io.c | 31 ++++++++++++++++++++++++++++++-
 1 file changed, 30 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index e38041ba9011..a008dc6d0216 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -2918,6 +2918,35 @@ static void end_page_read(struct page *page, bool uptodate, u64 start, u32 len)
 		btrfs_subpage_end_reader(fs_info, page, start, len);
 }
 
+/*
+ * Helper to find the extent buffer.
+ *
+ * This helper is for end_bio_extent_readpage(), thus we can't do any
+ * unsafe spinlock in endio context.
+ */
+static struct extent_buffer *find_extent_buffer_readpage(
+		struct btrfs_fs_info *fs_info, struct page *page, u64 bytenr)
+{
+	struct extent_buffer *ret;
+
+	/*
+	 * For regular sectorsize, we can use page->private to grab extent
+	 * buffer.
+	 */
+	if (fs_info->sectorsize == PAGE_SIZE) {
+		ASSERT(PagePrivate(page) && page->private);
+		return (struct extent_buffer *)page->private;
+	}
+
+	/* For subpage case, we need to lookup buffer radix tree. */
+	rcu_read_lock();
+	ret = radix_tree_lookup(&fs_info->buffer_radix,
+			       bytenr >> fs_info->sectorsize_bits);
+	rcu_read_unlock();
+	ASSERT(ret);
+	return ret;
+}
+
 /*
  * after a readpage IO is done, we need to:
  * clear the uptodate bits on error
@@ -3028,7 +3057,7 @@ static void end_bio_extent_readpage(struct bio *bio)
 		} else {
 			struct extent_buffer *eb;
 
-			eb = (struct extent_buffer *)page->private;
+			eb = find_extent_buffer_readpage(fs_info, page, start);
 			set_bit(EXTENT_BUFFER_READ_ERR, &eb->bflags);
 			eb->read_mirror = mirror;
 			atomic_dec(&eb->io_pages);
-- 
2.30.1

Re: [PATCH 1/2] btrfs: fix wild pointer access during metadata read failure for subpage

[Johannes Thumshirn]

On 15/03/2021 06:40, Qu Wenruo wrote:
> The difference against find_extent_buffer_nospinlock() is:
> - Also handles regular sectorsize == PAGE_SIZE case
> - No extent buffer refs increase/decrease
>   As extent buffer under IO must has non-zero refs.

Can these be merged into a single function? The sectorsie == PAGE_SIZE case
won't do anything for find_extent_buffer_nospinlock() and the 
atomic_inc_not_zero(&eb->refs) can be hidden behind a 'if (write)' check.

Note, I was looking at this version:
https://www.spinics.net/lists/linux-btrfs/msg111188.html

Re: [PATCH 1/2] btrfs: fix wild pointer access during metadata read failure for subpage

[Qu Wenruo]

On 2021/3/15 下午3:55, Johannes Thumshirn wrote:
> On 15/03/2021 06:40, Qu Wenruo wrote:
>> The difference against find_extent_buffer_nospinlock() is:
>> - Also handles regular sectorsize == PAGE_SIZE case
>> - No extent buffer refs increase/decrease
>>    As extent buffer under IO must has non-zero refs.
>
> Can these be merged into a single function? The sectorsie == PAGE_SIZE case
> won't do anything for find_extent_buffer_nospinlock() and the
> atomic_inc_not_zero(&eb->refs) can be hidden behind a 'if (write)' check.

That would make the eb refs change too inconsistent.

But I get your point.

How about calling find_extent_buffer_nospinlock() and then dec the refs
manually?

Thanks,
Qu

>
> Note, I was looking at this version:
> https://www.spinics.net/lists/linux-btrfs/msg111188.html
>

Re: [PATCH 1/2] btrfs: fix wild pointer access during metadata read failure for subpage

[David Sterba]

On Mon, Mar 15, 2021 at 04:25:32PM +0800, Qu Wenruo wrote:
> 
> 
> On 2021/3/15 下午3:55, Johannes Thumshirn wrote:
> > On 15/03/2021 06:40, Qu Wenruo wrote:
> >> The difference against find_extent_buffer_nospinlock() is:
> >> - Also handles regular sectorsize == PAGE_SIZE case
> >> - No extent buffer refs increase/decrease
> >>    As extent buffer under IO must has non-zero refs.
> >
> > Can these be merged into a single function? The sectorsie == PAGE_SIZE case
> > won't do anything for find_extent_buffer_nospinlock() and the
> > atomic_inc_not_zero(&eb->refs) can be hidden behind a 'if (write)' check.
> 
> That would make the eb refs change too inconsistent.
> 
> But I get your point.
> 
> How about calling find_extent_buffer_nospinlock() and then dec the refs
> manually?

Is this equivalent to this patch? Ie. the atomic_inc_not_zero in
find_extent_buffer_nospinlock happens inside the RCU section, while what
you suggest looks like

find_extent_buffer_nospinlock()
  rcu_lock
  radix_lookup
  rcu_unlock
atomic_inc_not_zero()

Re: [PATCH 1/2] btrfs: fix wild pointer access during metadata read failure for subpage

[Qu Wenruo]

On 2021/3/16 上午2:51, David Sterba wrote:
> On Mon, Mar 15, 2021 at 04:25:32PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2021/3/15 下午3:55, Johannes Thumshirn wrote:
>>> On 15/03/2021 06:40, Qu Wenruo wrote:
>>>> The difference against find_extent_buffer_nospinlock() is:
>>>> - Also handles regular sectorsize == PAGE_SIZE case
>>>> - No extent buffer refs increase/decrease
>>>>     As extent buffer under IO must has non-zero refs.
>>>
>>> Can these be merged into a single function? The sectorsie == PAGE_SIZE case
>>> won't do anything for find_extent_buffer_nospinlock() and the
>>> atomic_inc_not_zero(&eb->refs) can be hidden behind a 'if (write)' check.
>>
>> That would make the eb refs change too inconsistent.
>>
>> But I get your point.
>>
>> How about calling find_extent_buffer_nospinlock() and then dec the refs
>> manually?
>
> Is this equivalent to this patch? Ie. the atomic_inc_not_zero in
> find_extent_buffer_nospinlock happens inside the RCU section, while what
> you suggest looks like
>
> find_extent_buffer_nospinlock()
>    rcu_lock
>    radix_lookup
>    rcu_unlock
> atomic_inc_not_zero()
>
No, I mean just call find_extent_buffer_nospinlock() directly.

Then manually call atomic_dec() on the returned eb.

Thanks,
Qu

[PATCH 2/2] btrfs: make reada to be subpage compatible

[Qu Wenruo]

In readahead infrastructure, we are using a lot of hard coded PAGE_SHIFT
while we're not doing anything specific to PAGE_SIZE.

One of the most affected part is the radix tree operation of
btrfs_fs_info::reada_tree.

If using PAGE_SHIFT, subpage metadata readahead is almost broken and do
no help in reading ahead metadata.

Fix the problem by using btrfs_fs_info::sectorsize_bits so that
readahead could work for subpage.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/reada.c | 35 ++++++++++++++++++-----------------
 1 file changed, 18 insertions(+), 17 deletions(-)

diff --git a/fs/btrfs/reada.c b/fs/btrfs/reada.c
index 20fd4aa48a8c..06713a8fe26b 100644
--- a/fs/btrfs/reada.c
+++ b/fs/btrfs/reada.c
@@ -209,7 +209,7 @@ int btree_readahead_hook(struct extent_buffer *eb, int err)
 	/* find extent */
 	spin_lock(&fs_info->reada_lock);
 	re = radix_tree_lookup(&fs_info->reada_tree,
-			       eb->start >> PAGE_SHIFT);
+			       eb->start >> fs_info->sectorsize_bits);
 	if (re)
 		re->refcnt++;
 	spin_unlock(&fs_info->reada_lock);
@@ -240,7 +240,7 @@ static struct reada_zone *reada_find_zone(struct btrfs_device *dev, u64 logical,
 	zone = NULL;
 	spin_lock(&fs_info->reada_lock);
 	ret = radix_tree_gang_lookup(&dev->reada_zones, (void **)&zone,
-				     logical >> PAGE_SHIFT, 1);
+				     logical >> fs_info->sectorsize_bits, 1);
 	if (ret == 1 && logical >= zone->start && logical <= zone->end) {
 		kref_get(&zone->refcnt);
 		spin_unlock(&fs_info->reada_lock);
@@ -283,13 +283,13 @@ static struct reada_zone *reada_find_zone(struct btrfs_device *dev, u64 logical,
 
 	spin_lock(&fs_info->reada_lock);
 	ret = radix_tree_insert(&dev->reada_zones,
-				(unsigned long)(zone->end >> PAGE_SHIFT),
-				zone);
+			(unsigned long)(zone->end >> fs_info->sectorsize_bits),
+			zone);
 
 	if (ret == -EEXIST) {
 		kfree(zone);
 		ret = radix_tree_gang_lookup(&dev->reada_zones, (void **)&zone,
-					     logical >> PAGE_SHIFT, 1);
+					logical >> fs_info->sectorsize_bits, 1);
 		if (ret == 1 && logical >= zone->start && logical <= zone->end)
 			kref_get(&zone->refcnt);
 		else
@@ -315,7 +315,7 @@ static struct reada_extent *reada_find_extent(struct btrfs_fs_info *fs_info,
 	u64 length;
 	int real_stripes;
 	int nzones = 0;
-	unsigned long index = logical >> PAGE_SHIFT;
+	unsigned long index = logical >> fs_info->sectorsize_bits;
 	int dev_replace_is_ongoing;
 	int have_zone = 0;
 
@@ -497,7 +497,7 @@ static void reada_extent_put(struct btrfs_fs_info *fs_info,
 			     struct reada_extent *re)
 {
 	int i;
-	unsigned long index = re->logical >> PAGE_SHIFT;
+	unsigned long index = re->logical >> fs_info->sectorsize_bits;
 
 	spin_lock(&fs_info->reada_lock);
 	if (--re->refcnt) {
@@ -538,11 +538,12 @@ static void reada_extent_put(struct btrfs_fs_info *fs_info,
 static void reada_zone_release(struct kref *kref)
 {
 	struct reada_zone *zone = container_of(kref, struct reada_zone, refcnt);
+	struct btrfs_fs_info *fs_info = zone->device->fs_info;
 
-	lockdep_assert_held(&zone->device->fs_info->reada_lock);
+	lockdep_assert_held(&fs_info->reada_lock);
 
 	radix_tree_delete(&zone->device->reada_zones,
-			  zone->end >> PAGE_SHIFT);
+			  zone->end >> fs_info->sectorsize_bits);
 
 	kfree(zone);
 }
@@ -593,7 +594,7 @@ static int reada_add_block(struct reada_control *rc, u64 logical,
 static void reada_peer_zones_set_lock(struct reada_zone *zone, int lock)
 {
 	int i;
-	unsigned long index = zone->end >> PAGE_SHIFT;
+	unsigned long index = zone->end >> zone->device->fs_info->sectorsize_bits;
 
 	for (i = 0; i < zone->ndevs; ++i) {
 		struct reada_zone *peer;
@@ -628,7 +629,7 @@ static int reada_pick_zone(struct btrfs_device *dev)
 					     (void **)&zone, index, 1);
 		if (ret == 0)
 			break;
-		index = (zone->end >> PAGE_SHIFT) + 1;
+		index = (zone->end >> dev->fs_info->sectorsize_bits) + 1;
 		if (zone->locked) {
 			if (zone->elems > top_locked_elems) {
 				top_locked_elems = zone->elems;
@@ -709,7 +710,7 @@ static int reada_start_machine_dev(struct btrfs_device *dev)
 	 * plugging to speed things up
 	 */
 	ret = radix_tree_gang_lookup(&dev->reada_extents, (void **)&re,
-				     dev->reada_next >> PAGE_SHIFT, 1);
+				dev->reada_next >> fs_info->sectorsize_bits, 1);
 	if (ret == 0 || re->logical > dev->reada_curr_zone->end) {
 		ret = reada_pick_zone(dev);
 		if (!ret) {
@@ -718,7 +719,7 @@ static int reada_start_machine_dev(struct btrfs_device *dev)
 		}
 		re = NULL;
 		ret = radix_tree_gang_lookup(&dev->reada_extents, (void **)&re,
-					dev->reada_next >> PAGE_SHIFT, 1);
+				dev->reada_next >> fs_info->sectorsize_bits, 1);
 	}
 	if (ret == 0) {
 		spin_unlock(&fs_info->reada_lock);
@@ -885,7 +886,7 @@ static void dump_devs(struct btrfs_fs_info *fs_info, int all)
 				pr_cont(" curr off %llu",
 					device->reada_next - zone->start);
 			pr_cont("\n");
-			index = (zone->end >> PAGE_SHIFT) + 1;
+			index = (zone->end >> fs_info->sectorsize_bits) + 1;
 		}
 		cnt = 0;
 		index = 0;
@@ -910,7 +911,7 @@ static void dump_devs(struct btrfs_fs_info *fs_info, int all)
 				}
 			}
 			pr_cont("\n");
-			index = (re->logical >> PAGE_SHIFT) + 1;
+			index = (re->logical >> fs_info->sectorsize_bits) + 1;
 			if (++cnt > 15)
 				break;
 		}
@@ -926,7 +927,7 @@ static void dump_devs(struct btrfs_fs_info *fs_info, int all)
 		if (ret == 0)
 			break;
 		if (!re->scheduled) {
-			index = (re->logical >> PAGE_SHIFT) + 1;
+			index = (re->logical >> fs_info->sectorsize_bits) + 1;
 			continue;
 		}
 		pr_debug("re: logical %llu size %u list empty %d scheduled %d",
@@ -942,7 +943,7 @@ static void dump_devs(struct btrfs_fs_info *fs_info, int all)
 			}
 		}
 		pr_cont("\n");
-		index = (re->logical >> PAGE_SHIFT) + 1;
+		index = (re->logical >> fs_info->sectorsize_bits) + 1;
 	}
 	spin_unlock(&fs_info->reada_lock);
 }
-- 
2.30.1

Re: [PATCH 2/2] btrfs: make reada to be subpage compatible

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Re: [PATCH 0/2] btrfs: fixes for subpage which also affect read-only mount

[David Sterba]

On Mon, Mar 15, 2021 at 01:39:13PM +0800, Qu Wenruo wrote:
> During the fstests run for btrfs subpage read-write support, generic/475
> crashes the system with a very high chance.
> 
> It turns out the cause is also affecting btrfs subpage read-only mount
> so it's worthy a quick fix.
> 
> Also the crash call site shows a new rabbit hole of hard coded
> PAGE_SHIFT in readahead.

There's still a lot of PAGE_SHIFT use, not all of them are wrong but I
think we'll need to do an audit and categorize the valid uses, otherwise
it'll be a whack-a-mole.

Re: [PATCH 0/2] btrfs: fixes for subpage which also affect read-only mount

[Qu Wenruo]

On 2021/3/15 下午11:42, David Sterba wrote:
> On Mon, Mar 15, 2021 at 01:39:13PM +0800, Qu Wenruo wrote:
>> During the fstests run for btrfs subpage read-write support, generic/475
>> crashes the system with a very high chance.
>>
>> It turns out the cause is also affecting btrfs subpage read-only mount
>> so it's worthy a quick fix.
>>
>> Also the crash call site shows a new rabbit hole of hard coded
>> PAGE_SHIFT in readahead.
>
> There's still a lot of PAGE_SHIFT use, not all of them are wrong but I
> think we'll need to do an audit and categorize the valid uses, otherwise
> it'll be a whack-a-mole.
>

Already did that.

The current valid use case for PAGE_SHIFT are:
- Grab page
   Including:
   * compression
   * raid56
   * relocation
   * buffered write in file.c
   * sb cross page check in volumes.c
   * send
   * zoned
   * sb rw in disk-io.c
   * tree csum in disk-io.c
   * free space cache v1
- Some legacy code still runs in full page mode
   Including:
   * defrag

- Verification code
   That part has way more hardcoded part to be addressed.
   Will be addressed in the final part, along with selftest enhancement.

Although there can be something missing, I believe it shouldn't be that
hard to hit during fstests then.

Thanks,
Qu

Re: [PATCH 0/2] btrfs: fixes for subpage which also affect read-only mount

[David Sterba]

On Tue, Mar 16, 2021 at 08:29:35AM +0800, Qu Wenruo wrote:
> 
> 
> On 2021/3/15 下午11:42, David Sterba wrote:
> > On Mon, Mar 15, 2021 at 01:39:13PM +0800, Qu Wenruo wrote:
> >> During the fstests run for btrfs subpage read-write support, generic/475
> >> crashes the system with a very high chance.
> >>
> >> It turns out the cause is also affecting btrfs subpage read-only mount
> >> so it's worthy a quick fix.
> >>
> >> Also the crash call site shows a new rabbit hole of hard coded
> >> PAGE_SHIFT in readahead.
> >
> > There's still a lot of PAGE_SHIFT use, not all of them are wrong but I
> > think we'll need to do an audit and categorize the valid uses, otherwise
> > it'll be a whack-a-mole.
> >
> 
> Already did that.
> 
> The current valid use case for PAGE_SHIFT are:
> - Grab page
>    Including:
>    * compression
>    * raid56
>    * relocation
>    * buffered write in file.c
>    * sb cross page check in volumes.c
>    * send
>    * zoned
>    * sb rw in disk-io.c
>    * tree csum in disk-io.c
>    * free space cache v1
> - Some legacy code still runs in full page mode
>    Including:
>    * defrag
> 
> - Verification code
>    That part has way more hardcoded part to be addressed.
>    Will be addressed in the final part, along with selftest enhancement.
> 
> Although there can be something missing, I believe it shouldn't be that
> hard to hit during fstests then.

Great, please put that to subpage.c as first comment, and anything that
documents the high level topics regarding subpage. Lots of that is in
changelogs but we want that readily available in the sources too.
Thanks.

Re: [PATCH 0/2] btrfs: fixes for subpage which also affect read-only mount

[David Sterba]

On Mon, Mar 15, 2021 at 01:39:13PM +0800, Qu Wenruo wrote:
> During the fstests run for btrfs subpage read-write support, generic/475
> crashes the system with a very high chance.
> 
> It turns out the cause is also affecting btrfs subpage read-only mount
> so it's worthy a quick fix.
> 
> Also the crash call site shows a new rabbit hole of hard coded
> PAGE_SHIFT in readahead.
> This reada problem does not only greatly slow down my test on my ARM
> board, but also affects read-only mount.
> 
> So this patchset is here to address the problems, and hope these fixes
> can fit into current merge window.

Now added to the -rc queue, thanks.