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

[Jeff Moyer]

Shaohua Li <shli@xxxxxxxxxx> writes:

> 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;
>                         }
>                 }
>

dm-multipath exports flush and fua, even if underlying devices do not
support those flags (but this will change shortly).  It then issues I/O
using blk_insert_cloned_request, which bypasses generic_make_request.
Plus, the logic was clearly wrong so I think we should take the proposed
patch.

Cheers,
Jeff
--
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/