lkml.org 
[lkml]   [2011]   [Aug]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: block: properly handle flush/fua requests in blk_insert_cloned_request
    On Tue, Aug 09, 2011 at 01:43:47PM -0400, Mike Snitzer wrote:
    > On Tue, Aug 09 2011 at 12:13pm -0400,
    > Tejun Heo <tj@kernel.org> wrote:
    >
    > > Hello,
    > >
    > > On Tue, Aug 09, 2011 at 11:53:51AM -0400, Jeff Moyer wrote:
    > > > Tejun Heo <tj@kernel.org> writes:
    > > > > I'm a bit confused. We still need ELEVATOR_INSERT_FLUSH fix for
    > > > > insertion paths, right? Or is blk_insert_cloned_request() supposed to
    > > > > used only by request based dm which lives under the elevator? If so,
    > > > > it would be great to make that explicit in the comment. Maybe just
    > > > > renaming it to blk_insert_dm_cloned_request() would be better as it
    > > > > wouldn't be safe for other cases anyway.
    > > >
    > > > request-based dm is the only caller at present. I'm not a fan of
    > > > renaming the function, but I'm more than willing to comment it.
    > >
    > > I'm still confused and don't think the patch is correct (you can't
    > > turn off REQ_FUA without decomposing it to data + post flush).
    > >
    > > Going through flush machinery twice is okay and I think is the right
    > > thing to do. At the upper queue, the request is decomposed to member
    > > requests. After decomposition, it's either REQ_FLUSH w/o data or data
    > > request w/ or w/o REQ_FUA. When the decomposed request reaches lower
    > > queue, the lower queue will then either short-circuit it, execute
    > > as-is or decompose data w/ REQ_FUA into data + REQ_FLUSH sequence.
    > >
    > > AFAICS, the breakages are...
    > >
    > > * ELEVATOR_INSERT_FLUSH not used properly from insert paths.
    > >
    > > * Short circuit not kicking in for the dm requests. (the above and the
    > > policy patch should solve this, right?)
    > >
    > > * BUG(!rq->bio || ...) in blk_insert_flush(). I think we can lift
    > > this restriction for empty REQ_FLUSH but also dm can just send down
    > > requests with empty bio.
    >
    > [cc'ing dm-devel]
    >
    > All of these issues have come to light because DM was not setting
    > flush_flags based on the underlying device(s). Now fixed in v3.1-rc1:
    > ed8b752 dm table: set flush capability based on underlying devices
    >
    > Given that commit, and that request-based DM is beneath the elevator, it
    > seems any additional effort to have DM flushes re-enter the flush
    > machinary is unnecessary.
    >
    > We expect:
    > 1) flushes to have gone through the flush machinary
    > 2) no FLUSH/FUA should be entering underlying queues if not supported
    >
    > I think it best to just document the expectation that any FLUSH/FUA
    > request that enters blk_insert_cloned_request() will already match the
    > queue that the request is being sent to. One way to document it is to
    > change Jeff's flag striping in to pure BUG_ON()s, e.g.:
    >
    > ---
    > block/blk-core.c | 8 ++++++++
    > 1 files changed, 8 insertions(+), 0 deletions(-)
    >
    > diff --git a/block/blk-core.c b/block/blk-core.c
    > index b627558..201bb27 100644
    > --- a/block/blk-core.c
    > +++ b/block/blk-core.c
    > @@ -1710,6 +1710,14 @@ int blk_insert_cloned_request(struct request_queue *q, struct request *rq)
    > should_fail_request(&rq->rq_disk->part0, blk_rq_bytes(rq)))
    > return -EIO;
    >
    > + /*
    > + * All FLUSH/FUA requests are expected to have gone through the
    > + * flush machinary. If a request's cmd_flags doesn't match the
    > + * flush_flags of the underlying request_queue it is a bug.
    > + */
    > + BUG_ON((rq->cmd_flags & REQ_FLUSH) && !(q->flush_flags & REQ_FLUSH));
    > + BUG_ON((rq->cmd_flags & REQ_FUA) && !(q->flush_flags & REQ_FUA));
    > +

    Actually this makes sense and is simple. :-) Is BUG_ON() too harsh, how
    about WARN_ONCE() variants? To me system continues to work so warning
    is probably good enough.

    Thanks
    Vivek


    \
     
     \ /
      Last update: 2011-08-09 19:55    [W:3.046 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site