[PATCH v2 0/2] btrfs: zoned: two fixes for generic/475

[PATCH v2 0/2] btrfs: zoned: two fixes for generic/475

[Johannes Thumshirn]

Although generic/475 uses dm-error which is not compatible with a zoned btrfs,
there is nothing that prevents a zoned btrfs from running on dm-error.

Changes to v1:
- be more verbose in the error messages

Johannes Thumshirn (1):
  btrfs: zoned: bail out if we can't read a reliable write pointer

Naohiro Aota (1):
  btrfs: zoned: sanity check zone type

 fs/btrfs/zoned.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

-- 
2.31.1

[PATCH v2 1/2] btrfs: zoned: sanity check zone type

[Johannes Thumshirn]

From: Naohiro Aota <naohiro.aota@wdc.com>

The xfstests test case generic/475 creates a dm-linear device that gets
changed to a dm-error device. This leads to errors in loading the block
group's zone information when running on a zoned file system, ultimately
resulting in a list corruption. When running on a kernel with list
debugging enabled this leads to the following crash.

 BTRFS: error (device dm-2) in cleanup_transaction:1953: errno=-5 IO failure
 kernel BUG at lib/list_debug.c:54!
 invalid opcode: 0000 [#1] SMP PTI
 CPU: 1 PID: 2433 Comm: umount Tainted: G        W         5.12.0+ #1018
 RIP: 0010:__list_del_entry_valid.cold+0x1d/0x47
 RSP: 0018:ffffc90001473df0 EFLAGS: 00010296
 RAX: 0000000000000054 RBX: ffff8881038fd000 RCX: ffffc90001473c90
 RDX: 0000000100001a31 RSI: 0000000000000003 RDI: 0000000000000003
 RBP: ffff888308871108 R08: 0000000000000003 R09: 0000000000000001
 R10: 3961373532383838 R11: 6666666620736177 R12: ffff888308871000
 R13: ffff8881038fd088 R14: ffff8881038fdc78 R15: dead000000000100
 FS:  00007f353c9b1540(0000) GS:ffff888627d00000(0000) knlGS:0000000000000000
 CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
 CR2: 00007f353cc2c710 CR3: 000000018e13c000 CR4: 00000000000006a0
 DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
 DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
 Call Trace:
  btrfs_free_block_groups+0xc9/0x310 [btrfs]
  close_ctree+0x2ee/0x31a [btrfs]
  ? call_rcu+0x8f/0x270
  ? mutex_lock+0x1c/0x40
  generic_shutdown_super+0x67/0x100
  kill_anon_super+0x14/0x30
  btrfs_kill_super+0x12/0x20 [btrfs]
  deactivate_locked_super+0x31/0x90
  cleanup_mnt+0x13e/0x1b0
  task_work_run+0x63/0xb0
  exit_to_user_mode_loop+0xd9/0xe0
  exit_to_user_mode_prepare+0x3e/0x60
  syscall_exit_to_user_mode+0x1d/0x50
  entry_SYSCALL_64_after_hwframe+0x44/0xae

As dm-error has no support for zones, btrfs will run it's zone emulation
mode on this device. The zone emulation mode emulates conventional zones,
so bail out if the zone bitmap that gets populated on mount sees the zone
as sequential while we're thinking it's a conventional zone when creating
a block group.

Note: this scenario is unlikely in a real wold application and can only
happen by this (ab)use of device-mapper targets.

Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
[ jth: commit message and error messages ]
Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/zoned.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 70b23a0d03b1..6edf88580f47 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1126,6 +1126,15 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
 			goto out;
 		}
 
+		if (zone.type == BLK_ZONE_TYPE_CONVENTIONAL) {
+			btrfs_err(fs_info,
+				  "zoned: unexpected conventional zone: %llu on device %s (devid %llu)",
+				  zone.start << SECTOR_SHIFT,
+				  rcu_str_deref(device->name), device->devid);
+			ret = -EIO;
+			goto out;
+		}
+
 		switch (zone.cond) {
 		case BLK_ZONE_COND_OFFLINE:
 		case BLK_ZONE_COND_READONLY:
-- 
2.31.1

[PATCH v2 2/2] btrfs: zoned: bail out if we can't read a reliable write pointer

[Johannes Thumshirn]

If we can't read a reliable write pointer from a sequential zone fail
creating the block group with an I/O error.

Also if the read write pointer is beyond the end of the respective zone,
fail the creation of the block group on this zone with an I/O error.

While this could also happen in real world scenarios with misbehaving
drives, this issue addresses a problem uncovered by xfstest's test case
generic/475.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/btrfs/zoned.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
index 6edf88580f47..743334cf8bdd 100644
--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1191,6 +1191,13 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
 
 	switch (map->type & BTRFS_BLOCK_GROUP_PROFILE_MASK) {
 	case 0: /* single */
+		if (alloc_offsets[0] == WP_MISSING_DEV) {
+			btrfs_err(fs_info,
+				  "zoned: cannot read write pointer for zone %llu",
+				  physical >> ilog2(fs_info->zone_size));
+			ret = -EIO;
+			goto out;
+		}
 		cache->alloc_offset = alloc_offsets[0];
 		break;
 	case BTRFS_BLOCK_GROUP_DUP:
@@ -1208,6 +1215,13 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
 	}
 
 out:
+	if (cache->alloc_offset > fs_info->zone_size) {
+		btrfs_err(fs_info,
+			  "zoned: write pointer %llu in BG %llu behind end of zone",
+			  cache->alloc_offset, logical);
+		ret = -EIO;
+	}
+
 	/* An extent is allocated after the write pointer */
 	if (!ret && num_conventional && last_alloc > cache->alloc_offset) {
 		btrfs_err(fs_info,
-- 
2.31.1

Re: [PATCH v2 1/2] btrfs: zoned: sanity check zone type

[David Sterba]

On Tue, May 04, 2021 at 10:30:23AM +0200, Johannes Thumshirn wrote:
> +		if (zone.type == BLK_ZONE_TYPE_CONVENTIONAL) {
> +			btrfs_err(fs_info,
> +				  "zoned: unexpected conventional zone: %llu on device %s (devid %llu)",

Message looks in line with others in zoned.c, I just noticed that this
also requires the rcu wrapper, ie. btrfs_err_in_rcu becausse
rcu_str_deref would complain.

> +				  zone.start << SECTOR_SHIFT,
> +				  rcu_str_deref(device->name), device->devid);
> +			ret = -EIO;
> +			goto out;
> +		}
> +
>  		switch (zone.cond) {
>  		case BLK_ZONE_COND_OFFLINE:
>  		case BLK_ZONE_COND_READONLY:
> -- 
> 2.31.1

Re: [PATCH v2 2/2] btrfs: zoned: bail out if we can't read a reliable write pointer

[David Sterba]

On Tue, May 04, 2021 at 10:30:24AM +0200, Johannes Thumshirn wrote:
> +			btrfs_err(fs_info,
> +				  "zoned: cannot read write pointer for zone %llu",
> +				  physical >> ilog2(fs_info->zone_size));

This converts the offset to the zone index, I think we should keep it
consistent, one way or another. The other message was printing the raw
offset (pysical or logical) and this is IMHO more friendly as this is
related to other on-disk structures. Translation to the zone index can
be done but the offset is not dependent on the device so I'd prefer
that. I've checked other messages and there are always raw offsets so
I've switched that back to the 'physical'.

> +			ret = -EIO;
> +			goto out;
> +		}
>  		cache->alloc_offset = alloc_offsets[0];
>  		break;
>  	case BTRFS_BLOCK_GROUP_DUP:
> @@ -1208,6 +1215,13 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
>  	}
>  
>  out:
> +	if (cache->alloc_offset > fs_info->zone_size) {
> +		btrfs_err(fs_info,
> +			  "zoned: write pointer %llu in BG %llu behind end of zone",

s/BG/block group/

And I've added the block group start as this is an anchor for any
potential debugging.

> +			  cache->alloc_offset, logical);
> +		ret = -EIO;
> +	}
> +
>  	/* An extent is allocated after the write pointer */
>  	if (!ret && num_conventional && last_alloc > cache->alloc_offset) {
>  		btrfs_err(fs_info,
> -- 
> 2.31.1

Re: [PATCH v2 1/2] btrfs: zoned: sanity check zone type

[David Sterba]

On Tue, May 04, 2021 at 10:30:23AM +0200, Johannes Thumshirn wrote:
> From: Naohiro Aota <naohiro.aota@wdc.com>
> 
> The xfstests test case generic/475 creates a dm-linear device that gets
> changed to a dm-error device. This leads to errors in loading the block
> group's zone information when running on a zoned file system, ultimately
> resulting in a list corruption. When running on a kernel with list
> debugging enabled this leads to the following crash.
> 
>  BTRFS: error (device dm-2) in cleanup_transaction:1953: errno=-5 IO failure
>  kernel BUG at lib/list_debug.c:54!
>  invalid opcode: 0000 [#1] SMP PTI
>  CPU: 1 PID: 2433 Comm: umount Tainted: G        W         5.12.0+ #1018
>  RIP: 0010:__list_del_entry_valid.cold+0x1d/0x47
>  RSP: 0018:ffffc90001473df0 EFLAGS: 00010296
>  RAX: 0000000000000054 RBX: ffff8881038fd000 RCX: ffffc90001473c90
>  RDX: 0000000100001a31 RSI: 0000000000000003 RDI: 0000000000000003
>  RBP: ffff888308871108 R08: 0000000000000003 R09: 0000000000000001
>  R10: 3961373532383838 R11: 6666666620736177 R12: ffff888308871000
>  R13: ffff8881038fd088 R14: ffff8881038fdc78 R15: dead000000000100
>  FS:  00007f353c9b1540(0000) GS:ffff888627d00000(0000) knlGS:0000000000000000
>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>  CR2: 00007f353cc2c710 CR3: 000000018e13c000 CR4: 00000000000006a0
>  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>  Call Trace:
>   btrfs_free_block_groups+0xc9/0x310 [btrfs]
>   close_ctree+0x2ee/0x31a [btrfs]
>   ? call_rcu+0x8f/0x270
>   ? mutex_lock+0x1c/0x40
>   generic_shutdown_super+0x67/0x100
>   kill_anon_super+0x14/0x30
>   btrfs_kill_super+0x12/0x20 [btrfs]
>   deactivate_locked_super+0x31/0x90
>   cleanup_mnt+0x13e/0x1b0
>   task_work_run+0x63/0xb0
>   exit_to_user_mode_loop+0xd9/0xe0
>   exit_to_user_mode_prepare+0x3e/0x60
>   syscall_exit_to_user_mode+0x1d/0x50
>   entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> As dm-error has no support for zones, btrfs will run it's zone emulation
> mode on this device. The zone emulation mode emulates conventional zones,
> so bail out if the zone bitmap that gets populated on mount sees the zone
> as sequential while we're thinking it's a conventional zone when creating
> a block group.
> 
> Note: this scenario is unlikely in a real wold application and can only
> happen by this (ab)use of device-mapper targets.
> 
> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
> [ jth: commit message and error messages ]
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/btrfs/zoned.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
> index 70b23a0d03b1..6edf88580f47 100644
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -1126,6 +1126,15 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
>  			goto out;
>  		}
>  
> +		if (zone.type == BLK_ZONE_TYPE_CONVENTIONAL) {
> +			btrfs_err(fs_info,
> +				  "zoned: unexpected conventional zone: %llu on device %s (devid %llu)",
> +				  zone.start << SECTOR_SHIFT,
> +				  rcu_str_deref(device->name), device->devid);
> +			ret = -EIO;

I messed up the patch queues when sending the pull request and the V1
got merged, without the message. I got this diff after rebase of
misc-next

--- a/fs/btrfs/zoned.c
+++ b/fs/btrfs/zoned.c
@@ -1127,6 +1127,10 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
                }

                if (zone.type == BLK_ZONE_TYPE_CONVENTIONAL) {
+                       btrfs_err_in_rcu(fs_info,
+       "zoned: unexpected conventional zone %llu on device %s (devid %llu)",
+                               zone.start << SECTOR_SHIFT,
+                               rcu_str_deref(device->name), device->devid);
                        ret = -EIO;
                        goto out;
---

I can update the changelog to describe what happened and that we want
this message and you don't need to resend anything, or you can send it
but because that it's my fault I'd rather do that but wanted to let you
know.

Re: [PATCH v2 1/2] btrfs: zoned: sanity check zone type

[Johannes Thumshirn]

On 13/05/2021 00:33, David Sterba wrote:
> On Tue, May 04, 2021 at 10:30:23AM +0200, Johannes Thumshirn wrote:
>> From: Naohiro Aota <naohiro.aota@wdc.com>
>>
>> The xfstests test case generic/475 creates a dm-linear device that gets
>> changed to a dm-error device. This leads to errors in loading the block
>> group's zone information when running on a zoned file system, ultimately
>> resulting in a list corruption. When running on a kernel with list
>> debugging enabled this leads to the following crash.
>>
>>  BTRFS: error (device dm-2) in cleanup_transaction:1953: errno=-5 IO failure
>>  kernel BUG at lib/list_debug.c:54!
>>  invalid opcode: 0000 [#1] SMP PTI
>>  CPU: 1 PID: 2433 Comm: umount Tainted: G        W         5.12.0+ #1018
>>  RIP: 0010:__list_del_entry_valid.cold+0x1d/0x47
>>  RSP: 0018:ffffc90001473df0 EFLAGS: 00010296
>>  RAX: 0000000000000054 RBX: ffff8881038fd000 RCX: ffffc90001473c90
>>  RDX: 0000000100001a31 RSI: 0000000000000003 RDI: 0000000000000003
>>  RBP: ffff888308871108 R08: 0000000000000003 R09: 0000000000000001
>>  R10: 3961373532383838 R11: 6666666620736177 R12: ffff888308871000
>>  R13: ffff8881038fd088 R14: ffff8881038fdc78 R15: dead000000000100
>>  FS:  00007f353c9b1540(0000) GS:ffff888627d00000(0000) knlGS:0000000000000000
>>  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>  CR2: 00007f353cc2c710 CR3: 000000018e13c000 CR4: 00000000000006a0
>>  DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
>>  DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
>>  Call Trace:
>>   btrfs_free_block_groups+0xc9/0x310 [btrfs]
>>   close_ctree+0x2ee/0x31a [btrfs]
>>   ? call_rcu+0x8f/0x270
>>   ? mutex_lock+0x1c/0x40
>>   generic_shutdown_super+0x67/0x100
>>   kill_anon_super+0x14/0x30
>>   btrfs_kill_super+0x12/0x20 [btrfs]
>>   deactivate_locked_super+0x31/0x90
>>   cleanup_mnt+0x13e/0x1b0
>>   task_work_run+0x63/0xb0
>>   exit_to_user_mode_loop+0xd9/0xe0
>>   exit_to_user_mode_prepare+0x3e/0x60
>>   syscall_exit_to_user_mode+0x1d/0x50
>>   entry_SYSCALL_64_after_hwframe+0x44/0xae
>>
>> As dm-error has no support for zones, btrfs will run it's zone emulation
>> mode on this device. The zone emulation mode emulates conventional zones,
>> so bail out if the zone bitmap that gets populated on mount sees the zone
>> as sequential while we're thinking it's a conventional zone when creating
>> a block group.
>>
>> Note: this scenario is unlikely in a real wold application and can only
>> happen by this (ab)use of device-mapper targets.
>>
>> Signed-off-by: Naohiro Aota <naohiro.aota@wdc.com>
>> [ jth: commit message and error messages ]
>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>> ---
>>  fs/btrfs/zoned.c | 9 +++++++++
>>  1 file changed, 9 insertions(+)
>>
>> diff --git a/fs/btrfs/zoned.c b/fs/btrfs/zoned.c
>> index 70b23a0d03b1..6edf88580f47 100644
>> --- a/fs/btrfs/zoned.c
>> +++ b/fs/btrfs/zoned.c
>> @@ -1126,6 +1126,15 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
>>  			goto out;
>>  		}
>>  
>> +		if (zone.type == BLK_ZONE_TYPE_CONVENTIONAL) {
>> +			btrfs_err(fs_info,
>> +				  "zoned: unexpected conventional zone: %llu on device %s (devid %llu)",
>> +				  zone.start << SECTOR_SHIFT,
>> +				  rcu_str_deref(device->name), device->devid);
>> +			ret = -EIO;
> 
> I messed up the patch queues when sending the pull request and the V1
> got merged, without the message. I got this diff after rebase of
> misc-next
> 
> --- a/fs/btrfs/zoned.c
> +++ b/fs/btrfs/zoned.c
> @@ -1127,6 +1127,10 @@ int btrfs_load_block_group_zone_info(struct btrfs_block_group *cache, bool new)
>                 }
> 
>                 if (zone.type == BLK_ZONE_TYPE_CONVENTIONAL) {
> +                       btrfs_err_in_rcu(fs_info,
> +       "zoned: unexpected conventional zone %llu on device %s (devid %llu)",
> +                               zone.start << SECTOR_SHIFT,
> +                               rcu_str_deref(device->name), device->devid);
>                         ret = -EIO;
>                         goto out;
> ---
> 
> I can update the changelog to describe what happened and that we want
> this message and you don't need to resend anything, or you can send it
> but because that it's my fault I'd rather do that but wanted to let you
> know.
> 

Looks good thanks :)