Re: [PATCH] Re: NFS "dev_t" issues..

Re: [PATCH] Re: NFS "dev_t" issues..

[Andries.Brouwer]

    From alessandro.suardi@oracle.com Thu Jan  3 00:22:23 2002

    Andries.Brouwer@cwi.nl wrote:
    > 
    > I see lots of people sending patches for kdev_t.
    > In order to possibly avoid duplication of work,
    > I put my patch at ftp.kernel.org:
    > 2.5.2pre6-kdev_t-diff (415841 bytes)
    > 
    > It has kminor and kmajor, but if that is not desired
    > a very simple edit on the patch will turn them into
    > minor and major.
    > 
    > (It is incomplete, but a good start.)

    It doesn't build for me in make_rdonly() in ext3 with debug
     configured in:

Yes. Still w.i.p. but a better version is now
2.5.2pre6-kdev_t-diff-v3 (443024 bytes).

Andries

Re: [PATCH] Re: NFS "dev_t" issues..

[Alessandro Suardi]

Andries.Brouwer@cwi.nl wrote:
> 
>     From alessandro.suardi@oracle.com Thu Jan  3 00:22:23 2002
> 
>     It doesn't build for me in make_rdonly() in ext3 with debug
>      configured in:
> 
> Yes. Still w.i.p. but a better version is now
> 2.5.2pre6-kdev_t-diff-v3 (443024 bytes).

Compiles and boots fine with my config - laptop including ext2,
 ext3, fat, vfat, reiserfs, iso9660, tmpfs, ramfs, IrDA, PPP,
 Xircom Cardbus (old driver), parport, floppy, ramdisk, loop,
 IDE, ipv4, nfs/nfsd, samba, maestro3, usb UHCI and more. Not
 everything tested and one possibly related warning:

gcc -D__KERNEL__ -I/usr/src/linux-2.5.2-pre6/include -Wall -Wstrict-prototypes -Wno-trigraphs -O2 -fomit-frame-pointer -fno-strict-aliasing -fno-common -pipe -mpreferred-stack-boundary=2 -march=i686
-DMODULE   -c -o write.o write.c
write.c: In function `nfs_commit_done':
write.c:1204: warning: unsigned int format, kdev_t arg (arg 2)

Quite good so far :) thanks a lot,

--alessandro

 "this machine will, will not communicate
   these thoughts and the strain I am under
  be a world child, form a circle before we all go under"
                         (Radiohead, "Street Spirit [fade out]")

Re: [PATCH] Re: NFS "dev_t" issues..

[Andries.Brouwer]

I see lots of people sending patches for kdev_t.
In order to possibly avoid duplication of work,
I put my patch at ftp.kernel.org:
2.5.2pre6-kdev_t-diff (415841 bytes)

It has kminor and kmajor, but if that is not desired
a very simple edit on the patch will turn them into
minor and major.

(It is incomplete, but a good start.)

Andries

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

[PATCH] Re: NFS "dev_t" issues..

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 756 bytes --]

Linus Torvalds wrote:
> 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, assignments "kdev_t foo = 0" become "kdev_t foo = NODEV".

At least I hope so ;-)  The below patch fixes up rd.c and loop.c.

-- 
Jeff Garzik      | Only so many songs can be sung
Building 1024    | with two lips, two lungs, and one tongue.
MandrakeSoft     |         - nomeansno

[-- Attachment #2: block.patch --]
[-- Type: text/plain, Size: 4331 bytes --]

Index: drivers/block/rd.c
===================================================================
RCS file: /cvsroot/gkernel/linux_2_5/drivers/block/rd.c,v
retrieving revision 1.7
diff -u -r1.7 rd.c
--- drivers/block/rd.c	2001/12/20 18:55:32	1.7
+++ drivers/block/rd.c	2002/01/01 22:35:11
@@ -246,7 +246,7 @@
 	unsigned long offset, len;
 	int rw = sbh->bi_rw;
 
-	minor = MINOR(sbh->bi_dev);
+	minor = minor(sbh->bi_dev);
 
 	if (minor >= NUM_RAMDISKS)
 		goto fail;
@@ -280,10 +280,10 @@
 	int error = -EINVAL;
 	unsigned int minor;
 
-	if (!inode || !inode->i_rdev) 	
+	if (!inode || kdev_none(inode->i_rdev))
 		goto out;
 
-	minor = MINOR(inode->i_rdev);
+	minor = minor(inode->i_rdev);
 
 	switch (cmd) {
 		case BLKFLSBUF:
@@ -407,7 +407,7 @@
 		rd_bdev[i] = NULL;
 		if (bdev)
 			blkdev_put(bdev, BDEV_FILE);
-		destroy_buffers(MKDEV(MAJOR_NR, i));
+		destroy_buffers(mk_kdev(MAJOR_NR, i));
 	}
 
 	devfs_unregister (devfs_handle);
@@ -449,7 +449,7 @@
 			       &rd_bd_op, NULL);
 
 	for (i = 0; i < NUM_RAMDISKS; i++)
-		register_disk(NULL, MKDEV(MAJOR_NR,i), 1, &rd_bd_op, rd_size<<1);
+		register_disk(NULL, mk_kdev(MAJOR_NR,i), 1, &rd_bd_op, rd_size<<1);
 
 #ifdef CONFIG_BLK_DEV_INITRD
 	/* We ought to separate initrd operations here */
Index: drivers/block/loop.c
===================================================================
RCS file: /cvsroot/gkernel/linux_2_5/drivers/block/loop.c,v
retrieving revision 1.7
diff -u -r1.7 loop.c
--- drivers/block/loop.c	2001/12/30 22:22:58	1.7
+++ drivers/block/loop.c	2002/01/01 22:35:21
@@ -155,8 +155,8 @@
 {
 	if (S_ISREG(lo_dentry->d_inode->i_mode))
 		return (lo_dentry->d_inode->i_size - lo->lo_offset) >> BLOCK_SIZE_BITS;
-	if (blk_size[MAJOR(lodev)])
-		return blk_size[MAJOR(lodev)][MINOR(lodev)] -
+	if (blk_size[major(lodev)])
+		return blk_size[major(lodev)][minor(lodev)] -
                                 (lo->lo_offset >> BLOCK_SIZE_BITS);
 	return MAX_DISK_SIZE;
 }
@@ -379,7 +379,7 @@
  */
 static int loop_end_io_transfer(struct bio *bio, int nr_sectors)
 {
-	struct loop_device *lo = &loop_dev[MINOR(bio->bi_dev)];
+	struct loop_device *lo = &loop_dev[minor(bio->bi_dev)];
 	int uptodate = test_bit(BIO_UPTODATE, &bio->bi_flags);
 
 	if (!uptodate || bio_rw(bio) == WRITE) {
@@ -429,10 +429,10 @@
 	unsigned long IV;
 	int rw = bio_rw(rbh);
 
-	if (MINOR(rbh->bi_dev) >= max_loop)
+	if (minor(rbh->bi_dev) >= max_loop)
 		goto out;
 
-	lo = &loop_dev[MINOR(rbh->bi_dev)];
+	lo = &loop_dev[minor(rbh->bi_dev)];
 	spin_lock_irq(&lo->lo_lock);
 	if (lo->lo_state != Lo_bound)
 		goto inactive;
@@ -615,7 +615,7 @@
 
 	if (S_ISBLK(inode->i_mode)) {
 		lo_device = inode->i_rdev;
-		if (lo_device == dev) {
+		if (kdev_same(lo_device, dev)) {
 			error = -EBUSY;
 			goto out;
 		}
@@ -725,7 +725,7 @@
 	loop_release_xfer(lo);
 	lo->transfer = NULL;
 	lo->ioctl = NULL;
-	lo->lo_device = 0;
+	lo->lo_device = NODEV;
 	lo->lo_encrypt_type = 0;
 	lo->lo_offset = 0;
 	lo->lo_encrypt_key_size = 0;
@@ -818,12 +818,12 @@
 
 	if (!inode)
 		return -EINVAL;
-	if (MAJOR(inode->i_rdev) != MAJOR_NR) {
+	if (major(inode->i_rdev) != MAJOR_NR) {
 		printk(KERN_WARNING "lo_ioctl: pseudo-major != %d\n",
 		       MAJOR_NR);
 		return -ENODEV;
 	}
-	dev = MINOR(inode->i_rdev);
+	dev = minor(inode->i_rdev);
 	if (dev >= max_loop)
 		return -ENODEV;
 	lo = &loop_dev[dev];
@@ -873,11 +873,11 @@
 
 	if (!inode)
 		return -EINVAL;
-	if (MAJOR(inode->i_rdev) != MAJOR_NR) {
+	if (major(inode->i_rdev) != MAJOR_NR) {
 		printk(KERN_WARNING "lo_open: pseudo-major != %d\n", MAJOR_NR);
 		return -ENODEV;
 	}
-	dev = MINOR(inode->i_rdev);
+	dev = minor(inode->i_rdev);
 	if (dev >= max_loop)
 		return -ENODEV;
 
@@ -900,12 +900,12 @@
 
 	if (!inode)
 		return 0;
-	if (MAJOR(inode->i_rdev) != MAJOR_NR) {
+	if (major(inode->i_rdev) != MAJOR_NR) {
 		printk(KERN_WARNING "lo_release: pseudo-major != %d\n",
 		       MAJOR_NR);
 		return 0;
 	}
-	dev = MINOR(inode->i_rdev);
+	dev = minor(inode->i_rdev);
 	if (dev >= max_loop)
 		return 0;
 
@@ -1016,7 +1016,7 @@
 	blk_size[MAJOR_NR] = loop_sizes;
 	blksize_size[MAJOR_NR] = loop_blksizes;
 	for (i = 0; i < max_loop; i++)
-		register_disk(NULL, MKDEV(MAJOR_NR, i), 1, &lo_fops, 0);
+		register_disk(NULL, mk_kdev(MAJOR_NR, i), 1, &lo_fops, 0);
 
 	printk(KERN_INFO "loop: loaded (max %d devices)\n", max_loop);
 	return 0;

Re: [PATCH] Re: NFS "dev_t" issues..

[Linus Torvalds]

On Tue, 1 Jan 2002, Jeff Garzik wrote:
>
> And, assignments "kdev_t foo = 0" become "kdev_t foo = NODEV".

Indeed.

> At least I hope so ;-)  The below patch fixes up rd.c and loop.c.

Looks good, applied,

		Linus

[PATCH] Re: NFS "dev_t" issues..

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 380 bytes --]

Linus,

What do you think about the attached simple patch, making the cookie
size more explicit?

I was looking at fixing up reiserfs, which already has 32-bit storage
for dev_t on-disk, and the following change came to mind.

-- 
Jeff Garzik      | Only so many songs can be sung
Building 1024    | with two lips, two lungs, and one tongue.
MandrakeSoft     |         - nomeansno

[-- Attachment #2: kdev.patch --]
[-- Type: text/plain, Size: 528 bytes --]

Index: include/linux/kdev_t.h
===================================================================
RCS file: /cvsroot/gkernel/linux_2_5/include/linux/kdev_t.h,v
retrieving revision 1.2
diff -u -r1.2 kdev_t.h
--- include/linux/kdev_t.h	2002/01/01 22:41:11	1.2
+++ include/linux/kdev_t.h	2002/01/01 23:02:10
@@ -86,7 +86,7 @@
  * internal equality comparisons and for things
  * like NFS filehandle conversion.
  */
-static inline unsigned int kdev_val(kdev_t dev)
+static inline u32 kdev_val(kdev_t dev)
 {
 	return dev.value;
 }

Re: [PATCH] Re: NFS "dev_t" issues..

[Linus Torvalds]

On Tue, 1 Jan 2002, Jeff Garzik wrote:
>
> What do you think about the attached simple patch, making the cookie
> size more explicit?

Well, I suspect that we actually should also make the format explicit, and
basically use the same translation that I did for the NFS filehandle. That
way it's still just a cookie, but it's a cookie with (a) explicit size and
(b) meaning that won't change over different kernel revisions.

Hmm?

			Linus

Re: [PATCH] Re: NFS "dev_t" issues..

[Jeff Garzik]

Linus Torvalds wrote:
> 
> On Tue, 1 Jan 2002, Jeff Garzik wrote:
> >
> > What do you think about the attached simple patch, making the cookie
> > size more explicit?
> 
> Well, I suspect that we actually should also make the format explicit, and
> basically use the same translation that I did for the NFS filehandle. That
> way it's still just a cookie, but it's a cookie with (a) explicit size and
> (b) meaning that won't change over different kernel revisions.

true, each filesystem needs to figure out how to make sure their on-disk
format doesn't change across kernel revisions...  Storing the raw i_rdev
onto disk definitely silly but it appears to be an issue some
filesystems will have to deal with.  I'm leaving reiserfs alone so they
can make a policy decision...

-- 
Jeff Garzik      | Only so many songs can be sung
Building 1024    | with two lips, two lungs, and one tongue.
MandrakeSoft     |         - nomeansno