Re: [PATCH 4/6] md: don't export log device

[Neil Brown <neilb@suse.de>]

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

Shaohua Li <shli@fb.com> writes:

> On Thu, Oct 08, 2015 at 03:16:41PM +1100, Neil Brown wrote:
>> Shaohua Li <shli@fb.com> writes:
>> >> 
>> >> Neither of these chunks should be needed.
>> >> ->raid_disk of an active devices is only set to -1 if ->hot_remove_disk
>> >> succeeds.
>> >> You have make ->hot_remove_disk fail for Journal devices, so ->raid_disk
>> >> will be >= 0.
>> >
>> > I agree the raid5_remove_disk part is superficial, I fixed in an updated
>> > patch. I still didn't get the point what can prevent a journal disk is
>> > removed. Currently the raid_disk is always -1 for journal disk. If it
>> > should be >=0, what value it should be? We give journal disk a special
>> > role '0xfffd' currently.
>> 
>> Oh, are we leaving the ->raid_disk at -1 for the journal?  I hadn't
>> noticed that.  I don't feel comfortable it.  Too much code assumes that
>> <0 means "not in use".
>> 
>> Probably set it to 0, and add a check to setup_conf(), and adjust the
>> check in run().  md_update_sb() probably need to be careful of journals
>> too (to not change ->recovery_offset).
>> I wonder what 'slot_show' should report for the journal.... maybe
>> "journal"??
>
> ->raid_disk >= 0 is for normal raid disks. If we use it, we will have
> two disks with ->raid_disk 0, it sounds weird. Currently we add the
> 'test(Journal, rdev->flags)' check in different places to destinguish
> journal disk. We will need to audit the code which assumes ' < 0 means
> not in use'. We will probably need to audit the same code if we set
> ->raid_disk 0 for journal. Neither is perfect.

Having two disks with ->raid_disk==0 does seem a little weird, but we do
already have that in some cases.
When you have a hot-replace going, both the original and the replacement
have the same ->raid_disk numbers.  They can be distinguished by the
Replacement flag.
I'm suggesting the same (sort of) for journals, and distinguish by the
Journal flag.

I did quick audit and just found setup_conf, run() and md_update_sb().
If you could do an audit to that would be good.  I'd be surprised if you
find many more places where Journal needs to be tested with ->raid_disk.

Thanks,
NeilBrown

>
> Thanks,
> Shaohua

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