lkml.org 
[lkml]   [2005]   [Nov]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH linux-2.6-block:post-2.6.15 08/10] blk: update IDE to use new blk_ordered
    Hello, Bartlomiej.

    Bartlomiej Zolnierkiewicz wrote:
    > On 11/17/05, Tejun Heo <htejun@gmail.com> 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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2005-11-18 16:10    [W:0.030 / U:61.172 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site