lkml.org 
[lkml]   [2007]   [Dec]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    SubjectRe: [PATCH 01/28] blk_end_request: add new request completion interface (take 3)
    From
    Hi Boaz,

    On Tue, 04 Dec 2007 15:56:32 +0200, Boaz Harrosh <bharrosh@panasas.com> wrote:
    > > +int blk_end_request(struct request *rq, int uptodate, int nr_bytes)
    > > +{
    > > + struct request_queue *q = rq->q;
    > > + unsigned long flags = 0UL;
    > > +
    > > + if (blk_fs_request(rq) || blk_pc_request(rq)) {
    > > + if (__end_that_request_first(rq, uptodate, nr_bytes))
    > > + return 1;
    > > + }
    > > +
    > > + add_disk_randomness(rq->rq_disk);
    > > +
    > > + spin_lock_irqsave(q->queue_lock, flags);
    > > + complete_request(rq, uptodate);
    > > + spin_unlock_irqrestore(q->queue_lock, flags);
    > > +
    > > + return 0;
    > > +}
    > > +EXPORT_SYMBOL_GPL(blk_end_request);
    > > +
    > > +/**
    > > + * __blk_end_request - Helper function for drivers to complete the request.
    > > + *
    > > + * Description:
    > > + * Must be called with queue lock held unlike blk_end_request().
    > > + **/
    > > +int __blk_end_request(struct request *rq, int uptodate, int nr_bytes)
    > > +{
    > > + if (blk_fs_request(rq) || blk_pc_request(rq)) {
    > > + if (__end_that_request_first(rq, uptodate, nr_bytes))
    > > + return 1;
    > > + }
    > > +
    > > + add_disk_randomness(rq->rq_disk);
    > > +
    > > + complete_request(rq, uptodate);
    > > +
    > > + return 0;
    > > +}
    > > +EXPORT_SYMBOL_GPL(__blk_end_request);
    >
    > I don't like it that you have two Identical but slightly different
    > implementations I wish you would do an internal-with-flags
    > implementation and then API ones can call the internal one. Or maybe
    > just hold the spin_lock just a bit longer and have one call the other.
    > To prove my case see how hard it is to add new code like with
    > the bidi patch, where you need to add exact same code in 3 places.
    > (OK only 2 places actually, if _callback is gone)

    As for the internal-with-flags implementation, I once proposed
    something like below but it was rejected by Jens.
    (http://marc.info/?l=linux-kernel&m=118880584720600&w=2)
    ----------------------------------------------------------------------
    static int internal_function(rq, needlock)
    {
    end_that_request_chunk(rq);

    if (needlock)
    spin_lock_irqsave();
    end_that_request_last(rq);
    if (needlock)
    spin_unlock_irqrestore();
    }

    int blk_end_request(rq)
    {
    return internal_function(rq, 1);
    }

    int __blk_end_request(rq)
    {
    return internal_function(rq, 0);
    }
    ----------------------------------------------------------------------


    As for the holding-queue-lock-longer implementation,
    end_that_request_chunk() completes bios in the request and it can
    reaches filesystem layer and may take time.
    I guess many drivers like scsi are calling end_that_request_chunk()
    without queue's lock because of the reason above.

    I'll try to remove the duplication again by another patch-set
    after blk_end_request interfaces are merged.
    So I would like to leave the duplication for now.
    Is it acceptable for you?

    Thanks,
    Kiyoshi Ueda


    \
     
     \ /
      Last update: 2007-12-04 22:43    [W:0.028 / U:2.684 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site