All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] Fix device count check btrfs_rm_devices() and add comments
@ 2021-09-01  6:43 Anand Jain
  2021-09-01  6:43 ` [PATCH 1/2] btrfs: use num_device to check for the last surviving seed device Anand Jain
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Anand Jain @ 2021-09-01  6:43 UTC (permalink / raw)
  To: linux-btrfs

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 1/2] btrfs: use num_device to check for the last surviving seed device
  2021-09-01  6:43 [PATCH 0/2] Fix device count check btrfs_rm_devices() and add comments Anand Jain
@ 2021-09-01  6:43 ` Anand Jain
  2021-09-01  9:22   ` Nikolay Borisov
  2021-09-01  6:43 ` [PATCH 2/2] btrfs: add comments for device counts in struct btrfs_fs_devices Anand Jain
  2021-09-20  7:31 ` [PATCH 0/2] Fix device count check btrfs_rm_devices() and add comments Anand Jain
  2 siblings, 1 reply; 7+ messages in thread
From: Anand Jain @ 2021-09-01  6:43 UTC (permalink / raw)
  To: linux-btrfs

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH 2/2] btrfs: add comments for device counts in struct btrfs_fs_devices
  2021-09-01  6:43 [PATCH 0/2] Fix device count check btrfs_rm_devices() and add comments Anand Jain
  2021-09-01  6:43 ` [PATCH 1/2] btrfs: use num_device to check for the last surviving seed device Anand Jain
@ 2021-09-01  6:43 ` Anand Jain
  2021-09-20  7:31 ` [PATCH 0/2] Fix device count check btrfs_rm_devices() and add comments Anand Jain
  2 siblings, 0 replies; 7+ messages in thread
From: Anand Jain @ 2021-09-01  6:43 UTC (permalink / raw)
  To: linux-btrfs

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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] btrfs: use num_device to check for the last surviving seed device
  2021-09-01  6:43 ` [PATCH 1/2] btrfs: use num_device to check for the last surviving seed device Anand Jain
@ 2021-09-01  9:22   ` Nikolay Borisov
  2021-09-02 15:31     ` David Sterba
  0 siblings, 1 reply; 7+ messages in thread
From: Nikolay Borisov @ 2021-09-01  9:22 UTC (permalink / raw)
  To: Anand Jain, linux-btrfs



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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] btrfs: use num_device to check for the last surviving seed device
  2021-09-01  9:22   ` Nikolay Borisov
@ 2021-09-02 15:31     ` David Sterba
  2021-09-03  3:07       ` Anand Jain
  0 siblings, 1 reply; 7+ messages in thread
From: David Sterba @ 2021-09-02 15:31 UTC (permalink / raw)
  To: Nikolay Borisov; +Cc: Anand Jain, linux-btrfs

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.

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 1/2] btrfs: use num_device to check for the last surviving seed device
  2021-09-02 15:31     ` David Sterba
@ 2021-09-03  3:07       ` Anand Jain
  0 siblings, 0 replies; 7+ messages in thread
From: Anand Jain @ 2021-09-03  3:07 UTC (permalink / raw)
  To: dsterba, Nikolay Borisov; +Cc: linux-btrfs

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

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH 0/2] Fix device count check btrfs_rm_devices() and add comments
  2021-09-01  6:43 [PATCH 0/2] Fix device count check btrfs_rm_devices() and add comments Anand Jain
  2021-09-01  6:43 ` [PATCH 1/2] btrfs: use num_device to check for the last surviving seed device Anand Jain
  2021-09-01  6:43 ` [PATCH 2/2] btrfs: add comments for device counts in struct btrfs_fs_devices Anand Jain
@ 2021-09-20  7:31 ` Anand Jain
  2 siblings, 0 replies; 7+ messages in thread
From: Anand Jain @ 2021-09-20  7:31 UTC (permalink / raw)
  To: David Sterba; +Cc: linux-btrfs


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

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2021-09-20  7:31 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-01  6:43 [PATCH 0/2] Fix device count check btrfs_rm_devices() and add comments Anand Jain
2021-09-01  6:43 ` [PATCH 1/2] btrfs: use num_device to check for the last surviving seed device Anand Jain
2021-09-01  9:22   ` Nikolay Borisov
2021-09-02 15:31     ` David Sterba
2021-09-03  3:07       ` Anand Jain
2021-09-01  6:43 ` [PATCH 2/2] btrfs: add comments for device counts in struct btrfs_fs_devices Anand Jain
2021-09-20  7:31 ` [PATCH 0/2] Fix device count check btrfs_rm_devices() and add comments Anand Jain

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.