CHROMIUM: verity: only requeue if necessary

CHROMIUM: verity: only requeue if necessary

For the most part, I/Os will be ordered so it is more than likely that
PENDING I/Os will become ready. Instead of requeuing an I/O for which
there were pending I/Os, check the state of all dependent I/Os and
only requeue if necessary.

I also removed the _queue functions. They were two liners that could
easily be open-coded to reduce abstraction. Just makes it easier for
me to see what's really going on.

This change saves about 2 seconds off of a depth=3 boot:

Before:

  seconds_power_on_to_login                                       10.03
  seconds_power_on_to_login{1}                                    9.96
  seconds_power_on_to_login{2}                                    10.4
  seconds_power_on_to_login{3}                                    9.96
  seconds_power_on_to_login{4}                                    10.04
  seconds_power_on_to_login{5}                                    10.04
  seconds_power_on_to_login{6}                                    9.95
  seconds_power_on_to_login{7}                                    10.18
  seconds_power_on_to_login{8}                                    10.13
  seconds_power_on_to_login{9}                                    10.07

0 1740800 verity 0 0 0 438 4017

After:

  seconds_power_on_to_login                                       7.74
  seconds_power_on_to_login{1}                                    7.89
  seconds_power_on_to_login{2}                                    7.84
  seconds_power_on_to_login{3}                                    7.74
  seconds_power_on_to_login{4}                                    7.8
  seconds_power_on_to_login{5}                                    7.85
  seconds_power_on_to_login{6}                                    7.92
  seconds_power_on_to_login{7}                                    7.77
  seconds_power_on_to_login{8}                                    7.81
  seconds_power_on_to_login{9}                                    7.88

0 1740800 verity 0 0 0 0 4014

Notice: No more requeus in the after case.

BUG=chromium-os:9752
TEST=Ran platform_DMVerityCorruption on H/W. Ran platform_BootPerf on H/W.

Change-Id: I360cca3e93db1158694a216aafffc219c9715e88
Signed-off-by: Mandeep Singh Baines <msb@chromium.org>;

R=wad@chromium.org,taysom@chromium.org,ups@chromium.org

Committed: http://chrome-svn/viewvc/chromeos?view=rev&revision=c0b9a0a

[Will Drewry, 2011-03-28 20:35:28]

Nice time savings!  I like the final check before requeuing, and it's nice to
see it works (via the stats).  Two things I'm curious about:
- why is the delay now "1"?
- why did you drop all the REQTRACE code? 

I don't really care for the removal of the queue functions because they are
readable, do what they say, and allow easy instrumentation of common critical
path code.  It's another case where it is undoing code/an abstraction I put in
on purpose.  That said, if you keep it functionally-equivalent by including the
REQTRACE code too, then it becomes a matter of personal style which I won't be
as picky about.

Or do you think the REQTRACE code should be nixed too?  Have you not needed it
for any debugging yet?  Until we have some pain, I expect we won't miss it.  I
only needed it when I was trying to track down some off-by-ones and dumb state
transition issues.

http://codereview.chromium.org/6739006/diff/1/drivers/md/dm-verity.c
File drivers/md/dm-verity.c (left):

http://codereview.chromium.org/6739006/diff/1/drivers/md/dm-verity.c#oldcode722
drivers/md/dm-verity.c:722: ULL(io->block), io);
Not a fan of this part of the change as it means tracing is now a search and
replace operation and not just a flag change or a tweak to the queue function.

http://codereview.chromium.org/6739006/diff/1/drivers/md/dm-verity.c
File drivers/md/dm-verity.c (right):

http://codereview.chromium.org/6739006/diff/1/drivers/md/dm-verity.c#newcode643
drivers/md/dm-verity.c:643: queue_delayed_work(vc->io_queue, &io->work, 1);
Why is this now 1 and not (HZ / 10)?

[Paul T, 2011-03-28 21:49:41]

LGTM

[Mandeep Singh Baines, 2011-03-28 21:57:31]

Fixed. PTAL.

Changed delay back to HZ/10 and restored REQTRACE code.

wad@chromium.org (wad@chromium.org) wrote:
> Nice time savings!  I like the final check before requeuing, and
> it's nice to
> see it works (via the stats).  Two things I'm curious about:
> - why is the delay now "1"?
> - why did you drop all the REQTRACE code?
> 
> I don't really care for the removal of the queue functions because they are
> readable, do what they say, and allow easy instrumentation of common
> critical
> path code.  It's another case where it is undoing code/an
> abstraction I put in

I've worked real hard to simplify the code as much as possible. As a
side-effect, I've been able to remove some abstraction that previously
was necessary. verity_dec_pending uses to be much larger than it is now.
Its small enough now that it makes sense to pull-up kverityd_queue_*.

> on purpose.  That said, if you keep it functionally-equivalent by
> including the
> REQTRACE code too, then it becomes a matter of personal style which
> I won't be
> as picky about.
> 
> Or do you think the REQTRACE code should be nixed too?  Have you not
> needed it
> for any debugging yet?  Until we have some pain, I expect we won't

I have not used it yet.

> miss it.  I
> only needed it when I was trying to track down some off-by-ones and
> dumb state
> transition issues.
> 
> 
> http://codereview.chromium.org/6739006/diff/1/drivers/md/dm-verity.c
> File drivers/md/dm-verity.c (left):
> 
>
http://codereview.chromium.org/6739006/diff/1/drivers/md/dm-verity.c#oldcode722
> drivers/md/dm-verity.c:722: ULL(io->block), io);
> Not a fan of this part of the change as it means tracing is now a search
> and replace operation and not just a flag change or a tweak to the queue
> function.
> 
> http://codereview.chromium.org/6739006/diff/1/drivers/md/dm-verity.c
> File drivers/md/dm-verity.c (right):
> 
>
http://codereview.chromium.org/6739006/diff/1/drivers/md/dm-verity.c#newcode643
> drivers/md/dm-verity.c:643: queue_delayed_work(vc->io_queue, &io->work,
> 1);
> Why is this now 1 and not (HZ / 10)?
> 
> http://codereview.chromium.org/6739006/

[Will Drewry, 2011-03-28 22:13:57]

LGTM - thanks!

On 2011/03/28 21:57:31, Mandeep Singh Baines wrote:
> Fixed. PTAL.
> 
> Changed delay back to HZ/10 and restored REQTRACE code.
> 
> mailto:wad@chromium.org (mailto:wad@chromium.org) wrote:
> > Nice time savings!  I like the final check before requeuing, and
> > it's nice to
> > see it works (via the stats).  Two things I'm curious about:
> > - why is the delay now "1"?
> > - why did you drop all the REQTRACE code?
> > 
> > I don't really care for the removal of the queue functions because they are
> > readable, do what they say, and allow easy instrumentation of common
> > critical
> > path code.  It's another case where it is undoing code/an
> > abstraction I put in
> 
> I've worked real hard to simplify the code as much as possible. As a
> side-effect, I've been able to remove some abstraction that previously
> was necessary. verity_dec_pending uses to be much larger than it is now.

Yup, and we're reaping the rewards!  This wasn't a judgement on the clean up. 
It's a response to your CL description:
""
I also removed the _queue functions. They were two liners that could
easily be open-coded to reduce abstraction. Just makes it easier for
me to see what's really going on.
""
We just don't always agree on what makes code more readable.

> Its small enough now that it makes sense to pull-up kverityd_queue_*.

For example, I still prefer them as functions which can be inlined or whatever
by the compiler. :)

> > on purpose.  That said, if you keep it functionally-equivalent by
> > including the
> > REQTRACE code too, then it becomes a matter of personal style which
> > I won't be
> > as picky about.
> > 
> > Or do you think the REQTRACE code should be nixed too?  Have you not
> > needed it
> > for any debugging yet?  Until we have some pain, I expect we won't
> 
> I have not used it yet.

Good!  Hopefully we can kill that code as everything else stabilizes.  I suspect
that now that the system has been operational with no weird, nagging bugs/edge
cases, we won't need a dedicated tracing infrastructure more then just DM_DEBUG.

> 
> > miss it.  I
> > only needed it when I was trying to track down some off-by-ones and
> > dumb state
> > transition issues.
> > 
> > 
> > http://codereview.chromium.org/6739006/diff/1/drivers/md/dm-verity.c
> > File drivers/md/dm-verity.c (left):
> > 
> >
>
http://codereview.chromium.org/6739006/diff/1/drivers/md/dm-verity.c#oldcode722
> > drivers/md/dm-verity.c:722: ULL(io->block), io);
> > Not a fan of this part of the change as it means tracing is now a search
> > and replace operation and not just a flag change or a tweak to the queue
> > function.
> > 
> > http://codereview.chromium.org/6739006/diff/1/drivers/md/dm-verity.c
> > File drivers/md/dm-verity.c (right):
> > 
> >
>
http://codereview.chromium.org/6739006/diff/1/drivers/md/dm-verity.c#newcode643
> > drivers/md/dm-verity.c:643: queue_delayed_work(vc->io_queue, &io->work,
> > 1);
> > Why is this now 1 and not (HZ / 10)?
> > 
> > http://codereview.chromium.org/6739006/