Re: mdadm: Patch to restrict --size when shrinking unless forced

[NeilBrown <neilb@suse.com>]

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

On Sun, Oct 08 2017, John Stoffel wrote:

>>>>>> "NeilBrown" == NeilBrown  <neilb@suse.com> writes:
>
> NeilBrown> On Wed, Oct 04 2017, John Stoffel wrote:
>>> Since Eli had such a horrible experience where he shrunk the
>>> individual component raid device size, instead of growing the overall
>>> raid by adding a device, I came up with this hacky patch to warn you
>>> when you are about to shoot yourself in the foot.
>>> 
>>> The idea is it will warn you and exit unless you pass in the --force
>>> (or -f) switch when using the command.  For example, on a set of loop
>>> devices:
>>> 
>>> # cat /proc/mdstat
>>> Personalities : [linear] [raid0] [raid1] [raid10] [raid6] [raid5]
>>> [raid4] [multipath] [faulty]
>>> md99 : active raid6 loop4p1[4] loop3p1[3] loop2p1[2] loop1p1[1]
>>> loop0p1[0]
>>> 606720 blocks super 1.2 level 6, 512k chunk, algorithm 2 [5/5]
>>> [UUUUU]
>>> 
>>> # ./mdadm --grow /dev/md99 --size 128
>>> mdadm: Cannot set device size smaller than current component_size of /dev/md99 array.  Use -f to force change.
>>> 
>>> # ./mdadm --grow /dev/md99 --size 128 -f
>>> mdadm: component size of /dev/md99 has been set to 0K
>>> 
>
> NeilBrown> I'm not sure I like this.
> NeilBrown> The reason that mdadm will quietly accept a size change like this is
> NeilBrown> that it is trivial to revert - just set the same to a big number and all
> NeilBrown> your data is still there.
>
> This is wrong, because if you use --grow --size ### with a small
> enough number, it destroys the MD raid superblock.

If that is true, then it is a kernel bug and should be fixed in the kernel.

>  So again, I think
> the --force option is *critical* here.  Or we need to block the size
> change from going smaller than the superblock size.  Here's my test,
> where I just warn if the size is going to be smaller:
>
>     # ./mdadm --grow /dev/md99 --size 128
>     mdadm: setting raid component device size from 202240 to 128 in array /dev/md99,
>     this may need to be reverted if new size is smaller.
>     mdadm: component size of /dev/md99 has been set to 0K
>
>     # ./mdadm --grow /dev/md99 --size 202240
>     mdadm: setting raid component device size from 0 to 202240 in array /dev/md99,
>     this may need to be reverted if new size is smaller.
>     mdadm: Cannot set device size in this type of array.
>
>     # mdadm -E /dev/md99
>     mdadm: No md superblock detected on /dev/md99.
>
> So I think this argues for a much stronger check, and/or the --force
> option when shrinking.  I'll re-spin my patch series into two chunks,
> one just the message if changing size.  The second to require the
> --force option.

Why don't you like my suggestion that you should need to reduce the
--array-size first?

Thanks,
NeilBrown

>
> And I think we need a third option to make sure the size can't be
> smaller than the array superblock size as well.  Otherwise a simple
> mistake trashes your array.
>
> My current warning only patch (with whitespace damage...)
>
>> git diff
> diff --git a/Grow.c b/Grow.c
> index 455c5f9..18aea63 100755
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1625,6 +1625,10 @@ int Grow_reshape(char *devname, int fd,
>                 return 1;
> 		        }
>
> +       if (s->size != (unsigned)array.size) {
> +               pr_err("setting raid component device size from %u to %llu in array %s,\nthis may need to be reverted if new size is smaller.\n",(unsigned)array.size,s->size,devname);
> +       }
> +
>         st = super_by_fd(fd, &subarray);
> 	        if (!st) {
> 		                pr_err("Unable to determine metadata format for %s\n", devname);
> 				
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]