diff mbox series

[2/6] btrfs: metadata_uuid: move split-brain handling from fs_id() to new function

Message ID 20191212110132.11063-3-Damenly_Su@gmx.com (mailing list archive)
State New, archived
Headers show
Series btrfs: metadata uuid fixes and enhancements | expand

Commit Message

Su Yue Dec. 12, 2019, 11:01 a.m. UTC 
From: Su Yue <Damenly_Su@gmx.com>

The parameter @metadata_fsid of fs_id() is not NULL while scanned device
has metadata_uuid but not changing. Obviously, the cases handling part
in fs_id() is for this situation. Move the logic into new function
find_fsid_changing_metadata_uuid().

Signed-off-by: Su Yue <Damenly_Su@gmx.com>
---
 fs/btrfs/volumes.c | 78 ++++++++++++++++++++++++----------------------
 1 file changed, 41 insertions(+), 37 deletions(-)

Comments

Nikolay Borisov Dec. 12, 2019, 1:05 p.m. UTC | #1 
On 12.12.19 г. 13:01 ч., damenly.su@gmail.com wrote:
> From: Su Yue <Damenly_Su@gmx.com>
> 
> The parameter @metadata_fsid of fs_id() is not NULL while scanned device
> has metadata_uuid but not changing. Obviously, the cases handling part
> in fs_id() is for this situation. Move the logic into new function
> find_fsid_changing_metadata_uuid().

I think the following changelog might sound better.

metadata_fsid parameter of find_fsid is non-null in once specific case
in device_list_add. Factor the code out of find_fsid to a dedicated
function to improve readability.


> 
> Signed-off-by: Su Yue <Damenly_Su@gmx.com>
> ---
>  fs/btrfs/volumes.c | 78 ++++++++++++++++++++++++----------------------
>  1 file changed, 41 insertions(+), 37 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 9efa4123c335..b08b06a89a77 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -438,40 +438,6 @@ static noinline struct btrfs_fs_devices *find_fsid(
>  
>  	ASSERT(fsid);
>  
> -	if (metadata_fsid) {
> -		/*
> -		 * Handle scanned device having completed its fsid change but
> -		 * belonging to a fs_devices that was created by first scanning
> -		 * a device which didn't have its fsid/metadata_uuid changed
> -		 * at all and the CHANGING_FSID_V2 flag set.
> -		 */
> -		list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
> -			if (fs_devices->fsid_change &&
> -			    memcmp(metadata_fsid, fs_devices->fsid,
> -				   BTRFS_FSID_SIZE) == 0 &&
> -			    memcmp(fs_devices->fsid, fs_devices->metadata_uuid,
> -				   BTRFS_FSID_SIZE) == 0) {
> -				return fs_devices;
> -			}
> -		}
> -		/*
> -		 * Handle scanned device having completed its fsid change but
> -		 * belonging to a fs_devices that was created by a device that
> -		 * has an outdated pair of fsid/metadata_uuid and
> -		 * CHANGING_FSID_V2 flag set.
> -		 */
> -		list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
> -			if (fs_devices->fsid_change &&
> -			    memcmp(fs_devices->metadata_uuid,
> -				   fs_devices->fsid, BTRFS_FSID_SIZE) != 0 &&
> -			    memcmp(metadata_fsid, fs_devices->metadata_uuid,
> -				   BTRFS_FSID_SIZE) == 0) {
> -				return fs_devices;
> -			}
> -		}
> -	}
> -
> -	/* Handle non-split brain cases */
>  	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
>  		if (metadata_fsid) {
>  			if (memcmp(fsid, fs_devices->fsid, BTRFS_FSID_SIZE) == 0
> @@ -712,6 +678,46 @@ static struct btrfs_fs_devices *find_fsid_changed(
>  
>  	return NULL;
>  }
> +
> +static struct btrfs_fs_devices *find_fsid_changing_metada_uuid(

That function name is long (and contains a typo). More like something like:

find_fsid_with_metadata_uuid.

> +					struct btrfs_super_block *disk_super)
> +{
> +	struct btrfs_fs_devices *fs_devices;
> +
> +	/*
> +	 * Handle scanned device having completed its fsid change but
> +	 * belonging to a fs_devices that was created by first scanning
> +	 * a device which didn't have its fsid/metadata_uuid changed
> +	 * at all and the CHANGING_FSID_V2 flag set.
> +	 */
> +	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
> +		if (fs_devices->fsid_change &&
> +		    memcmp(disk_super->metadata_uuid, fs_devices->fsid,
> +			   BTRFS_FSID_SIZE) == 0 &&
> +		    memcmp(fs_devices->fsid, fs_devices->metadata_uuid,
> +			   BTRFS_FSID_SIZE) == 0) {
> +			return fs_devices;
> +		}
> +	}
> +	/*
> +	 * Handle scanned device having completed its fsid change but
> +	 * belonging to a fs_devices that was created by a device that
> +	 * has an outdated pair of fsid/metadata_uuid and
> +	 * CHANGING_FSID_V2 flag set.
> +	 */
> +	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
> +		if (fs_devices->fsid_change &&
> +		    memcmp(fs_devices->metadata_uuid,
> +			   fs_devices->fsid, BTRFS_FSID_SIZE) != 0 &&
> +		    memcmp(disk_super->metadata_uuid, fs_devices->metadata_uuid,
> +			   BTRFS_FSID_SIZE) == 0) {
> +			return fs_devices;
> +		}
> +	}
> +
> +	return find_fsid(disk_super->fsid, disk_super->metadata_uuid);
> +}
> +
>  /*
>   * Add new device to list of registered devices
>   *
> @@ -751,13 +757,11 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>  			fs_devices = find_fsid_changed(disk_super);
>  		}
>  	} else if (has_metadata_uuid) {
> -		fs_devices = find_fsid(disk_super->fsid,
> -				       disk_super->metadata_uuid);
> +		fs_devices = find_fsid_changing_metada_uuid(disk_super);
>  	} else {
>  		fs_devices = find_fsid(disk_super->fsid, NULL);
>  	}
>  
> -
>  	if (!fs_devices) {
>  		if (has_metadata_uuid)
>  			fs_devices = alloc_fs_devices(disk_super->fsid,
>
Su Yue Dec. 12, 2019, 1:32 p.m. UTC | #2 
On 2019/12/12 9:05 PM, Nikolay Borisov wrote:
>
>
> On 12.12.19 г. 13:01 ч., damenly.su@gmail.com wrote:
>> From: Su Yue <Damenly_Su@gmx.com>
>>
>> The parameter @metadata_fsid of fs_id() is not NULL while scanned device
>> has metadata_uuid but not changing. Obviously, the cases handling part
>> in fs_id() is for this situation. Move the logic into new function
>> find_fsid_changing_metadata_uuid().
>
> I think the following changelog might sound better.
>
> metadata_fsid parameter of find_fsid is non-null in once specific case
> in device_list_add. Factor the code out of find_fsid to a dedicated
> function to improve readability.
>

Thanks for the changelog! will replace.
>
>>
>> Signed-off-by: Su Yue <Damenly_Su@gmx.com>
>> ---
>>   fs/btrfs/volumes.c | 78 ++++++++++++++++++++++++----------------------
>>   1 file changed, 41 insertions(+), 37 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 9efa4123c335..b08b06a89a77 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -438,40 +438,6 @@ static noinline struct btrfs_fs_devices *find_fsid(
>>
>>   	ASSERT(fsid);
>>
>> -	if (metadata_fsid) {
>> -		/*
>> -		 * Handle scanned device having completed its fsid change but
>> -		 * belonging to a fs_devices that was created by first scanning
>> -		 * a device which didn't have its fsid/metadata_uuid changed
>> -		 * at all and the CHANGING_FSID_V2 flag set.
>> -		 */
>> -		list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
>> -			if (fs_devices->fsid_change &&
>> -			    memcmp(metadata_fsid, fs_devices->fsid,
>> -				   BTRFS_FSID_SIZE) == 0 &&
>> -			    memcmp(fs_devices->fsid, fs_devices->metadata_uuid,
>> -				   BTRFS_FSID_SIZE) == 0) {
>> -				return fs_devices;
>> -			}
>> -		}
>> -		/*
>> -		 * Handle scanned device having completed its fsid change but
>> -		 * belonging to a fs_devices that was created by a device that
>> -		 * has an outdated pair of fsid/metadata_uuid and
>> -		 * CHANGING_FSID_V2 flag set.
>> -		 */
>> -		list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
>> -			if (fs_devices->fsid_change &&
>> -			    memcmp(fs_devices->metadata_uuid,
>> -				   fs_devices->fsid, BTRFS_FSID_SIZE) != 0 &&
>> -			    memcmp(metadata_fsid, fs_devices->metadata_uuid,
>> -				   BTRFS_FSID_SIZE) == 0) {
>> -				return fs_devices;
>> -			}
>> -		}
>> -	}
>> -
>> -	/* Handle non-split brain cases */
>>   	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
>>   		if (metadata_fsid) {
>>   			if (memcmp(fsid, fs_devices->fsid, BTRFS_FSID_SIZE) == 0
>> @@ -712,6 +678,46 @@ static struct btrfs_fs_devices *find_fsid_changed(
>>
>>   	return NULL;
>>   }
>> +
>> +static struct btrfs_fs_devices *find_fsid_changing_metada_uuid(
>
> That function name is long (and contains a typo). More like something like:
>
> find_fsid_with_metadata_uuid.

Fine. Naming is so hard to me.

Thanks.
>
>> +					struct btrfs_super_block *disk_super)
>> +{
>> +	struct btrfs_fs_devices *fs_devices;
>> +
>> +	/*
>> +	 * Handle scanned device having completed its fsid change but
>> +	 * belonging to a fs_devices that was created by first scanning
>> +	 * a device which didn't have its fsid/metadata_uuid changed
>> +	 * at all and the CHANGING_FSID_V2 flag set.
>> +	 */
>> +	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
>> +		if (fs_devices->fsid_change &&
>> +		    memcmp(disk_super->metadata_uuid, fs_devices->fsid,
>> +			   BTRFS_FSID_SIZE) == 0 &&
>> +		    memcmp(fs_devices->fsid, fs_devices->metadata_uuid,
>> +			   BTRFS_FSID_SIZE) == 0) {
>> +			return fs_devices;
>> +		}
>> +	}
>> +	/*
>> +	 * Handle scanned device having completed its fsid change but
>> +	 * belonging to a fs_devices that was created by a device that
>> +	 * has an outdated pair of fsid/metadata_uuid and
>> +	 * CHANGING_FSID_V2 flag set.
>> +	 */
>> +	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
>> +		if (fs_devices->fsid_change &&
>> +		    memcmp(fs_devices->metadata_uuid,
>> +			   fs_devices->fsid, BTRFS_FSID_SIZE) != 0 &&
>> +		    memcmp(disk_super->metadata_uuid, fs_devices->metadata_uuid,
>> +			   BTRFS_FSID_SIZE) == 0) {
>> +			return fs_devices;
>> +		}
>> +	}
>> +
>> +	return find_fsid(disk_super->fsid, disk_super->metadata_uuid);
>> +}
>> +
>>   /*
>>    * Add new device to list of registered devices
>>    *
>> @@ -751,13 +757,11 @@ static noinline struct btrfs_device *device_list_add(const char *path,
>>   			fs_devices = find_fsid_changed(disk_super);
>>   		}
>>   	} else if (has_metadata_uuid) {
>> -		fs_devices = find_fsid(disk_super->fsid,
>> -				       disk_super->metadata_uuid);
>> +		fs_devices = find_fsid_changing_metada_uuid(disk_super);
>>   	} else {
>>   		fs_devices = find_fsid(disk_super->fsid, NULL);
>>   	}
>>
>> -
>>   	if (!fs_devices) {
>>   		if (has_metadata_uuid)
>>   			fs_devices = alloc_fs_devices(disk_super->fsid,
>>
diff mbox series

Patch

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 9efa4123c335..b08b06a89a77 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -438,40 +438,6 @@  static noinline struct btrfs_fs_devices *find_fsid(
 
 	ASSERT(fsid);
 
-	if (metadata_fsid) {
-		/*
-		 * Handle scanned device having completed its fsid change but
-		 * belonging to a fs_devices that was created by first scanning
-		 * a device which didn't have its fsid/metadata_uuid changed
-		 * at all and the CHANGING_FSID_V2 flag set.
-		 */
-		list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
-			if (fs_devices->fsid_change &&
-			    memcmp(metadata_fsid, fs_devices->fsid,
-				   BTRFS_FSID_SIZE) == 0 &&
-			    memcmp(fs_devices->fsid, fs_devices->metadata_uuid,
-				   BTRFS_FSID_SIZE) == 0) {
-				return fs_devices;
-			}
-		}
-		/*
-		 * Handle scanned device having completed its fsid change but
-		 * belonging to a fs_devices that was created by a device that
-		 * has an outdated pair of fsid/metadata_uuid and
-		 * CHANGING_FSID_V2 flag set.
-		 */
-		list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
-			if (fs_devices->fsid_change &&
-			    memcmp(fs_devices->metadata_uuid,
-				   fs_devices->fsid, BTRFS_FSID_SIZE) != 0 &&
-			    memcmp(metadata_fsid, fs_devices->metadata_uuid,
-				   BTRFS_FSID_SIZE) == 0) {
-				return fs_devices;
-			}
-		}
-	}
-
-	/* Handle non-split brain cases */
 	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
 		if (metadata_fsid) {
 			if (memcmp(fsid, fs_devices->fsid, BTRFS_FSID_SIZE) == 0
@@ -712,6 +678,46 @@  static struct btrfs_fs_devices *find_fsid_changed(
 
 	return NULL;
 }
+
+static struct btrfs_fs_devices *find_fsid_changing_metada_uuid(
+					struct btrfs_super_block *disk_super)
+{
+	struct btrfs_fs_devices *fs_devices;
+
+	/*
+	 * Handle scanned device having completed its fsid change but
+	 * belonging to a fs_devices that was created by first scanning
+	 * a device which didn't have its fsid/metadata_uuid changed
+	 * at all and the CHANGING_FSID_V2 flag set.
+	 */
+	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
+		if (fs_devices->fsid_change &&
+		    memcmp(disk_super->metadata_uuid, fs_devices->fsid,
+			   BTRFS_FSID_SIZE) == 0 &&
+		    memcmp(fs_devices->fsid, fs_devices->metadata_uuid,
+			   BTRFS_FSID_SIZE) == 0) {
+			return fs_devices;
+		}
+	}
+	/*
+	 * Handle scanned device having completed its fsid change but
+	 * belonging to a fs_devices that was created by a device that
+	 * has an outdated pair of fsid/metadata_uuid and
+	 * CHANGING_FSID_V2 flag set.
+	 */
+	list_for_each_entry(fs_devices, &fs_uuids, fs_list) {
+		if (fs_devices->fsid_change &&
+		    memcmp(fs_devices->metadata_uuid,
+			   fs_devices->fsid, BTRFS_FSID_SIZE) != 0 &&
+		    memcmp(disk_super->metadata_uuid, fs_devices->metadata_uuid,
+			   BTRFS_FSID_SIZE) == 0) {
+			return fs_devices;
+		}
+	}
+
+	return find_fsid(disk_super->fsid, disk_super->metadata_uuid);
+}
+
 /*
  * Add new device to list of registered devices
  *
@@ -751,13 +757,11 @@  static noinline struct btrfs_device *device_list_add(const char *path,
 			fs_devices = find_fsid_changed(disk_super);
 		}
 	} else if (has_metadata_uuid) {
-		fs_devices = find_fsid(disk_super->fsid,
-				       disk_super->metadata_uuid);
+		fs_devices = find_fsid_changing_metada_uuid(disk_super);
 	} else {
 		fs_devices = find_fsid(disk_super->fsid, NULL);
 	}
 
-
 	if (!fs_devices) {
 		if (has_metadata_uuid)
 			fs_devices = alloc_fs_devices(disk_super->fsid,