[PATCH 0/4] btrfs: enhanced logic for stray single device

[PATCH 0/4] btrfs: enhanced logic for stray single device

[Anand Jain]

This patch series comprises two preparatory patches (patches 1 and 3
below), one bug fix (patch 2), and one logic hardening patch (patch 4)
designed to address issues with stray single devices.

The objective is to ensure that single devices are not left lingering in
the device list unless they are properly mounted.

Anand Jain (4):
  btrfs: declare btrfs_free_stale_devices non-static
  btrfs: forget stray device on failed open
  btrfs: refactor btrfs_free_stale_devices to free single stray device
  btrfs: validate device_list at scan for stray free

 fs/btrfs/super.c   |  3 +++
 fs/btrfs/volumes.c | 18 ++++++++++++++----
 fs/btrfs/volumes.h |  2 ++
 3 files changed, 19 insertions(+), 4 deletions(-)

-- 
2.38.1

[PATCH 1/4] btrfs: declare btrfs_free_stale_devices non-static

[Anand Jain]

When the open fails, to remove the stray device, we need
btrfs_free_stale_devices() function. Make it non-static.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 2dc926ac9137..7821c152d956 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -515,7 +515,7 @@ btrfs_get_bdev_and_sb(const char *device_path, blk_mode_t flags, void *holder,
  *		-EBUSY if @devt is a mounted device.
  *		-ENOENT if @devt does not match any device in the list.
  */
-static int btrfs_free_stale_devices(dev_t devt, struct btrfs_device *skip_device)
+int btrfs_free_stale_devices(dev_t devt, struct btrfs_device *skip_device)
 {
 	struct btrfs_fs_devices *fs_devices, *tmp_fs_devices;
 	struct btrfs_device *device, *tmp_device;
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index feba8d53526c..44942b7b36b8 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -681,6 +681,7 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 		       blk_mode_t flags, void *holder);
 struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
 					   bool mount_arg_dev);
+int btrfs_free_stale_devices(dev_t devt, struct btrfs_device *skip_device);
 int btrfs_forget_devices(dev_t devt);
 void btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
 void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices);
-- 
2.38.1

[PATCH 2/4] btrfs: forget stray device on failed open

[Anand Jain]

If the physical device of a flakey dm device is tried mounting it fails
to open the device with handle, and leaves behind a stray single device
in the device list.

Remove it if the open fails and if it is a single device. As we don't
register a single device in the device list unless it is mounted.

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

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 29fab56c8152..4b73c3a2d7ab 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1820,6 +1820,9 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 	fs_info->fs_devices = fs_devices;
 
 	ret = btrfs_open_devices(fs_devices, mode, &btrfs_fs_type);
+	if (ret && fs_devices->total_devices == 1)
+		btrfs_free_stale_devices(device->devt, NULL);
+
 	mutex_unlock(&uuid_mutex);
 	if (ret)
 		return ret;
-- 
2.38.1

[PATCH 3/4] btrfs: refactor btrfs_free_stale_devices to free single stray device

[Anand Jain]

Refactor the function btrfs_free_stale_devices() to search for devices
with a single device and unmounted, freeing it.

This a preparation harden the reliance of tempfsid on a stray-free single
device, allowing temp fsid activation on a device.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/super.c   |  2 +-
 fs/btrfs/volumes.c | 16 ++++++++++++----
 fs/btrfs/volumes.h |  3 ++-
 3 files changed, 15 insertions(+), 6 deletions(-)

diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 4b73c3a2d7ab..d381abb275d1 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -1821,7 +1821,7 @@ static int btrfs_get_tree_super(struct fs_context *fc)
 
 	ret = btrfs_open_devices(fs_devices, mode, &btrfs_fs_type);
 	if (ret && fs_devices->total_devices == 1)
-		btrfs_free_stale_devices(device->devt, NULL);
+		btrfs_free_stale_devices(device->devt, NULL, false);
 
 	mutex_unlock(&uuid_mutex);
 	if (ret)
diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 7821c152d956..60d848392cd0 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -515,7 +515,8 @@ btrfs_get_bdev_and_sb(const char *device_path, blk_mode_t flags, void *holder,
  *		-EBUSY if @devt is a mounted device.
  *		-ENOENT if @devt does not match any device in the list.
  */
-int btrfs_free_stale_devices(dev_t devt, struct btrfs_device *skip_device)
+int btrfs_free_stale_devices(dev_t devt, struct btrfs_device *skip_device,
+			     bool free_stray_single)
 {
 	struct btrfs_fs_devices *fs_devices, *tmp_fs_devices;
 	struct btrfs_device *device, *tmp_device;
@@ -529,6 +530,12 @@ int btrfs_free_stale_devices(dev_t devt, struct btrfs_device *skip_device)
 	list_for_each_entry_safe(fs_devices, tmp_fs_devices, &fs_uuids, fs_list) {
 
 		mutex_lock(&fs_devices->device_list_mutex);
+
+		if (free_stray_single && fs_devices->total_devices != 1) {
+			mutex_unlock(&fs_devices->device_list_mutex);
+			continue;
+		}
+
 		list_for_each_entry_safe(device, tmp_device,
 					 &fs_devices->devices, dev_list) {
 			if (skip_device && skip_device == device)
@@ -1307,7 +1314,7 @@ int btrfs_forget_devices(dev_t devt)
 	int ret;
 
 	mutex_lock(&uuid_mutex);
-	ret = btrfs_free_stale_devices(devt, NULL);
+	ret = btrfs_free_stale_devices(devt, NULL, false);
 	mutex_unlock(&uuid_mutex);
 
 	return ret;
@@ -1416,7 +1423,8 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
 			  path, MAJOR(bdev_handle->bdev->bd_dev),
 			  MINOR(bdev_handle->bdev->bd_dev));
 
-		btrfs_free_stale_devices(bdev_handle->bdev->bd_dev, NULL);
+		btrfs_free_stale_devices(bdev_handle->bdev->bd_dev, NULL,
+					 false);
 
 		device = NULL;
 		goto free_disk_super;
@@ -1424,7 +1432,7 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
 
 	device = device_list_add(path, disk_super, &new_device_added);
 	if (!IS_ERR(device) && new_device_added)
-		btrfs_free_stale_devices(device->devt, device);
+		btrfs_free_stale_devices(device->devt, device, false);
 
 free_disk_super:
 	btrfs_release_disk_super(disk_super);
diff --git a/fs/btrfs/volumes.h b/fs/btrfs/volumes.h
index 44942b7b36b8..0ac25ccde96e 100644
--- a/fs/btrfs/volumes.h
+++ b/fs/btrfs/volumes.h
@@ -681,7 +681,8 @@ int btrfs_open_devices(struct btrfs_fs_devices *fs_devices,
 		       blk_mode_t flags, void *holder);
 struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
 					   bool mount_arg_dev);
-int btrfs_free_stale_devices(dev_t devt, struct btrfs_device *skip_device);
+int btrfs_free_stale_devices(dev_t devt, struct btrfs_device *skip_device,
+			     bool free_stray_single);
 int btrfs_forget_devices(dev_t devt);
 void btrfs_close_devices(struct btrfs_fs_devices *fs_devices);
 void btrfs_free_extra_devids(struct btrfs_fs_devices *fs_devices);
-- 
2.38.1

[PATCH 4/4] btrfs: validate device_list at scan for stray free

[Anand Jain]

Tempfsid assumes all registered single devices in the fs_devicies list are
to be mounted; otherwise, they won't be in the btrfs_device list.

We recently fixed a related bug caused by leaving failed-open device in
the list. This triggered tempfsid activation upon subsequent mounts of the
same fsid wrongly.

To prevent this, scan the entire device list at mount for any stray
device and free them in btrfs_scan_one_device().

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 60d848392cd0..bb0857cfbef2 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1382,6 +1382,8 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
 
 	lockdep_assert_held(&uuid_mutex);
 
+	btrfs_free_stale_devices(0, NULL, true);
+
 	/*
 	 * we would like to check all the supers, but that would make
 	 * a btrfs mount succeed after a mkfs from a different FS.
-- 
2.38.1

Re: [PATCH 0/4] btrfs: enhanced logic for stray single device

[Anand Jain]

Gentle ping for any reviews?. Thx.


On 3/9/24 19:14, Anand Jain wrote:
> This patch series comprises two preparatory patches (patches 1 and 3
> below), one bug fix (patch 2), and one logic hardening patch (patch 4)
> designed to address issues with stray single devices.
> 
> The objective is to ensure that single devices are not left lingering in
> the device list unless they are properly mounted.
> 
> Anand Jain (4):
>    btrfs: declare btrfs_free_stale_devices non-static
>    btrfs: forget stray device on failed open
>    btrfs: refactor btrfs_free_stale_devices to free single stray device
>    btrfs: validate device_list at scan for stray free
> 
>   fs/btrfs/super.c   |  3 +++
>   fs/btrfs/volumes.c | 18 ++++++++++++++----
>   fs/btrfs/volumes.h |  2 ++
>   3 files changed, 19 insertions(+), 4 deletions(-)
> 

Re: [PATCH 2/4] btrfs: forget stray device on failed open

[Boris Burkov]

On Sat, Mar 09, 2024 at 07:14:29PM +0530, Anand Jain wrote:
> If the physical device of a flakey dm device is tried mounting it fails
> to open the device with handle, and leaves behind a stray single device
> in the device list.
> 
> Remove it if the open fails and if it is a single device. As we don't
> register a single device in the device list unless it is mounted.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/super.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 29fab56c8152..4b73c3a2d7ab 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -1820,6 +1820,9 @@ static int btrfs_get_tree_super(struct fs_context *fc)
>  	fs_info->fs_devices = fs_devices;
>  
>  	ret = btrfs_open_devices(fs_devices, mode, &btrfs_fs_type);
> +	if (ret && fs_devices->total_devices == 1)
> +		btrfs_free_stale_devices(device->devt, NULL);
> +

It feels like we need to do this free no matter what the mount error is
after this point, not just in this one place.

>  	mutex_unlock(&uuid_mutex);
>  	if (ret)
>  		return ret;
> -- 
> 2.38.1
> 

Re: [PATCH 4/4] btrfs: validate device_list at scan for stray free

[Boris Burkov]

On Sat, Mar 09, 2024 at 07:14:31PM +0530, Anand Jain wrote:
> Tempfsid assumes all registered single devices in the fs_devicies list are
> to be mounted; otherwise, they won't be in the btrfs_device list.
> 
> We recently fixed a related bug caused by leaving failed-open device in
> the list. This triggered tempfsid activation upon subsequent mounts of the
> same fsid wrongly.
> 
> To prevent this, scan the entire device list at mount for any stray
> device and free them in btrfs_scan_one_device().

Is this an additional precaution on top of maintaining an invariant on
every umount/failed mount that we have freed stale devices of single
device fs-es? Or is it fundamentally impossible for us to enforce that
invariant?

It feels like overkill to hack up free_stale_devices in this way,
compared to just ensuring that we manage cleaning up single devices
fs-es correctly when we are in a cleanup context. If this is practically
the best way to ensure we don't get caught with our pants down by a
random stale device, then I suppose it's fine.

A total aside I just thought of:
I think it might also make sense to consider adding logic to look for
single device fs-es with a device->bdev that is stale from the block
layer's perspective, and somehow marking those in a way that tempfsid
cares about. That would help with things that like that case where we
delete the block dev out from under a mounted fs and mount it a second
time with tempfsid after it's recreated. Not a huge deal, as we've
already discussed, though.

> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 60d848392cd0..bb0857cfbef2 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1382,6 +1382,8 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>  
>  	lockdep_assert_held(&uuid_mutex);
>  
> +	btrfs_free_stale_devices(0, NULL, true);
> +
>  	/*
>  	 * we would like to check all the supers, but that would make
>  	 * a btrfs mount succeed after a mkfs from a different FS.
> -- 
> 2.38.1
> 

Re: [PATCH 2/4] btrfs: forget stray device on failed open

[Anand Jain]

On 3/14/24 22:35, Boris Burkov wrote:
> On Sat, Mar 09, 2024 at 07:14:29PM +0530, Anand Jain wrote:
>> If the physical device of a flakey dm device is tried mounting it fails
>> to open the device with handle, and leaves behind a stray single device
>> in the device list.
>>
>> Remove it if the open fails and if it is a single device. As we don't
>> register a single device in the device list unless it is mounted.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/super.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 29fab56c8152..4b73c3a2d7ab 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -1820,6 +1820,9 @@ static int btrfs_get_tree_super(struct fs_context *fc)
>>   	fs_info->fs_devices = fs_devices;
>>   
>>   	ret = btrfs_open_devices(fs_devices, mode, &btrfs_fs_type);
>> +	if (ret && fs_devices->total_devices == 1)
>> +		btrfs_free_stale_devices(device->devt, NULL);
>> +
> 
> It feels like we need to do this free no matter what the mount error is
> after this point, not just in this one place.
> 

Indeed. At goto error, btrfs_close_devices() is called, in turn
calls free_fs_devices() for single-device filesystems.

Thanks, Anand


>>   	mutex_unlock(&uuid_mutex);
>>   	if (ret)
>>   		return ret;
>> -- 
>> 2.38.1
>>

Re: [PATCH 4/4] btrfs: validate device_list at scan for stray free

[Anand Jain]

On 3/14/24 22:41, Boris Burkov wrote:
> On Sat, Mar 09, 2024 at 07:14:31PM +0530, Anand Jain wrote:
>> Tempfsid assumes all registered single devices in the fs_devicies list are
>> to be mounted; otherwise, they won't be in the btrfs_device list.
>>
>> We recently fixed a related bug caused by leaving failed-open device in
>> the list. This triggered tempfsid activation upon subsequent mounts of the
>> same fsid wrongly.
>>
>> To prevent this, scan the entire device list at mount for any stray
>> device and free them in btrfs_scan_one_device().
> 
> Is this an additional precaution on top of maintaining an invariant on
> every umount/failed mount that we have freed stale devices of single
> device fs-es? Or is it fundamentally impossible for us to enforce that
> invariant?
> 

Hmm. That's the ultimate goal: maintaining such an invariant. However,
there are bugs. So, this is the place where we can detect whether we
are successful. If we aren't, then we can work around it by freeing
the stale device and avoiding bad consequences.
I think I should also include a warning message when we detect and
free, so that it can be reviewed for the proper fix.
Does that seem reasonable?

> It feels like overkill to hack up free_stale_devices in this way,
> compared to just ensuring that we manage cleaning up single devices
> fs-es correctly when we are in a cleanup context. If this is practically
> the best way to ensure we don't get caught with our pants down by a
> random stale device, then I suppose it's fine.
> 
> A total aside I just thought of:
> I think it might also make sense to consider adding logic to look for
> single device fs-es with a device->bdev that is stale from the block
> layer's perspective, and somehow marking those in a way that tempfsid
> cares about. 


How would we know if the block layer considers a certain device's
block device (device->bdev) as stale?

If you mention a Write IO failure, we already put the filesystem
in read-only mode if that happens. But, we can't close the device
due to the pending write. (Some operating systems have an option
to call panic, which dumps the memory to the coredump device and
reboots.).

> That would help with things that like that case where we
> delete the block dev out from under a mounted fs and mount it a second
> time with tempfsid after it's recreated. Not a huge deal, as we've
> already discussed, though.

Yeah, thanks for brainstorming. Basically, we need a way to distinguish 
between the same physical-device with multiple nodes and different 
physical-devices with the same filesystem.

Thanks, Anand

> 
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 60d848392cd0..bb0857cfbef2 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1382,6 +1382,8 @@ struct btrfs_device *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>>   
>>   	lockdep_assert_held(&uuid_mutex);
>>   
>> +	btrfs_free_stale_devices(0, NULL, true);
>> +
>>   	/*
>>   	 * we would like to check all the supers, but that would make
>>   	 * a btrfs mount succeed after a mkfs from a different FS.
>> -- 
>> 2.38.1
>>

Re: [PATCH 4/4] btrfs: validate device_list at scan for stray free

[Anand Jain]

Boris,

Gentle ping?

Thanks.

On 3/16/24 19:43, Anand Jain wrote:
> On 3/14/24 22:41, Boris Burkov wrote:
>> On Sat, Mar 09, 2024 at 07:14:31PM +0530, Anand Jain wrote:
>>> Tempfsid assumes all registered single devices in the fs_devicies 
>>> list are
>>> to be mounted; otherwise, they won't be in the btrfs_device list.
>>>
>>> We recently fixed a related bug caused by leaving failed-open device in
>>> the list. This triggered tempfsid activation upon subsequent mounts 
>>> of the
>>> same fsid wrongly.
>>>
>>> To prevent this, scan the entire device list at mount for any stray
>>> device and free them in btrfs_scan_one_device().
>>
>> Is this an additional precaution on top of maintaining an invariant on
>> every umount/failed mount that we have freed stale devices of single
>> device fs-es? Or is it fundamentally impossible for us to enforce that
>> invariant?
>>
> 
> Hmm. That's the ultimate goal: maintaining such an invariant. However,
> there are bugs. So, this is the place where we can detect whether we
> are successful. If we aren't, then we can work around it by freeing
> the stale device and avoiding bad consequences.
> I think I should also include a warning message when we detect and
> free, so that it can be reviewed for the proper fix.
> Does that seem reasonable?
> 
>> It feels like overkill to hack up free_stale_devices in this way,
>> compared to just ensuring that we manage cleaning up single devices
>> fs-es correctly when we are in a cleanup context. If this is practically
>> the best way to ensure we don't get caught with our pants down by a
>> random stale device, then I suppose it's fine.
>>
>> A total aside I just thought of:
>> I think it might also make sense to consider adding logic to look for
>> single device fs-es with a device->bdev that is stale from the block
>> layer's perspective, and somehow marking those in a way that tempfsid
>> cares about. 
> 
> 
> How would we know if the block layer considers a certain device's
> block device (device->bdev) as stale?
> 
> If you mention a Write IO failure, we already put the filesystem
> in read-only mode if that happens. But, we can't close the device
> due to the pending write. (Some operating systems have an option
> to call panic, which dumps the memory to the coredump device and
> reboots.).
> 
>> That would help with things that like that case where we
>> delete the block dev out from under a mounted fs and mount it a second
>> time with tempfsid after it's recreated. Not a huge deal, as we've
>> already discussed, though.
> 
> Yeah, thanks for brainstorming. Basically, we need a way to distinguish 
> between the same physical-device with multiple nodes and different 
> physical-devices with the same filesystem.
> 
> Thanks, Anand
> 
>>
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>   fs/btrfs/volumes.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 60d848392cd0..bb0857cfbef2 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -1382,6 +1382,8 @@ struct btrfs_device 
>>> *btrfs_scan_one_device(const char *path, blk_mode_t flags,
>>>       lockdep_assert_held(&uuid_mutex);
>>> +    btrfs_free_stale_devices(0, NULL, true);
>>> +
>>>       /*
>>>        * we would like to check all the supers, but that would make
>>>        * a btrfs mount succeed after a mkfs from a different FS.
>>> -- 
>>> 2.38.1
>>>
>