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

[NeilBrown <neilb@suse.de>]

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

On Tue, 8 Nov 2011 18:23:49 +0200 Alexander Lyakas <alex.bolshoy@gmail.com>
wrote:

> Hi Neil,
> 
> 1)
> after looking at this more I see the following: on ADD_NEW_DISK
> sequence (to a running array, not during assembly) in the kernel, the
> desc_nr is loaded from the superblock of the disk to be added in
> super_1_load():
> 	if (sb->level == cpu_to_le32(LEVEL_MULTIPATH))
> 		rdev->desc_nr = -1;
> 	else
> 		rdev->desc_nr = le32_to_cpu(sb->dev_number);
> And later, in bind_rdev_to_array() it is indeed checked that desc_nr is unique.
> 
> However, at least for 1.2 arrays, I believe this is too restrictive,
> don't you think? If the raid slot (not desc_nr) of the device being
> re-added is *not occupied* yet, can't we just select a free desc_nr
> for the new disk on that path?
> Or perhaps, mdadm on the re-add path can select a free desc_nr
> (disc.number) for it (just as it does for --add), after ensuring that
> the slot is not occupied yet? Where it is better to do it?
> Otherwise, the re-add fails, while it can perfectly succeed (only pick
> a different desc_nr).

I think I see what you are saying.
However my question is: is this really an issue.
Is there a credible sequence of events that results in the current code makes
an undesirable decision?  Of course I do not count deliberately editing the
metadata as part of a credible sequence of events.


> 
> 2)
> About your suggestion of setting the In_sync flag via sysfs, I think
> it's too dangerous to blindly set it, so I am not going to do it. Let
> the kernel look at event count and decide.
> 
> 3)
> So it looks like at present, my best way to add (not re-add) a disk
> into a specific slot is the following:
> - borrow some code from mdadm to initialize the superblock of the new
> disk, as belonging to this array (init_super).
> - use the sysfs to do what you suggested before:
> echo frozen > sync_action
> echo $major:$minor > new_dev
> echo $slot > dev-$DEVNAME/slot
> echo idle > sync_action
> 
> Does this makes sense?

Yes it does.

NeilBrown



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