Re: [PATCH 2/2] md/raid1: serialize overlapped io for writemostly disk

On Wed, Feb 11, 2026 at 11:15 AM Yu Kuai <[email protected]> wrote:
>
> Hi,
>
> 在 2026/2/11 10:44, Xiao Ni 写道:
> > On Wed, Feb 11, 2026 at 10:32 AM Yu Kuai <[email protected]> wrote:
> >> Hi,
> >>
> >> 在 2026/2/6 13:38, Xiao Ni 写道:
> >>> In behind mode, overlap bios for writemostly device are queued. They
> >>> In behind mode, bios which overlap the bio from the interval tree
> >>> need to wait in waitqueue. Those bios will be handled once the in-tree
> >>> bio finishes. They are waken up and try to get the tree lock. The bio
> >>> which gets the lock will be handled. So the sequence can't be guaranteed.
> >>>
> >>> For example
> >>> bio1(100,200)
> >>> bio2(150,200)
> >>> bio3(150,300)
> >>> The write sequence of fast device is bio1,bio2,bio3. But the write
> >>> sequence of slow device is bio1,bio3,bio2. This causes data corruption.
> >>> Replace waitqueue with a fifo list to guarantee the write sequence.
> >> To be honest, I think this is fine with the respect user won't send overlap
> >> bios. In this case, bio1 is done in fast disk and pending in slow disk, then
> >> bio2 will stuck while issuing and waiting for bio1 to return from slow disk.
> >> So send bio3 in this case is not expected, and if you do you can't guarantee
> >> the complete order from fast disk as bio2, bio3 as well.
> > Hi Kuai
> >
> > In write behind mode, raid1 returns bio1 to the upper layer once the
> > write on the fast disk finishes. The upper layer (app/filesystem)
> > thinks that bio1 has finished, while the write of bio1 to the slow
> > disk is still in progress. So bio2 will not be stuck while issuing, am
> > I right? If so, bio2 can be submitted to raid1 once the write of bio1
> > to the fast disk finishes. And bio3 can be submitted to raid1 once the
> > write of bio2 to fast disk finishes.
>
> I just take a look at raid1 code, turns out raid1_write_request() just allocate
> bio and issue it directly in each rdev iteration, I misunderstood.
>
> This solution looks fine to me, however, I'll suggest to keep the wait_queue and
> use prepare_to_wait_exclusive() to guarantee the fifo order.

Thanks for the suggestion. I'll use prepare_to_wait_exclusive and send v2.

Best Regards
Xiao
>
> >
> > Best Regards
> > Xiao
> >>> Signed-off-by: Xiao Ni <[email protected]>
> >>> ---
> >>>    drivers/md/md.c    |  1 -
> >>>    drivers/md/md.h    |  4 +++-
> >>>    drivers/md/raid1.c | 39 +++++++++++++++++++++++++++++----------
> >>>    3 files changed, 32 insertions(+), 12 deletions(-)
> >>>
> >>> diff --git a/drivers/md/md.c b/drivers/md/md.c
> >>> index 59cd303548de..ab91d17c2d68 100644
> >>> --- a/drivers/md/md.c
> >>> +++ b/drivers/md/md.c
> >>> @@ -188,7 +188,6 @@ static int rdev_init_serial(struct md_rdev *rdev)
> >>>
> >>>                spin_lock_init(&serial_tmp->serial_lock);
> >>>                serial_tmp->serial_rb = RB_ROOT_CACHED;
> >>> -             init_waitqueue_head(&serial_tmp->serial_io_wait);
> >>>        }
> >>>
> >>>        rdev->serial = serial;
> >>> diff --git a/drivers/md/md.h b/drivers/md/md.h
> >>> index ac84289664cd..64ffbb2efb87 100644
> >>> --- a/drivers/md/md.h
> >>> +++ b/drivers/md/md.h
> >>> @@ -126,7 +126,6 @@ enum sync_action {
> >>>    struct serial_in_rdev {
> >>>        struct rb_root_cached serial_rb;
> >>>        spinlock_t serial_lock;
> >>> -     wait_queue_head_t serial_io_wait;
> >>>    };
> >>>
> >>>    /*
> >>> @@ -382,6 +381,9 @@ struct serial_info {
> >>>        sector_t start;         /* start sector of rb node */
> >>>        sector_t last;          /* end sector of rb node */
> >>>        sector_t _subtree_last; /* highest sector in subtree of rb node */
> >>> +     struct list_head        list_node;
> >>> +     struct list_head        waiters;
> >>> +     struct completion       ready;
> >>>    };
> >>>
> >>>    /*
> >>> diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> >>> index a41b1ec3d695..38c73538d038 100644
> >>> --- a/drivers/md/raid1.c
> >>> +++ b/drivers/md/raid1.c
> >>> @@ -57,19 +57,25 @@ INTERVAL_TREE_DEFINE(struct serial_info, node, sector_t, _subtree_last,
> >>>                     START, LAST, static inline, raid1_rb);
> >>>
> >>>    static int check_and_add_serial(struct md_rdev *rdev, struct r1bio *r1_bio,
> >>> -                             struct serial_info *si, int idx)
> >>> +                             struct serial_info *si)
> >>>    {
> >>>        unsigned long flags;
> >>>        int ret = 0;
> >>>        sector_t lo = r1_bio->sector;
> >>>        sector_t hi = lo + r1_bio->sectors - 1;
> >>> +     int idx = sector_to_idx(r1_bio->sector);
> >>>        struct serial_in_rdev *serial = &rdev->serial[idx];
> >>> +     struct serial_info *head_si;
> >>>
> >>>        spin_lock_irqsave(&serial->serial_lock, flags);
> >>>        /* collision happened */
> >>> -     if (raid1_rb_iter_first(&serial->serial_rb, lo, hi))
> >>> +     head_si = raid1_rb_iter_first(&serial->serial_rb, lo, hi);
> >>> +     if (head_si && head_si != si) {
> >>> +             si->start = lo;
> >>> +             si->last = hi;
> >>> +             list_add_tail(&si->list_node, &head_si->waiters);
> >>>                ret = -EBUSY;
> >>> -     else {
> >>> +     } else if (!head_si) {
> >>>                si->start = lo;
> >>>                si->last = hi;
> >>>                raid1_rb_insert(si, &serial->serial_rb);
> >>> @@ -83,19 +89,23 @@ static void wait_for_serialization(struct md_rdev *rdev, struct r1bio *r1_bio)
> >>>    {
> >>>        struct mddev *mddev = rdev->mddev;
> >>>        struct serial_info *si;
> >>> -     int idx = sector_to_idx(r1_bio->sector);
> >>> -     struct serial_in_rdev *serial = &rdev->serial[idx];
> >>>
> >>>        if (WARN_ON(!mddev->serial_info_pool))
> >>>                return;
> >>>        si = mempool_alloc(mddev->serial_info_pool, GFP_NOIO);
> >>> -     wait_event(serial->serial_io_wait,
> >>> -                check_and_add_serial(rdev, r1_bio, si, idx) == 0);
> >>> +     INIT_LIST_HEAD(&si->waiters);
> >>> +     INIT_LIST_HEAD(&si->list_node);
> >>> +     init_completion(&si->ready);
> >>> +     while (check_and_add_serial(rdev, r1_bio, si)) {
> >>> +             wait_for_completion(&si->ready);
> >>> +             reinit_completion(&si->ready);
> >>> +     }
> >>>    }
> >>>
> >>>    static void remove_serial(struct md_rdev *rdev, sector_t lo, sector_t hi)
> >>>    {
> >>>        struct serial_info *si;
> >>> +     struct serial_info *first_si;
> >>>        unsigned long flags;
> >>>        int found = 0;
> >>>        struct mddev *mddev = rdev->mddev;
> >>> @@ -106,16 +116,25 @@ static void remove_serial(struct md_rdev *rdev, sector_t lo, sector_t hi)
> >>>        for (si = raid1_rb_iter_first(&serial->serial_rb, lo, hi);
> >>>             si; si = raid1_rb_iter_next(si, lo, hi)) {
> >>>                if (si->start == lo && si->last == hi) {
> >>> -                     raid1_rb_remove(si, &serial->serial_rb);
> >>> -                     mempool_free(si, mddev->serial_info_pool);
> >>>                        found = 1;
> >>>                        break;
> >>>                }
> >>>        }
> >>>        if (!found)
> >>>                WARN(1, "The write IO is not recorded for serialization\n");
> >>> +     else {
> >>> +             raid1_rb_remove(si, &serial->serial_rb);
> >>> +             if (!list_empty(&si->waiters)) {
> >>> +                     first_si = list_first_entry(&si->waiters,
> >>> +                                             struct serial_info, list_node);
> >>> +                     list_del_init(&first_si->list_node);
> >>> +                     list_splice_init(&si->waiters, &first_si->waiters);
> >>> +                     raid1_rb_insert(first_si, &serial->serial_rb);
> >>> +                     complete(&first_si->ready);
> >>> +             }
> >>> +             mempool_free(si, mddev->serial_info_pool);
> >>> +     }
> >>>        spin_unlock_irqrestore(&serial->serial_lock, flags);
> >>> -     wake_up(&serial->serial_io_wait);
> >>>    }
> >>>
> >>>    /*
> >> --
> >> Thansk,
> >> Kuai
> >>
> >
> --
> Thansk,
> Kuai
>