Re: [Xen-devel] [PATCH V4 2/4] Introduce xen-scsifront module

[Juergen Gross <jgross@xxxxxxxx>]

On 08/11/2014 07:50 PM, Christoph Hellwig wrote:
> On Mon, Aug 11, 2014 at 12:27:29PM +0200, Juergen Gross wrote:
> > What do you mean with "unusual"? You mean transferring the EH action to
> > Dom0?
> Yes.  Note that hyperv tries something similar and they've run into
> timeout issues, you might want to read up the recent thread on that.
>
> > > > +                               } else {
> > > > +                                       xenbus_printf(XBT_NIL, dev->nodename,
> > > > +                                                     state_str, "%d",
> > > > +                                                     XenbusStateConnected);
> > > > +                               }
> > > Just print this message in ->slave_configure.
> > This is calling for problems, I think. xenbus_printf() is not just a
> > printing function, but it changes an entry in the xenstore. And this
> > requires locking, switching threads, ...
> >
> > I doubt doing this while holding SCSI-internal locks is a good idea.
> Oh, I thought xenbus_printf was just a logging wrapper.
>
> Doing major work in the slave_* callouts is not a problem, that's what
> they were designed for.
Okay.

> For the successful case the xenbus_printf should be done in
> ->slave_configure.  For the failure case you probably want to do it
> from ->slave_destroy based on the absence of a flag set in ->slave_configure,
> e.g. in slave_configure:
>
>         sdev->hostdata = (void *)1UL;
>
> and in ->slave_destroy:
>
>         if (!sdev->hostdata)
I don't think I'll need the flag. The action is the same if the device
is being destroyed again because of already existing or when it is
really removed.

>                 ...
>
> although you might see something like this based on external scanning
> through procfs/sysfs as mentioned earlier, so please take a look at
> how all these corner cases could effect you.
I'll add a check if .slave_configure() and .slave_destroy() are running
in the same task as scsi_add_device() or scsi_remove_device(). This
should rule out all of these corner cases.


Juergen


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel