Don't prevent removal of devices that break raid reqs

Don't prevent removal of devices that break raid reqs

[Alexandre Oliva]

Instead of preventing the removal of devices that would render existing
raid10 or raid1 impossible, warn but go ahead with it; the rebalancing
code is smart enough to use different block group types.

Should the refusal remain, so that we'd only proceed with a
newly-introduced --force option or so?

Signed-off-by: Alexandre Oliva <oliva@lsd.ic.unicamp.br>
---
 fs/btrfs/volumes.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4d5b29f..507afca 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1281,18 +1281,14 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID10) &&
 	    root->fs_info->fs_devices->num_devices <= 4) {
-		printk(KERN_ERR "btrfs: unable to go below four devices "
-		       "on raid10\n");
-		ret = -EINVAL;
-		goto out;
+		printk(KERN_ERR "btrfs: going below four devices "
+		       "will turn raid10 into raid1\n");
 	}
 
 	if ((all_avail & BTRFS_BLOCK_GROUP_RAID1) &&
 	    root->fs_info->fs_devices->num_devices <= 2) {
-		printk(KERN_ERR "btrfs: unable to go below two "
-		       "devices on raid1\n");
-		ret = -EINVAL;
-		goto out;
+		printk(KERN_ERR "btrfs: going below two devices "
+		       "will lose raid1 redundancy\n");
 	}
 
 	if (strcmp(device_path, "missing") == 0) {
-- 
1.7.4.4


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: Don't prevent removal of devices that break raid reqs

[Chris Mason]

On Thu, Nov 10, 2011 at 05:32:48PM -0200, Alexandre Oliva wrote:
> Instead of preventing the removal of devices that would render existing
> raid10 or raid1 impossible, warn but go ahead with it; the rebalancing
> code is smart enough to use different block group types.
> 
> Should the refusal remain, so that we'd only proceed with a
> newly-introduced --force option or so?

Hmm, going to three devices on raid10 doesn't turn it into
raid1.  It turns it into a degraded raid10.

We'll need a --force or some kind.  There are definitely cases users
have wanted to do this but it is rarely a good idea ;)

-chris

> 
> Signed-off-by: Alexandre Oliva <oliva@lsd.ic.unicamp.br>
> ---
>  fs/btrfs/volumes.c |   12 ++++--------
>  1 files changed, 4 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 4d5b29f..507afca 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1281,18 +1281,14 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
>  
>  	if ((all_avail & BTRFS_BLOCK_GROUP_RAID10) &&
>  	    root->fs_info->fs_devices->num_devices <= 4) {
> -		printk(KERN_ERR "btrfs: unable to go below four devices "
> -		       "on raid10\n");
> -		ret = -EINVAL;
> -		goto out;
> +		printk(KERN_ERR "btrfs: going below four devices "
> +		       "will turn raid10 into raid1\n");
>  	}
>  
>  	if ((all_avail & BTRFS_BLOCK_GROUP_RAID1) &&
>  	    root->fs_info->fs_devices->num_devices <= 2) {
> -		printk(KERN_ERR "btrfs: unable to go below two "
> -		       "devices on raid1\n");
> -		ret = -EINVAL;
> -		goto out;
> +		printk(KERN_ERR "btrfs: going below two devices "
> +		       "will lose raid1 redundancy\n");
>  	}
>  
>  	if (strcmp(device_path, "missing") == 0) {
> -- 
> 1.7.4.4
> 
> 
> -- 
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist      Red Hat Brazil Compiler Engineer
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Don't prevent removal of devices that break raid reqs

[Ilya Dryomov]

On Thu, Nov 10, 2011 at 09:21:00PM -0500, Chris Mason wrote:
> On Thu, Nov 10, 2011 at 05:32:48PM -0200, Alexandre Oliva wrote:
> > Instead of preventing the removal of devices that would render existing
> > raid10 or raid1 impossible, warn but go ahead with it; the rebalancing
> > code is smart enough to use different block group types.
> > 
> > Should the refusal remain, so that we'd only proceed with a
> > newly-introduced --force option or so?
> 
> Hmm, going to three devices on raid10 doesn't turn it into
> raid1.  It turns it into a degraded raid10.
> 
> We'll need a --force or some kind.  There are definitely cases users
> have wanted to do this but it is rarely a good idea ;)

I'm not sure about use cases Chris talks about, but sans those I think
we should prevent breaking raids.  If user wants to downgrade his FS he
can do that explicitly with restriper.  As for the relocation code
'smartness', we already have a confusing case where balancing silently
upgrades single to raid0.

Chris, can you describe those cases in detail so I can integrate and
align this whole thing with restriper before it's merged ?  (I added a
--force option for some of the transitions, probably best not to add
another closely related one)

Thanks,

		Ilya

Re: Don't prevent removal of devices that break raid reqs

[Chris Mason]

On Tue, Nov 15, 2011 at 11:37:13AM +0200, Ilya Dryomov wrote:
> On Thu, Nov 10, 2011 at 09:21:00PM -0500, Chris Mason wrote:
> > On Thu, Nov 10, 2011 at 05:32:48PM -0200, Alexandre Oliva wrote:
> > > Instead of preventing the removal of devices that would render existing
> > > raid10 or raid1 impossible, warn but go ahead with it; the rebalancing
> > > code is smart enough to use different block group types.
> > > 
> > > Should the refusal remain, so that we'd only proceed with a
> > > newly-introduced --force option or so?
> > 
> > Hmm, going to three devices on raid10 doesn't turn it into
> > raid1.  It turns it into a degraded raid10.
> > 
> > We'll need a --force or some kind.  There are definitely cases users
> > have wanted to do this but it is rarely a good idea ;)
> 
> I'm not sure about use cases Chris talks about, but sans those I think
> we should prevent breaking raids.  If user wants to downgrade his FS he
> can do that explicitly with restriper.  As for the relocation code
> 'smartness', we already have a confusing case where balancing silently
> upgrades single to raid0.
> 
> Chris, can you describe those cases in detail so I can integrate and
> align this whole thing with restriper before it's merged ?  (I added a
> --force option for some of the transitions, probably best not to add
> another closely related one)

There are a few valid use cases where people want to be able to "break"
a raid1.  I'd put it at the very bottom of the list of interesting
things, just because I see a long list of bug reports that start with:

my FS was broken, so I told it to remove device xxyzzz, and that didn't
work so I ran --force, and then (sad story follows).

-chris

Re: Don't prevent removal of devices that break raid reqs

[Alexandre Oliva]

On Nov 15, 2011, Ilya Dryomov <idryomov@gmail.com> wrote:

> On Thu, Nov 10, 2011 at 09:21:00PM -0500, Chris Mason wrote:
>> On Thu, Nov 10, 2011 at 05:32:48PM -0200, Alexandre Oliva wrote:
>> > Instead of preventing the removal of devices that would render existing
>> > raid10 or raid1 impossible, warn but go ahead with it; the rebalancing
>> > code is smart enough to use different block group types.
>> > 
>> > Should the refusal remain, so that we'd only proceed with a
>> > newly-introduced --force option or so?
>> 
>> Hmm, going to three devices on raid10 doesn't turn it into
>> raid1.  It turns it into a degraded raid10.

Oops.  I *knew* there was some case I had failed to test.  I thought I'd
the two relevant cases, but I must have tested only raid0 to single and
raid1 to dup.

> If user wants to downgrade his FS he
> can do that explicitly with restriper.

Didn't I just read in another thread that restriper won't let one switch
from raid1 to dup without going through single in between?  That's what
I actually needed to do that drove me to write the patch a while ago
after accidentally adding a device to the wrong filesystem, and if
restriper won't do it without going through a riskier stage in between
(metadata loss due to e.g. a bad block), I wish it was improved so that
this is possible.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: Don't prevent removal of devices that break raid reqs

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 561 bytes --]

On Nov 11, 2011, Chris Mason <chris.mason@oracle.com> wrote:

> On Thu, Nov 10, 2011 at 05:32:48PM -0200, Alexandre Oliva wrote:
>> Instead of preventing the removal of devices that would render existing
>> raid10 or raid1 impossible, warn but go ahead with it; the rebalancing
>> code is smart enough to use different block group types.

> We'll need a --force or some kind.  There are definitely cases users
> have wanted to do this but it is rarely a good idea ;)

Even if it's just metadata that will turn from raid1 to dup, as in the
revised patch below?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Btrfs-enable-removal-of-second-disk-with-raid1-metad.patch --]
[-- Type: text/x-diff, Size: 1324 bytes --]

>From 276b1af70556bf5bdbaa1f81cb630d6c83962323 Mon Sep 17 00:00:00 2001
From: Alexandre Oliva <lxoliva@fsfla.org>
Date: Tue, 8 Nov 2011 12:33:11 -0200
Subject: [PATCH 1/8] Btrfs: enable removal of second disk with raid1 metadata

Enable removal of a second disk even if that requires conversion of
metadata from raid1 to dup, but not when data would lose replication.

Signed-off-by: Alexandre Oliva <oliva@lsd.ic.unicamp.br>
---
 fs/btrfs/volumes.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index c37433d..7b348c2 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1290,12 +1290,16 @@ int btrfs_rm_device(struct btrfs_root *root, char *device_path)
 		goto out;
 	}
 
-	if ((all_avail & BTRFS_BLOCK_GROUP_RAID1) &&
+	if ((root->fs_info->avail_data_alloc_bits & BTRFS_BLOCK_GROUP_RAID1) &&
 	    root->fs_info->fs_devices->num_devices <= 2) {
 		printk(KERN_ERR "btrfs: unable to go below two "
 		       "devices on raid1\n");
 		ret = -EINVAL;
 		goto out;
+	} else if ((all_avail & BTRFS_BLOCK_GROUP_RAID1) &&
+		   root->fs_info->fs_devices->num_devices <= 2) {
+		printk(KERN_ERR "btrfs: going below two devices "
+		       "will switch metadata from raid1 to dup\n");
 	}
 
 	if (strcmp(device_path, "missing") == 0) {
-- 
1.7.4.4


[-- Attachment #3: Type: text/plain, Size: 258 bytes --]



-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: Don't prevent removal of devices that break raid reqs

[Phillip Susi]

On 11/10/2011 2:32 PM, Alexandre Oliva wrote:
> Instead of preventing the removal of devices that would render existing
> raid10 or raid1 impossible, warn but go ahead with it; the rebalancing
> code is smart enough to use different block group types.
>
> Should the refusal remain, so that we'd only proceed with a
> newly-introduced --force option or so?

I just thought of something.  When adding the second device, balance 
converts DUP to RAID1 automatically, and it is the RAID1 that prevents 
removing the second disk.  What if the chunks were left with both the 
DUP and RAID1 flags set?  That way if you explicitly requested raid1, 
then it won't let you accidentally drop below two disks, but if it were 
auto promoted from DUP, then going back to DUP is ok.