linux-kernel - Re: Block regression since 3.1-rc3

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: <20111011205251.GA6281@google.com>
Date:	Tue, 11 Oct 2011 13:52:51 -0700
From:	Tejun Heo <tj@...nel.org>
To:	Mike Snitzer <snitzer@...hat.com>
Cc:	Shaohua Li <shli@...nel.org>, Jeff Moyer <jmoyer@...hat.com>,
	Jens Axboe <axboe@...nel.dk>,
	device-mapper development <dm-devel@...hat.com>,
	Christophe Saout <christophe@...ut.de>,
	linux-kernel@...r.kernel.org
Subject: Re: Block regression since 3.1-rc3

Hello, Mike.

On Tue, Oct 11, 2011 at 03:56:12PM -0400, Mike Snitzer wrote:
> > I don't object to the immediate fix but think that adding such special
> > case is gonna make the thing even more brittle and make future changes
> > even more difficult.  Those one off cases tend to cause pretty severe
> > headache when someone wants to evolve common code, so let's please
> > find out what went wrong and fix it properly so that everyone follows
> > the same set of rules.
> 
> Are you referring to Jeff's fix as "the immediate fix"?  Christophe
> seems to have had success with it after all.

I meant reverting the previous commit.  Oops... it seems like I
misread Jeff's patch.  Please read on.

> As for the special case that you're suggesting makes the code more
> brittle, etc.  If you could be more specific that'd be awesome.

I was still talking about the previous attempt of making dm treated
special by flush machinery.  (the purity thing someone was talking
about)

> Jeff asked a question about the need to kick the queue in this case (as
> he didn't feel he had a proper justification for why it was needed).
> 
> If we can get a proper patch header together to justify Jeff's patch
> that'd be great.  And then revisit any of the special casing you'd like
> us to avoid in >= 3.2?
> 
> (we're obviously _very_ short on time for a 3.1 fix right now).
...
> > Hmmm... another rather nasty assumption the current flush code makes
> > is that every flush request has either zero or single bio attached to
> > it.  The assumption has always been there for quite some time now.
> 
> OK.
> 
> > That somehow seems broken by request based dm (either that or wrong
> > request is taking INSERT_FLUSH path).
> 
> Where was this issue of a flush having multiple bios reported?

I was misreading Jeff's patch, so the problem is request w/o bio
reaching INSERT_FLUSH, not rq's with multiple bio's.  Sorry about
that.  Having another look...

Ah, okay, so, blk-flush on the lower layer device is seeing
q->flush_rq of the upper layer which doesn't have bio.  Yes, the
BUG_ON() change looks correct to me.  That or we can do

  BUG_ON(rq->bio != rq->bio_tail); /* assumes zero or single bio rq */

As for the blk_run_queue_async(), it's a bit confusing.  Currently,
the block layer isn't clear about who's responsible kicking the queue
after putting a request onto elevator and I suppose Jeff put it there
because blk_insert_cloned_request() doesn't kick the queue.

Hmm... Jeff, you also added blk_run_queue_async() call in
4853abaae7e4a too.  Is there a reason why blk_insert_cloned_request()
isn't calling __blk_run_queue() or async variant of it like
blk_insert_request() does?

At any rate, the queue kicking is a different issue.  Let's not mix
the two here.  The BUG_ON() change looks good to me.

Thank you.

-- 
tejun
--
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/