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@xxxxxxxxxxx>
> Cc: Hannes Reinecke <hare@xxxxxxxx>
> ---
> 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@xxxxxxxx>
Cheers,
Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
hare@xxxxxxx +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
