----- 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 1:41:50 PM
> Subject: Re: [PATCH 1/1] raid5-cache: rename r5l_flush_stripe_to_raid to r5l_flush_stripe_to_journal
>
>
>
> > 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?
It makes sense. Thanks for the explanation.
Regards
Xiao
>
> 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
> >>>
> >>
> >>
>
>
