Re: [PATCH v6 03/11] md/r5cache: State machine for raid5-cache write back mode

[NeilBrown <neilb@suse.com>]

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

On Wed, Nov 16 2016, Song Liu wrote:

>> This bothers me.  Why would a stripe *ever* be in "caching mode" (or
>> "caching phase") when the array is in write-through?  It doesn't seem to
>> make sense.
>
> I was thinking about replacing STRIPE_R5C_WRITE_OUT with something
> like STRIPE_R5C_CACHING. So that:
>
> caching-phase is STRIPE_R5C_CACHING == 1
> write-out phase is STRIPE_R5C_CACHING == 0 
>
> In this case, stripes in write-through mode will always have 
> STRIPE_R5C_CACHING == 0. 
>
> This requires some changes to current state machine, but it might work out. 
>
> How do you like this? 

I almost suggested that myself :-)
I'm not against it, but now that I think of "write_out" as an action, it
seems to make sense for that to be a flag.
Before we had the journal we didn't need a flag.  We just assessed the
state of the stripe and then either
 - read some blocks, or
 - calculate parity and write some blocks.

Now that we have the journal, write-out is multi-stage and
"write-to-journal" isn't always followed by 'write to RAID'.  So an extra
flag is needed.
So I'm now happy with WRITE_OUT, but I'd probably be happy with CACHING
too.


>
>
>> There are two actions that can be taken when where are ->towrite blocks
>> on a stripe.  We can enter WRITE_OUT, or they can be cached in the
>> journal.  Also we can enter WRITE_OUT when a stripe needs to be removed
>> From memory or from the journal.
>> This makes "writeout" and "cache" seem more like "actions" than states,
>> modes, or phases.  Naming is hard.
>
> Yes, it is more like action. We used to name it as "modes" as different *mode*
> handles writes with different *action*. So at end of the day, it doesn't really 
> matter?

The exact choice of word isn't vital but it is important to ensure the
code is maintainable, and that means it must be easy to understand.  The
fact that we have had trouble describing what is happening suggests that
this is an area that needs to be clearly explained.
So where the flag is declared, it would help a lot to document:

 /*  set when ...
  *  cleared when ...
  *  used for ....
  */
because whatever name we use for the flag, it won't by itself be
enough.  We need to also explain what it means.

Thanks,
NeilBrown


>
> Thanks,
> Song

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