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

[Jeff Moyer <jmoyer@redhat.com>]

Shaohua Li <shli@kernel.org> writes:

> 2011/8/2 Jeff Moyer <jmoyer@redhat.com>:
>> 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@redhat.com>
>>
>> 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