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

[NeilBrown <neilb@suse.de>]

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

On Tue, 1 Nov 2011 18:26:14 +0200 Alexander Lyakas <alex.bolshoy@gmail.com>
wrote:

> Hi Neil,
> I hope you don't find my questions too pesky; I hope as time goes, I
> will be able to contribute as well, and not just ask for information.

Enquiring minds should always be encouraged!

> 
> What you suggested works great, but only if the drive already has a
> valid superblock. This is according to md_import_device(), which calls
> super_1_load(). Although it calls it with NULL refdev, but it still
> performs basic validity checks of the superblock. So it is needed to
> have functionality of write_init_super() of mdadm (perhaps can be an
> mdadm operation, not sure how dangerous this is to expose such
> functionality).

Correct.  With v1.x metadata - and even more so with DDF and IMSM - the
kernel does not know everything about the metadata and cannot create new
superlocks.  At best it can edit what is there.

As for adding things to mdadm -  you just need a genuine use-case.
I can see it could be valid to add a '--role' option to --add so you can add
a device to a specific slot.  I doubt it would be used much, but if there was
a concrete need I wouldn't object.


> 
> Another question I was digging for, is to find a spot where kernel
> determines whether it is going to do a bitmap-based reconstruction or
> a full reconstruction. Can you verify that I found it correctly, or I
> am way off?
> 
> In super_1_validate():
> 		if (ev1 < mddev->bitmap->events_cleared)
> 			return 0;
> This means that the array bitmap was cleared after the drive was
> detached, so we bail right out, and do not set rdev->raid_disk, and
> leave all flags off. In that case, eventually, we will need full
> reconstruction.
> Otherwise, after we decide that this drive is an active device (not
> failed or spare):
> if ((le32_to_cpu(sb->feature_map) &  MD_FEATURE_RECOVERY_OFFSET))
>     rdev->recovery_offset = le64_to_cpu(sb->recovery_offset);
> else
>     set_bit(In_sync, &rdev->flags);
> Here we might or might not set the In_sync flag.
> 

Yes. 'In_sync' effectively means that recovery_offset == MAX.


> Then in slot_store() and in add_new_disk(), we set the important flag
> for that matter:
> if (test_bit(In_sync, &rdev->flags))
>     rdev->saved_raid_disk = slot;
> else
>     rdev->saved_raid_disk = -1;
> 
> And later, in hot_add_disk (like raid5_add_disk, raid1_add_disk), we
> check this flag and set/not set the fullsync flag. And in
> raid1_add_disk, like you said, the slot doesn't matter, any valid
> saved_raid_disk is good.
> 
> Is this correct? If yes, then in the sequence you suggested, we will
> always do full reconstruction. Because new_dev_store() and
> slot_store() sequence do not call validate_super(), so In_sync is
> never set, so saved_raid_disk remains -1. This is perfectly fine.

Yes, this is all correct.
In the sequence I suggested you could add
   echo insync > dev-$DEVNAME/state
before
   echo $slot > dev-$DEVNAME/slot

and it would result in saved_raid_disk being set.

NeilBrown


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