Re: [patch 0/3] clean gendisk out of scsi ULD structs
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 - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 0/3] clean gendisk out of scsi ULD structs
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 - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 0/3] clean gendisk out of scsi ULD structs
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 - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 0/3] clean gendisk out of scsi ULD structs
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 - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 0/3] clean gendisk out of scsi ULD structs
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. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 0/3] clean gendisk out of scsi ULD structs
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. - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [patch 0/3] clean gendisk out of scsi ULD structs
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 - To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
