Re: [patch] blk-flush: fix flush policy calculation

[Shaohua Li]

2011/8/2 Jeff Moyer <jmoyer@xxxxxxxxxx>:
> Hi,
>
> Reading through the code in blk-flush.c, it appears that there is an
> oversight in the policy returned from blk_flush_policy:
>
>        if (fflags & REQ_FLUSH) {
>                if (rq->cmd_flags & REQ_FLUSH)
>                        policy |= REQ_FSEQ_PREFLUSH;
>                if (blk_rq_sectors(rq))
>                        policy |= REQ_FSEQ_DATA;
>                if (!(fflags & REQ_FUA) && (rq->cmd_flags & REQ_FUA))
>                        policy |= REQ_FSEQ_POSTFLUSH;
>        }
>        return policy;
>
> This means that REQ_FSEQ_DATA can only be set if the queue flush_flags
> include FLUSH and/or FUA.  However, the short-circuit for not issuing
> flushes when the device doesn't need/support them depends on
> REQ_FSEQ_DATA being set while the other two bits are clear:
>
>        /*
>         * If there's data but flush is not necessary, the request can be
>         * processed directly without going through flush machinery.  Queue
>         * for normal execution.
>         */
>        if ((policy & REQ_FSEQ_DATA) &&
>            !(policy & (REQ_FSEQ_PREFLUSH | REQ_FSEQ_POSTFLUSH))) {
>                list_add_tail(&rq->queuelist, &q->queue_head);
>                return;
>        }
>
> Given the code as it stands, I don't think the body of this if statement
> will ever be executed.  I've attached a fix for this below.  It seems
> like this could be both a performance and a correctness issue, though
> I've not run into any problems I can directly attribute to this (perhaps
> due to file systems not issuing flushes when support is not advertised?).
>
> Comments are appreciated.
>
> Cheers,
> Jeff
>
> Signed-off-by: Jeff Moyer <jmoyer@xxxxxxxxxx>
>
> diff --git a/block/blk-flush.c b/block/blk-flush.c
> index bb21e4c..3a06118 100644
> --- a/block/blk-flush.c
> +++ b/block/blk-flush.c
> @@ -95,11 +95,11 @@ static unsigned int blk_flush_policy(unsigned int fflags, struct request *rq)
>  {
>        unsigned int policy = 0;
>
> +       if (blk_rq_sectors(rq))
> +               policy |= REQ_FSEQ_DATA;
>        if (fflags & REQ_FLUSH) {
>                if (rq->cmd_flags & REQ_FLUSH)
>                        policy |= REQ_FSEQ_PREFLUSH;
> -               if (blk_rq_sectors(rq))
> -                       policy |= REQ_FSEQ_DATA;
>                if (!(fflags & REQ_FUA) && (rq->cmd_flags & REQ_FUA))
>                        policy |= REQ_FSEQ_POSTFLUSH;
>        }
> --
__generic_make_request always handles this:
if ((bio->bi_rw & (REQ_FLUSH | REQ_FUA)) && !q->flush_flags) {
                        bio->bi_rw &= ~(REQ_FLUSH | REQ_FUA);
                        if (!nr_sectors) {
                                err = 0;
                                goto end_io;
                        }
                }

Thanks,
Shaohua
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/