Re: [PATCH Linux 2.6.12-rc5-mm2 06/09] blk: update SCSI to use the new blk_ordered

Jeff Garzik
2005-06-05T07:09:22Z

Tejun Heo wrote:
> 06_blk_update_scsi_to_use_new_ordered.patch
>
> All ordered request related stuff delegated to HLD. Midlayer
> now doens't deal with ordered setting or prepare_flush
> callback. sd.c updated to deal with blk_queue_ordered
> setting. Currently, ordered tag isn't used as SCSI midlayer
> cannot guarantee request ordering.
>
> Signed-off-by: Tejun Heo <htejun@gmail.com>
>
> drivers/scsi/hosts.c | 9 -----
> drivers/scsi/scsi_lib.c | 46 --------------------------
> drivers/scsi/sd.c | 79 +++++++++++++++++++--------------------------
> include/scsi/scsi_driver.h | 1
> include/scsi/scsi_host.h | 1
> 5 files changed, 34 insertions(+), 102 deletions(-)
>
> Index: blk-fixes/drivers/scsi/sd.c
> ===================================================================
> --- blk-fixes.orig/drivers/scsi/sd.c 2005-06-05 14:53:32.000000000 +0900
> +++ blk-fixes/drivers/scsi/sd.c 2005-06-05 14:53:35.000000000 +0900
> @@ -103,6 +103,7 @@ struct scsi_disk {
> u8 write_prot;
> unsigned WCE : 1; /* state of disk WCE bit */
> unsigned RCD : 1; /* state of disk RCD bit, unused */
> + unsigned DPOFUA : 1; /* state of disk DPOFUA bit */
> };
>
> static DEFINE_IDR(sd_index_idr);
> @@ -122,8 +123,7 @@ static void sd_shutdown(struct device *d
> static void sd_rescan(struct device *);
> static int sd_init_command(struct scsi_cmnd *);
> static int sd_issue_flush(struct device *, sector_t *);
> -static void sd_end_flush(request_queue_t *, struct request *);
> -static int sd_prepare_flush(request_queue_t *, struct request *);
> +static void sd_prepare_flush(request_queue_t *, struct request *);
> static void sd_read_capacity(struct scsi_disk *sdkp, char *diskname,
> struct scsi_request *SRpnt, unsigned char *buffer);
>
> @@ -138,8 +138,6 @@ static struct scsi_driver sd_template =
> .rescan = sd_rescan,
> .init_command = sd_init_command,
> .issue_flush = sd_issue_flush,
> - .prepare_flush = sd_prepare_flush,
> - .end_flush = sd_end_flush,
> };
>
> /*
> @@ -346,6 +344,7 @@ static int sd_init_command(struct scsi_c
>
> if (block > 0xffffffff) {
> SCpnt->cmnd[0] += READ_16 - READ_6;
> + SCpnt->cmnd[1] |= blk_fua_rq(rq) ? 0x8 : 0;
> SCpnt->cmnd[2] = sizeof(block) > 4 ? (unsigned char) (block >> 56) & 0xff : 0;
> SCpnt->cmnd[3] = sizeof(block) > 4 ? (unsigned char) (block >> 48) & 0xff : 0;
> SCpnt->cmnd[4] = sizeof(block) > 4 ? (unsigned char) (block >> 40) & 0xff : 0;
> @@ -360,11 +359,12 @@ static int sd_init_command(struct scsi_c
> SCpnt->cmnd[13] = (unsigned char) this_count & 0xff;
> SCpnt->cmnd[14] = SCpnt->cmnd[15] = 0;
> } else if ((this_count > 0xff) || (block > 0x1fffff) ||
> - SCpnt->device->use_10_for_rw) {
> + SCpnt->device->use_10_for_rw || blk_fua_rq(rq)) {

This seems suspicious, like it would cause unwanted use of READ(10) for
some devices that prefer READ(6) ?

> if (this_count > 0xffff)
> this_count = 0xffff;
>
> SCpnt->cmnd[0] += READ_10 - READ_6;
> + SCpnt->cmnd[1] |= blk_fua_rq(rq) ? 0x8 : 0;
> SCpnt->cmnd[2] = (unsigned char) (block >> 24) & 0xff;
> SCpnt->cmnd[3] = (unsigned char) (block >> 16) & 0xff;
> SCpnt->cmnd[4] = (unsigned char) (block >> 8) & 0xff;
> @@ -739,43 +739,12 @@ static int sd_issue_flush(struct device
> return sd_sync_cache(sdp);
> }
>
> -static void sd_end_flush(request_queue_t *q, struct request *flush_rq)
> +static void sd_prepare_flush(request_queue_t *q, struct request *rq)
> {
> - struct request *rq = flush_rq->end_io_data;
> - struct scsi_cmnd *cmd = rq->special;
> - unsigned int bytes = rq->hard_nr_sectors << 9;
> -
> - if (!flush_rq->errors) {
> - spin_unlock(q->queue_lock);
> - scsi_io_completion(cmd, bytes, 0);
> - spin_lock(q->queue_lock);
> - } else if (blk_barrier_postflush(rq)) {
> - spin_unlock(q->queue_lock);
> - scsi_io_completion(cmd, 0, bytes);
> - spin_lock(q->queue_lock);
> - } else {
> - /*
> - * force journal abort of barriers
> - */
> - end_that_request_first(rq, -EOPNOTSUPP, rq->hard_nr_sectors);
> - end_that_request_last(rq, -EOPNOTSUPP);
> - }
> -}
> -
> -static int sd_prepare_flush(request_queue_t *q, struct request *rq)
> -{
> - struct scsi_device *sdev = q->queuedata;
> - struct scsi_disk *sdkp = dev_get_drvdata(&sdev->sdev_gendev);
> -
> - if (sdkp->WCE) {
> - memset(rq->cmd, 0, sizeof(rq->cmd));
> - rq->flags |= REQ_BLOCK_PC | REQ_SOFTBARRIER;
> - rq->timeout = SD_TIMEOUT;
> - rq->cmd[0] = SYNCHRONIZE_CACHE;
> - return 1;
> - }
> -
> - return 0;
> + memset(rq->cmd, 0, sizeof(rq->cmd));
> + rq->flags |= REQ_BLOCK_PC | REQ_SOFTBARRIER;
> + rq->timeout = SD_TIMEOUT;
> + rq->cmd[0] = SYNCHRONIZE_CACHE;
> }
>
> static void sd_rescan(struct device *dev)
> @@ -1433,10 +1402,13 @@ sd_read_cache_type(struct scsi_disk *sdk
> sdkp->RCD = 0;
> }
>
> + sdkp->DPOFUA = (data.device_specific & 0x10) != 0;
> +
> ct = sdkp->RCD + 2*sdkp->WCE;
>
> - printk(KERN_NOTICE "SCSI device %s: drive cache: %s\n",
> - diskname, types[ct]);
> + printk(KERN_NOTICE "SCSI device %s: drive cache: %s%s\n",
> + diskname, types[ct],
> + sdkp->DPOFUA ? " with forced unit access" : "");

This is IMO a bit verbose. Just " w/ FUA" might be better.

> return;
> }
> @@ -1469,6 +1441,7 @@ static int sd_revalidate_disk(struct gen
> struct scsi_device *sdp = sdkp->device;
> struct scsi_request *sreq;
> unsigned char *buffer;
> + unsigned ordered;
>
> SCSI_LOG_HLQUEUE(3, printk("sd_revalidate_disk: disk=%s\n", disk->disk_name));
>
> @@ -1514,7 +1487,22 @@ static int sd_revalidate_disk(struct gen
> sreq, buffer);
> sd_read_cache_type(sdkp, disk->disk_name, sreq, buffer);
> }
> -
> +
> + /*
> + * We now have all cache related info, determine how we deal
> + * with ordered requests. Note that as the current SCSI
> + * dispatch function can alter request order, we cannot use
> + * QUEUE_ORDERED_TAG_* even when ordered tag is supported.
> + */
> + if (sdkp->WCE)
> + ordered = sdkp->DPOFUA
> + ? QUEUE_ORDERED_DRAIN_FUA : QUEUE_ORDERED_DRAIN_FLUSH;

Certainly 'DPO and FUA' implies we have FUA, but I wonder if this test
is unnecessarily narrow.

Jeff