lkml.org 
[lkml]   [2011]   [Aug]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [patch] blk-flush: fix flush policy calculation
    Date
    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
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-08-02 17:31    [W:0.036 / U:0.100 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site