| Message ID | 20170426183728.10821-12-bart.vanassche@sandisk.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Mike Snitzer |
| Headers | show |
On 04/26/2017 08:37 PM, Bart Van Assche wrote: > Instead of checking MPATHF_QUEUE_IF_NO_PATH, > MPATHF_SAVED_QUEUE_IF_NO_PATH and the no_flush flag to decide whether > or not to push back a request if there are no paths available, only > clear MPATHF_QUEUE_IF_NO_PATH in queue_if_no_path() if no_flush has > not been set. The result is that only a single bit has to be tested > in the hot path to decide whether or not a request must be pushed > back and also that m->lock does not have to be taken in the hot path. > > Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> > Cc: Hannes Reinecke <hare@suse.com> > --- > drivers/md/dm-mpath.c | 70 ++++++++------------------------------------------- > 1 file changed, 11 insertions(+), 59 deletions(-) > > diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c > index adde8a51e020..0eafe7a2070b 100644 > --- a/drivers/md/dm-mpath.c > +++ b/drivers/md/dm-mpath.c > @@ -442,47 +442,6 @@ static struct pgpath *choose_pgpath(struct multipath *m, size_t nr_bytes) > } > > /* > - * Check whether bios must be queued in the device-mapper core rather > - * than here in the target. > - * > - * If m->queue_if_no_path and m->saved_queue_if_no_path hold the > - * same value then we are not between multipath_presuspend() > - * and multipath_resume() calls and we have no need to check > - * for the DMF_NOFLUSH_SUSPENDING flag. > - */ > -static bool __must_push_back(struct multipath *m) > -{ > - return ((test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags) != > - test_bit(MPATHF_SAVED_QUEUE_IF_NO_PATH, &m->flags)) && > - dm_noflush_suspending(m->ti)); > -} > - > -static bool must_push_back_rq(struct multipath *m) > -{ > - bool r; > - unsigned long flags; > - > - spin_lock_irqsave(&m->lock, flags); > - r = (test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags) || > - __must_push_back(m)); > - spin_unlock_irqrestore(&m->lock, flags); > - > - return r; > -} > - > -static bool must_push_back_bio(struct multipath *m) > -{ > - bool r; > - unsigned long flags; > - > - spin_lock_irqsave(&m->lock, flags); > - r = __must_push_back(m); > - spin_unlock_irqrestore(&m->lock, flags); > - > - return r; > -} > - > -/* > * Map cloned requests (request-based multipath) > */ > static int multipath_clone_and_map(struct dm_target *ti, struct request *rq, > @@ -503,7 +462,7 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq, > pgpath = choose_pgpath(m, nr_bytes); > > if (!pgpath) { > - if (must_push_back_rq(m)) { > + if (test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) { > pr_debug("no path - requeueing\n"); > return DM_MAPIO_DELAY_REQUEUE; > } > @@ -581,9 +540,9 @@ static int __multipath_map_bio(struct multipath *m, struct bio *bio, struct dm_m > } > > if (!pgpath) { > - if (!must_push_back_bio(m)) > - return -EIO; > - return DM_MAPIO_REQUEUE; > + if (test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) > + return DM_MAPIO_REQUEUE; > + return -EIO; > } > > mpio->pgpath = pgpath; > @@ -675,7 +634,7 @@ static int queue_if_no_path(struct multipath *m, bool queue_if_no_path, > else > clear_bit(MPATHF_SAVED_QUEUE_IF_NO_PATH, &m->flags); > } > - if (queue_if_no_path) > + if (queue_if_no_path || dm_noflush_suspending(m->ti)) > set_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags); > else > clear_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags); > @@ -1523,12 +1482,9 @@ static int do_end_io(struct multipath *m, struct request *clone, > if (mpio->pgpath) > fail_path(mpio->pgpath); > > - if (!atomic_read(&m->nr_valid_paths)) { > - if (!test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) { > - if (!must_push_back_rq(m)) > - r = -EIO; > - } > - } > + if (atomic_read(&m->nr_valid_paths) == 0 && > + !test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) > + r = -EIO; > > return r; > } > @@ -1569,13 +1525,9 @@ static int do_end_io_bio(struct multipath *m, struct bio *clone, > if (mpio->pgpath) > fail_path(mpio->pgpath); > > - if (!atomic_read(&m->nr_valid_paths)) { > - if (!test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) { > - if (!must_push_back_bio(m)) > - return -EIO; > - return DM_ENDIO_REQUEUE; > - } > - } > + if (atomic_read(&m->nr_valid_paths) == 0 && > + !test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) > + return -EIO; > > /* Queue for the daemon to resubmit */ > dm_bio_restore(get_bio_details_from_bio(clone), clone); > _Looks_ okay. But the entire 'must_push_back' stuff is so convoluted (plus not actually required for rq-based multipathing) that I'll leave the final decision to Mike. But anyway: Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c index adde8a51e020..0eafe7a2070b 100644 --- a/drivers/md/dm-mpath.c +++ b/drivers/md/dm-mpath.c @@ -442,47 +442,6 @@ static struct pgpath *choose_pgpath(struct multipath *m, size_t nr_bytes) } /* - * Check whether bios must be queued in the device-mapper core rather - * than here in the target. - * - * If m->queue_if_no_path and m->saved_queue_if_no_path hold the - * same value then we are not between multipath_presuspend() - * and multipath_resume() calls and we have no need to check - * for the DMF_NOFLUSH_SUSPENDING flag. - */ -static bool __must_push_back(struct multipath *m) -{ - return ((test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags) != - test_bit(MPATHF_SAVED_QUEUE_IF_NO_PATH, &m->flags)) && - dm_noflush_suspending(m->ti)); -} - -static bool must_push_back_rq(struct multipath *m) -{ - bool r; - unsigned long flags; - - spin_lock_irqsave(&m->lock, flags); - r = (test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags) || - __must_push_back(m)); - spin_unlock_irqrestore(&m->lock, flags); - - return r; -} - -static bool must_push_back_bio(struct multipath *m) -{ - bool r; - unsigned long flags; - - spin_lock_irqsave(&m->lock, flags); - r = __must_push_back(m); - spin_unlock_irqrestore(&m->lock, flags); - - return r; -} - -/* * Map cloned requests (request-based multipath) */ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq, @@ -503,7 +462,7 @@ static int multipath_clone_and_map(struct dm_target *ti, struct request *rq, pgpath = choose_pgpath(m, nr_bytes); if (!pgpath) { - if (must_push_back_rq(m)) { + if (test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) { pr_debug("no path - requeueing\n"); return DM_MAPIO_DELAY_REQUEUE; } @@ -581,9 +540,9 @@ static int __multipath_map_bio(struct multipath *m, struct bio *bio, struct dm_m } if (!pgpath) { - if (!must_push_back_bio(m)) - return -EIO; - return DM_MAPIO_REQUEUE; + if (test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) + return DM_MAPIO_REQUEUE; + return -EIO; } mpio->pgpath = pgpath; @@ -675,7 +634,7 @@ static int queue_if_no_path(struct multipath *m, bool queue_if_no_path, else clear_bit(MPATHF_SAVED_QUEUE_IF_NO_PATH, &m->flags); } - if (queue_if_no_path) + if (queue_if_no_path || dm_noflush_suspending(m->ti)) set_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags); else clear_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags); @@ -1523,12 +1482,9 @@ static int do_end_io(struct multipath *m, struct request *clone, if (mpio->pgpath) fail_path(mpio->pgpath); - if (!atomic_read(&m->nr_valid_paths)) { - if (!test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) { - if (!must_push_back_rq(m)) - r = -EIO; - } - } + if (atomic_read(&m->nr_valid_paths) == 0 && + !test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) + r = -EIO; return r; } @@ -1569,13 +1525,9 @@ static int do_end_io_bio(struct multipath *m, struct bio *clone, if (mpio->pgpath) fail_path(mpio->pgpath); - if (!atomic_read(&m->nr_valid_paths)) { - if (!test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) { - if (!must_push_back_bio(m)) - return -EIO; - return DM_ENDIO_REQUEUE; - } - } + if (atomic_read(&m->nr_valid_paths) == 0 && + !test_bit(MPATHF_QUEUE_IF_NO_PATH, &m->flags)) + return -EIO; /* Queue for the daemon to resubmit */ dm_bio_restore(get_bio_details_from_bio(clone), clone);
Instead of checking MPATHF_QUEUE_IF_NO_PATH, MPATHF_SAVED_QUEUE_IF_NO_PATH and the no_flush flag to decide whether or not to push back a request if there are no paths available, only clear MPATHF_QUEUE_IF_NO_PATH in queue_if_no_path() if no_flush has not been set. The result is that only a single bit has to be tested in the hot path to decide whether or not a request must be pushed back and also that m->lock does not have to be taken in the hot path. Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com> Cc: Hannes Reinecke <hare@suse.com> --- drivers/md/dm-mpath.c | 70 ++++++++------------------------------------------- 1 file changed, 11 insertions(+), 59 deletions(-)