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

[Garrett D'Amore]

 cc

On Jul 4, 2011, at 10:04 AM, "Gordon Ross" <gordon.w.ross at gmail.com> wrote:

> 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.

This function is only called when reading or altering the label, so it is not a terribly hot function.  Furthermore, ddi_get_soft_state is a fairly cheap and totally lock free function.  So I am not going to change the way this works, but thanks for the feedback.

(The other approaches you made suggested seem less clean since they rely on assumptions such as whether or not it is possible to supply a different cookie to cmlb, or whether dump is always the last entry point called. They may be correct, but I prefer to avoid the assumptions if possible.)


> 
> 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" ...)

Maybe.  Though admittedly I am not that worried.. the code will behave properly in the event that the consuming driver gets it wrong, and adding the bloat there doesn't seem to add much value.

Still if you really want this change, I can make it.

> 
> 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.

Look again, this is part of a loop.  I have to reacquire the mutex and loop around again.

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

Thanks for reviewing.

  -- Garrett
> 
> Gordon