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

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

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)

quoted

+               bio_wouldblock_error(bi);
+               return true;
+

Please let me know if these make sense.

Thanks,
Song


Makes sense. Thanks for your feedback. I'll incorporate it and test.

When testing, please make sure we hit all different conditions with REQ_NOWAIT.

Thanks,
Song