[PATCH 0/2] Fix device count check btrfs_rm_devices() and add comments

[PATCH 0/2] Fix device count check btrfs_rm_devices() and add comments

[Anand Jain]

Patch 1/2 fixes a bug that we checked open_devices to check if the deleted
device was the last surviving device in the seed fsid.
Patch 2/2 adds comments about the device counts in struct btrfs_fs_devices.

Anand Jain (2):
  btrfs: use num_device to check for the last surviving seed device
  btrfs: add comments for device counts in struct btrfs_fs_devices

 fs/btrfs/volumes.c |  2 +-
 fs/btrfs/volumes.h | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 1 deletion(-)

-- 
2.31.1

[PATCH 1/2] btrfs: use num_device to check for the last surviving seed device

[Anand Jain]

For both sprout and seed fsids,
 btrfs_fs_devices::num_devices provides device count including missing
 btrfs_fs_devices::open_devices provides device count excluding missing

We create a dummy struct btrfs_device for the missing device, so
num_devices != open_devices when there is a missing device.

In btrfs_rm_devices() we wrongly check for %cur_devices->open_devices
before freeing the seed fs_devices. Instead we should check for
%cur_devices->num_devices.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 53ead67b625c..5b36859a7b5e 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2184,7 +2184,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	synchronize_rcu();
 	btrfs_free_device(device);
 
-	if (cur_devices->open_devices == 0) {
+	if (cur_devices->num_devices == 0) {
 		list_del_init(&cur_devices->seed_list);
 		close_fs_devices(cur_devices);
 		free_fs_devices(cur_devices);
-- 
2.31.1

Re: [PATCH 1/2] btrfs: use num_device to check for the last surviving seed device

[Nikolay Borisov]

On 1.09.21 г. 9:43, Anand Jain wrote:
> For both sprout and seed fsids,
>  btrfs_fs_devices::num_devices provides device count including missing
>  btrfs_fs_devices::open_devices provides device count excluding missing
> 
> We create a dummy struct btrfs_device for the missing device, so
> num_devices != open_devices when there is a missing device.
> 
> In btrfs_rm_devices() we wrongly check for %cur_devices->open_devices
> before freeing the seed fs_devices. Instead we should check for
> %cur_devices->num_devices.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>

Is there a sequence of step that reproduce the problem?

> ---
>  fs/btrfs/volumes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 53ead67b625c..5b36859a7b5e 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -2184,7 +2184,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  	synchronize_rcu();
>  	btrfs_free_device(device);
>  
> -	if (cur_devices->open_devices == 0) {
> +	if (cur_devices->num_devices == 0) {
>  		list_del_init(&cur_devices->seed_list);
>  		close_fs_devices(cur_devices);
>  		free_fs_devices(cur_devices);
> 

Re: [PATCH 1/2] btrfs: use num_device to check for the last surviving seed device

[David Sterba]

On Wed, Sep 01, 2021 at 12:22:30PM +0300, Nikolay Borisov wrote:
> 
> 
> On 1.09.21 г. 9:43, Anand Jain wrote:
> > For both sprout and seed fsids,
> >  btrfs_fs_devices::num_devices provides device count including missing
> >  btrfs_fs_devices::open_devices provides device count excluding missing
> > 
> > We create a dummy struct btrfs_device for the missing device, so
> > num_devices != open_devices when there is a missing device.
> > 
> > In btrfs_rm_devices() we wrongly check for %cur_devices->open_devices
> > before freeing the seed fs_devices. Instead we should check for
> > %cur_devices->num_devices.
> > 
> > Signed-off-by: Anand Jain <anand.jain@oracle.com>
> 
> Is there a sequence of step that reproduce the problem?

Yeah that would be great, I don't have much idea what actually happens
here and what is the bug.

Re: [PATCH 1/2] btrfs: use num_device to check for the last surviving seed device

[Anand Jain]

On 02/09/2021 23:31, David Sterba wrote:
> On Wed, Sep 01, 2021 at 12:22:30PM +0300, Nikolay Borisov wrote:
>>
>>
>> On 1.09.21 г. 9:43, Anand Jain wrote:
>>> For both sprout and seed fsids,
>>>   btrfs_fs_devices::num_devices provides device count including missing
>>>   btrfs_fs_devices::open_devices provides device count excluding missing
>>>
>>> We create a dummy struct btrfs_device for the missing device, so
>>> num_devices != open_devices when there is a missing device.
>>>
>>> In btrfs_rm_devices() we wrongly check for %cur_devices->open_devices
>>> before freeing the seed fs_devices. Instead we should check for
>>> %cur_devices->num_devices.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>
>> Is there a sequence of step that reproduce the problem?


Fundamentally per our design, we should have a struct btrfs_device even
for a missing device.

Here is an example where we break that rule after deleting a device in
a degraded seed device.


The below example needs boilerplate code here [1], to dump the struct
btrfs_fs_devices from the kernel.

[1] 
https://github.com/asj/bbp/blob/master/misc-next/0001-btrfs-boilerplate-procfs-devlist-and-fsinfo.patch


---------- cut ------
# Steps

# Create raid1 seed device and degrade it.
# Mount it with -o degraded.
# Add a RW device (sprout).
# Remount it so that it is writable now.
# Still we can't remove the seed device because it is degraded.
# So convert the raid1 chunks to single.
# Now remove the seed device (note: missing device is still there).
# At this stage the total devices should be 2 (one sprout and another
#  missing).


DEV1=/dev/vg/scratch0
DEV2=/dev/vg/scratch1
DEV3=/dev/vg/scratch2

local 
dfilter='fsid:|devid:|missing|total_devices|num_devices|open_devices|device:|MISSING|UUID'

         mkfs.btrfs -fq -draid1 -mraid1 $DEV1 $DEV2
         mount $DEV1 /btrfs
         xfs_io -f -c "pwrite 0 1M" /btrfs/foo
         umount /btrfs
         btrfstune -S1 $DEV1
         wipefs -a $DEV2
         btrfs dev scan --forget
         mount -o degraded $DEV1 /btrfs
         btrfs dev add -f $DEV3 /btrfs
         mount -o remount,rw $DEV3 /btrfs
         btrfs bal start --force -dconvert=single -mconvert=single /btrfs
         btrfs dev del $DEV1 /btrfs
	# dump btrfs_fs_devices.
         cat /proc/fs/btrfs/devlist  | egrep '$dfilter'
----------------


Before the patch:

total_devices = 2 but, we see only one device with devid 3.

[fsid: f3bd00d8-7be3-491c-a785-0d207a43b248]
	num_devices:		1
	open_devices:		1
	missing_devices:	0
	total_devices:		2
	[[UUID: 8c2d2dc6-7c0c-4a02-b63e-109131edd563]]
		device:		/dev/mapper/vg-scratch2  <-- DEV3
		devid:		3


After the patch:

total_devices = 2 and, we see both devid 3 and devid 2 (with the missing 
flag).

[fsid: 8542998a-64e8-4180-bb29-978403beb81c]
	num_devices:		1
	open_devices:		1
	missing_devices:	0
	total_devices:		2
	[[UUID: b0960a13-f956-4a0b-9ec2-b0da05060ec6]]
		device:		/dev/mapper/vg-scratch2  <-- DEV3
		devid:		3
	[[seed_fsid: 8dbae94f-f4c2-4875-9a88-b7db7847e740]]
		sprout_fsid:		8542998a-64e8-4180-bb29-978403beb81c
		num_devices:		1
		open_devices:		0
		missing_devices:	1
		total_devices:		1
		[[UUID: 9ade13a3-8264-4e41-908d-048dae3af370]]
			device:		(null)    <-- DEV1
			devid:		2
			dev_state:	|IN_FS_METADATA|MISSING|dev_stats_valid



Thanks, Anand

[PATCH 2/2] btrfs: add comments for device counts in struct btrfs_fs_devices

[Anand Jain]

A bug was was checking a wrong device count before we delete the struct
btrfs_fs_devices in btrfs_rm_device(). To avoid future confusion and
easy reference add a comment about the various device counts that we have
in the struct btrfs_fs_devices.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.h | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 150b4cd8f81f..32ff06d4cc17 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -236,11 +236,30 @@ struct btrfs_fs_devices {
 	bool fsid_change;
 	struct list_head fs_list;
 
+	/*
+	 * Number of devices under this fsid including missing and
+	 * replace-target device and excludes seed devices.
+	 */
 	u64 num_devices;
+
+	/*
+	 * The number of devices that successfully opened, including
+	 * replace-target, excludes seed devices.
+	 */
 	u64 open_devices;
+
+	/* The number of devices that are under the chunk allocation list. */
 	u64 rw_devices;
+
+	/* Count of missing devices under this fsid excluding seed device. */
 	u64 missing_devices;
 	u64 total_rw_bytes;
+
+	/*
+	 * Count of devices from btrfs_super_block::num_devices for this fsid,
+	 * which includes the seed device, excludes the transient replace-target
+	 * device.
+	 */
 	u64 total_devices;
 
 	/* Highest generation number of seen devices */
-- 
2.31.1

Re: [PATCH 0/2] Fix device count check btrfs_rm_devices() and add comments

[Anand Jain]

Ping?

Thanks, Anand

On 01/09/2021 14:43, Anand Jain wrote:
> Patch 1/2 fixes a bug that we checked open_devices to check if the deleted
> device was the last surviving device in the seed fsid.
> Patch 2/2 adds comments about the device counts in struct btrfs_fs_devices.
> 
> Anand Jain (2):
>    btrfs: use num_device to check for the last surviving seed device
>    btrfs: add comments for device counts in struct btrfs_fs_devices
> 
>   fs/btrfs/volumes.c |  2 +-
>   fs/btrfs/volumes.h | 19 +++++++++++++++++++
>   2 files changed, 20 insertions(+), 1 deletion(-)
>