Re: [PATCH v3 5/5] md/raid10: convert resync_lock to use seqlock

[Song Liu]

On Mon, Sep 19, 2022 at 3:28 AM Guoqing Jiang <guoqing.jiang@xxxxxxxxx> wrote:
>
>
>
> On 9/19/22 9:08 AM, Yu Kuai wrote:
> > Hi,
> >
> > 在 2022/09/18 19:36, Guoqing Jiang 写道:
> >>
> >>
> >> On 9/16/22 7:34 PM, Yu Kuai wrote:
> >>> From: Yu Kuai <yukuai3@xxxxxxxxxx>
> >>>
> >>> Currently, wait_barrier() will hold 'resync_lock' to read
> >>> 'conf->barrier',
> >>> and io can't be dispatched until 'barrier' is dropped.
> >>>
> >>> Since holding the 'barrier' is not common, convert 'resync_lock' to use
> >>> seqlock so that holding lock can be avoided in fast path.
> >>>
> >>> Signed-off-by: Yu Kuai <yukuai3@xxxxxxxxxx>
> >>> ---
> >>>   drivers/md/raid10.c | 87
> >>> ++++++++++++++++++++++++++++++---------------
> >>>   drivers/md/raid10.h |  2 +-
> >>>   2 files changed, 59 insertions(+), 30 deletions(-)
> >>>
> >>> diff --git a/drivers/md/raid10.c b/drivers/md/raid10.c
> >>> index 9a28abd19709..2daa7d57034c 100644
> >>> --- a/drivers/md/raid10.c
> >>> +++ b/drivers/md/raid10.c
> >>> @@ -79,6 +79,21 @@ static void end_reshape(struct r10conf *conf);
> >>>   #include "raid1-10.c"
> >>> +#define NULL_CMD
> >>> +#define cmd_before(conf, cmd) \
> >>> +    do { \
> >>> +        write_sequnlock_irq(&(conf)->resync_lock); \
> >>> +        cmd; \
> >>> +    } while (0)
> >>> +#define cmd_after(conf) write_seqlock_irq(&(conf)->resync_lock)
> >>
> >> The two is not paired well given only cmd_before needs the 'cmd'.
> >
> > May be should I just remove cmd_after?
>
> I'd remove it but just my personal flavor.
>
> >>
> >>> +
> >>> +#define wait_event_barrier_cmd(conf, cond, cmd) \
> >>> +    wait_event_cmd((conf)->wait_barrier, cond, cmd_before(conf,
> >>> cmd), \
> >>> +               cmd_after(conf))
> >>> +
> >>> +#define wait_event_barrier(conf, cond) \
> >>> +    wait_event_barrier_cmd(conf, cond, NULL_CMD)
> >>
> >> What is the issue without define NULL_CMD?
> >>
> >
> > Checkpatch will complain this:
> >
> > ERROR: space prohibited before that close parenthesis ')'
> > #38: FILE: drivers/md/raid10.c:94:
> > +       wait_event_barrier_cmd(conf, cond, )
> >
> > total: 1 errors, 0 warnings, 169 lines checked
>
> Hmm, freeze_array has a different usage for it, so two cmds before sleep
> and one cmd after sleep, perhaps it is the best way for now.

Current version looks good to me. Please let me know if there is more
concern.

Applied the set to md-next. Thanks!

Song