linux-kernel - Re: [PATCH v3 5/5] md/raid10: convert resync

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CAPhsuW50E0zwrV0axpVA18qf6fRRQqNAZ1gqYQcqXinEPjS-2A@mail.gmail.com>
Date:   Mon, 19 Sep 2022 12:08:05 -0700
From:   Song Liu <song@...nel.org>
To:     Guoqing Jiang <guoqing.jiang@...ux.dev>
Cc:     Yu Kuai <yukuai1@...weicloud.com>,
        Logan Gunthorpe <logang@...tatee.com>,
        Paul Menzel <pmenzel@...gen.mpg.de>,
        linux-raid <linux-raid@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>, yi.zhang@...wei.com,
        "yukuai3@...wei.com >> yukuai (C)" <yukuai3@...wei.com>
Subject: Re: [PATCH v3 5/5] md/raid10: convert resync_lock to use seqlock

On Mon, Sep 19, 2022 at 3:28 AM Guoqing Jiang <guoqing.jiang@...ux.dev> 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@...wei.com>
> >>>
> >>> 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@...wei.com>
> >>> ---
> >>>   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