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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <x49mxfru92d.fsf@segfault.boston.devel.redhat.com>
Date:	Tue, 02 Aug 2011 11:28:26 -0400
From:	Jeff Moyer <jmoyer@...hat.com>
To:	Shaohua Li <shli@...nel.org>
Cc:	linux-kernel@...r.kernel.org, Tejun Heo <tj@...nel.org>,
	Jens Axboe <jaxboe@...ionio.com>, msnitzer@...hat.com
Subject: Re: [patch] blk-flush: fix flush policy calculation

Shaohua Li <shli@...nel.org> writes:

> 2011/8/2 Jeff Moyer <jmoyer@...hat.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@...hat.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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/