disk quotas fail when /etc/mtab is symlinked to /proc/mounts

The attached patchs modify the EXT[2|3] and [X|J]FS codes to add the

Quoted 3 lines · Mark Bellon

The attached patchs modify the EXT[2|3] and [X|J]FS codes to add the

The XFS component is incorrect, we're already doing this elsewhere (over in fs/xfs/quota/xfs_qm_bhv.c), so please drop this last part from your patch...

fs/xfs/xfs_vfsops.c line 1890

Quoted 8 lines

diff -Naur linux-2.6.13-rc3-git9-orig/fs/xfs/xfs_vfsops.c linux-2.6.13-rc3-git9/fs/xfs/xfs_vfsops.c
...

fs/xfs/xfs_vfsops.c +2 -0

+		{ XFS_UQUOTA_ACTIVE,		"," "usrquota" },
+		{ XFS_GQUOTA_ACTIVE,		"," "grpquota" },

thanks.

If /etc/mtab is a regular file all of the mount options (of a file
system) are written to /etc/mtab by the mount command. The quota tools look there for the quota strings for their operation. If, however, /etc/mtab is a symlink to /proc/mounts (a "good thing" in some environments) the tools don't write anything - they assume the kernel will take care of things.

While the quota options are sent down to the kernel via the mount system call and the file system codes handle them properly unfortunately there is no code to echo the quota strings into /proc/mounts and the quota tools fail in the symlink case.

The attached patchs modify the EXT[2|3] and [X|J]FS codes to add the necessary hooks. The show_options function of each file system in these patches currently deal with only those things that seemed related to quotas; especially in the EXT3 case more can be done (later?).

Quoted 5 lines · Mark Bellon

If /etc/mtab is a regular file all of the mount options (of a file system) are written to /etc/mtab by the mount command. The quota tools look there for the quota strings for their operation. If, however, /etc/mtab is a symlink to /proc/mounts (a "good thing" in some environments) the tools don't write anything - they assume the kernel will take care of things.

While the quota options are sent down to the kernel via the mount system call and the file system codes handle them properly unfortunately there is no code to echo the quota strings into /proc/mounts and the quota tools fail in the symlink case.

hm. Perhaps others with operational experience in that area can comment.

Quoted 5 lines

The attached patchs modify the EXT[2|3] and [X|J]FS codes to add the necessary hooks. The show_options function of each file system in these patches currently deal with only those things that seemed related to quotas; especially in the EXT3 case more can be done (later?).

It seems sad to do it in each filesystem. Is there no way in which we can do this for all filesystems, in a single place in the VFS?

Quoted 12 lines · Andrew Morton

Quoted 4 lines · Mark Bellon

If /etc/mtab is a regular file all of the mount options (of a file system) are written to /etc/mtab by the mount command. The quota tools look there for the quota strings for their operation. If, however, /etc/mtab is a symlink to /proc/mounts (a "good thing" in some environments) the tools don't write anything - they assume the kernel will take care of things.

While the quota options are sent down to the kernel via the mount system call and the file system codes handle them properly unfortunately there is no code to echo the quota strings into /proc/mounts and the quota tools fail in the symlink case.

hm. Perhaps others with operational experience in that area can comment.

OK.

Quoted 6 lines

Quoted 4 lines

The attached patchs modify the EXT[2|3] and [X|J]FS codes to add the necessary hooks. The show_options function of each file system in these patches currently deal with only those things that seemed related to quotas; especially in the EXT3 case more can be done (later?).

Quoted 4 lines

It seems sad to do it in each filesystem. Is there no way in which we can do this for all filesystems, in a single place in the VFS?

Each file system must be able to echo it's own FS specific options, hence the show_options hook (XFS is a good example). EXT3 has it's own form of journalled quota file options, hence the need for the specific hook.

The "older style" (e.g. "usrquota", "grpquota", "quota") could be done generically but a FS might want any number of quota options. The few lines of code in each file system didn't seem so bad especially if the show_function start echoing more.

If /etc/mtab is a regular file all of the mount options (of a file system) are written to /etc/mtab by the mount command. The quota tools look there for the quota strings for their operation. If, however, /etc/mtab is a symlink to /proc/mounts (a "good thing" in some environments) the tools don't write anything - they assume the kernel will take care of things.

While the quota options are sent down to the kernel via the mount system call and the file system codes handle them properly unfortunately there is no code to echo the quota strings into /proc/mounts and the quota tools fail in the symlink case.

The attached patchs modify the EXT[2|3] and [X|J]FS codes to add the necessary hooks. The show_options function of each file system in these patches currently deal with only those things that seemed related to quotas; especially in the EXT3 case more can be done (later?).

The EXT3 has added error checking and has two minor changes:
The "quota" option is considered part of the older style quotas Journalled quotas and older style quotas are mutually exclusive.
- both discussable topics

mark

Signed-off-by Mark Bellon <[email protected]>

fs/ext3/super.c +19 -0

+
+	if (sbi->s_jquota_fmt)
+		seq_printf(seq, ",jqfmt=%s",
+		(sbi->s_jquota_fmt == QFMT_VFS_OLD) ? "vfsold": "vfsv0");
+
+	if (sbi->s_qf_names[USRQUOTA])
+		seq_printf(seq, ",usrjquota=%s", sbi->s_qf_names[USRQUOTA]);
+
+	if (sbi->s_qf_names[GRPQUOTA])
+		seq_printf(seq, ",grpjquota=%s", sbi->s_qf_names[GRPQUOTA]);
+
+	if (sbi->s_mount_opt & EXT3_MOUNT_USRQUOTA)
+		seq_puts(seq, ",usrquota");
+
+	if (sbi->s_mount_opt & EXT3_MOUNT_GRPQUOTA)
+		seq_puts(seq, ",grpquota");
+
+	return 0;
+}
 
 #define QTYPE2NAME(t) ((t)==USRQUOTA?"user":"group")
 #define QTYPE2MOPT(on, t) ((t)==USRQUOTA?((on)##USRJQUOTA):((on)##GRPJQUOTA))

fs/ext3/super.c +8 -3

 #endif

@@ -962,11 +1010,36 @@
 		}
 	}
 #ifdef CONFIG_QUOTA
-	if (!sbi->s_jquota_fmt && (sbi->s_qf_names[USRQUOTA] ||
-	    sbi->s_qf_names[GRPQUOTA])) {
-		printk(KERN_ERR
+	if (sbi->s_qf_names[USRQUOTA] || sbi->s_qf_names[GRPQUOTA]) {
+		if ((sbi->s_mount_opt & EXT3_MOUNT_USRQUOTA) || 
+		    (sbi->s_mount_opt & EXT3_MOUNT_GRPQUOTA)) {
+			printk(KERN_ERR
+			"EXT3-fs: only one type of quotas allowed.\n");
+
+			return 0;
+		}

fs/ext3/super.c +16 -0

+			printk(KERN_ERR
+	"EXT3-fs: journalled quota specified when data journalling is not.\n");
+
+			return 0;
+		}
+	}
+	else {
+		if (sbi->s_jquota_fmt) {
+			printk(KERN_ERR
+"EXT3-fs: journalled quota format specified with no journalling enabled.\n");
+
+			return 0;
+		}
 	}
 #endif
 
+#if defined(CONFIG_QUOTA)
+		case Opt_usrquota:
+			set_opt(sbi->s_mount_opt, USRQUOTA);

Hello,

Quoted 5 lines

If /etc/mtab is a regular file all of the mount options (of a file system) are written to /etc/mtab by the mount command. The quota tools look there for the quota strings for their operation. If, however, /etc/mtab is a symlink to /proc/mounts (a "good thing" in some environments) the tools don't write anything - they assume the kernel will take care of things.

While the quota options are sent down to the kernel via the mount system call and the file system codes handle them properly unfortunately there is no code to echo the quota strings into /proc/mounts and the quota tools fail in the symlink case.

Yes, I agree that it's a good think to have quota options in
/proc/mounts. Doing it per filesystem is not nice but I don't know a nice way of doing it a VFS level either...

Quoted 10 lines

The attached patchs modify the EXT[2|3] and [X|J]FS codes to add the necessary hooks. The show_options function of each file system in these patches currently deal with only those things that seemed related to quotas; especially in the EXT3 case more can be done (later?).

The EXT3 has added error checking and has two minor changes:
The "quota" option is considered part of the older style quotas Journalled quotas and older style quotas are mutually exclusive.
- both discussable topics

Ack.

Quoted 6 lines

mark

Signed-off-by Mark Bellon <[email protected]>

Thanks for the patch - I have some comments below...

<snip>

Quoted 5 lines

#ifdef CONFIG_QUOTA

fs/ext3/super.c +2 -0

+static int ext3_show_options(struct seq_file *seq, struct vfsmount *vfs)
+{

fs/ext3/super.c line 525

Quoted 10 lines

fs/ext3/super.c +10 -0

+	struct ext3_sb_info *sbi = EXT3_SB(vfs->mnt_sb);
+
+	if (sbi->s_mount_opt & EXT3_MOUNT_JOURNAL_DATA)
+		seq_puts(seq, ",data=journal");
+
+	if (sbi->s_mount_opt & EXT3_MOUNT_ORDERED_DATA)
+		seq_puts(seq, ",data=ordered");
+
+	if (sbi->s_mount_opt & EXT3_MOUNT_WRITEBACK_DATA)
+		seq_puts(seq, ",data=writeback");

Showing 'data' option only when quota is compile is ugly... Please move
CONFIG_QUOTA inside only around quota specific stuff.

Quoted 23 lines

fs/ext3/super.c +19 -0

+
+	if (sbi->s_jquota_fmt)
+		seq_printf(seq, ",jqfmt=%s",
+		(sbi->s_jquota_fmt == QFMT_VFS_OLD) ? "vfsold": "vfsv0");
+
+	if (sbi->s_qf_names[USRQUOTA])
+		seq_printf(seq, ",usrjquota=%s", sbi->s_qf_names[USRQUOTA]);
+
+	if (sbi->s_qf_names[GRPQUOTA])
+		seq_printf(seq, ",grpjquota=%s", sbi->s_qf_names[GRPQUOTA]);
+
+	if (sbi->s_mount_opt & EXT3_MOUNT_USRQUOTA)
+		seq_puts(seq, ",usrquota");
+
+	if (sbi->s_mount_opt & EXT3_MOUNT_GRPQUOTA)
+		seq_puts(seq, ",grpquota");
+
+	return 0;
+}

#define QTYPE2NAME(t) ((t)==USRQUOTA?"user":"group")
#define QTYPE2MOPT(on, t) ((t)==USRQUOTA?((on)##USRJQUOTA):((on)##GRPJQUOTA))

fs/ext3/super.c line 609

Quoted 6 lines

fs/ext3/super.c +1 -0

@@ -572,6 +604,7 @@
 #ifdef CONFIG_QUOTA
 	.quota_read	= ext3_quota_read,
 	.quota_write	= ext3_quota_write,
+	.show_options	= ext3_show_options,
 
 

Probably set this function everytime...

<snip>

Quoted 8 lines

fs/ext3/super.c +3 -0

+		case Opt_quota:
+		case Opt_usrquota:
+		case Opt_grpquota:

case Opt_usrjquota:
case Opt_grpjquota:
case Opt_offusrjquota:

fs/ext3/super.c line 978

Quoted 7 lines

fs/ext3/super.c +0 -1

@@ -924,7 +973,6 @@
 				"EXT3-fs: journalled quota options not "
 				"supported.\n");
 			break;
-		case Opt_quota:
 		case Opt_noquota:
 			break;
 

I'm not sure with the above change.. Previously if you mounted a
filesystem with 'usrquota' option without a kernel quota support the mount succeeded. With your patch it will fail. I agree that that makes more sense but I'm not sure it's correct to change a behaviour so suddently. Maybe just issue a warning but let the mount succeed.

Quoted 18 lines

#endif

fs/ext3/super.c +10 -3

@@ -962,11 +1010,36 @@
 		}
 	}
 #ifdef CONFIG_QUOTA
-	if (!sbi->s_jquota_fmt && (sbi->s_qf_names[USRQUOTA] ||
-	    sbi->s_qf_names[GRPQUOTA])) {
-		printk(KERN_ERR
+	if (sbi->s_qf_names[USRQUOTA] || sbi->s_qf_names[GRPQUOTA]) {
+		if ((sbi->s_mount_opt & EXT3_MOUNT_USRQUOTA) || 
+		    (sbi->s_mount_opt & EXT3_MOUNT_GRPQUOTA)) {
+			printk(KERN_ERR
+			"EXT3-fs: only one type of quotas allowed.\n");
+
+			return 0;
+		}
+
+		if (!sbi->s_jquota_fmt) {

fs/ext3/super.c line 1031

Quoted 10 lines

fs/ext3/super.c +6 -1

+			printk(KERN_ERR
 			"EXT3-fs: journalled quota format not specified.\n");
-		return 0;
+
+			return 0;
+		}
+
+		if ((sbi->s_mount_opt & EXT3_MOUNT_JOURNAL_DATA) == 0) {
 
 

This test does not make sense - journaled quota in recent kernels
works with arbitrary journaling data mode.

Quoted 21 lines

fs/ext3/super.c +13 -0

+			printk(KERN_ERR
+	"EXT3-fs: journalled quota specified when data journalling is not.\n");
+
+			return 0;
+		}
+	}
+	else {
+		if (sbi->s_jquota_fmt) {
+			printk(KERN_ERR
+"EXT3-fs: journalled quota format specified with no journalling enabled.\n");
+
+			return 0;
+		}
 	}

#endif

fs/ext2/super.c +3 -0

+#if defined(CONFIG_QUOTA)
+		case Opt_usrquota:
+			set_opt(sbi->s_mount_opt, USRQUOTA);

fs/ext2/super.c line 440

Quoted 10 lines

fs/ext2/super.c +10 -0

+			break;
+
+		case Opt_grpquota:
+			set_opt(sbi->s_mount_opt, GRPQUOTA);
+			break;
+
+		case Opt_quota:
+			set_opt(sbi->s_mount_opt, GRPQUOTA);
+			set_opt(sbi->s_mount_opt, USRQUOTA);
+			break;

The old 'quota' option means the same as 'usrquota' - at least tools
consider it like that.

fs/ext2/super.c line 449

Quoted 11 lines

fs/ext2/super.c +9 -0

+#else
+		case Opt_quota:
+		case Opt_usrquota:
+		case Opt_grpquota:
+			printk(KERN_ERR
+				"EXT2-fs: quota operations not supported.\n");
+
+			break;
+#endif

The same as with ext3 - I don't like this change...

<snip>

Honza

The ATA specification tells large disk drives to return C/H/S data of 16383/16/63 regardless of their actual size (other variations on this return include 15 heads and/or 4092 cylinders). Unfortunately these CHS data confuse the existing IDE code and cause it to report invalid geometries in /proc when the disk runs in LBA mode.

The invalid geometries can cause failures in the partitioning tools; partitioning may be impossible or illogical size limitations occur. This also leads to various forms of human confusion.

I attach a patch that fixes this problem while strongly attempting to not break any existing side effects and await any comments.

mark

Signed-off-by Mark Bellon <[email protected]>

Did you read ATA-1 through ATA-7 to understand all the variations?

Quoted 16 lines

The ATA specification tells large disk drives to return C/H/S data of 16383/16/63 regardless of their actual size (other variations on this return include 15 heads and/or 4092 cylinders). Unfortunately these CHS data confuse the existing IDE code and cause it to report invalid geometries in /proc when the disk runs in LBA mode.

The invalid geometries can cause failures in the partitioning tools; partitioning may be impossible or illogical size limitations occur. This also leads to various forms of human confusion.

I attach a patch that fixes this problem while strongly attempting to not break any existing side effects and await any comments.

mark

Signed-off-by Mark Bellon <[email protected]>

Did you read ATA-1 through ATA-7 to understand all the variations?

Quoted 10 lines · Mark Bellon

The ATA specification tells large disk drives to return C/H/S data of 16383/16/63 regardless of their actual size (other variations on this return include 15 heads and/or 4092 cylinders). Unfortunately these CHS data confuse the existing IDE code and cause it to report invalid geometries in /proc when the disk runs in LBA mode.

The invalid geometries can cause failures in the partitioning tools; partitioning may be impossible or illogical size limitations occur. This also leads to various forms of human confusion.

Quoted 8 lines

I attach a patch that fixes this problem while strongly attempting to not break any existing side effects and await any comments.

mark

Signed-off-by Mark Bellon <[email protected]>

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

Hi,

The topic was discussed to death on linux-kernel.

Mark, you need to fix your applications and stop using /proc/ide/hd*/geometry or/and HDIO_GET_GEO ioctl (which BTW your patch also affects).

Bartlomiej

Quoted 34 lines · Andre Hedrick

Did you read ATA-1 through ATA-7 to understand all the variations?

Quoted 10 lines · Mark Bellon

The ATA specification tells large disk drives to return C/H/S data of 16383/16/63 regardless of their actual size (other variations on this return include 15 heads and/or 4092 cylinders). Unfortunately these CHS data confuse the existing IDE code and cause it to report invalid geometries in /proc when the disk runs in LBA mode.

The invalid geometries can cause failures in the partitioning tools; partitioning may be impossible or illogical size limitations occur. This also leads to various forms of human confusion.

Quoted 10 lines

I attach a patch that fixes this problem while strongly attempting to not break any existing side effects and await any comments.

mark

Signed-off-by Mark Bellon <[email protected]>

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

Quoted 8 lines · Bartlomiej Zolnierkiewicz

Hi,

The topic was discussed to death on linux-kernel.

Mark, you need to fix your applications and stop using /proc/ide/hd*/geometry or/and HDIO_GET_GEO ioctl (which BTW your patch also affects).

Fixing the applications I can understand but the patch still seems necessary, to me, so the
HDIO_GET_GEO returns "rational" values.

I tested HDIO_GET_GEO and it returns the same broken values as go into /proc (no surprises) without my patch.

If a drive is in LBA mode (28 or 48 bit) the existing code doesn't always "fix up" the geometry properly for some value returns. It only tries with 48 bit mode and it fails there for some values. My patch forces a complete geometry and appears (to me) to preserve the side efefcts of the existing code.

Am I missing something?

mark

Quoted 27 lines

Bartlomiej

Quoted 22 lines · Andre Hedrick

Did you read ATA-1 through ATA-7 to understand all the variations?

Quoted 16 lines · Mark Bellon

The ATA specification tells large disk drives to return C/H/S data of 16383/16/63 regardless of their actual size (other variations on this return include 15 heads and/or 4092 cylinders). Unfortunately these CHS data confuse the existing IDE code and cause it to report invalid geometries in /proc when the disk runs in LBA mode.

The invalid geometries can cause failures in the partitioning tools; partitioning may be impossible or illogical size limitations occur. This also leads to various forms of human confusion.

I attach a patch that fixes this problem while strongly attempting to not break any existing side effects and await any comments.

mark

Signed-off-by Mark Bellon <[email protected]>

Quoted 18 lines · Mark Bellon

Quoted 10 lines · Bartlomiej Zolnierkiewicz

Hi,

The topic was discussed to death on linux-kernel.

Mark, you need to fix your applications and stop using /proc/ide/hd*/geometry or/and HDIO_GET_GEO ioctl (which BTW your patch also affects).

Fixing the applications I can understand but the patch still seems necessary, to me, so the
HDIO_GET_GEO returns "rational" values.

Quoted 4 lines

I tested HDIO_GET_GEO and it returns the same broken values as go into /proc (no surprises) without my patch.

Please explain.

Quoted 11 lines

If a drive is in LBA mode (28 or 48 bit) the existing code doesn't always "fix up" the geometry properly for some value returns. It only tries with 48 bit mode and it fails there for some values. My patch forces a complete geometry and appears (to me) to preserve the side efefcts of the existing code.

Am I missing something?

diff -Naur linux-2.6.13-rc3-git9-orig/drivers/ide/ide-disk.c linux-2.6.13-rc3-git9/drivers/ide/ide-disk.c
--- linux-2.6.13-rc3-git9-orig/drivers/ide/ide-disk.c 2005-08-01
13:48:21.000000000 -0700
+++ linux-2.6.13-rc3-git9/drivers/ide/ide-disk.c 2005-08-02
12:14:43.000000000 -0700

drivers/ide/ide-disk.c +14 -5

@@ -884,10 +884,17 @@
 	ide_add_setting(drive,	"max_failures",		SETTING_RW,					-1,			-1,			TYPE_INT,	0,	65535,				1,	1,	&drive->max_failures,		NULL);
 }
 
+static uint32_t do_div64_32 (__u64 n, uint32_t d)
+{
+	do_div(n, d);
+
+	return n;
+}
+
 static void idedisk_setup (ide_drive_t *drive)
 {
 	struct hd_driveid *id = drive->id;
-	unsigned long long capacity;
+	__u64 capacity;
 	int barrier;
 
 	idedisk_add_settings(drive);

@@ -949,27 +956,32 @@
 	 */
 	capacity = idedisk_capacity (drive);
 	if (!drive->forced_geom) {
+		uint32_t cylsz, cyl;
 
-		if (idedisk_supports_lba48(drive->id)) {
-			/* compatibility */
-			drive->bios_sect = 63;
-			drive->bios_head = 255;
+		/*
+		 * In LBA mode the geometry isn't used to talk to the drive
+		 * so always create a "rational" geometry from the capacity.
+		 */
+		if (drive->select.b.lba) {
 

why are you forcing drive->bios_sect and drive->bios_head values for LBA28 drives?

drivers/ide/ide-disk.c +2 -0

+			drive->bios_sect = drive->sect = 63;
+			drive->bios_head = drive->head = 255;

changing drive->sect and drive->head is just wrong, these are values reported by the device - do not touch them

drivers/ide/ide-disk.c +3 -0

+
+			cylsz = drive->bios_sect * drive->bios_head;
+			cyl = do_div64_32(capacity, cylsz);

division by zero now is possible (yes, 0/0/0 is possible)

drivers/ide/ide-disk.c +4 -0

+
+			/* "fake out" works up to ~500 GB */
+			cyl = (cyl > 65535) ? 65535 : cyl;
+			drive->bios_cyl = drive->cyl = cyl;

drive->cyl shouldn't be changed

		}

		if (drive->bios_sect && drive->bios_head) {

drivers/ide/ide-disk.c +2 -4

-			unsigned int cap0 = capacity; /* truncate to 32 bits */
-			unsigned int cylsz, cyl;
+			cylsz = drive->bios_sect * drive->bios_head;
+			cyl = do_div64_32(capacity, cylsz);
 
-			if (cap0 != capacity)
-				drive->bios_cyl = 65535;

cap0 != capacity => set the max possible drive->bios_cyl, no need for adjusting it

drivers/ide/ide-disk.c +0 -1

-			else {

cap0 == capacity => no need for do_div(), only lower 32-bits are used, adjust drive->bios_cyl

drivers/ide/ide-disk.c +4 -7

-				cylsz = drive->bios_sect * drive->bios_head;
-				cyl = cap0 / cylsz;
-				if (cyl > 65535)
-					cyl = 65535;
-				if (cyl > drive->bios_cyl)
-					drive->bios_cyl = cyl;
-			}
+			if (cyl > 65535)
+				cyl = 65535;
+			if (cyl > drive->bios_cyl)
+				drive->bios_cyl = cyl;

		}
	}
	printk(KERN_INFO "%s: %llu sectors (%llu MB)",

Quoted 39 lines

mark

Quoted 33 lines

Bartlomiej

Quoted 26 lines · Andre Hedrick

Did you read ATA-1 through ATA-7 to understand all the variations?

Quoted 18 lines · Mark Bellon

The ATA specification tells large disk drives to return C/H/S data of 16383/16/63 regardless of their actual size (other variations on this return include 15 heads and/or 4092 cylinders). Unfortunately these CHS data confuse the existing IDE code and cause it to report invalid geometries in /proc when the disk runs in LBA mode.

The invalid geometries can cause failures in the partitioning tools; partitioning may be impossible or illogical size limitations occur. This also leads to various forms of human confusion.

I attach a patch that fixes this problem while strongly attempting to not break any existing side effects and await any comments.

mark

Signed-off-by Mark Bellon <[email protected]>

Quoted 21 lines · Bartlomiej Zolnierkiewicz

Quoted 18 lines · Mark Bellon

Quoted 10 lines · Bartlomiej Zolnierkiewicz

Hi,

The topic was discussed to death on linux-kernel.

Mark, you need to fix your applications and stop using /proc/ide/hd*/geometry or/and HDIO_GET_GEO ioctl (which BTW your patch also affects).

Fixing the applications I can understand but the patch still seems necessary, to me, so the
HDIO_GET_GEO returns "rational" values.

Quoted 8 lines

Quoted 4 lines

I tested HDIO_GET_GEO and it returns the same broken values as go into /proc (no surprises) without my patch.

Please explain.

Let me explain more below. Thanks for the response as now I can understand better the issue (which was what I was fishing for).

Quoted 53 lines

Quoted 11 lines

If a drive is in LBA mode (28 or 48 bit) the existing code doesn't always "fix up" the geometry properly for some value returns. It only tries with 48 bit mode and it fails there for some values. My patch forces a complete geometry and appears (to me) to preserve the side efefcts of the existing code.

Am I missing something?

diff -Naur linux-2.6.13-rc3-git9-orig/drivers/ide/ide-disk.c linux-2.6.13-rc3-git9/drivers/ide/ide-disk.c
--- linux-2.6.13-rc3-git9-orig/drivers/ide/ide-disk.c 2005-08-01 13:48:21.000000000 -0700
+++ linux-2.6.13-rc3-git9/drivers/ide/ide-disk.c 2005-08-02
12:14:43.000000000 -0700

drivers/ide/ide-disk.c +14 -5

@@ -884,10 +884,17 @@
      ide_add_setting(drive,  "max_failures",         SETTING_RW,                                     -1,                     -1,                     TYPE_INT,       0,      65535,                          1,      1,      &drive->max_failures,           NULL);
 }
 
+static uint32_t do_div64_32 (__u64 n, uint32_t d)
+{
+      do_div(n, d);
+
+      return n;
+}
+
 static void idedisk_setup (ide_drive_t *drive)
 {
      struct hd_driveid *id = drive->id;
-      unsigned long long capacity;
+      __u64 capacity;
      int barrier;
 
      idedisk_add_settings(drive);

@@ -949,27 +956,32 @@
       */
      capacity = idedisk_capacity (drive);
      if (!drive->forced_geom) {
+              uint32_t cylsz, cyl;
 
-              if (idedisk_supports_lba48(drive->id)) {
-                      /* compatibility */
-                      drive->bios_sect = 63;
-                      drive->bios_head = 255;
+              /*
+               * In LBA mode the geometry isn't used to talk to the drive
+               * so always create a "rational" geometry from the capacity.
+               */
+              if (drive->select.b.lba) {
 

Quoted 3 lines

why are you forcing drive->bios_sect and drive->bios_head values for LBA28 drives?

Any LBA 28 drive can return fixed geometry (regardless of size) just as well as an LBA 48 drive can. I've got drives all over the place that do just that. In any event there are machines which do not have have an BIOS, like a PPC target, and the BIOS values can be "fixed up".

The ioctl returns the BIOS values and these are totally wrong on some targets. Comments?

drivers/ide/ide-disk.c line 967

Quoted 3 lines

drivers/ide/ide-disk.c +2 -0

+                      drive->bios_sect = drive->sect = 63;
+                      drive->bios_head = drive->head = 255;

Quoted 3 lines

these are values reported by the device - do not touch them

If drive->cyl, drive-head amd drive->sect are to be left alone in all cases I have no problem.

Quoted 7 lines

drivers/ide/ide-disk.c +3 -0

+
+                      cylsz = drive->bios_sect * drive->bios_head;
+                      cyl = do_div64_32(capacity, cylsz);

division by zero now is possible (yes, 0/0/0 is possible)

Then the original code is prefered.

Quoted 8 lines

drivers/ide/ide-disk.c +4 -0

+
+                      /* "fake out" works up to ~500 GB */
+                      cyl = (cyl > 65535) ? 65535 : cyl;
+                      drive->bios_cyl = drive->cyl = cyl;

drive->cyl shouldn't be changed

Same comment as above. Only change BIOS values.

Quoted 25 lines

              }

              if (drive->bios_sect && drive->bios_head) {

drivers/ide/ide-disk.c +2 -4

-                      unsigned int cap0 = capacity; /* truncate to 32 bits */
-                      unsigned int cylsz, cyl;
+                      cylsz = drive->bios_sect * drive->bios_head;
+                      cyl = do_div64_32(capacity, cylsz);
 
-                      if (cap0 != capacity)
-                              drive->bios_cyl = 65535;

cap0 != capacity => set the max possible drive->bios_cyl, no need for adjusting it

- else {

cap0 == capacity => no need for do_div(), only lower 32-bits are used, adjust drive->bios_cyl

drivers/ide/ide-disk.c +0 -4

-                              cylsz = drive->bios_sect * drive->bios_head;
-                              cyl = cap0 / cylsz;
-                              if (cyl > 65535)
-                                      cyl = 65535;

drivers/ide/ide-disk.c line 987

Quoted 10 lines

drivers/ide/ide-disk.c +4 -3

-                              if (cyl > drive->bios_cyl)
-                                      drive->bios_cyl = cyl;
-                      }
+                      if (cyl > 65535)
+                              cyl = 65535;
+                      if (cyl > drive->bios_cyl)
+                              drive->bios_cyl = cyl;
              }
      }
      printk(KERN_INFO "%s: %llu sectors (%llu MB)",

I understand clearly now...

mark

Quoted 39 lines

Quoted 39 lines

mark

Quoted 33 lines

Bartlomiej

Quoted 26 lines · Andre Hedrick

Did you read ATA-1 through ATA-7 to understand all the variations?

Quoted 18 lines · Mark Bellon

The ATA specification tells large disk drives to return C/H/S data of 16383/16/63 regardless of their actual size (other variations on this return include 15 heads and/or 4092 cylinders). Unfortunately these CHS data confuse the existing IDE code and cause it to report invalid geometries in /proc when the disk runs in LBA mode.

The invalid geometries can cause failures in the partitioning tools; partitioning may be impossible or illogical size limitations occur. This also leads to various forms of human confusion.

I attach a patch that fixes this problem while strongly attempting to not break any existing side effects and await any comments.

mark

Signed-off-by Mark Bellon <[email protected]>

Simple: do not use BIOS values.
[ Yes, there should be some warning from kernel. ]

Quoted 4 lines

Simple: do not use BIOS values.
[ Yes, there should be some warning from kernel. ]

On that matter, I get a warning from LILO wrt cyls and stuff:

07:47 spectre:~ # cat /proc/ide/hda/geometry physical 16383/16/63
logical 65535/16/63
07:58 spectre:~ # lilo
Warning: Kernel & BIOS return differing head/sector geometries for device 0x80
Kernel: 65535 cylinders, 16 heads, 63 sectors

     BIOS: 1023 cylinders, 255 heads, 63 sectors

Added linux *
07:59 spectre:~ # fdisk -l

Disk /dev/hda: 40.9 GB, 40982151168 bytes
255 heads, 63 sectors/track, 4982 cylinders

All of these numbers are virtual, since CHS is not really used anymore, as we know. But, which of these fake CHS values (16383/16/63 | 65535/16/63 | 1023/255/63) is the right one? 255/63/4982 is another matter, since it [almost] matches the actual size of the disk while the other three are just "for the bios".

All of these numbers are virtual, since CHS is not really used anymore, as we know. But, which of these fake CHS values (16383/16/63 | 65535/16/63 | 1023/255/63) is the right one? 255/63/4982 is another matter, since it [almost] matches the actual size of the disk while the other three are just "for the bios".