[patch 0/3] clean gendisk out of scsi ULD structs

[patch 0/3] clean gendisk out of scsi ULD structs

[Kristen Carlson Accardi]

Since gendisk will now become part of struct scsi_device, we don't need
to store this value in any private data structs where they already store
scsi_device.  This series cleans up a few drivers which did this.

Kristen

-- 

Re: [patch 0/3] clean gendisk out of scsi ULD structs

[Douglas Gilbert]

Kristen Carlson Accardi wrote:
> Since gendisk will now become part of struct scsi_device, we don't need
> to store this value in any private data structs where they already store
> scsi_device.  This series cleans up a few drivers which did this.

Since a scsi_device object is usually a SCSI logical unit,
one wonders why it would contain a gendisk object. Logical
units aren't necessarily disks, they might be enclosures or
just place holders that respond to an INQUIRY (e.g. lun=0
when the enclosing target has other active lus whose lun!=0).

Doug Gilbert

Re: [patch 0/3] clean gendisk out of scsi ULD structs

[Al Viro]

On Thu, Jul 05, 2007 at 02:06:36PM -0700, Kristen Carlson Accardi wrote:
> Since gendisk will now become part of struct scsi_device, we don't need
> to store this value in any private data structs where they already store
> scsi_device.  This series cleans up a few drivers which did this.

What the hell?  gendisks are *NOT* supposed to be embedded into other
data structures, you'll screw up the lifetime rules for them.

Re: [patch 0/3] clean gendisk out of scsi ULD structs

[Al Viro]

On Thu, Jul 05, 2007 at 06:09:27PM -0400, Douglas Gilbert wrote:
> Since a scsi_device object is usually a SCSI logical unit,
> one wonders why it would contain a gendisk object. Logical
> units aren't necessarily disks, they might be enclosures or
> just place holders that respond to an INQUIRY (e.g. lun=0
> when the enclosing target has other active lus whose lun!=0).

gendisk is just a tag for requests coming into given queue.

Re: [patch 0/3] clean gendisk out of scsi ULD structs

[James Bottomley]

On Fri, 2007-07-06 at 00:02 +0100, Al Viro wrote:
> On Thu, Jul 05, 2007 at 02:06:36PM -0700, Kristen Carlson Accardi wrote:
> > Since gendisk will now become part of struct scsi_device, we don't need
> > to store this value in any private data structs where they already store
> > scsi_device.  This series cleans up a few drivers which did this.
> 
> What the hell?  gendisks are *NOT* supposed to be embedded into other
> data structures, you'll screw up the lifetime rules for them.

Don't panic .. they're not ... we have a pointer to the gendisk in our
SCSI structures (properly refcounted).  The reason is historical and
actually goes back to 2002 when we first got rid of the static arrays of
structures we used to keep around.

Doug ... don't think 'disk' when you see 'gendisk' just think of it as a
useful infrastructure library.

James

Re: [patch 0/3] clean gendisk out of scsi ULD structs

[James Bottomley]

On Thu, 2007-07-05 at 14:06 -0700, Kristen Carlson Accardi wrote:
> Since gendisk will now become part of struct scsi_device, we don't need
> to store this value in any private data structs where they already store
> scsi_device.  This series cleans up a few drivers which did this.

Actually, as Al pointed out, we do have lifetime rules issues with doing
this.  The problem is that gendisk itself always has a shorter lifetime
than scsi_device (not much shorter, usually, but if you execute a legal
ULD unbind manoeuvre you'll end up with a dangling gendisk pointer).

The other problem with taking gendisk out of the ULD structure and
putting it into the scsi_device is that for the sg driver, we have two
of them (one for the attached ULD and one for the sg driver).

The fundamental issue seems to be that the gendisk is the holder of all
the other info (queue, ULD etc) not vice versa ... and this patch is
trying to reverse that relationship.

James

Re: [patch 0/3] clean gendisk out of scsi ULD structs

[Douglas Gilbert]

James Bottomley wrote:
> On Thu, 2007-07-05 at 14:06 -0700, Kristen Carlson Accardi wrote:
>> Since gendisk will now become part of struct scsi_device, we don't need
>> to store this value in any private data structs where they already store
>> scsi_device.  This series cleans up a few drivers which did this.
> 
> Actually, as Al pointed out, we do have lifetime rules issues with doing
> this.  The problem is that gendisk itself always has a shorter lifetime
> than scsi_device (not much shorter, usually, but if you execute a legal
> ULD unbind manoeuvre you'll end up with a dangling gendisk pointer).

What about having short-lived scsi_device objects? For example:
one that lives long enough for a pass-through to send a
SCSI command (and receive its response) to one of a target's
well known logical units.

> The other problem with taking gendisk out of the ULD structure and
> putting it into the scsi_device is that for the sg driver, we have two
> of them (one for the attached ULD and one for the sg driver).

Add the bsg driver and that would make three of them. Or; if
the lu's peripheral device type was not of interest to sd, st,
sr, and osst; back to two gendisk objects (i.e. one each
for sg and bsg).

> The fundamental issue seems to be that the gendisk is the holder of all
> the other info (queue, ULD etc) not vice versa ... and this patch is
> trying to reverse that relationship.

A minor issue is the name gendisk ... unless, of course,
you go and look at its definition in linux/genhd.h in
which case the name looks somewhat appropriate. It looks
like a mess [queue, ULD name, major/minor(s), partitions,
capacity, disk_stats, kobjects, etc]. That is a considerable
amount of superfluous information for "just a tag for
requests coming into (a) given queue" when that queue leads
to a non-block device.

Doug Gilbert

Re: [patch 0/3] clean gendisk out of scsi ULD structs

[James Bottomley]

On Fri, 2007-07-06 at 23:25 -0400, Douglas Gilbert wrote:
> James Bottomley wrote:
> > On Thu, 2007-07-05 at 14:06 -0700, Kristen Carlson Accardi wrote:
> >> Since gendisk will now become part of struct scsi_device, we don't need
> >> to store this value in any private data structs where they already store
> >> scsi_device.  This series cleans up a few drivers which did this.
> > 
> > Actually, as Al pointed out, we do have lifetime rules issues with doing
> > this.  The problem is that gendisk itself always has a shorter lifetime
> > than scsi_device (not much shorter, usually, but if you execute a legal
> > ULD unbind manoeuvre you'll end up with a dangling gendisk pointer).
> 
> What about having short-lived scsi_device objects? For example:
> one that lives long enough for a pass-through to send a
> SCSI command (and receive its response) to one of a target's
> well known logical units.

This is sort of what we already do for REPORT_LUNS (except that we use
lun 0 instead of the REPORT LUN well known lun).  What additions do you
want to see?

> > The other problem with taking gendisk out of the ULD structure and
> > putting it into the scsi_device is that for the sg driver, we have two
> > of them (one for the attached ULD and one for the sg driver).
> 
> Add the bsg driver and that would make three of them. Or; if
> the lu's peripheral device type was not of interest to sd, st,
> sr, and osst; back to two gendisk objects (i.e. one each
> for sg and bsg).

gendisk is actually used to facilitate the SCSI ULD infrastructure; bsg,
being block, doesn't actually use it, so we'd still only have two.

> > The fundamental issue seems to be that the gendisk is the holder of all
> > the other info (queue, ULD etc) not vice versa ... and this patch is
> > trying to reverse that relationship.
> 
> A minor issue is the name gendisk ... unless, of course,
> you go and look at its definition in linux/genhd.h in
> which case the name looks somewhat appropriate. It looks
> like a mess [queue, ULD name, major/minor(s), partitions,
> capacity, disk_stats, kobjects, etc]. That is a considerable
> amount of superfluous information for "just a tag for
> requests coming into (a) given queue" when that queue leads
> to a non-block device.

The benefits outweigh the costs ... in the same way that we have a block
queue above all the character devices so we can treat SCSI queueing in a
uniform fashion regardless of device.

James