lkml.org 
[lkml]   [2014]   [Sep]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 5/8] block: introduce blk_flush_queue to drive flush machinery
    On Tue, Sep 09, 2014 at 09:05:46PM +0800, Ming Lei wrote:
    > This patch introduces 'struct blk_flush_queue' and puts all
    > flush machinery related stuff into this strcuture, so that

    s/stuff/fields/
    s/strcuture/structure/

    Looks good, but a few more nitpicks below.

    Reviewed-by: Christoph Hellwig <hch@lst.de>

    > +int blk_init_flush(struct request_queue *q)
    > +{
    > + int ret;
    > + struct blk_flush_queue *fq = kzalloc(sizeof(*fq), GFP_KERNEL);
    >
    > + if (!fq)
    > return -ENOMEM;
    >
    > + q->fq = fq;

    I think it would be cleaner to return the flush data structure and
    assign it in the caller.

    > + INIT_LIST_HEAD(&fq->flush_queue[0]);
    > + INIT_LIST_HEAD(&fq->flush_queue[1]);
    > + INIT_LIST_HEAD(&fq->flush_data_in_flight);
    > +
    > + if (q->mq_ops) {
    > + ret = blk_mq_init_flush(q);

    I think we can just remove blk_mq_init_flush now that it's only
    called in blk-flush.c anyway.

    > void blk_exit_flush(struct request_queue *q)
    > {
    > + if (q->mq_ops)
    > + blk_mq_exit_flush(q);
    > + else {
    > + struct blk_flush_queue *fq = blk_get_flush_queue(q);
    > + kfree(fq->flush_rq);
    > + kfree(fq);
    > + }

    Similarly I would pass the flush structure here.

    > +struct blk_flush_queue {
    > + unsigned int flush_queue_delayed:1;
    > + unsigned int flush_pending_idx:1;
    > + unsigned int flush_running_idx:1;
    > + unsigned long flush_pending_since;
    > + struct list_head flush_queue[2];
    > + struct list_head flush_data_in_flight;
    > + struct request *flush_rq;
    > + spinlock_t mq_flush_lock;
    > +};

    As this isn't really a queue I would call it blk_flush_data.

    > +static inline struct blk_flush_queue *blk_get_flush_queue(
    > + struct request_queue *q)
    > +{
    > + return q->fq;
    > +}

    I don't think there is a need for this helper.



    \
     
     \ /
      Last update: 2014-09-09 21:21    [W:4.246 / U:0.004 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site