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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




 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 <[email protected]>

 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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux