From: Mike Christie <michaelc@cs.wisc.edu>
To: Grant Grundler <grundler@google.com>
Cc: Mike Anderson <andmike@linux.vnet.ibm.com>,
linux-scsi@vger.kernel.org, Jens Axboe <jens.axboe@oracle.com>,
Alasdair G Kergon <agk@redhat.com>, Neil Brown <neilb@suse.de>,
Martin Schwidefsky <schwidefsky@de.ibm.com>
Subject: Re: [PATCH 01/14] block: separate failfast into multiple bits.
Date: Tue, 02 Sep 2008 11:59:18 -0500 [thread overview]
Message-ID: <48BD70E6.5020702@cs.wisc.edu> (raw)
In-Reply-To: <da824cf30809020935k6a05df54yf0a2eda1ecc9e65e@mail.gmail.com>
Grant Grundler wrote:
> On Tue, Sep 2, 2008 at 9:05 AM, Mike Anderson
> <andmike@linux.vnet.ibm.com> wrote:
>> From: Mike Christie <michaelc@cs.wisc.edu>
>>
>> Multipath is best at handling transport errors. If it gets a device
>> error then there is not much the multipath layer can do. It will just
>> access the same device but from a different path.
>>
>> This patch breaks up failfast into device, transport and driver errors.
>
> Is there any document that describes what those errors are for each
> class of transport?
Not yet. For SCSI I was still trying to classify the host byte errors,
because drivers are using them differently. I had sent patches in the
thread here
http://marc.info/?l=linux-scsi&m=121918956332584&w=2
that just start syncing up the transport errors for SCSI by adding
some new transport host byte errors and converting drivers and transport
classes to them.
>
> This great work though...I'm looking forward to a storage subsystem
> where each level
> can cooperate with the ones above it.
>
>> The multipath layers (md and dm mutlipath) only ask the lower levels to
>> fast fail transport errors. The user of failfast, read ahead, will ask
>> to fast fail on all errors.
>>
>> Note that blk_noretry_request will return true if any failfast bit
>> is set. This allows drivers that do not support the multipath failfast
>> bits to continue to fail on any failfast error like before. Drivers
>> like scsi that are able to fail fast specific errors can check
>> for the specific fail fast type. In the next patch I will convert
>> scsi.
>>
>> Signed-off-by: Mike Christie <michaelc@cs.wisc.edu>
>> Cc: Jens Axboe <jens.axboe@oracle.com>
>> Cc: Alasdair G Kergon <agk@redhat.com>
>> Cc: Neil Brown <neilb@suse.de>
>> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
>> Signed-off-by: Mike Anderson <andmike@linux.vnet.ibm.com>
>> ---
>> block/blk-core.c | 11 +++++++++--
>> drivers/md/dm-mpath.c | 2 +-
>> drivers/md/multipath.c | 4 ++--
>> drivers/s390/block/dasd_diag.c | 2 +-
>> drivers/s390/block/dasd_eckd.c | 2 +-
>> drivers/s390/block/dasd_fba.c | 2 +-
>> drivers/scsi/device_handler/scsi_dh_alua.c | 3 ++-
>> drivers/scsi/device_handler/scsi_dh_emc.c | 3 ++-
>> drivers/scsi/device_handler/scsi_dh_hp_sw.c | 6 ++++--
>> drivers/scsi/device_handler/scsi_dh_rdac.c | 3 ++-
>> drivers/scsi/scsi_transport_spi.c | 4 +++-
>> include/linux/bio.h | 26 +++++++++++++++++---------
>> include/linux/blkdev.h | 15 ++++++++++++---
>> 13 files changed, 57 insertions(+), 26 deletions(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 4889eb8..f3c29d0 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -1073,8 +1073,15 @@ void init_request_from_bio(struct request *req, struct bio *bio)
>> /*
>> * inherit FAILFAST from bio (for read-ahead, and explicit FAILFAST)
>> */
>> - if (bio_rw_ahead(bio) || bio_failfast(bio))
>> - req->cmd_flags |= REQ_FAILFAST;
>> + if (bio_rw_ahead(bio))
>> + req->cmd_flags |= (REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
>> + REQ_FAILFAST_DRIVER);
>> + if (bio_failfast_dev(bio))
>> + req->cmd_flags |= REQ_FAILFAST_DEV;
>> + if (bio_failfast_transport(bio))
>> + req->cmd_flags |= REQ_FAILFAST_TRANSPORT;
>> + if (bio_failfast_driver(bio))
>> + req->cmd_flags |= REQ_FAILFAST_DRIVER;
>
> This is open source.
> Why can't something like this be done?
> req->cmd_flags |= bio_failfast_flags(bio);
We used to do that when it was just 1 bit for failfast, and it kept
getting messed up. I fixed it once before and Tomo or someone else had
to fix it again later when it got messed up again, so I thought this is
more clear and would prevent future screw ups.
>
> I'm assuming the REQ_FAILFAST_* flags can be equated to the same bits
> in the bio layer.
>
>
>> /*
>> * REQ_BARRIER implies no merging, but lets make it explicit
>> diff --git a/drivers/md/dm-mpath.c b/drivers/md/dm-mpath.c
>> index 71dd65a..b48e201 100644
>> --- a/drivers/md/dm-mpath.c
>> +++ b/drivers/md/dm-mpath.c
>> @@ -827,7 +827,7 @@ static int multipath_map(struct dm_target *ti, struct bio *bio,
>> dm_bio_record(&mpio->details, bio);
>>
>> map_context->ptr = mpio;
>> - bio->bi_rw |= (1 << BIO_RW_FAILFAST);
>> + bio->bi_rw |= (1 << BIO_RW_FAILFAST_TRANSPORT);
>> r = map_io(m, bio, mpio, 0);
>> if (r < 0 || r == DM_MAPIO_REQUEUE)
>> mempool_free(mpio, m->mpio_pool);
>> diff --git a/drivers/md/multipath.c b/drivers/md/multipath.c
>> index c4779cc..2426201 100644
>> --- a/drivers/md/multipath.c
>> +++ b/drivers/md/multipath.c
>> @@ -172,7 +172,7 @@ static int multipath_make_request (struct request_queue *q, struct bio * bio)
>> mp_bh->bio = *bio;
>> mp_bh->bio.bi_sector += multipath->rdev->data_offset;
>> mp_bh->bio.bi_bdev = multipath->rdev->bdev;
>> - mp_bh->bio.bi_rw |= (1 << BIO_RW_FAILFAST);
>> + mp_bh->bio.bi_rw |= (1 << BIO_RW_FAILFAST_TRANSPORT);
>> mp_bh->bio.bi_end_io = multipath_end_request;
>> mp_bh->bio.bi_private = mp_bh;
>> generic_make_request(&mp_bh->bio);
>> @@ -398,7 +398,7 @@ static void multipathd (mddev_t *mddev)
>> *bio = *(mp_bh->master_bio);
>> bio->bi_sector += conf->multipaths[mp_bh->path].rdev->data_offset;
>> bio->bi_bdev = conf->multipaths[mp_bh->path].rdev->bdev;
>> - bio->bi_rw |= (1 << BIO_RW_FAILFAST);
>> + bio->bi_rw |= (1 << BIO_RW_FAILFAST_TRANSPORT);
>> bio->bi_end_io = multipath_end_request;
>> bio->bi_private = mp_bh;
>> generic_make_request(bio);
>> diff --git a/drivers/s390/block/dasd_diag.c b/drivers/s390/block/dasd_diag.c
>> index 85fcb43..7844461 100644
>> --- a/drivers/s390/block/dasd_diag.c
>> +++ b/drivers/s390/block/dasd_diag.c
>> @@ -544,7 +544,7 @@ static struct dasd_ccw_req *dasd_diag_build_cp(struct dasd_device *memdev,
>> }
>> cqr->retries = DIAG_MAX_RETRIES;
>> cqr->buildclk = get_clock();
>> - if (req->cmd_flags & REQ_FAILFAST)
>> + if (blk_noretry_request(req))
>> set_bit(DASD_CQR_FLAGS_FAILFAST, &cqr->flags);
>> cqr->startdev = memdev;
>> cqr->memdev = memdev;
>> diff --git a/drivers/s390/block/dasd_eckd.c b/drivers/s390/block/dasd_eckd.c
>> index 773b3fe..b11a221 100644
>> --- a/drivers/s390/block/dasd_eckd.c
>> +++ b/drivers/s390/block/dasd_eckd.c
>> @@ -1683,7 +1683,7 @@ static struct dasd_ccw_req *dasd_eckd_build_cp(struct dasd_device *startdev,
>> recid++;
>> }
>> }
>> - if (req->cmd_flags & REQ_FAILFAST)
>> + if (blk_noretry_request(req))
>> set_bit(DASD_CQR_FLAGS_FAILFAST, &cqr->flags);
>> cqr->startdev = startdev;
>> cqr->memdev = startdev;
>> diff --git a/drivers/s390/block/dasd_fba.c b/drivers/s390/block/dasd_fba.c
>> index aa0c533..115e032 100644
>> --- a/drivers/s390/block/dasd_fba.c
>> +++ b/drivers/s390/block/dasd_fba.c
>> @@ -355,7 +355,7 @@ static struct dasd_ccw_req *dasd_fba_build_cp(struct dasd_device * memdev,
>> recid++;
>> }
>> }
>> - if (req->cmd_flags & REQ_FAILFAST)
>> + if (blk_noretry_request(req))
>> set_bit(DASD_CQR_FLAGS_FAILFAST, &cqr->flags);
>> cqr->startdev = memdev;
>> cqr->memdev = memdev;
>> diff --git a/drivers/scsi/device_handler/scsi_dh_alua.c b/drivers/scsi/device_handler/scsi_dh_alua.c
>> index 994da56..6bc55a6 100644
>> --- a/drivers/scsi/device_handler/scsi_dh_alua.c
>> +++ b/drivers/scsi/device_handler/scsi_dh_alua.c
>> @@ -109,7 +109,8 @@ static struct request *get_alua_req(struct scsi_device *sdev,
>> }
>>
>> rq->cmd_type = REQ_TYPE_BLOCK_PC;
>> - rq->cmd_flags |= REQ_FAILFAST | REQ_NOMERGE;
>> + rq->cmd_flags |= REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
>> + REQ_FAILFAST_DRIVER | REQ_NOMERGE;
>> rq->retries = ALUA_FAILOVER_RETRIES;
>> rq->timeout = ALUA_FAILOVER_TIMEOUT;
>>
>> diff --git a/drivers/scsi/device_handler/scsi_dh_emc.c b/drivers/scsi/device_handler/scsi_dh_emc.c
>> index b9d23e9..64a56e5 100644
>> --- a/drivers/scsi/device_handler/scsi_dh_emc.c
>> +++ b/drivers/scsi/device_handler/scsi_dh_emc.c
>> @@ -304,7 +304,8 @@ static struct request *get_req(struct scsi_device *sdev, int cmd,
>>
>> rq->cmd[4] = len;
>> rq->cmd_type = REQ_TYPE_BLOCK_PC;
>> - rq->cmd_flags |= REQ_FAILFAST;
>> + rq->cmd_flags |= REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
>> + REQ_FAILFAST_DRIVER;
>> rq->timeout = CLARIION_TIMEOUT;
>> rq->retries = CLARIION_RETRIES;
>>
>> diff --git a/drivers/scsi/device_handler/scsi_dh_hp_sw.c b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
>> index a6a4ef3..08ba1ce 100644
>> --- a/drivers/scsi/device_handler/scsi_dh_hp_sw.c
>> +++ b/drivers/scsi/device_handler/scsi_dh_hp_sw.c
>> @@ -112,7 +112,8 @@ static int hp_sw_tur(struct scsi_device *sdev, struct hp_sw_dh_data *h)
>> return SCSI_DH_RES_TEMP_UNAVAIL;
>>
>> req->cmd_type = REQ_TYPE_BLOCK_PC;
>> - req->cmd_flags |= REQ_FAILFAST;
>> + req->cmd_flags |= REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
>> + REQ_FAILFAST_DRIVER;
>> req->cmd_len = COMMAND_SIZE(TEST_UNIT_READY);
>> memset(req->cmd, 0, MAX_COMMAND_SIZE);
>> req->cmd[0] = TEST_UNIT_READY;
>> @@ -205,7 +206,8 @@ static int hp_sw_start_stop(struct scsi_device *sdev, struct hp_sw_dh_data *h)
>> return SCSI_DH_RES_TEMP_UNAVAIL;
>>
>> req->cmd_type = REQ_TYPE_BLOCK_PC;
>> - req->cmd_flags |= REQ_FAILFAST;
>> + req->cmd_flags |= REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
>> + REQ_FAILFAST_DRIVER;
>> req->cmd_len = COMMAND_SIZE(START_STOP);
>> memset(req->cmd, 0, MAX_COMMAND_SIZE);
>> req->cmd[0] = START_STOP;
>> diff --git a/drivers/scsi/device_handler/scsi_dh_rdac.c b/drivers/scsi/device_handler/scsi_dh_rdac.c
>> index 2dee69d..c504afe 100644
>> --- a/drivers/scsi/device_handler/scsi_dh_rdac.c
>> +++ b/drivers/scsi/device_handler/scsi_dh_rdac.c
>> @@ -228,7 +228,8 @@ static struct request *get_rdac_req(struct scsi_device *sdev,
>> memset(rq->cmd, 0, BLK_MAX_CDB);
>>
>> rq->cmd_type = REQ_TYPE_BLOCK_PC;
>> - rq->cmd_flags |= REQ_FAILFAST | REQ_NOMERGE;
>> + rq->cmd_flags |= REQ_FAILFAST_DEV | REQ_FAILFAST_TRANSPORT |
>> + REQ_FAILFAST_DRIVER;
>
> Was dropping the REQ_NOMERGE intentional?
Yes, and no.
> Everywhere else it seems REQ_FAILFAST was simply replaced with the
> three new flag bits.
We do not need it, because blk_execute_rq_nowait sets it for us, so I
dropped it when I first made the patches a long time ago. It was a
different patch for a while, but I think it got merged with this one a
while back by accident (the alua NOMERGE setting did not get dropped
when it could have, because it was added to scsi-misc at a different time).
Thanks for the review.
next prev parent reply other threads:[~2008-09-02 17:05 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-09-02 16:05 [PATCH 0/14] scsi: scsi_decide_dispostion update Mike Anderson
2008-09-02 16:05 ` [PATCH 01/14] block: separate failfast into multiple bits Mike Anderson
2008-09-02 16:35 ` Grant Grundler
2008-09-02 16:59 ` Mike Christie [this message]
2008-09-02 17:31 ` Mike Anderson
2008-09-03 8:27 ` Boaz Harrosh
2008-09-02 16:05 ` [PATCH 02/14] scsi: add transport host byte errors (v3) Mike Anderson
2008-09-02 16:05 ` [PATCH 03/14] scsi: Move wait_for check Mike Anderson
2008-09-02 16:05 ` [PATCH 04/14] scsi: Move retries check Mike Anderson
2008-09-04 18:27 ` James Bottomley
2008-09-04 19:52 ` Mike Anderson
2008-09-04 21:21 ` James Bottomley
2008-09-02 16:05 ` [PATCH 05/14] scsi: Move blk_noretry_request Mike Anderson
2008-09-02 16:05 ` [PATCH 06/14] scsi: remove maybe_retry Mike Anderson
2008-09-02 16:05 ` [PATCH 07/14] scsi: change return codes in scsi_decide_disposition Mike Anderson
2008-09-02 16:05 ` [PATCH 08/14] scsi: rename scsi_queue_insert to scsi_attempt_requeue_command Mike Anderson
2008-09-02 16:05 ` [PATCH 09/14] scsi: have device handlers return SCSI_MLQUEUE error value Mike Anderson
2008-09-02 16:05 ` [PATCH 10/14] scsi: convert other scsi_check_sense users to new error codes Mike Anderson
2008-09-02 16:05 ` [PATCH 11/14] scsi: fix up SCSI_MLQUEUE defintions and add driver, device and transport ones Mike Anderson
2008-09-02 16:05 ` [PATCH 12/14] scsi: move device online check to scsi_attempt_requeue_command Mike Anderson
2008-09-02 16:05 ` [PATCH 13/14] scsi: remove scsi_device_online from scsi_decide_disposition Mike Anderson
2008-09-02 16:05 ` [PATCH 14/14] scsi: update scsi_log_completion disposition decoding Mike Anderson
2008-09-02 17:03 ` [PATCH 0/14] scsi: scsi_decide_dispostion update Mike Christie
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=48BD70E6.5020702@cs.wisc.edu \
--to=michaelc@cs.wisc.edu \
--cc=agk@redhat.com \
--cc=andmike@linux.vnet.ibm.com \
--cc=grundler@google.com \
--cc=jens.axboe@oracle.com \
--cc=linux-scsi@vger.kernel.org \
--cc=neilb@suse.de \
--cc=schwidefsky@de.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox