> On Nov 15, 2016, at 1:49 PM, Shaohua Li <shli@xxxxxx> wrote:
>
> On Tue, Nov 15, 2016 at 07:08:29PM +0000, Song Liu wrote:
>
>>>>
>>>> might_sleep();
>>>>
>>>> - if (r5l_write_stripe(conf->log, sh) == 0)
>>>> - return;
>>>> + if (test_bit(STRIPE_R5C_WRITE_OUT, &sh->state)) {
>>>> + /* writing out mode */
>>>> + if (r5l_write_stripe(conf->log, sh) == 0)
>>>> + return;
>>>> + } else {
>>>> + /* caching mode */
>>>> + if (test_bit(STRIPE_LOG_TRAPPED, &sh->state)) {
>>>
>>> The STRIPE_LOG_TRAPPED bit isn't designed for this, maybe it just works, I have
>>> to double check. but this one is a little confusing.
>>
>> This is somehow reuse the flag in caching mode/phase. In handle_stripe(), a large part of the
>> logic is skipped with STRIPE_LOG_TRAPPED. This behavior is the same in caching mode/phase
>> and write_out mode/phase.
>
> I'd think this is another abuse of the bit. Both writeout mode and the caching
> mode could set the bit. Here you are using the bit to determine if entering
> caching mode. This isn't clear at all to me. I'd add another bit to indicate
> the caching mode if necessary.
I guess the comment made it confusing... STRIPE_LOG_TRAPPED is not
used to determine where it is in caching phase. The /* caching phase */ is
for the "else" statement:
if (test_bit(STRIPE_R5C_WRITE_OUT, &sh->state)) {
/* writing out phase */
if (r5l_write_stripe(conf->log, sh) == 0)
return;
} else { /* caching phase i.e. STRIPE_R5C_WRITE_OUT == 0 */
if (test_bit(STRIPE_LOG_TRAPPED, &sh->state)) {
r5c_cache_data(conf->log, sh, s);
return;
}
}
In caching phase, STRIPE_LOG_TRAPPED is used as a gate of
r5c_cache_data(). It is set in r5c_handle_dirtying() and cleared in
endio of journal writes. It is kind similar to the name "log trapped"
meaning it is time to write data to the log.
>
>>>> @@ -1547,6 +1611,7 @@ ops_run_biodrain(struct stripe_head *sh, struct dma_async_tx_descriptor *tx)
>>>>
>>>> again:
>>>> dev = &sh->dev[i];
>>>> + clear_bit(R5_InJournal, &dev->flags);
>>>
>>> why clear the bit here? isn't this bit cleared when the stripe is initialized?
>>
>> This is necessary when we rewrite a page that is already in journal. This means there is new data coming to
>> this stripe and dev, so we should not consider the page is secured in journal.
>
> This sounds suggest we should clear the bit after writeout is done.
That may also work. Let me confirm.
Thanks,
Song--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
