Messages in this thread |  | | | From | Jeff Moyer <> | | Subject | Re: [patch] blk-flush: fix flush policy calculation | | Date | Tue, 02 Aug 2011 11:28:26 -0400 |
| |
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/
|  |