NFS "dev_t" issues..

NFS "dev_t" issues..

[Linus Torvalds]

I made a pre6, which contains a new-and-anal "kdev_t".

The format of the thing is the same as it used to be, ie 16 bits of
information, but I made it a structure so that you _couldn't_ mix up
"dev_t" and "kdev_t", or use the "kdev_t" as a number (so when kdev_t
expands to 12+20 bits later in 2.5.x you shouldn't get surprises)

I fixed up the stuff I use and which showed up in compiles (on a source
level, it's so far totally untested), but I'd really like people to check
out their own subsystems. _Especially_ NFS and NFSD, which had several
cases of mixing the two dev_t's around, and which also used them as
numbers. Trond, Neil?

Because the types aren't at all compatible any more, the macros that are
used for user-level "dev_t" are no longer working for a kdev_t. So we have

	dev_t			kdev_t

	MKDEV(major,minor)	mk_kdev(major, minor)
	MAJOR(dev)		major(dev)
	MINOR(dev)		minor(dev)
	dev == dev2		kdev_same(dev, dev2)
	!dev			kdev_none(dev)

and _most_ of the time the fixes are trivial - just translate as above. It
only gets interesting when you have code that looks at the value or starts
mixing the two and compares a "dev_t" against a "kdev_t", which can be
quite interesting.

The knfsd file handle thing is also an issue - Neil, please check out that
what I did looks sane, and would be on-the-wire-compatible with the old
behaviour, even when we expand kdev_t to 12+20 bits, ok?

(Marcelo, for easier backporting of drivers to 2.4.x, we'll probably want
to eventually add

	#define mk_kdev(a,b) MKDEV(a,b)
	#define major(d) MAJOR(d)
	...

to the 2.4.x <linux/kdev_t.h> so that you can move drivers back and
forth).

Apart from some knfsd issues, most of the kdev_t users were proper. The
strict type-checking found one bug in the SCSI layer (which I knew about,
and was one of the impetuses for doing it in the first place), and found a
lot of small "works-but-will-break-with-a-bigger-kdev_t" issues).

			Linus

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

NFS "dev_t" issues..

[Linus Torvalds]

I made a pre6, which contains a new-and-anal "kdev_t".

The format of the thing is the same as it used to be, ie 16 bits of
information, but I made it a structure so that you _couldn't_ mix up
"dev_t" and "kdev_t", or use the "kdev_t" as a number (so when kdev_t
expands to 12+20 bits later in 2.5.x you shouldn't get surprises)

I fixed up the stuff I use and which showed up in compiles (on a source
level, it's so far totally untested), but I'd really like people to check
out their own subsystems. _Especially_ NFS and NFSD, which had several
cases of mixing the two dev_t's around, and which also used them as
numbers. Trond, Neil?

Because the types aren't at all compatible any more, the macros that are
used for user-level "dev_t" are no longer working for a kdev_t. So we have

	dev_t			kdev_t

	MKDEV(major,minor)	mk_kdev(major, minor)
	MAJOR(dev)		major(dev)
	MINOR(dev)		minor(dev)
	dev == dev2		kdev_same(dev, dev2)
	!dev			kdev_none(dev)

and _most_ of the time the fixes are trivial - just translate as above. It
only gets interesting when you have code that looks at the value or starts
mixing the two and compares a "dev_t" against a "kdev_t", which can be
quite interesting.

The knfsd file handle thing is also an issue - Neil, please check out that
what I did looks sane, and would be on-the-wire-compatible with the old
behaviour, even when we expand kdev_t to 12+20 bits, ok?

(Marcelo, for easier backporting of drivers to 2.4.x, we'll probably want
to eventually add

	#define mk_kdev(a,b) MKDEV(a,b)
	#define major(d) MAJOR(d)
	...

to the 2.4.x <linux/kdev_t.h> so that you can move drivers back and
forth).

Apart from some knfsd issues, most of the kdev_t users were proper. The
strict type-checking found one bug in the SCSI layer (which I knew about,
and was one of the impetuses for doing it in the first place), and found a
lot of small "works-but-will-break-with-a-bigger-kdev_t" issues).

			Linus

Re: NFS "dev_t" issues..

[Alexander Viro]

On Tue, 1 Jan 2002, Linus Torvalds wrote:

> Apart from some knfsd issues, most of the kdev_t users were proper. The
> strict type-checking found one bug in the SCSI layer (which I knew about,
> and was one of the impetuses for doing it in the first place), and found a
> lot of small "works-but-will-break-with-a-bigger-kdev_t" issues).

Sigh...  Most of the ->i_dev instances are crap and ought to be replaced
with ->i_sb.  At the very least, let's

--- C2-pre6/fs/namei.c	Tue Jan  1 17:49:13 2002
+++ /tmp/namei.c	Tue Jan  1 17:54:08 2002
@@ -1589,7 +1589,7 @@
 		goto exit_lock;
 
 	error = -EXDEV;
-	if (!kdev_same(dir->i_dev, inode->i_dev))
+	if (dir->i_sb != inode->i_sb)
 		goto exit_lock;
 
 	/*
@@ -1707,7 +1707,7 @@
 	if (error)
 		return error;
 
-	if (!kdev_same(new_dir->i_dev, old_dir->i_dev))
+	if (new_dir->i_sb != old_dir->i_sb)
 		return -EXDEV;
 
 	if (!new_dentry->d_inode)
@@ -1787,7 +1787,7 @@
 	if (error)
 		return error;
 
-	if (!kdev_same(new_dir->i_dev, old_dir->i_dev))
+	if (new_dir->i_sb != old_dir->i_sb)
 		return -EXDEV;
 
 	if (!new_dentry->d_inode)

Re: NFS "dev_t" issues..

[Linus Torvalds]

On Tue, 1 Jan 2002, Alexander Viro wrote:
>
> Sigh...  Most of the ->i_dev instances are crap and ought to be replaced
> with ->i_sb.  At the very least, let's

Looks good, and yes, it makes a lot more sense from an EXDEV standpoint to
check the superblock, as "i_dev" really has no good meaning for many
filesystems.

		Linus

Re: NFS "dev_t" issues..

[Andries.Brouwer]

> I made a pre6, which contains a new-and-anal "kdev_t".

Nice! And even in my original form, with these heavy kdev_same
and kdev_none things :-).

I booted a kernel but had quite a lot to change, the patch
is large. I can send it, but instead:

(i) I changed almost every single MKDEV to mk_kdev.
Of course, the kernel was rather kdev_t clean, it was
not difficult to run with kdev_t a different type, so
there are very few places where this is inappropriate,
and the number of correct places is so large that a
global command, followed by a revert in these very few
places, seems more appropriate than a large patch.
Moreover, probably you and others did part of this already.

Not to be changed:
./init/do_mounts.c: sys_mknod("/dev/console", S_IFCHR|0600, MKDEV(TTYAUX_MAJOR, 1));
./arch/sparc64/solaris/fs.c: sys_mknod((const char *)A(path), mode, MKDEV(major,minor));
./include/linux/kdev_t.h

All else should be changed (or at least: did I change, I may have
overlooked sth).

In do_mounts.c there is a real_root_dev set via /proc, and I left it
an integer, while ROOT_DEV is a kdev_t, which implies the
appropriate conversions there.

(ii) Then there are MAJOR and MINOR. I did not change these to
major and minor, mainly because in some possible futures
it will be necessary to do a lot of grepping for these again -
almost all occurrences should be removed - and major and minor
are such common words. It is nicer to have something more unique,
like kmajor and kminor. Moreover, major and minor do at present
also occur as ordinary variables.
Are kmajor, kminor acceptable?

Andries

Re: NFS "dev_t" issues..

[Greg KH]

On Tue, Jan 01, 2002 at 02:15:58PM -0800, Linus Torvalds wrote:
> 
> I made a pre6, which contains a new-and-anal "kdev_t".

Here's a patch to fix the usb code drivers.

thanks,

greg k-h


diff -Nru a/drivers/usb/acm.c b/drivers/usb/acm.c
--- a/drivers/usb/acm.c	Tue Jan  1 21:33:37 2002
+++ b/drivers/usb/acm.c	Tue Jan  1 21:33:37 2002
@@ -297,7 +297,7 @@
 
 static int acm_tty_open(struct tty_struct *tty, struct file *filp)
 {
-	struct acm *acm = acm_table[MINOR(tty->device)];
+	struct acm *acm = acm_table[minor(tty->device)];
 
 	if (!acm || !acm->dev) return -EINVAL;
 
diff -Nru a/drivers/usb/bluetooth.c b/drivers/usb/bluetooth.c
--- a/drivers/usb/bluetooth.c	Tue Jan  1 21:33:37 2002
+++ b/drivers/usb/bluetooth.c	Tue Jan  1 21:33:37 2002
@@ -360,7 +360,7 @@
 	tty->driver_data = NULL;
 
 	/* get the bluetooth object associated with this tty pointer */
-	bluetooth = get_bluetooth_by_minor (MINOR(tty->device));
+	bluetooth = get_bluetooth_by_minor (minor(tty->device));
 
 	if (bluetooth_paranoia_check (bluetooth, __FUNCTION__)) {
 		return -ENODEV;
diff -Nru a/drivers/usb/dabusb.c b/drivers/usb/dabusb.c
--- a/drivers/usb/dabusb.c	Tue Jan  1 21:33:37 2002
+++ b/drivers/usb/dabusb.c	Tue Jan  1 21:33:37 2002
@@ -579,7 +579,7 @@
 
 static int dabusb_open (struct inode *inode, struct file *file)
 {
-	int devnum = MINOR (inode->i_rdev);
+	int devnum = minor (inode->i_rdev);
 	pdabusb_t s;
 
 	if (devnum < DABUSB_MINOR || devnum >= (DABUSB_MINOR + NRDABUSB))
diff -Nru a/drivers/usb/dc2xx.c b/drivers/usb/dc2xx.c
--- a/drivers/usb/dc2xx.c	Tue Jan  1 21:33:37 2002
+++ b/drivers/usb/dc2xx.c	Tue Jan  1 21:33:37 2002
@@ -298,7 +298,7 @@
 	int			value = 0;
 
 	down (&state_table_mutex);
-	subminor = MINOR (inode->i_rdev) - USB_CAMERA_MINOR_BASE;
+	subminor = minor (inode->i_rdev) - USB_CAMERA_MINOR_BASE;
 	if (subminor < 0 || subminor >= MAX_CAMERAS
 			|| !(camera = minor_data [subminor])) {
 		up (&state_table_mutex);
diff -Nru a/drivers/usb/hiddev.c b/drivers/usb/hiddev.c
--- a/drivers/usb/hiddev.c	Tue Jan  1 21:33:37 2002
+++ b/drivers/usb/hiddev.c	Tue Jan  1 21:33:37 2002
@@ -218,7 +218,7 @@
 static int hiddev_open(struct inode * inode, struct file * file) {
 	struct hiddev_list *list;
 
-	int i = MINOR(inode->i_rdev) - HIDDEV_MINOR_BASE;
+	int i = minor(inode->i_rdev) - HIDDEV_MINOR_BASE;
 
 	if (i >= HIDDEV_MINORS || !hiddev_table[i])
 		return -ENODEV;
diff -Nru a/drivers/usb/printer.c b/drivers/usb/printer.c
--- a/drivers/usb/printer.c	Tue Jan  1 21:33:37 2002
+++ b/drivers/usb/printer.c	Tue Jan  1 21:33:37 2002
@@ -201,7 +201,7 @@
 
 static int usblp_open(struct inode *inode, struct file *file)
 {
-	int minor = MINOR(inode->i_rdev) - USBLP_MINOR_BASE;
+	int minor = minor(inode->i_rdev) - USBLP_MINOR_BASE;
 	struct usblp *usblp;
 	int retval;
 
diff -Nru a/drivers/usb/scanner.c b/drivers/usb/scanner.c
--- a/drivers/usb/scanner.c	Tue Jan  1 21:33:37 2002
+++ b/drivers/usb/scanner.c	Tue Jan  1 21:33:37 2002
@@ -365,7 +365,7 @@
 	struct scn_usb_data *scn;
 	struct usb_device *dev;
 
-	kdev_t scn_minor;
+	int scn_minor;
 
 	int err=0;
 
@@ -432,7 +432,7 @@
 {
 	struct scn_usb_data *scn;
 
-	kdev_t scn_minor;
+	int scn_minor;
 
 	scn_minor = USB_SCN_MINOR (inode);
 
@@ -469,7 +469,7 @@
 	ssize_t bytes_written = 0; /* Overall count of bytes written */
 	ssize_t ret = 0;
 
-	kdev_t scn_minor;
+	int scn_minor;
 
 	int this_write;		/* Number of bytes to write */
 	int partial;		/* Number of bytes successfully written */
@@ -556,8 +556,7 @@
 	ssize_t bytes_read;	/* Overall count of bytes_read */
 	ssize_t ret;
 
-	kdev_t scn_minor;
-
+	int scn_minor;
 	int partial;		/* Number of bytes successfully read */
 	int this_read;		/* Max number of bytes to read */
 	int result;
@@ -671,7 +670,7 @@
 {
 	struct usb_device *dev;
 
-	kdev_t scn_minor;
+	int scn_minor;
 
 	scn_minor = USB_SCN_MINOR(inode);
 
@@ -810,8 +809,7 @@
 
 	int ep_cnt;
 	int ix;
-
-	kdev_t scn_minor;
+	int scn_minor;
 
 	char valid_device = 0;
 	char have_bulk_in, have_bulk_out, have_intr;
diff -Nru a/drivers/usb/scanner.h b/drivers/usb/scanner.h
--- a/drivers/usb/scanner.h	Tue Jan  1 21:33:37 2002
+++ b/drivers/usb/scanner.h	Tue Jan  1 21:33:37 2002
@@ -203,7 +203,7 @@
 #define IS_EP_BULK_OUT(ep) (IS_EP_BULK(ep) && ((ep).bEndpointAddress & USB_ENDPOINT_DIR_MASK) == USB_DIR_OUT)
 #define IS_EP_INTR(ep) ((ep).bmAttributes == USB_ENDPOINT_XFER_INT ? 1 : 0)
 
-#define USB_SCN_MINOR(X) MINOR((X)->i_rdev) - SCN_BASE_MNR
+#define USB_SCN_MINOR(X) minor((X)->i_rdev) - SCN_BASE_MNR
 
 #ifdef DEBUG
 #define SCN_DEBUG(X) X
@@ -243,7 +243,7 @@
 	devfs_handle_t devfs;	/* devfs device */
 	struct urb scn_irq;
 	unsigned int ifnum;	/* Interface number of the USB device */
-	kdev_t scn_minor;	/* Scanner minor - used in disconnect() */
+	int scn_minor;		/* Scanner minor - used in disconnect() */
 	unsigned char button;	/* Front panel buffer */
 	char isopen;		/* Not zero if the device is open */
 	char present;		/* Not zero if device is present */
diff -Nru a/drivers/usb/serial/usbserial.c b/drivers/usb/serial/usbserial.c
--- a/drivers/usb/serial/usbserial.c	Tue Jan  1 21:33:37 2002
+++ b/drivers/usb/serial/usbserial.c	Tue Jan  1 21:33:37 2002
@@ -514,14 +514,14 @@
 	tty->driver_data = NULL;
 
 	/* get the serial object associated with this tty pointer */
-	serial = get_serial_by_minor (MINOR(tty->device));
+	serial = get_serial_by_minor (minor(tty->device));
 
 	if (serial_paranoia_check (serial, __FUNCTION__)) {
 		return -ENODEV;
 	}
 
 	/* set up our port structure making the tty driver remember our port object, and us it */
-	portNumber = MINOR(tty->device) - serial->minor;
+	portNumber = minor(tty->device) - serial->minor;
 	port = &serial->port[portNumber];
 	tty->driver_data = port;
 	port->tty = tty;

NFS "dev_t" issues..

[Trond Myklebust]

>>>>> " " == Linus Torvalds <torvalds@transmeta.com> writes:

     > I made a pre6, which contains a new-and-anal "kdev_t".

<snip>

     > I fixed up the stuff I use and which showed up in compiles (on
     > a source level, it's so far totally untested), but I'd really
     > like people to check out their own subsystems. _Especially_ NFS
     > and NFSD, which had several cases of mixing the two dev_t's
     > around, and which also used them as numbers. Trond, Neil?

Hi Linus & Marcelo,

  Sorry I'm a bit late in replying. AFAICS as of 2.5.2-pre9, all is
more or less well, however when reviewing that code, I noticed what is
probably a bug:

  Given that (for character devices) the value of inode->i_cdev in
2.[45].x depends on the i_rdev, it would appear to be a bug for us to
be able to change inode->i_rdev *after* we've called
init_special_inode().
For this reason, I'd advocate removing the lines in
nfs_refresh_inode() that reset the inode->i_rdev (as per the patch
below) and instead rely on the ordinary stale inode checks to tell us
if/when the inode->i_rdev has changed.

It's hardly a new bug. It's been around ever since we added
init_special_inode(), so it's clearly not one that bites us every day
of the year. Even so, the same patch should probably be applied to
2.4.x.

Cheers,
  Trond


diff -u --recursive --new-file linux-2.5.2-pre9/fs/nfs/inode.c linux-2.5.2-fix/fs/nfs/inode.c
--- linux-2.5.2-pre9/fs/nfs/inode.c	Mon Jan  7 16:57:18 2002
+++ linux-2.5.2-fix/fs/nfs/inode.c	Mon Jan  7 17:08:42 2002
@@ -1107,9 +1107,6 @@
  		inode->i_blocks = fattr->du.nfs2.blocks;
  		inode->i_blksize = fattr->du.nfs2.blocksize;
  	}
- 	inode->i_rdev = NODEV;
- 	if (S_ISCHR(inode->i_mode) || S_ISBLK(inode->i_mode))
- 		inode->i_rdev = to_kdev_t(fattr->rdev);
  
 	/* Update attrtimeo value */
 	if (!invalid && time_after(jiffies, NFS_ATTRTIMEO_UPDATE(inode)+NFS_ATTRTIMEO(inode))) {