Re: [PATCH v2 02/20] md: skip bitmap accounting for empty write ranges

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Using `md_io_clone->start_time == 0` to check whether IO accounting was started is unsafe because `jiffies` can legitimately be exactly 0, permanently leaking the device's `in_flight` counter.
--

commit ff695222c639e0740f645b5e4a15298c468511f4
Author: Yu Kuai <yukuai@fygo.io>

md: skip bitmap accounting for empty write ranges

This commit updates md bitmap accounting to handle zero-sector flush/FUA bios.
It treats both naturally empty bios and ranges trimmed by the prepare_range
hook as not started, preventing the completion path from calling end_write()
on an empty range.

> diff --git a/drivers/md/md.c b/drivers/md/md.c
> index 3b20a57b8c7ed..0b59c676f7c03 100644
> --- a/drivers/md/md.c
> +++ b/drivers/md/md.c

[ ... ]

> @@ -9423,10 +9426,12 @@ static void md_clone_bio(struct mddev *mddev, struct bio **bio)
>  	md_io_clone = container_of(clone, struct md_io_clone, bio_clone);
>  	md_io_clone->orig_bio = *bio;
>  	md_io_clone->mddev = mddev;
> +	md_io_clone->sectors = 0;
>  	if (blk_queue_io_stat(bdev->bd_disk->queue))
>  		md_io_clone->start_time = bio_start_io_acct(*bio);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to use
md_io_clone->start_time as a boolean flag to track if IO accounting was
started?

Looking at bio_start_io_acct(), it returns the current jiffies value. If an
IO request is submitted exactly when the jiffies counter wraps to 0,
start_time will be set to 0.

When the IO completes, md_end_clone_io() evaluates the following condition:

    if (md_io_clone->start_time)
        bio_end_io_acct(orig_bio, md_io_clone->start_time);

If start_time happens to be 0, it skips bio_end_io_acct() and the block 
device's in_flight counter is permanently leaked. This prevents
is_mddev_idle() from ever returning true, which could permanently throttle
background sync operations until the system is rebooted.

> -	if (bio_data_dir(*bio) == WRITE && md_bitmap_enabled(mddev, false)) {
> +	if (bio_data_dir(*bio) == WRITE && bio_sectors(*bio) &&
> +	    md_bitmap_enabled(mddev, false)) {
>  		md_io_clone->offset = (*bio)->bi_iter.bi_sector;
>  		md_io_clone->sectors = bio_sectors(*bio);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1782282042.git.yukuai@kernel.org?part=2