diff mbox series

[v3] btrfs: allow single disk devices to mount with older generations

Message ID 1bf56a77b8b57cc3b993fda00752e79830685ffc.1674772170.git.josef@toxicpanda.com (mailing list archive)
State New, archived
Headers show
Series [v3] btrfs: allow single disk devices to mount with older generations | expand

Commit Message

Josef Bacik Jan. 26, 2023, 10:31 p.m. UTC 
We have this check to make sure we don't accidentally add older devices
that may have disappeared and re-appeared with an older generation from
being added to an fs_devices.  This makes sense, we don't want stale
disks in our file system.  However for single disks this doesn't really
make sense.  I've seen this in testing, but I was provided a reproducer
from a project that builds btrfs images on loopback devices.  The
loopback device gets cached with the new generation, and then if it is
re-used to generate a new file system we'll fail to mount it because the
new fs is "older" than what we have in cache.

Fix this by simply ignoring this check if we're a single disk file
system, as we're not going to cause problems for the fs by allowing the
disk to be mounted with an older generation than what is in our cache.

I've also added a error message for this case, as it was kind of
annoying to find originally.

Reviewed-by: Anand Jain <anand.jain@oracle.com>
Reported-by: Daan De Meyer <daandemeyer@fb.com>
Signed-off-by: Josef Bacik <josef@toxicpanda.com>
---
v2->v3:
- Dropped the printk as we now have a printk here to indicate that something
  went wrong.
- Validated that it still fixes btrfs/219.  That test validates a few corner
  cases that I could think of at the time, and looking at it again I've covered
  everything that comes to mind.

 fs/btrfs/volumes.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Comments

Anand Jain Jan. 27, 2023, 12:48 a.m. UTC | #1 
Josef,

An alternative approach is to free the device allocation for a single
device filesystem during the close_ctree() [1]. In my opinion, this
solution is more elegant. I would appreciate your thoughts on this.

[1]
 
https://patchwork.kernel.org/project/linux-btrfs/patch/faf1de6f88707dbf0406ab85e094e72107b30637.1674221591.git.anand.jain@oracle.com/

Thanks, Anand


On 1/27/23 06:31, Josef Bacik wrote:
> We have this check to make sure we don't accidentally add older devices
> that may have disappeared and re-appeared with an older generation from
> being added to an fs_devices.  This makes sense, we don't want stale
> disks in our file system.  However for single disks this doesn't really
> make sense.  I've seen this in testing, but I was provided a reproducer
> from a project that builds btrfs images on loopback devices.  The
> loopback device gets cached with the new generation, and then if it is
> re-used to generate a new file system we'll fail to mount it because the
> new fs is "older" than what we have in cache.
> 
> Fix this by simply ignoring this check if we're a single disk file
> system, as we're not going to cause problems for the fs by allowing the
> disk to be mounted with an older generation than what is in our cache.
> 
> I've also added a error message for this case, as it was kind of
> annoying to find originally.
> 
> Reviewed-by: Anand Jain <anand.jain@oracle.com>
> Reported-by: Daan De Meyer <daandemeyer@fb.com>
> Signed-off-by: Josef Bacik <josef@toxicpanda.com>
> ---
> v2->v3:
> - Dropped the printk as we now have a printk here to indicate that something
>    went wrong.
> - Validated that it still fixes btrfs/219.  That test validates a few corner
>    cases that I could think of at the time, and looking at it again I've covered
>    everything that comes to mind.
> 
>   fs/btrfs/volumes.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 707dd0456cea..b17b4a66a8d4 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -766,6 +766,7 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>   		BTRFS_FEATURE_INCOMPAT_METADATA_UUID);
>   	bool fsid_change_in_progress = (btrfs_super_flags(disk_super) &
>   					BTRFS_SUPER_FLAG_CHANGING_FSID_V2);
> +	bool multi_disk = btrfs_super_num_devices(disk_super) > 1;
>   
>   	error = lookup_bdev(path, &path_devt);
>   	if (error) {
> @@ -902,7 +903,8 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>   		 * tracking a problem where systems fail mount by subvolume id
>   		 * when we reject replacement on a mounted FS.
>   		 */
> -		if (!fs_devices->opened && found_transid < device->generation) {
> +		if (multi_disk && !fs_devices->opened &&
> +		    found_transid < device->generation) {
>   			/*
>   			 * That is if the FS is _not_ mounted and if you
>   			 * are here, that means there is more than one
diff mbox series

Patch

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 707dd0456cea..b17b4a66a8d4 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -766,6 +766,7 @@  static noinline struct btrfs_device *device_list_add(const char *path,
 		BTRFS_FEATURE_INCOMPAT_METADATA_UUID);
 	bool fsid_change_in_progress = (btrfs_super_flags(disk_super) &
 					BTRFS_SUPER_FLAG_CHANGING_FSID_V2);
+	bool multi_disk = btrfs_super_num_devices(disk_super) > 1;
 
 	error = lookup_bdev(path, &path_devt);
 	if (error) {
@@ -902,7 +903,8 @@  static noinline struct btrfs_device *device_list_add(const char *path,
 		 * tracking a problem where systems fail mount by subvolume id
 		 * when we reject replacement on a mounted FS.
 		 */
-		if (!fs_devices->opened && found_transid < device->generation) {
+		if (multi_disk && !fs_devices->opened &&
+		    found_transid < device->generation) {
 			/*
 			 * That is if the FS is _not_ mounted and if you
 			 * are here, that means there is more than one