* [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 related [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
* [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 related [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.