Messages in this thread Patch in this message |  | | Date | Tue, 8 Jan 2013 10:04:23 -0800 (PST) | Subject | Re: [block] allow blk_flush_policy to return REQ_FSEQ_DATA independent of *FLUSH | From | ajithb.kumar@gmail ... |
| |
Hi, Could you please provide clarity on the following. "> Hmmm... yes, this can become a correctness issue if (and only if) > blk_queue_flush() is called to change q->flush_flags while requests > are in-flight;" Could you please clarify as to why is it a correctness issue only if blk_queue_flush() is used to change flush_flags when requests are in flight ? As I understand, XFS does set WRITE_FLUSH_FUA flag in _xfs_buf_ioapply() function irrespective of whether the underlying device supports flush capabilities or not which will flow into blk_insert_flush(). Is my reading of the code correct and is there a general correctness issue here which potentially results in XFS file system corruption in case of an abrupt shutdown independent of q->flush_flags getting changed while request is in flight.
Thanks, Ajith
On Tuesday, 9 August 2011 20:54:35 UTC+5:30, Jeff Moyer wrote: > Hi, > > blk_insert_flush has the following check: > > /* > * 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; > } > > However, blk_flush_policy will not return with policy set to only > REQ_FSEQ_DATA: > > static unsigned int blk_flush_policy(unsigned int fflags, struct request *rq) > { > unsigned int policy = 0; > > 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; > } > > Notice that REQ_FSEQ_DATA is only set if REQ_FLUSH is set. Fix this > mismatch by moving the setting of REQ_FSEQ_DATA outside of the REQ_FLUSH > check. > > Tejun notes: > > Hmmm... yes, this can become a correctness issue if (and only if) > blk_queue_flush() is called to change q->flush_flags while requests > are in-flight; otherwise, requests wouldn't reach the function at all. > Also, I think it would be a generally good idea to always set > FSEQ_DATA if the request has data. > > Cheers, > Jeff > > Signed-off-by: Jeff Moyer <jmoyer@redhat.com> > Acked-by: Tejun Heo <tj@kernel.org> > > diff --git a/block/blk-flush.c b/block/blk-flush.c > index bb21e4c..2d162bd 100644 > --- a/block/blk-flush.c > +++ b/block/blk-flush.c > @@ -95,11 +95,12 @@ 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; > } > -- > 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/
|  |