Received-SPF: pass (google.com: domain of linux-kernel+bounces-2418-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231215013931.3329455-1-linan666@huaweicloud.com>
 <20231215013931.3329455-2-linan666@huaweicloud.com> <CAPhsuW6VTvXy3L9CUhTrSC3+_-_n9FDVrtdzQ7SWWkukoQg13Q@mail.gmail.com>
 <be8d9147-4f7f-2fab-da2a-bb4cde46fd12@huaweicloud.com>
In-Reply-To: <be8d9147-4f7f-2fab-da2a-bb4cde46fd12@huaweicloud.com>
From: Song Liu <song@kernel.org>
Date: Sat, 16 Dec 2023 16:28:11 -0800
Message-ID: <CAPhsuW6kv7FRB_1NoheiDqvmLmongiJ-ty9mYRNvFw3yecE_Ug@mail.gmail.com>
Subject: Re: [PATCH v2 1/2] md: Fix overflow in is_mddev_idle
To: Yu Kuai <yukuai1@huaweicloud.com>
Cc: linan666@huaweicloud.com, axboe@kernel.dk, linux-raid@vger.kernel.org, 
	linux-kernel@vger.kernel.org, linux-block@vger.kernel.org, 
	yi.zhang@huawei.com, houtao1@huawei.com, yangerkun@huawei.com, 
	"yukuai (C)" <yukuai3@huawei.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Dec 15, 2023 at 6:24=E2=80=AFPM Yu Kuai <yukuai1@huaweicloud.com> w=
rote:
[...]
> >>   static inline void md_sync_acct_bio(struct bio *bio, unsigned long n=
r_sectors)
> >> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> >> index 3f8a21cd9233..d28b98adf457 100644
> >> --- a/include/linux/blkdev.h
> >> +++ b/include/linux/blkdev.h
> >> @@ -170,7 +170,7 @@ struct gendisk {
> >>          struct list_head slave_bdevs;
> >>   #endif
> >>          struct timer_rand_state *random;
> >> -       atomic_t sync_io;               /* RAID */
> >> +       atomic64_t sync_io;             /* RAID */
> >>          struct disk_events *ev;
> >
> > As we are on this, I wonder whether we really need this.
> > AFAICT, is_mddev_idle() is the only consumer of sync_io.
> > We can probably do the same check in is_mddev_idle()
> > without sync_io.
>
> After reviewing some code, what I'm understand is following:
>
> I think 'sync_io' is used to distinguish 'sync io' from raid(this can
> from different raid, for example, different partition is used for
> different array) and other io(any io, even it's not from raid). And
> if there are any other IO, sync speed is limited to min, otherwise it's
> limited to max.
>
> If we want to keep this behaviour, I can't think of any other way for
> now...

Thanks for looking into this. To keep current behavior, we will need it
in gendisk.

[...]

> >> @@ -8496,14 +8496,15 @@ static int is_mddev_idle(struct mddev *mddev, =
int init)
> >>   {
> >>          struct md_rdev *rdev;
> >>          int idle;
> >> -       int curr_events;
> >> +       long long curr_events;
> >>
> >>          idle =3D 1;
> >>          rcu_read_lock();
> >>          rdev_for_each_rcu(rdev, mddev) {
> >>                  struct gendisk *disk =3D rdev->bdev->bd_disk;
> >> -               curr_events =3D (int)part_stat_read_accum(disk->part0,=
 sectors) -
> >> -                             atomic_read(&disk->sync_io);
> >> +               curr_events =3D
> >> +                       (long long)part_stat_read_accum(disk->part0, s=
ectors) -
> >> +                             atomic64_read(&disk->sync_io);
>
> By the way, I don't think this overflow is problematic, assume that
> sectors accumulate by 'A' and sync_io accumulate by 'B':
>
> (setros + A) - (sync_io + B) -(sectors - sync_io) =3D (A - B)
>
> Nomatter overflow or truncation happened of not in the abouve addition
> and subtraction, the result is correct.
>
> And even if io accounting is disabled, which means sectors and A is
> always 0, the result will always be -B that is <=3D 0, hence
> is_mddev_idle() will always return true, and sync_speed will be limited
> to max in this case.

We only use  this for idle or not check, the behavior is OK (I think).
However, this logic is error prone.

On 64-bit systems, there is a 4-byte hole behind sync_io. I think we can
just use it for atomic64_t so that we don't have to worry about overflow.

Thanks,
Song