[patches] rq_dev removal

[patches] rq_dev removal

[Alexander Viro]

	Folks, ->rq_dev is going away in 2.5 RSN.  The only non-trivial
remaining uses were in SCSI (scsi_lib.c::scsi_get_request_dev() and
uses in st.c, osst.c and sg.c to identify a device).

	Series of patches on ftp.math.psu.edu/pub/viro/P/P*-C45 deals
with that stuff and removes most of the uses of minor() in there.  Please,
review it.

	Contents:

	* osst.c and st.c get (inlined) helper functions - tape_name(STp).
Most of TAPE_NR() uses replaced with use of these animals.
	* osst.c and st.c ->open() puts pointer to Scsi_Tape into
filp->private_data, the same way as it's done by sg.c
	* sd.c and sr.c got a pointer to Scsi_Device_Template in
scsi_disk and scsi_cd resp.  That pointer is initialized to &s{d,r}_template.
disk->private_data points to that field (instead of entire scsi_disk/scsi_cd)
and old uses of disk->private_data are updated to compensate.
	* st.c, osst.c and sg.c got gendisks allocated, but not registered.
Places that set ->rq_dev set ->rq_disk as well.  Scsi_Tape and friends
got a pointer to Scsi_Device_Template (similar to sd/sr above) and
->private_data is set in similar fashion.  Users of ->rq_dev are using
->rq_disk->private_data now.

	* Now we can deal with scsi_get_request_dev().  Indeed, all places
that set ->rq_dev set ->rq_disk, and we are guaranteed that either
	+ ->rq_disk is NULL and ->rq_dev is 0
or
	+ ->rq_disk is non-NULL and
 *(struct Scsi_Device_Template **)rq->rq_disk->private_data
	  is the address of template in question.

In other words, scsi_get_request_dev() is two-liner now:

	struct gendisk *p = req->rq_disk;
	return p ? *(struct Scsi_Device_Template **)p->private_data : NULL;

and ->max_major, ->min_major and ->major from Scsi_Device_Template are
not used anymore (and thus can die).


	Please, review.  If somebody has objections - yell, otherwise this
stuff goes to Linus tomorrow.  Interesting things are in the small patches
- huge pair in the beginning of series is tape_name() stuff, i.e. purely
mechanical work.

Re: [patches] rq_dev removal

[James Bottomley]

viro@math.psu.edu said:
> 	Please, review.  If somebody has objections - yell, otherwise this
> stuff goes to Linus tomorrow.  Interesting things are in the small
> patches - huge pair in the beginning of series is tape_name() stuff,
> i.e. purely mechanical work. 

To facilitate quick review, there's also a BK tree with this in at 

http://linux-scsi.bkbits.net/scsi-viro-2.5

NOTE: The patches in this tree will not be the official BK ones, so watch out 
for contaminating your own trees

James

Re: [patches] rq_dev removal

[Willem Riede]

On 2002.10.31 17:43 Alexander Viro wrote:
> 	Folks, ->rq_dev is going away in 2.5 RSN.  The only non-trivial
> remaining uses were in SCSI (scsi_lib.c::scsi_get_request_dev() and
> uses in st.c, osst.c and sg.c to identify a device).
> 
> 	Series of patches on ftp.math.psu.edu/pub/viro/P/P*-C45 deals
> with that stuff and removes most of the uses of minor() in there.  Please,
> review it.

I reviewed the changes to osst (which I maintain).

[snip]
> 	* sd.c and sr.c got a pointer to Scsi_Device_Template in
> scsi_disk and scsi_cd resp.  That pointer is initialized to &s{d,r}_template.
> disk->private_data points to that field (instead of entire scsi_disk/scsi_cd)
> and old uses of disk->private_data are updated to compensate.
> 	* st.c, osst.c and sg.c got gendisks allocated, but not registered.

How appropriate is it, to have something called '*disk' in a tape driver?
I assume that at least the data in it is valid for this type of driver?

> Places that set ->rq_dev set ->rq_disk as well.  Scsi_Tape and friends
> got a pointer to Scsi_Device_Template (similar to sd/sr above) and
> ->private_data is set in similar fashion.  Users of ->rq_dev are using
> ->rq_disk->private_data now.
> 
> 	* Now we can deal with scsi_get_request_dev().  Indeed, all places
> that set ->rq_dev set ->rq_disk, and we are guaranteed that either
> 	+ ->rq_disk is NULL and ->rq_dev is 0
> or
> 	+ ->rq_disk is non-NULL and
>  *(struct Scsi_Device_Template **)rq->rq_disk->private_data
> 	  is the address of template in question.
> 
> In other words, scsi_get_request_dev() is two-liner now:
> 
> 	struct gendisk *p = req->rq_disk;
> 	return p ? *(struct Scsi_Device_Template **)p->private_data : NULL;
> 
> and ->max_major, ->min_major and ->major from Scsi_Device_Template are
> not used anymore (and thus can die).
> 
> 
> 	Please, review.  If somebody has objections - yell, otherwise this
> stuff goes to Linus tomorrow.  Interesting things are in the small patches
> - huge pair in the beginning of series is tape_name() stuff, i.e. purely
> mechanical work.

I don't object (providing it works, which I expect it does). I am working to
adapt osst for 2.5.x to some other changes, and will incorporate your patches
in that WIP too.

Thanks, Willem Riede.

Re: [patches] rq_dev removal

[Alexander Viro]

On Thu, 31 Oct 2002, Willem Riede wrote:

> > and old uses of disk->private_data are updated to compensate.
> > 	* st.c, osst.c and sg.c got gendisks allocated, but not registered.
> 
> How appropriate is it, to have something called '*disk' in a tape driver?
> I assume that at least the data in it is valid for this type of driver?

About as much as having character device number stored in ->rq_dev?
As for the data in it - it is valid, but obviously not suitable for
opening a block device (->fops is NULL).

It's allocated and initialized, but never activated (i.e. add_disk()
is never called for it).  So it's perfectly valid, but unreachable
from the data structures used by blkdev_open() and friends - from
their POV it isn't there.

Re: [patches] rq_dev removal

[Douglas Gilbert]

Willem Riede wrote:
> On 2002.10.31 17:43 Alexander Viro wrote:
> 
>>	Folks, ->rq_dev is going away in 2.5 RSN.  The only non-trivial
>>remaining uses were in SCSI (scsi_lib.c::scsi_get_request_dev() and
>>uses in st.c, osst.c and sg.c to identify a device).
>>
>>	Series of patches on ftp.math.psu.edu/pub/viro/P/P*-C45 deals
>>with that stuff and removes most of the uses of minor() in there.  Please,
>>review it.
> 
> 
> I reviewed the changes to osst (which I maintain).
> 
> [snip]
> 
>>	* sd.c and sr.c got a pointer to Scsi_Device_Template in
>>scsi_disk and scsi_cd resp.  That pointer is initialized to &s{d,r}_template.
>>disk->private_data points to that field (instead of entire scsi_disk/scsi_cd)
>>and old uses of disk->private_data are updated to compensate.
>>	* st.c, osst.c and sg.c got gendisks allocated, but not registered.
> 
> 
> How appropriate is it, to have something called '*disk' in a tape driver?
> I assume that at least the data in it is valid for this type of driver?

It seems as though sg's (scsi) _device_ abstraction has
been replaced by a _disk_ abstraction. At the very least
this is misleading for the dvd/cd writers, scanners,
tape robots and enclosures that sg typically deals with.

st, osst and sg are character device drivers (not block device
drivers).

<snip>

The patch on sg looks sound.

Doug Gilbert

Re: [patches] rq_dev removal

[James Bottomley]

dougg@torque.net said:
> It seems as though sg's (scsi) _device_ abstraction has been replaced
> by a _disk_ abstraction. At the very least this is misleading for the
> dvd/cd writers, scanners, tape robots and enclosures that sg typically
> deals with. 

Well, yes and no.  SCSI makes use of the block layer queueing facilities for 
all SCSI commands (including those for character devices).  Technically, block 
devices are "disc like" and hence can have gendisk structures associated with 
them.  Since we have a block layer queue for every scsi device, we can make 
use of the gendisk api if we wish.  I suppose you could look at it the other 
way and say that gendisk has become more removed from an actual partitionable 
physical disk, and thus of more use to general block devices and their queues.

What the change does is leverage the major/minor device handling from the 
gendisk and solve our upper layer device identification problem in the block 
queue (this was the scsi_get_request_dev() function).

I think modulo a few list_entry() calls which should be container_of(), the 
changes look good to me.

James