Re: [RFC PATCH v4 4/4] md: raid456 add nowait support

On 12/13/21 3:43 PM, Vishal Verma wrote:

On 12/12/21 10:56 PM, Song Liu wrote:

quoted

On Fri, Dec 10, 2021 at 10:26 AM Vishal Verma [off-list ref] wrote:

quoted

On 12/9/21 7:16 PM, Song Liu wrote:

quoted

On Wed, Nov 10, 2021 at 10:15 AM Vishal Verma [off-list ref] wrote:

quoted

Returns EAGAIN in case the raid456 driver would block
waiting for situations like:

    - Reshape operation,
    - Discard operation.

Signed-off-by: Vishal Verma <redacted>
---
   drivers/md/raid5.c | 14 ++++++++++++++
   1 file changed, 14 insertions(+)

diff --git a/drivers/md/raid5.c b/drivers/md/raid5.c
index 9c1a5877cf9f..fa64ee315241 100644
--- a/drivers/md/raid5.c
+++ b/drivers/md/raid5.c

@@ -5710,6 +5710,11 @@ static void make_discard_request(struct mddev *mddev, struct bio *bi)
                  int d;
          again:
                  sh = raid5_get_active_stripe(conf, logical_sector, 0, 0, 0);
+               /* Bail out if REQ_NOWAIT is set */
+               if (bi->bi_opf & REQ_NOWAIT) {
+                       bio_wouldblock_error(bi);
+                       return;
+               }

This is not right. raid5_get_active_stripe() gets refcount on the sh,
we cannot simply
return here. I think we need the logic after raid5_release_stripe()
and before schedule().

quoted

                  prepare_to_wait(&conf->wait_for_overlap, &w,
                                  TASK_UNINTERRUPTIBLE);
                  set_bit(R5_Overlap, &sh->dev[sh->pd_idx].flags);

@@ -5820,6 +5825,15 @@ static bool raid5_make_request(struct mddev *mddev, struct bio * bi)
          bi->bi_next = NULL;

          md_account_bio(mddev, &bi);
+       /* Bail out if REQ_NOWAIT is set */
+       if (bi->bi_opf & REQ_NOWAIT &&
+           conf->reshape_progress != MaxSector &&
+           mddev->reshape_backwards
+           ? logical_sector < conf->reshape_safe
+           : logical_sector >= conf->reshape_safe) {
+               bio_wouldblock_error(bi);
+               return true;
+       }

This is also problematic, and is the trigger of those error messages.
We only want to trigger -EAGAIN when logical_sector is between
reshape_progress and reshape_safe.

Just to clarify, did you mean doing something like:
if (bi->bi_opf & REQ_NOWAIT &&
+           conf->reshape_progress != MaxSector &&
+           (mddev->reshape_backwards
+           ? (logical_sector > conf->reshape_progress && logical_sector < conf->reshape_safe)
+           : logical_sector >= conf->reshape_safe)) {

I think this should be
   :   (logical_sector >= conf->reshape_safe && logical_sector <
conf->reshape_progress)

if (bi->bi_opf & REQ_NOWAIT &&
                 conf->reshape_progress != MaxSector &&
                 (mddev->reshape_backwards
                 ? (logical_sector > conf->reshape_progress && 
logical_sector <= conf->reshape_safe)
                 : (logical_sector >= conf->reshape_safe && 
logical_sector < conf->reshape_progress))) {
                         bio_wouldblock_error(bi);
                         return true;
         }

After making this change along with other changes, I ran some tests with 
100% reads, 70%read30%writes and 100% writes on a clean raid5 array.

Unfortunately, I ran into this following task hung with 100% writes 
(with both libaio and io_uring):

[21876.856692] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" 
disables this message.
[21876.864518] task:md5_raid5       state:D stack:    0 pid:11675 
ppid:     2 flags:0x00004000
[21876.864522] Call Trace:
[21876.864526]  __schedule+0x2d4/0x970
[21876.864532]  ? wbt_cleanup_cb+0x20/0x20
[21876.864535]  schedule+0x4e/0xb0
[21876.864537]  io_schedule+0x3f/0x70
[21876.864539]  rq_qos_wait+0xb9/0x130
[21876.864542]  ? sysv68_partition+0x280/0x280
[21876.864543]  ? wbt_cleanup_cb+0x20/0x20
[21876.864545]  wbt_wait+0x92/0xc0
[21876.864546]  __rq_qos_throttle+0x25/0x40
[21876.864548]  blk_mq_submit_bio+0xc6/0x5d0
[21876.864551]  ? submit_bio_checks+0x39e/0x5f0
[21876.864554]  __submit_bio+0x1bc/0x1d0
[21876.864555]  ? kmem_cache_free+0x378/0x3c0
[21876.864558]  ? mempool_free_slab+0x17/0x20
[21876.864562]  submit_bio_noacct+0x256/0x2a0
[21876.864565]  0xffffffffc01fa6d9
[21876.864568]  ? 0xffffffffc01f5d01
[21876.864569]  raid5_get_active_stripe+0x16c0/0x3e00 [raid456]
[21876.864571]  ? __wake_up_common_lock+0x8a/0xc0
[21876.864575]  raid5_get_active_stripe+0x2839/0x3e00 [raid456]
[21876.864577]  raid5_get_active_stripe+0x2d6e/0x3e00 [raid456]
[21876.864579]  md_thread+0xae/0x170
[21876.864581]  ? wait_woken+0x60/0x60
[21876.864582]  ? md_start_sync+0x60/0x60
[21876.864584]  kthread+0x127/0x150
[21876.864586]  ? set_kthread_struct+0x40/0x40
[21876.864588]  ret_from_fork+0x1f/0x30

What kernel base are you using for your patches?

-- 
Jens Axboe