Re: [PATCH v3 2/2] md: raid1 add nowait support

[Vishal Verma <vverma@xxxxxxxxxxxxxxxx>]

Hi Song,

I did modify raid456 and raid10 with nowait support, but unfortunately 
have been running into kernel task hung and panics while doing write IO 
and having hard time trying to debug.

Shall I post the patches in to get the feedback or go with an 
alternative route to have a flag to only enable nowait for raid1 for now?

Thanks,

Vishal

On 11/8/21 3:39 PM, Vishal Verma wrote:
> On 11/8/21 3:32 PM, Song Liu wrote:
> > On Wed, Nov 3, 2021 at 9:52 PM Vishal Verma <vverma@xxxxxxxxxxxxxxxx> 
> > wrote:
> > > This adds nowait support to the RAID1 driver. It makes RAID1 driver
> > > return with EAGAIN for situations where it could wait for eg:
> > >
> > >    - Waiting for the barrier,
> > >    - Array got frozen,
> > >    - Too many pending I/Os to be queued.
> > >
> > > wait_barrier() fn is modified to return bool to support error for
> > > wait barriers. It returns true in case of wait or if wait is not
> > > required and returns false if wait was required but not performed
> > > to support nowait.
> > >
> > > Signed-off-by: Vishal Verma <vverma@xxxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/md/raid1.c | 74 
> > > +++++++++++++++++++++++++++++++++++-----------
> > >   1 file changed, 57 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/md/raid1.c b/drivers/md/raid1.c
> > > index 7dc8026cf6ee..2e191fc2147b 100644
> > > --- a/drivers/md/raid1.c
> > > +++ b/drivers/md/raid1.c
> > > @@ -929,8 +929,9 @@ static void lower_barrier(struct r1conf *conf, 
> > > sector_t sector_nr)
> > >          wake_up(&conf->wait_barrier);
> > >   }
> > >
> > > -static void _wait_barrier(struct r1conf *conf, int idx)
> > > +static bool _wait_barrier(struct r1conf *conf, int idx, bool nowait)
> > >   {
> > > +       bool ret = true;
> > >          /*
> > >           * We need to increase conf->nr_pending[idx] very early here,
> > >           * then raise_barrier() can be blocked when it waits for
> > > @@ -961,7 +962,7 @@ static void _wait_barrier(struct r1conf *conf, 
> > > int idx)
> > >           */
> > >          if (!READ_ONCE(conf->array_frozen) &&
> > >              !atomic_read(&conf->barrier[idx]))
> > > -               return;
> > > +               return ret;
> > >
> > >          /*
> > >           * After holding conf->resync_lock, conf->nr_pending[idx]
> > > @@ -979,18 +980,27 @@ static void _wait_barrier(struct r1conf *conf, 
> > > int idx)
> > >           */
> > >          wake_up(&conf->wait_barrier);
> > >          /* Wait for the barrier in same barrier unit bucket to 
> > > drop. */
> > > -       wait_event_lock_irq(conf->wait_barrier,
> > > -                           !conf->array_frozen &&
> > > - !atomic_read(&conf->barrier[idx]),
> > > -                           conf->resync_lock);
> > > +       if (conf->array_frozen || atomic_read(&conf->barrier[idx])) {
> > > +               /* Return false when nowait flag is set */
> > > +               if (nowait)
> > > +                       ret = false;
> > > +               else {
> > > + wait_event_lock_irq(conf->wait_barrier,
> > > +                                       !conf->array_frozen &&
> > > + !atomic_read(&conf->barrier[idx]),
> > > +                                       conf->resync_lock);
> > > +               }
> > > +       }
> > >          atomic_inc(&conf->nr_pending[idx]);
> > >          atomic_dec(&conf->nr_waiting[idx]);
> > >          spin_unlock_irq(&conf->resync_lock);
> > > +       return ret;
> > >   }
> > >
> > > -static void wait_read_barrier(struct r1conf *conf, sector_t sector_nr)
> > > +static bool wait_read_barrier(struct r1conf *conf, sector_t 
> > > sector_nr, bool nowait)
> > >   {
> > >          int idx = sector_to_idx(sector_nr);
> > > +       bool ret = true;
> > >
> > >          /*
> > >           * Very similar to _wait_barrier(). The difference is, for 
> > > read
> > > @@ -1002,7 +1012,7 @@ static void wait_read_barrier(struct r1conf 
> > > *conf, sector_t sector_nr)
> > >          atomic_inc(&conf->nr_pending[idx]);
> > >
> > >          if (!READ_ONCE(conf->array_frozen))
> > > -               return;
> > > +               return ret;
> > >
> > >          spin_lock_irq(&conf->resync_lock);
> > >          atomic_inc(&conf->nr_waiting[idx]);
> > > @@ -1013,19 +1023,27 @@ static void wait_read_barrier(struct r1conf 
> > > *conf, sector_t sector_nr)
> > >           */
> > >          wake_up(&conf->wait_barrier);
> > >          /* Wait for array to be unfrozen */
> > > -       wait_event_lock_irq(conf->wait_barrier,
> > > -                           !conf->array_frozen,
> > > -                           conf->resync_lock);
> > > +       if (conf->array_frozen || atomic_read(&conf->barrier[idx])) {
> > > +               if (nowait)
> > > +                       /* Return false when nowait flag is set */
> > > +                       ret = false;
> > > +               else {
> > > + wait_event_lock_irq(conf->wait_barrier,
> > > + !conf->array_frozen,
> > > +                                       conf->resync_lock);
> > > +               }
> > > +       }
> > >          atomic_inc(&conf->nr_pending[idx]);
> > >          atomic_dec(&conf->nr_waiting[idx]);
> > >          spin_unlock_irq(&conf->resync_lock);
> > > +       return ret;
> > >   }
> > >
> > > -static void wait_barrier(struct r1conf *conf, sector_t sector_nr)
> > > +static bool wait_barrier(struct r1conf *conf, sector_t sector_nr, 
> > > bool nowait)
> > >   {
> > >          int idx = sector_to_idx(sector_nr);
> > >
> > > -       _wait_barrier(conf, idx);
> > > +       return _wait_barrier(conf, idx, nowait);
> > >   }
> > >
> > >   static void _allow_barrier(struct r1conf *conf, int idx)
> > > @@ -1236,7 +1254,11 @@ static void raid1_read_request(struct mddev 
> > > *mddev, struct bio *bio,
> > >           * Still need barrier for READ in case that whole
> > >           * array is frozen.
> > >           */
> > > -       wait_read_barrier(conf, bio->bi_iter.bi_sector);
> > > +       if (!wait_read_barrier(conf, bio->bi_iter.bi_sector,
> > > +                               bio->bi_opf & REQ_NOWAIT)) {
> > > +               bio_wouldblock_error(bio);
> > > +               return;
> > > +       }
> > >
> > >          if (!r1_bio)
> > >                  r1_bio = alloc_r1bio(mddev, bio);
> > > @@ -1336,6 +1358,10 @@ static void raid1_write_request(struct mddev 
> > > *mddev, struct bio *bio,
> > >                       bio->bi_iter.bi_sector, bio_end_sector(bio))) {
> > >
> > >                  DEFINE_WAIT(w);
> > > +               if (bio->bi_opf & REQ_NOWAIT) {
> > > +                       bio_wouldblock_error(bio);
> > > +                       return;
> > > +               }
> > >                  for (;;) {
> > > prepare_to_wait(&conf->wait_barrier,
> > >                                          &w, TASK_IDLE);
> > > @@ -1353,17 +1379,26 @@ static void raid1_write_request(struct mddev 
> > > *mddev, struct bio *bio,
> > >           * thread has put up a bar for new requests.
> > >           * Continue immediately if no resync is active currently.
> > >           */
> > > -       wait_barrier(conf, bio->bi_iter.bi_sector);
> > > +       if (!wait_read_barrier(conf, bio->bi_iter.bi_sector,
> > We change wait_barrier to wait_read_barrier here, I guess this is a 
> > typo?
> >
> > Please include changes in raid10 and raid456 (or don't set 
> > QUEUE_FLAG_NOWAIT
> > for these personalities) and resend the patch. We will target it for
> > the next merge
> > window (5.17).
> >
> > Thanks,
> > Song
> >
> > Thanks Song. I am almost done with the raid10 change and will do the
> > raid456 soon and resend.
> > > + bio->bi_opf & REQ_NOWAIT)) {
> > > +               bio_wouldblock_error(bio);
> > > +               return;
> > > +       }
> > >
> > >          r1_bio = alloc_r1bio(mddev, bio);
> > >          r1_bio->sectors = max_write_sectors;
> > >
> > >          if (conf->pending_count >= max_queued_requests) {
> > >                  md_wakeup_thread(mddev->thread);
> > > +               if (bio->bi_opf & REQ_NOWAIT) {
> > > +                       bio_wouldblock_error(bio);
> > > +                       return;
> > > +               }
> > >                  raid1_log(mddev, "wait queued");
> > >                  wait_event(conf->wait_barrier,
> > >                             conf->pending_count < max_queued_requests);
> > >          }
> > > +
> > >          /* first select target devices under rcu_lock and
> > >           * inc refcount on their rdev.  Record them by setting
> > >           * bios[x] to bio
> > > @@ -1458,9 +1493,14 @@ static void raid1_write_request(struct mddev 
> > > *mddev, struct bio *bio,
> > > rdev_dec_pending(conf->mirrors[j].rdev, mddev);
> > >                  r1_bio->state = 0;
> > >                  allow_barrier(conf, bio->bi_iter.bi_sector);
> > > +
> > > +               if (bio->bi_opf & REQ_NOWAIT) {
> > > +                       bio_wouldblock_error(bio);
> > > +                       return;
> > > +               }
> > >                  raid1_log(mddev, "wait rdev %d blocked", 
> > > blocked_rdev->raid_disk);
> > >                  md_wait_for_blocked_rdev(blocked_rdev, mddev);
> > > -               wait_barrier(conf, bio->bi_iter.bi_sector);
> > > +               wait_barrier(conf, bio->bi_iter.bi_sector, false);
> > >                  goto retry_write;
> > >          }
> > >
> > > @@ -1687,7 +1727,7 @@ static void close_sync(struct r1conf *conf)
> > >          int idx;
> > >
> > >          for (idx = 0; idx < BARRIER_BUCKETS_NR; idx++) {
> > > -               _wait_barrier(conf, idx);
> > > +               _wait_barrier(conf, idx, false);
> > >                  _allow_barrier(conf, idx);
> > >          }
> > >
> > > --
> > > 2.17.1