> On May 10, 2019, at 5:05 PM, Xiao Ni <xni@xxxxxxxxxx> wrote:
>
>
>
> ----- Original Message -----
>> From: "Song Liu" <songliubraving@xxxxxx>
>> To: "Xiao Ni" <xni@xxxxxxxxxx>
>> Cc: "linux-raid" <linux-raid@xxxxxxxxxxxxxxx>, heinzm@xxxxxxxxxx, ncroxon@xxxxxxxxxx
>> Sent: Saturday, May 11, 2019 12:35:02 AM
>> Subject: Re: [PATCH 1/1] raid5-cache: rename r5l_flush_stripe_to_raid to r5l_flush_stripe_to_journal
>>
>>
>>
>>> On May 10, 2019, at 12:45 AM, Xiao Ni <xni@xxxxxxxxxx> wrote:
>>>
>>> When journal device supports volatile write cache, it needs to flush to
>>> make sure data is settled
>>> down in journal device. It's the usage of function
>>> r5l_flush_stripe_to_raid. The data is flushed
>>> from stripe cache to journal device. Rename the function name to make it
>>> more proper.
>>
>> I think current name is more accurate. It is actually the beginning of
>> writing data to raid
>> disks. While it does flush the journal device, it also calls
>> r5l_log_flush_endio(), which
>> kicks off the write to raid disks.
>>
>> Does this make sense?
>
> The write bio writes to journal device first. In the callback r5l_log_endio it
> checks whether it needs to flush the data to journal device. If it needs to flush,
> it calls r5l_move_to_end_ios which moves io_units to log->io_end_ios. Then
> r5l_flush_stripe_to_raid submit the flush bio. If it doesn't need to flush, it
> calls r5l_log_run_stripes->r5l_io_run_stripes to kick off write to raid disks.
> In fact r5l_io_run_stripes is the function which kicks off write to raid disks.
>
> So there are two steps here. One is write data to journal device. It needs to flush
> data to journal device in this step. Two is write data to raid disks by state machine.
> I still think it's better to change the name to r5l_flush_stripe_to_journal to describe
> the thing what it does in this function directly. Even though it calls handle_stripe
> in the callback function.
>
> At first reading of this function, I searched where are the bios that are submitted to
> the raid disks. Then I found it calls handle_stripe in the callback function. It's a little
> confusing. It's the reason I want to rename it. But it depends what people understand.
> So it's ok for me if we use r5l_flush_stripe_to_raid :)
This function is called to finish processing pending stripes (at the end of raid5d,
and raid5worker). So the goal is to flush all data to raid disks. If we change it
to flush journal, it will be confusing from the caller side: why only flush journal?
shouldn't we flush raid disks as well?
Let's keep it as-is for now.
Thanks,
Song
>
> Regards
> Xiao
>
>>
>> Thanks,
>> Song
>>
>>>
>>> Signed-off-by: Xiao Ni <xni@xxxxxxxxxx>
>>> ---
>>> drivers/md/raid5-cache.c | 2 +-
>>> drivers/md/raid5.c | 6 +++---
>>> 2 files changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/md/raid5-cache.c b/drivers/md/raid5-cache.c
>>> index cbbe6b6..689a59e 100644
>>> --- a/drivers/md/raid5-cache.c
>>> +++ b/drivers/md/raid5-cache.c
>>> @@ -1294,7 +1294,7 @@ static void r5l_log_flush_endio(struct bio *bio)
>>> * only write stripes of an io_unit to raid disks till the io_unit is the
>>> first
>>> * one whose data/parity is in log.
>>> */
>>> -void r5l_flush_stripe_to_raid(struct r5l_log *log)
>>> +void r5l_flush_stripe_to_journal(struct r5l_log *log)
>>> {
>>> bool do_flush;
>>>
>>> diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
>>> index 7fde645..56d9e6e 100644
>>> --- a/drivers/md/raid5.c
>>> +++ b/drivers/md/raid5.c
>>> @@ -6206,7 +6206,7 @@ static int handle_active_stripes(struct r5conf *conf,
>>> int group,
>>> release_inactive_stripe_list(conf, temp_inactive_list,
>>> NR_STRIPE_HASH_LOCKS);
>>>
>>> - r5l_flush_stripe_to_raid(conf->log);
>>> + r5l_flush_stripe_to_journal(conf->log);
>>> if (release_inactive) {
>>> spin_lock_irq(&conf->device_lock);
>>> return 0;
>>> @@ -6262,7 +6262,7 @@ static void raid5_do_work(struct work_struct *work)
>>>
>>> flush_deferred_bios(conf);
>>>
>>> - r5l_flush_stripe_to_raid(conf->log);
>>> + r5l_flush_stripe_to_journal(conf->log);
>>>
>>> async_tx_issue_pending_all();
>>> blk_finish_plug(&plug);
>>> @@ -6349,7 +6349,7 @@ static void raid5d(struct md_thread *thread)
>>>
>>> flush_deferred_bios(conf);
>>>
>>> - r5l_flush_stripe_to_raid(conf->log);
>>> + r5l_flush_stripe_to_journal(conf->log);
>>>
>>> async_tx_issue_pending_all();
>>> blk_finish_plug(&plug);
>>> --
>>> 2.7.5
>>>
>>
>>
