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

From: Tejun Heo
Date: Mon Jun 06 2005 - 21:02:01 EST



Hi, Jeff.

Jeff Garzik wrote:
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@xxxxxxxxx>

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) ?


As READ/WRITE(6) doesn't have FUA field, we have to use WRITE(10) or WRITE(16) on FUA writes. Above addition affects only FUA writes (no effect on READs or normal WRITEs).

FUA write requests are generated only when a device reports DPOFUA capability during sd attach, and only READ/WRITE(>=10) have FUA field, so I think it's logical to assume that devices which report DPOFUA support READ/WRITE(10).

Also, currently all SCSI disks default to READ/WRITE(10) and use_10_for_rw is turned off only when READ/WRITE(10) fails w/ ILLEGAL_REQUEST. So, the assumption is that devices are at least capable of properly terminating READ/WRITE(10)s with ILLEGAL REQUEST when it doesn't like'em (thus turning off FUA).

Hmmm... Do you think I should add more tests before enabling FUA such that we know WRITE(10) is supported (capacity test comes to mind).


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.


Sure.


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.

Meaning...?

--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/