Ingo Oeser wrote:
>Hi Neil,
>
>I think the names in this patch don't match the description at all.
>May I suggest different ones?
>
>On Monday, 31. July 2006 09:32, NeilBrown wrote:
>
>
>>Instead of magic numbers (0,1,2,3) in sb_dirty, we have
>>some flags instead:
>>MD_CHANGE_DEVS
>> Some device state has changed requiring superblock update
>> on all devices.
>>
>>
>
>MD_SB_STALE or MD_SB_NEED_UPDATE
>
>
I think STALE is better, it is unambigous.
>
>
>>MD_CHANGE_CLEAN
>> The array has transitions from 'clean' to 'dirty' or back,
>> requiring a superblock update on active devices, but possibly
>> not on spares
>>
>>
>
>Maybe split this into MD_SB_DIRTY and MD_SB_CLEAN ?
>
>
I don't think the split is beneficial, but I don't care for the name
much. Some name like SB_UPDATE_NEEDED or the like might be better.
>
>
>>MD_CHANGE_PENDING
>> A superblock update is underway.
>>
>>
>
>MD_SB_PENDING_UPDATE
>
>
>
I would have said UPDATE_PENDING, but either is more descriptive than
the original.
Neil - the logic in this code is pretty complex, all the help you can
give the occasional reader, by using very descriptive names for things,
is helpful to the reader and reduces your "question due to
misunderstanding" load.
--
bill davidsen <[email protected]>
CTO TMR Associates, Inc
Doing interesting things with small computers since 1979
On Tue, 2006-08-01 at 13:12 -0400, Bill Davidsen wrote:
> Ingo Oeser wrote:
>
> >Hi Neil,
> >
> >I think the names in this patch don't match the description at all.
> >May I suggest different ones?
> >
> >On Monday, 31. July 2006 09:32, NeilBrown wrote:
> >
> >
> >>Instead of magic numbers (0,1,2,3) in sb_dirty, we have
> >>some flags instead:
> >>MD_CHANGE_DEVS
> >> Some device state has changed requiring superblock update
> >> on all devices.
> >>
> >>
> >
> >MD_SB_STALE or MD_SB_NEED_UPDATE
> >
> >
> I think STALE is better, it is unambigous.
> >
> >
> >>MD_CHANGE_CLEAN
> >> The array has transitions from 'clean' to 'dirty' or back,
> >> requiring a superblock update on active devices, but possibly
> >> not on spares
> >>
> >>
> >
> >Maybe split this into MD_SB_DIRTY and MD_SB_CLEAN ?
> >
> >
> I don't think the split is beneficial, but I don't care for the name
> much. Some name like SB_UPDATE_NEEDED or the like might be better.
Yeah, no split. The absence of a dirty flag denotes clean.
> >
> >
> >>MD_CHANGE_PENDING
> >> A superblock update is underway.
> >>
> >>
> >
> >MD_SB_PENDING_UPDATE
> >
> >
> >
> I would have said UPDATE_PENDING, but either is more descriptive than
> the original.
>
> Neil - the logic in this code is pretty complex, all the help you can
> give the occasional reader, by using very descriptive names for things,
> is helpful to the reader and reduces your "question due to
> misunderstanding" load.
Well, if you don't mind the length you could do:
MD_SB_WRITEOUT_ALL /* we need to flush all superblocks due
* to a device change. */
MD_SB_WRITEOUT_SOME /* we need to flush the active devices
* superblocks due to normal write operations */
MD_SB_WRITEOUT_PENDING /* the flush is underway */
In trying to come up with descriptive names for this, it really points
out how having both the "I need something" and "I'm handling something"
flags in the same name space sucks.
Anyway, I don't think Neil's original names were that bad, just
obviously the names describe the condition that precipitated the state,
not the current state, implying that a reader of that code should
probably be thinking about what caused the state more than the state
when determining the appropriate course of action. It may be preferable
to leave them that way to push readers towards that sort of analysis,
whatever Neil thinks is best on that.
--
Doug Ledford <[email protected]>
GPG KeyID: CFBFF194
http://people.redhat.com/dledford
Infiniband specific RPMs available at
http://people.redhat.com/dledford/Infiniband