[lkml]   [2002]   [Jul]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    Patch in this message
    SubjectRe: [PATCH] 2.5.28 small REQ_SPECIAL abstraction
    On Sun, Jul 28 2002, Linus Torvalds wrote:
    > On Mon, 29 Jul 2002, Jens Axboe wrote:
    > >
    > > I think Martin's was wrong in concept, mine was wrong in implementation.
    > I don't understand why you think the concept is wrong. Right now all users
    > clearly do want to free the tag on re-issue, and doing so clearly cleans
    > up the code and avoids duplication.
    > So I still don't see the advantage of your patch, even once you've fixed
    > the locking issue.

    Ok... I had two issues with the patch. 1) it did

    rq->flags &= REQ_QUEUED;

    which is just broken. 2) it combined the act of inserting back into the
    block queue with clearing the tag associated with the request. #1 is
    clearly a bug that should be fixed regardless of what we do. Right now,
    yes, the only user of blk_insert_request (SCSI) needs the tag cleared. I
    still don't think that's a reason to mingle the two different tasks into
    one. Code duplication is not an argument, the two scsi_insert_* should
    be folded into one. The only difference is SRpnt->sr_request vs
    SCpnt->request after all.

    > HOWEVER, if you really think that some future users might not want to have
    > the tag played with, how about making the "at_head" thing a flags field,
    > and letting people say so by having "INSERT_NOTAG" (and making the
    > existing bit be INSERT_ATHEAD).
    > So then the SCSI users would look like
    > blk_insert_request(q, SRpnt->sr_request,
    > at_head ? INSERT_ATHEAD : 0,
    > SRpnt)
    > while your future non-tag user might do
    > blk_insert_request(q, newreq,
    > channel);
    > _without_ having that unnecessary code duplication.

    *shrug* I guess we could do that. I don't see any immediate use beyond
    at_head/back and tag clearing.

    I'll back down, it's not a matter of life and death after all. Here's
    the minimal patch that corrects the flag thing, and also makes
    blk_insert_request() conform to kernel style. Are we all happy?

    # This is a BitKeeper generated patch for the following project:
    # Project Name: Linux kernel tree
    # This patch format is intended for GNU patch command version 2.5 or higher.
    # This patch includes the following deltas:
    # ChangeSet 1.509 -> 1.510
    # drivers/block/ll_rw_blk.c 1.96 -> 1.97
    # The following is the BitKeeper ChangeSet Log
    # --------------------------------------------
    # 02/07/29 1.510
    # fix REQ_QUEUED clearing in blk_insert_request()
    # --------------------------------------------
    diff -Nru a/drivers/block/ll_rw_blk.c b/drivers/block/ll_rw_blk.c
    --- a/drivers/block/ll_rw_blk.c Mon Jul 29 12:42:43 2002
    +++ b/drivers/block/ll_rw_blk.c Mon Jul 29 12:42:43 2002
    @@ -1253,7 +1253,7 @@
    * host that is unable to accept a particular command.
    void blk_insert_request(request_queue_t *q, struct request *rq,
    - int at_head, void *data)
    + int at_head, void *data)
    unsigned long flags;

    @@ -1262,15 +1262,18 @@
    * must not attempt merges on this) and that it acts as a soft
    * barrier
    - rq->flags &= REQ_QUEUED;
    rq->flags |= REQ_SPECIAL | REQ_BARRIER;

    rq->special = data;

    spin_lock_irqsave(q->queue_lock, flags);
    - /* If command is tagged, release the tag */
    - if(blk_rq_tagged(rq))
    + /*
    + * If command is tagged, release the tag
    + */
    + if (blk_rq_tagged(rq))
    blk_queue_end_tag(q, rq);
    _elv_add_request(q, rq, !at_head, 0);
    spin_unlock_irqrestore(q->queue_lock, flags);

    Jens Axboe

    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to
    More majordomo info at
    Please read the FAQ at

     \ /
      Last update: 2005-03-22 13:22    [W:0.026 / U:9.812 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site