On Fri, Dec 10, 2021 at 10:26 AM Vishal Verma <vverma@xxxxxxxxxxxxxxxx> wrote:
>
>
> On 12/9/21 7:16 PM, Song Liu wrote:
> > On Wed, Nov 10, 2021 at 10:15 AM Vishal Verma <vverma@xxxxxxxxxxxxxxxx> wrote:
> >> Returns EAGAIN in case the raid456 driver would block
> >> waiting for situations like:
> >>
> >> - Reshape operation,
> >> - Discard operation.
> >>
> >> Signed-off-by: Vishal Verma <vverma@xxxxxxxxxxxxxxxx>
> >> ---
> >> 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().
> >
> >> 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)
> > + 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
