Re: [PATCH] mdadm: Check bitmap first when reshape raid1 to raid0

[Neil Brown <neilb@suse.de>]

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

On Mon, Oct 26 2015, Xiao Ni wrote:

>> 
>
> Thanks for printing out the mistake. I checked the nearby code and found 
> I missed close the cfd. Is this I missed? But I'm wondering that it doesn't 
> close it in the following code. Now it's just closed when it's failed to
> reshape raid1/raid10 to raid0.

That is a very sensible question to ask - thanks.
After a quick look - it does seem very strange.  That handling of 'cfd'
and 'fd' seems quite odd.

If you would like to work out what should happen and find a simple way
to fix the code, that would be great.  But I don't insist.

The patch below is quite acceptable as it is a simple solution to a
simple problem and appear consistent with nearby code.  So if you gave
it a proper description, formatted it properly and posted it in a way
that didn't mess up all the white space, I would very likely accept it.

However:
 - maybe we should just remove the bitmap rather than complain if we
   find one.  After all, we are removing other things (extra devices).
   A raid0 can never had a bitmap, so when converting to a raid0, it
   does make sense to remove the bitmap.

 - The test you used only checks for an internal bitmap, not an external
   one.  External bitmaps are hardly ever used except by people who know
   exactly what they are doing, so I'm not too fussed about handling
   them perfectly, but testing for and removing an external bitmap
   might make sense.

Thanks,
NeilBrown


>
>  Grow.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/Grow.c b/Grow.c
> index 80d7b22..d711884 100644
> --- a/Grow.c
> +++ b/Grow.c
> @@ -1898,6 +1898,15 @@ size_change_error:
>              array.layout == ((1 << 8) + 2) && !(array.raid_disks & 1)) ||
>             (s->level == 0 && array.level == 1 && sra)) {
>                 int err;
> +               
> +               if (array.state & (1<<MD_SB_BITMAP_PRESENT)) {
> +                       dprintf("Bitmap must be removed before level can be changed\n");
> +                       if (cfd > -1)
> +                               close(cfd);
> +                       rv = 1;
> +                       goto release;
> +               }
> +
>                 err = remove_disks_for_takeover(st, sra, array.layout);
>
> Best Regards
> Xiao
>
> --
> 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: 818 bytes --]