| Message ID | 6d332e954da29e8418689cb49294a68da2fa5376.1437683940.git.osandov@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 23, 2015 at 01:51:50PM -0700, Omar Sandoval wrote: > 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> Looks great! Tested with default configs and user-defined configs. Reviewed-by: Eryu Guan <eguan@redhat.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.6 > > -- > 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 -- 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
On Thu, Jul 23, 2015 at 01:51:50PM -0700, Omar Sandoval wrote: > + # 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" DUP does imply single device, but why does 'single' ? -- 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
On Fri, Jul 24, 2015 at 02:09:46PM +0200, David Sterba wrote: > On Thu, Jul 23, 2015 at 01:51:50PM -0700, Omar Sandoval wrote: > > + # 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" > > DUP does imply single device, but why does 'single' ? It does not, I apparently forgot that you could use single to concatenate multiple devices. I'll fix that in v2. Thanks for reviewing!
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 2015/07/24 07:50 PM, Omar Sandoval wrote: > On Fri, Jul 24, 2015 at 02:09:46PM +0200, David Sterba wrote: >> On Thu, Jul 23, 2015 at 01:51:50PM -0700, Omar Sandoval wrote: >>> + # 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" >> >> DUP does imply single device, but why does 'single' ? > > It does not, I apparently forgot that you could use single to > concatenate multiple devices. I'll fix that in v2. > > Thanks for reviewing! > Late to the party. DUP *implies* single device but there are cases where dup is used on a multi-device fs. Even if the use-cases aren't good or intended to be long-term, they are still valid, right? - --
On Tue, Jul 28, 2015 at 12:22:56AM +0200, Brendan Hide wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA1 > > On 2015/07/24 07:50 PM, Omar Sandoval wrote: > > On Fri, Jul 24, 2015 at 02:09:46PM +0200, David Sterba wrote: > >> On Thu, Jul 23, 2015 at 01:51:50PM -0700, Omar Sandoval wrote: > >>> + # 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" > >> > >> DUP does imply single device, but why does 'single' ? > > > > It does not, I apparently forgot that you could use single to > > concatenate multiple devices. I'll fix that in v2. > > > > Thanks for reviewing! > > > Late to the party. DUP *implies* single device but there are cases > where dup is used on a multi-device fs. Even if the use-cases aren't > good or intended to be long-term, they are still valid, right? Yes, but bear in mind that any new (presumably metadata) chunks allocated on the multi-device FS will end up as RAID-1, because of the automatic upgrade from DUP to RAID-1. Any balance will also do the upgrade. Hugo. PS. Can we get rid of that upgrade? It's a pain the arse in so many ways. > - -- > __________ > Brendan Hide > http://swiftspirit.co.za/ > http://www.webafrica.co.za/?AFF1E97 > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v2.0.22 (MingW32) > > iQEcBAEBAgAGBQJVtq9AAAoJEE+uni74c4qN5eYIAJAGznsi3RD1tchbSLwhMXJk > bJJ4ORB9taLXHykSfYTsHIaUoVpcVR6tT/I1jz5070DY3mKkQ16a8nwtSxPba4Lv > QiS8YRegFiHMYzZbH1T7Tnm6R9g/RZsaU7GS3JhP9HUYG7hIWGRRuoiOjYn/hoLw > uMXuIFOkPKGYDgyAhDIp3KDYlBjMHT6Oun7CcpvTjXiOnzJFFp3MSt3b6mmmdMVV > YKWpWyKVh7qlENEoqKb4exqr1WGYKU+kBLXRs4wdm3xb66EcWYs0Er1u6v+K1trx > nryFrfUxYtMJsSuR9ZJm88DOsXKAuX1LEdRKVOlq7krsIK8HlTizccMXAl10gKk= > =ndkL > -----END PGP SIGNATURE-----
On Tue, Jul 28, 2015 at 12:22:56AM +0200, Brendan Hide wrote: > > It does not, I apparently forgot that you could use single to > > concatenate multiple devices. I'll fix that in v2. > > > > Thanks for reviewing! > > > Late to the party. DUP *implies* single device but there are cases > where dup is used on a multi-device fs. Even if the use-cases aren't > good or intended to be long-term, they are still valid, right? You're right, DUP is reported by 'fi df' after 2nd device is added and this state (even if it's temporary) has to be taken into account. -- 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
On Tue, Jul 28, 2015 at 06:52:07PM +0200, David Sterba wrote: > On Tue, Jul 28, 2015 at 12:22:56AM +0200, Brendan Hide wrote: > > > It does not, I apparently forgot that you could use single to > > > concatenate multiple devices. I'll fix that in v2. > > > > > > Thanks for reviewing! > > > > > Late to the party. DUP *implies* single device but there are cases > > where dup is used on a multi-device fs. Even if the use-cases aren't > > good or intended to be long-term, they are still valid, right? > > You're right, DUP is reported by 'fi df' after 2nd device is added and > this state (even if it's temporary) has to be taken into account. I wouldn't read too much into this, _btrfs_get_profile_configs is just for tests that mkfs a bunch of new filesystems for testing, and in that case, DUP is invalid for multiple devices. It's not meant to be an authoritative source of truth regarding what metadata/data profiles could be legal. Thanks,
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 }
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(-)