Received-SPF: pass (google.com: domain of linux-kernel+bounces-9353-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
References: <20231221104343.5557-1-2045gemini@gmail.com> <abc324aa-1ccc-c8fd-1437-a77465f6e4be@huaweicloud.com>
 <CAOPYjvbfGZObUa+P5Bo_syLMpyMNEPU6SNm6xJPSqSZYREmNfw@mail.gmail.com>
 <CAOPYjvYhEzeF3vdd9GXCX+k_-OmsE1yP7VNozcMt4vOyFLDAfw@mail.gmail.com> <ae9fc764-4765-8c13-8d59-56bc8582c094@huaweicloud.com>
In-Reply-To: <ae9fc764-4765-8c13-8d59-56bc8582c094@huaweicloud.com>
From: 20 39 <2045gemini@gmail.com>
Date: Fri, 22 Dec 2023 12:56:06 +0800
Message-ID: <CAOPYjvaDCFhFCSc+LhpDRq2ZB0jOP8xx0t3OVL95C3WfG-NimA@mail.gmail.com>
Subject: Re: [PATCH] md/raid5: fix atomicity violation in raid5_cache_count
To: Yu Kuai <yukuai1@huaweicloud.com>
Cc: song@kernel.org, linux-raid@vger.kernel.org, linux-kernel@vger.kernel.org, 
	baijiaju1990@outlook.com, BassCheck <bass@buaa.edu.cn>, 
	"yukuai (C)" <yukuai3@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi,
We've updated to use READ_ONCE() in PATCH v2, Thank you for helpful advice.
Best regards,
Han

Yu Kuai <yukuai1@huaweicloud.com> =E4=BA=8E2023=E5=B9=B412=E6=9C=8822=E6=97=
=A5=E5=91=A8=E4=BA=94 10:53=E5=86=99=E9=81=93=EF=BC=9A
>
> Hi,
>
> =E5=9C=A8 2023/12/22 10:34, 20 39 =E5=86=99=E9=81=93:
> > Hi Kuai,
> >
> > Thank you for your patience. This email is essentially the same as my
> > previous one, only now adjusted to plain text format. I apologize for
> > any inconvenience caused earlier.
> >
> > Thanks for your email and the insightful points you've raised. Let me
> > clarify a few aspects regarding the raid5_cache_count() and
> > raid5_set_cache_size() functions.
> >
> > 1. Callback Function in setup_conf(): You mentioned that
> > raid5_cache_count() is called from setup_conf() where reconfig_mutex
> > is held. While this is true, it's important to note that
> > raid5_cache_count() is actually initialized as a callback function in
> > setup_conf(), as described in /include/linux/shrinker.h. This means it
> > could be invoked later in a context where the reconfig_mutex isn't
> > necessarily held. The documentation in shrinker.h indicates potential
> > invocation scenarios beyond the initial setup context.
>
> Yes, you're right. I misread the code. Then this patch looks good to me,
> just one nit below.
>
> >>>> @@ -7390,11 +7390,12 @@ static unsigned long raid5_cache_count(struc=
t shrinker *shrink,
> >>>>                                       struct shrink_control *sc)
> >>>>    {
> >>>>        struct r5conf *conf =3D shrink->private_data;
> >>>> -
> >>>> -     if (conf->max_nr_stripes < conf->min_nr_stripes)
> >>>> +     int max_stripes =3D conf->max_nr_stripes;
> >>>> +     int min_stripes =3D conf->min_nr_stripes;
>
> Since read and write can concurrent, I'll suggest to use READ_ONCE() and
> WRITE_ONCE() for max/min_nr_stripes.
>
> Thanks,
> Kuai
> >>>> +     if (max_stripes < min_stripes)
> >>>>                /* unlikely, but not impossible */
> >>>>                return 0;
> >>>> -     return conf->max_nr_stripes - conf->min_nr_stripes;
> >>>> +     return max_stripes - min_stripes;
> >>>>    }
> >>>>
> >>>>    static struct r5conf *setup_conf(struct mddev *mddev)
> >>>>
> >>>
> > .
> >
>