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@xxxxxx>
> +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.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
