On Thu, 2011-08-11 at 15:11 -0400, Neil Horman wrote:
> On Thu, Aug 11, 2011 at 06:36:10PM +0000, James Bottomley wrote:
> > > +++ b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
> > > @@ -1303,9 +1303,13 @@ static void cxgb3i_dev_close(struct t3cdev *t3dev)
> > >
> > > if (!cdev || cdev->flags & CXGBI_FLAG_ADAPTER_RESET) {
> > > pr_info("0x%p close, f 0x%x.\n", cdev, cdev ? cdev->flags : 0);
> > > + if (cdev)
> > > + cdev_put(cdev);
> > > + while (cdev && atomic_read(&cdev->use_count.refcount) > 1)
> > > + msleep(1);
> >
> > OK, so this was what leapt out the last time as a big no-no. Firstly,
> > do you really have to wait for destruction? If so, use a completion.
> >
> So, this is really the crux of the issue. I'm sorry If it wasn't sufficiently
> clear in the explination above. Let me try again:
>
> The root of the problem is the racy assignments of ->lldev in offload_open and
> ofload_close. cdev->lldev is dereferenced in init_act_open every time a new
> iscsi connection is established. If we encounter an adapter error and go down
> the cxgb3 EEH path, we wind up in offload_close, which sets->lldev to NULL. If
> this occurs while a connection is being established in init_act_open, we can get
> a NULL pointer as ->lldev, and we get the oops above.
>
> So my approach to fix this was to serialize the setting of cdev->lldev to NULL behind
> a guarantee that there are no current users of cdev. Since the cxgb3i driver
> only grabs cdevs via cxgbi_find_by_lldev or find_by_netdev, a reference count
> seemed like the way to go. Not sure I could do the same efficiently with a
> completion.
So this basically means you use the above code to hold offload_close()
at cxgb3_remove_clients() until there are no more references ... which
is an undefined length of time.
That's actually rather a horrible hack.
Why does offload_close() have to NULL out tdev->lldev? Since the model
is effectively refcounted
> I had also thought about moving the NULL setting into part of the release
> routine of kref_put (or previously my raw atomic use), but since that code (or
> the code that accesses it isn't at all serialized with a spinlock, I figured a
> reference count would be more efficient. It all boils down (In my mind) to the
> fact that we're just modifying a pointer here, not actually dstroying a struct.
>
> Truthfully, the way this code works probably needs a larger re-working than what
> I'm doing here, given how often I got turned around in it, but I'm nowhere near
> familiar enough with this driver to do that effectively. I'm just trying to fix
> the oops at hand.
Can't this all be fixed just by making init_act_open() check for a NULL
t3 dev and return error if it is?
This is effectively equivalent to a cancellation path. Now that's not
always possible, so if it's not possible, the rework would have to
refcount around lldev I think.
James
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
