[illumos-Developer] webrev & design change: blkdev should support dump

[Gordon Ross]

On Wed, Jun 29, 2011 at 5:13 PM, Garrett D'Amore <garrett at nexenta.com> wrote:
> As part of some some future support coming (virtio block device
> support), I've added support for dump(9e) to blkdev.  This necessitated
> some changes to the blkdev API, as it turns out that I need to be able
> to *read* a label from a blkdev device even with interrupts disabled.
>
> The webrev is here:
>
> http://mexico.purplecow.org/gdamore/webrev/blkdevdump/

This looks OK, but I think you can do a little better.

I'm not wild about seeing bd_tg_rdwr calling
ddi_get_soft_state on every request.  So I'll
suggest some alternatives:

Revert the changes to pass NULL instead of bp as the
"cookie" to the cmlb functions (etc), and then either:
  (a) pass NULL only in bd_dump and reverse the
      logic for computing kmflag in bd_tg_rdwr, or
  (b) introduce a global: bd_dump_softst and have
      bd_dump set it to the bd it's using for dump;
      then in bd_tg_rdwr, do something like:
          kmflag = (bd == bd_dump_softc) ?
              KM_NOSLEEP : KM_SLEEP;
      (And similar for setting ->i_flags)

I think I prefer (b) because then we don't have an
inconsistency.  The cookie is always bd.

Some other things I noticed:
blkdev.c : 897
   Maybe add a check here?  something like:
    if ((bp->b_flags & B_DONE) == 0)
       cmn_err("device %s did not complete polled I/O" ...)

1268-1270
   I'd suggest putting a { mutex_exit; return; } there
   so you don't mutex_enter/mutex_exit for no reason
   when taking the rv == 0 path.

Otherwise looks good.  API changes look fine.
Will be great to have virtio!

Gordon