Re: Question about mdadm commit d6508f0cfb60edf07b36f1532eae4d9cddf7178b "be more careful about add attempts"

[NeilBrown <neilb@suse.de>]

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

On Wed, 26 Oct 2011 19:02:37 +0200 Alexander Lyakas <alex.bolshoy@gmail.com>
wrote:

> Greetings everybody,
> I have a question about the following code in Manage.c:Manage_subdevs()
> 
> disc.number = mdi.disk.number;
> if (ioctl(fd, GET_DISK_INFO, &disc) != 0
>     || disc.major != 0 || disc.minor != 0
>     || !enough_fd(fd))
>     goto skip_re_add;
> 
> I do not underatand why the checks: disc.major != 0 || disc.minor != 0
> are required. This basically means that the kernel already has an
> rdev->desc_nr equal to disc.number. But why fail the re-add procedure?
> 
> Let's say that enough_fd() returns true, and we go ahead an issue
> ioctl(ADD_NEW_DISK). In this case, according to the kernel code in
> add_new_disk(), it will not even look at info->number. It will
> initialize rdev->desc_nr to -1, and will allocate a free desc_nr for
> the rdev later.
> 
> Doing this with mdadm 3.1.4, where this check is not present, actually
> succeeds. I understand that this code was added for cases when
> enough_fd() returns false, which sounds perfectly fine to protect
> from.
> 
> I was thinking that this code should actually check something like:
> if (ioctl(fd, GET_DISK_INFO, &disc) != 0
>     || disk.raid_disk != mdi.disk.raid_disk
>     || !enough_fd(fd))
>     goto skip_re_add;
> 
> That is to check that the slot that was being occupied by the drive we
> are trying to add, is already occupied by a different drive (need also
> to cover cases of raid_disk <0, raid_disk >= raid_disks etc...) and
> not the desc_nr, which does not have any persistent meaning.
> 
> Perhaps there are some compatibility issues with old kernels? Or
> special considerations for ... containers? non-persistent arrays?

The point of this code is to make --re-add fail unless mdadm is certain that
the kernel will accept the re-add, rather than turn the device into a spare.

If a device already exists with the same disk.number, a re-add cannot
succeed, so mdadm doesn't even try.

When you say in 3.1.4 it "actually succeeds" - what succeeds?  Does it re-add
the device to the array, or does it turn the device into a spare?
I particularly do not want --re-add to turn a device into a spare because
people sometimes use it in cases where it cannot work, their device gets
turned into a spare, and they lose information that could have been used to
reconstruct the array.

That that make sense?

NeilBrown


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