btrfs: add a test of replace missing dev in diff raid
diff mbox

Message ID 20150626222409.GA27386@huxley.DHCP.TheFacebook.com
State New
Headers show

Commit Message

Omar Sandoval June 26, 2015, 10:24 p.m. UTC
On Fri, Jun 26, 2015 at 01:21:03PM +0800, Wang Yanfeng wrote:
[snip]
> +_test "raid1"
> +_test "raid10"
> +_test "raid5"
> +_test "raid6"

Hi, Wang Yanfeng,

Thanks a lot for posting this. This looks similar to the stuff in, e.g.,
btrfs/071:

	_btrfs_get_profile_configs replace
	...
	echo "Silence is golden"
	for t in "${_btrfs_profile_configs[@]}"; do
		run_test "$t"
	done

It'd be nice to do it the same way in this test. _btrfs_profile_configs
would need to be updated to support RAID 5/6 and be aware of replace
missing. I'll attach a patch, feel free to use it. Additionally, we
could also update btrfs/011 to test RAID 5/6.

Thanks,

Comments

Wang Yanfeng June 29, 2015, 9:13 a.m. UTC | #1
Hello, Omar:

To update some cases to support RAID5/6 is necessary.
Like btrfs/011 btrfs/071 , and the man page of BTRFS-REPLACE,
they all should be modified.

But equipment damage and missing device is small probability event when
we use disks in our daily life. So it shouldn't be so important that we let
_btrfs_get_profile_confilgs know it.
Besides, every case tests a different direction of replace is good. e.g.
011 071  020 etc.

So I think to test it in a new case is better.  What is your opinion?

cheers,
wangyf


On 06/27/2015 06:24 AM, Omar Sandoval wrote:
> On Fri, Jun 26, 2015 at 01:21:03PM +0800, Wang Yanfeng wrote:
> [snip]
>> +_test "raid1"
>> +_test "raid10"
>> +_test "raid5"
>> +_test "raid6"
> Hi, Wang Yanfeng,
>
> Thanks a lot for posting this. This looks similar to the stuff in, e.g.,
> btrfs/071:
>
> 	_btrfs_get_profile_configs replace
> 	...
> 	echo "Silence is golden"
> 	for t in "${_btrfs_profile_configs[@]}"; do
> 		run_test "$t"
> 	done
>
> It'd be nice to do it the same way in this test. _btrfs_profile_configs
> would need to be updated to support RAID 5/6 and be aware of replace
> missing. I'll attach a patch, feel free to use it. Additionally, we
> could also update btrfs/011 to test RAID 5/6.
>
> Thanks,

--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Omar Sandoval June 29, 2015, 6:06 p.m. UTC | #2
On 06/29/2015 02:13 AM, wangyf wrote:
> Hello, Omar:
> 
> To update some cases to support RAID5/6 is necessary.
> Like btrfs/011 btrfs/071 , and the man page of BTRFS-REPLACE,
> they all should be modified.
> 
> But equipment damage and missing device is small probability event when
> we use disks in our daily life. So it shouldn't be so important that we let
> _btrfs_get_profile_confilgs know it.
> Besides, every case tests a different direction of replace is good. e.g.
> 011 071  020 etc.
> 
> So I think to test it in a new case is better.  What is your opinion?
> 
> cheers,
> wangyf

Hi, Yanfeng,

Sorry, I think I might have been unclear. I definitely agree that the
missing case should be tested in a separate test case. However, I do
think that _btrfs_get_profile_configs should be updated so we can keep
things in one place and make sure that all profiles are thoroughly
tested. With _btrfs_get_profile_configs aware of RAID 5/6 and missing
device replacement, the new test case can just make use of that instead
of hardcoding the profiles to test. Does that sound alright?

Thanks!
Wang Yanfeng June 30, 2015, 1:51 a.m. UTC | #3
On 06/30/2015 02:06 AM, Omar Sandoval wrote:
> On 06/29/2015 02:13 AM, wangyf wrote:
>> Hello, Omar:
>>
>> To update some cases to support RAID5/6 is necessary.
>> Like btrfs/011 btrfs/071 , and the man page of BTRFS-REPLACE,
>> they all should be modified.
>>
>> But equipment damage and missing device is small probability event when
>> we use disks in our daily life. So it shouldn't be so important that we let
>> _btrfs_get_profile_confilgs know it.
>> Besides, every case tests a different direction of replace is good. e.g.
>> 011 071  020 etc.
>>
>> So I think to test it in a new case is better.  What is your opinion?
>>
>> cheers,
>> wangyf
> Hi, Yanfeng,
>
> Sorry, I think I might have been unclear. I definitely agree that the
> missing case should be tested in a separate test case. However, I do
> think that _btrfs_get_profile_configs should be updated so we can keep
> things in one place and make sure that all profiles are thoroughly
> tested. With _btrfs_get_profile_configs aware of RAID 5/6 and missing
> device replacement, the new test case can just make use of that instead
> of hardcoding the profiles to test. Does that sound alright?
>
> Thanks!

Hi, Omar:
I think it again, and now I think you are right.
To let _btrfs_get_profile_confilgs be aware of replace+missing
many redundant codes can be evitable.
We can use it in a new test case to test missing device.

regards.

--
To unsubscribe from this list: send the line "unsubscribe fstests" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

From 2b038178335105055966a84173fa39b91591b43e Mon Sep 17 00:00:00 2001
Message-Id: <2b038178335105055966a84173fa39b91591b43e.1435356729.git.osandov@fb.com>
From: Omar Sandoval <osandov@fb.com>
Date: Fri, 26 Jun 2015 15:09:50 -0700
Subject: [PATCH] btrfs: add replace missing and replace RAID 5/6 to profile
 configs

Replacing and scrubbing RAID 5/6 is now supported on Btrfs. Enable it in
_btrfs_get_profile_configs while making it more generic to also support
replace missing.

Signed-off-by: Omar Sandoval <osandov@fb.com>
---
 common/rc | 96 ++++++++++++++++++++++++++++++++-------------------------------
 1 file changed, 49 insertions(+), 47 deletions(-)

diff --git a/common/rc b/common/rc
index 610045eab304..3e6fdb6ebcfa 100644
--- a/common/rc
+++ b/common/rc
@@ -2748,60 +2748,62 @@  _btrfs_get_profile_configs()
 		return
 	fi
 
-	# no user specified btrfs profile configs, export the default configs
 	if [ -z "$BTRFS_PROFILE_CONFIGS" ]; then
-		# default configs
-		_btrfs_profile_configs=(
-			"-m single -d single"
-			"-m dup -d single"
-			"-m raid0 -d raid0"
-			"-m raid1 -d raid0"
-			"-m raid1 -d raid1"
-			"-m raid10 -d raid10"
-			"-m raid5 -d raid5"
-			"-m raid6 -d raid6"
+		# Default configurations to test.
+		local configs=(
+			"single:single"
+			"dup:single"
+			"raid0:raid0"
+			"raid1:raid0"
+			"raid1:raid1"
+			"raid10:raid10"
+			"raid5:raid5"
+			"raid6:raid6"
 		)
+	else
+		# User-provided configurations.
+		local configs=(${BTRFS_PROFILE_CONFIGS[@]})
+	fi
 
-		# remove dup/raid5/raid6 profiles if we're doing device replace
-		# dup profile indicates only one device being used (SCRATCH_DEV),
-		# but we don't want to replace SCRATCH_DEV, which will be used in
-		# _scratch_mount/_check_scratch_fs etc.
-		# and raid5/raid6 doesn't support replace yet
+	_btrfs_profile_configs=()
+	for cfg in "${configs[@]}"; do
+		local supported=true
+		local profiles=(${cfg/:/ })
 		if [ "$1" == "replace" ]; then
-			_btrfs_profile_configs=(
-				"-m single -d single"
-				"-m raid0 -d raid0"
-				"-m raid1 -d raid0"
-				"-m raid1 -d raid1"
-				"-m raid10 -d raid10"
-				# add these back when raid5/6 is working with replace
-				#"-m raid5 -d raid5"
-				#"-m raid6 -d raid6"
+			# We can't do replace with these profiles because they
+			# imply only one device ($SCRATCH_DEV), and we need to
+			# keep $SCRATCH_DEV around for _scratch_mount
+			# and _check_scratch_fs.
+			local unsupported=(
+				"single"
+				"dup"
 			)
+		elif [ "$1" == "replace-missing" ]; then
+			# We can't replace missing devices with these profiles
+			# because there isn't enough redundancy.
+			local unsupported=(
+				"single"
+				"dup"
+				"raid0"
+			)
+		else
+			local unsupported=()
 		fi
-		export _btrfs_profile_configs
-		return
-	fi
-
-	# parse user specified btrfs profile configs
-	local i=0
-	local cfg=""
-	for cfg in $BTRFS_PROFILE_CONFIGS; do
-		# turn "metadata:data" format to "-m metadata -d data"
-		# and assign it to _btrfs_profile_configs array
-		cfg=`echo "$cfg" | sed -e 's/^/-m /' -e 's/:/ -d /'`
-		_btrfs_profile_configs[$i]="$cfg"
-		let i=i+1
-	done
-
-	if [ "$1" == "replace" ]; then
-		if echo ${_btrfs_profile_configs[*]} | grep -q raid[56]; then
-			_notrun "RAID5/6 doesn't support btrfs device replace yet"
-		fi
-		if echo ${_btrfs_profile_configs[*]} | grep -q dup; then
-			_notrun "Do not set dup profile in btrfs device replace test"
+		for unsupp in "${unsupported[@]}"; do
+			if [ "${profiles[0]}" == "$unsupp" -o "${profiles[1]}" == "$unsupp" ]; then
+			     if [ -z "$BTRFS_PROFILE_CONFIGS" ]; then
+				     # For the default config, just omit it.
+				     supported=false
+			     else
+				     # For user-provided config, don't run the test.
+				     _notrun "Profile $unsupp not supported for $1"
+			     fi
+			fi
+		done
+		if "$supported"; then
+			_btrfs_profile_configs+=("-m ${profiles[0]} -d ${profiles[1]}")
 		fi
-	fi
+	done
 	export _btrfs_profile_configs
 }
 
-- 
2.4.4