Re: [usb-storage] UAS hangs khubd on USB disconnect

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, 2013-12-13 at 19:48 -0500, Alan Stern wrote:
> On Fri, 13 Dec 2013, James Bottomley wrote:
> 
> > On Fri, 2013-12-13 at 16:06 -0500, Alan Stern wrote:
> > > On Fri, 13 Dec 2013, James Bottomley wrote:
> > > 
> > > > Actually, I think I have this figured out.  There's a thinko in one of
> > > > the scsi_target_reap() cases.  The original (and still existing) problem
> > > > with targets is that nothing creates them and nothing destroys them, so,
> > > > while we could rely on the refcounting of the device model to preserve
> > > > the actual target object, we had no idea when to remove it from
> > > > visibility.  That was the job of the reap reference, to track
> > > > visibility.  It looks like the reap on device last put is occurring too
> > > > late.  I think we should reap immediately after doing the sdev
> > > > device_del, so does this fix the warn on? (I'm not sure because no-one
> > > > has actually posted a backtrace, but it sounds like this is the
> > > > problem).
> > > > 
> > > > James
> > > > 
> > > > ---
> > > > 
> > > > diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
> > > > index 8ff62c2..98d4eb3 100644
> > > > --- a/drivers/scsi/scsi_sysfs.c
> > > > +++ b/drivers/scsi/scsi_sysfs.c
> > > > @@ -399,8 +399,6 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
> > > >  	/* NULL queue means the device can't be used */
> > > >  	sdev->request_queue = NULL;
> > > >  
> > > > -	scsi_target_reap(scsi_target(sdev));
> > > > -
> > > >  	kfree(sdev->inquiry);
> > > >  	kfree(sdev);
> > > >  
> > > > @@ -1044,6 +1042,8 @@ void __scsi_remove_device(struct scsi_device *sdev)
> > > >  	} else
> > > >  		put_device(&sdev->sdev_dev);
> > > >  
> > > > +	scsi_target_reap(scsi_target(sdev));
> > > > +
> > > >  	/*
> > > >  	 * Stop accepting new requests and wait until all queuecommand() and
> > > >  	 * scsi_run_queue() invocations have finished before tearing down the
> > > 
> > > This is not right.  The problem is that you don't keep track explicitly 
> > > of the number of references to a target; you rely implicitly on 
> > > starget->devices being non-empty.  starget->reap_ref is only a count of 
> > > local operations that should block removal.
> > 
> > No, it was supposed explicitly to be a visibility counter to answer the
> > question when can we delete the target.  It's incremented every time we
> > add a device to the target (and when we do an operation that may remove
> > one to keep an atomic context before we blow it away) and decremented
> > every time we remove one.
> 
> Sorry, but you're wrong.  starget->reap_ref is _not_ incremented every 
> time we add a device to the target.  That's one of the things we need to 
> fix.

Well, then we would have a pretty astonishing cockup in the code.  The
found case of scsi_alloc_target increments the reference each time it's
called, so scsi_add_device() definitely behaves like this.  I suppose
it's possible the list_empty() check is covering a miscount in some of
the other probing routines, but that would mean we have stale targets
for a lot of our use cases.  I'll audit the code.

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




[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [SCSI Target Devel]     [Linux SCSI Target Infrastructure]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Linux IIO]     [Samba]     [Device Mapper]
  Powered by Linux