Re: [PATCH linux-2.6-block:post-2.6.15 08/10] blk: update IDE to use new blk_ordered

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

 



Hello, Bartlomiej.

Bartlomiej Zolnierkiewicz wrote:
On 11/17/05, Tejun Heo <[email protected]> wrote:

I fail to see how the partial completions (good + bad sectors)
are done in your new scheme, please explain.


It doesn't. I've noted this way back when I posted this patchset the second time.

http://marc.theaimsgroup.com/?l=linux-kernel&m=111795127124020&w=2

Rationales

* The actual barrier IO request is issued as a part of ordered sequence. When any part of this sequence fails (any of leading flush, barrier IO or post flush), the whole sequence should be considered to have failed.

e.g. if leading flush fails, there's no point in reporting partial or full success of barrier IO. Ditto for tailing flush. We can special case when only part of barrier IO fails and report partial barrier success, but 1. benefits are doubtful 2. even if it's implemented, it wouldn't work (see next rationale)

* Barrier requests are not mergeable. ie. Each barrier bio is turned into one barrier request and partially completing the request doesn't result in any successfully completed bio.

* SCSI doesn't handle partial completion of barrier IOs.


-
-static int idedisk_prepare_flush(request_queue_t *q, struct request *rq)
-{
-       ide_drive_t *drive = q->queuedata;
-
-       if (!drive->wcache)
-               return 0;


What does happen if somebody disables drive->wcache later?


Thanks for pointing out. I've moved ordered configuration into write_cache such that ordered is reconfigured when write_cache changes.

There can be in-flight barrier requests which are inconsistent with the newly updated setting, but 1. it's not too unfair to assume that user is responsible for that synchronization 2. the original implementation had the same issue 3. the consequence is not catastrophic.


       memset(rq->cmd, 0, sizeof(rq->cmd));

@@ -735,9 +694,8 @@ static int idedisk_prepare_flush(request
               rq->cmd[0] = WIN_FLUSH_CACHE;


-       rq->flags |= REQ_DRIVE_TASK | REQ_SOFTBARRIER;
+       rq->flags |= REQ_DRIVE_TASK;
       rq->buffer = rq->cmd;
-       return 1;
}

static int idedisk_issue_flush(request_queue_t *q, struct gendisk *disk,
@@ -1012,11 +970,12 @@ static void idedisk_setup (ide_drive_t *
       printk(KERN_INFO "%s: cache flushes %ssupported\n",
               drive->name, barrier ? "" : "not ");
       if (barrier) {
-               blk_queue_ordered(drive->queue, QUEUE_ORDERED_FLUSH);
-               drive->queue->prepare_flush_fn = idedisk_prepare_flush;
-               drive->queue->end_flush_fn = idedisk_end_flush;
+               blk_queue_ordered(drive->queue, QUEUE_ORDERED_DRAIN_FLUSH,
+                                 idedisk_prepare_flush, GFP_KERNEL);
               blk_queue_issue_flush_fn(drive->queue, idedisk_issue_flush);
-       }
+       } else if (!drive->wcache)
+               blk_queue_ordered(drive->queue, QUEUE_ORDERED_DRAIN,
+                                 NULL, GFP_KERNEL);


What does happen if somebody enables drive->wcache later?


ditto.


}

static void ide_cacheflush_p(ide_drive_t *drive)
@@ -1034,6 +993,8 @@ static int ide_disk_remove(struct device
       struct ide_disk_obj *idkp = drive->driver_data;
       struct gendisk *g = idkp->disk;

+       blk_queue_ordered(drive->queue, QUEUE_ORDERED_NONE, NULL, 0);
+


Shouldn't this be done in ide_disk_release()?

Hmmm... The thing is that, AFAIK, requests are not supposed to be issued after ->remove is called (->remove is called only on module unload unless hardware is hot-unplugged and HL driver cannot be unloaded while it's still opened). I think that's why both sd and ide-disk issue the last cache flush in ->remove callbacks but not in ->release.

I think the most natural place to put ordered deconfiguration is right above the last cache flush. Hmmm... If above is not true, I think we should move both ordered deconfig and the last cache flushes to ->release callbacks. What do you think?

Thanks.

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