pdcraid and weird IDE geometry

Hello all,

I ran into a weird problem when I received my latest replacement DeskStar drive from Hitachi. It was the same size as the GXP60 it replaced, but had much different geometry. I use these two 40GB drives with a Promise FastTrak 20276 controller as a raid0 array. The controller finds the drives and sets up the raid no problem. The pdcraid module from vanilla 2.4.21 only found 1 drive however, and it was the original existing drive. Testing with Promise's partial opensource driver shows that the array does indeed work correctly with their driver.

A few printk's later, I determined the problem. The geometry of the two drives are as follows:

cat /proc/ide/hde/geometry (Old Drive)
physical 79780/16/63
logical 79780/16/63

cat /proc/ide/hdg/geometry (New Drive)
physical 5005/255/63
logical 5005/255/63

The calc_pdcblock_offset function calculates lba by taking the capacity of the drive and dividing it by (head * sector), multiplying the result times (head * sector) and subtracting the sector (SPT) count. Unfortunately, with the strange geometry reported by the new drive, using INTs to store these values will fail. The capacity of each drive is exactly the same of 80418240 as reported by procfs and ideinfo->capacity. In order to return the superblock offset correctly I had to use non-integer vars. I've tested the resulting module and it now correctly locates both drives, read and writes as expected and is compatible with the binary FastTrak.o module. I'm not much of a coder, so if this could be done more efficiently than my attached patch, please let me know. Please CC any replies. Thanks,

-Walt Holman

@@ -361,8 +361,14 @@
        if (ideinfo->sect==0)
                return 0;
-       lba = (ideinfo->capacity / (ideinfo->head*ideinfo->sect));
-       lba = lba * (ideinfo->head*ideinfo->sect);
-       lba = lba - ideinfo->sect;
+
+       float lbatemp = 0;
+       float head = ideinfo->head;
+       float sect = ideinfo->sect;
+       float capacity = ideinfo->capacity;
+       lbatemp = (capacity / (head*sect));
+       lbatemp = lbatemp * (head*sect);
+       lbatemp = lbatemp - sect;
 
+       lba = lbatemp;
        return lba;
 }

(un)fortionatly it's not valid to use floating point in the kernel. Could you try the same thing by using u64 as type instead please ?

I've tried it using unsigned long, and it will fail to find the second drive. The problem is that the FastTrak bios writes the superblock to the second drive in the same place as if it had the geometry of the first drive. How it does it, I do not know. What I know is that the offset for the superblock on both drives lies at: 80418177

On the first drive, you get there like this:

capacity = 80418240, head = 16, sect = 63
lba = capacity / (head * sect) = 79780
lba = lba * (head * sect) = 80418240
lba = lba - sect = 80418177
This one's correct.

On the second drive, it's like this:
capacity = 80418240, head=255, sect = 63
lba = capacity / (head * sect) = 5005 int or 5005.80 float lba = lba * (head * sect) = 80405325 int or 80418240.01 float lba = lba - sect = 80405262 int or 80418177 float

If integer results are used, the second drive's offset is returned as 80405262, which is not the offset for the superblock. It lies at the same location as the first drive.

Insmodding the module compiled with ints as calculations results in this:

Jul 17 07:15:16 waltsathlon kernel:  ataraid/disc0/disc: p1 p2 < >
Jul 17 07:15:16 waltsathlon kernel: Drive 0 is 39266 Mb (33 / 0)
Jul 17 07:15:16 waltsathlon kernel: Raid0 array consists of 1 drives.
Jul 17 07:15:16 waltsathlon kernel: Promise Fasttrak(tm) Softwareraid

driver for linux version 0.03beta

While the one with floating calcs finds both drives.
My only guess at this point, is that the FastTrak bios is using different geometry than what's reported to us. I'm not sure how this would work, but I've thought about storing the offset of the working drive to use in the event that the offset calculation fails and the capacity is identical on additional drives. Seems kinda hacky to me, but then what do I know :) I'm up for trying things, any other ideas?

-Walt Holman

Would fixed point solve this. Start from capacity <<= 16 and then do the maths. That would put lba in 65536ths of a sector which should have the same result as your float maths

Ach Alan - I have not seen these earlier posts, but float or fixed point here is just nonsense.

The purpose of

	A = B/C;
	A *= C;
can only be to round B down to the largest multiple of C below it.
Using infinite precision just turns this into
	A = B;

He needs the first sector of the last cylinder, in a setup where cylinders have size 16*63 or so, but the surrounding software thinks that it is 255*63, a mistake.

I don't know anything about these RAIDs, but possibly it would help to give boot parameters for this disk.

Maybe he is victim of the completely ridiculous

	drive->head = 255;

in ide-disk.c.
(We have drive->head, the number of physical heads, and drive->bios_head, the translation presently used by the BIOS - or at least that is the intention. It is a bug if the former is larger than 16. The latter may well be 255.)

Andries

OK. Just got home from work. I've tried booting and specifying geometry via hdg=79780,16,63 hdg=noprobe etc... The geometry is accepted, however, drive access fails when trying to read the disk. This geometry is the geometry reported by hde (my old drive without screwy geometry).
The code in calc_pdcblock_offset to calculate the offset is unchanged
in my patch (except the date type conversion to float) and calls get_info_ptr for geometry. From what I can tell, this call returns the physical geometry of the disk so using fdisk to fool it doesn't work. I've also tried using unsigned long to store the calcs, multiplying the capacity by 100 to start, doing the maths and dividing the final lba by
100. This doesn't work cause it overflows the data type. Won't I have similar problems using fixed point? At this point, my very limited knowledge of C keeps me from getting much further.

-Walt

I don't understand. Did you introduce some float? Remove it immediately.

You just replace

        lba = (ideinfo->capacity / (ideinfo->head*ideinfo->sect));
        lba = lba * (ideinfo->head*ideinfo->sect);
        lba = lba - ideinfo->sect;

by

	lba = ideinfo->capacity - 63;

Then everything works for you, I suppose. Subsequently we wait for other people with the same hardware and see how the 63 varies as a function of their setup. (Or maybe you can go into the BIOS and specify different translations yourself?)

(By the way, didnt your boot parameters lead to ideinfo->head = 16 and ideinfo->sect = 63?)

Andries

No, you're right. I was just trying to clarify the changes that I had originally made. It seemed as there may have been some confusion that I was doing more than the original. In my case, the simplified ideinfo->capacity - ideinfo->sect should work just fine.
The boot parameters did take. The geometry was reported (correctly?) as I had passed, but when trying to load the pdcraid module, access to the disk failed with I/O errors. Seemed as if it was trying to read beyond the end of the device. I used the identical geometry as reported by the working drive.
Unfortunately, since this is the embedded FastTrak stuff, the BIOS doesn't allow me to setup geometry for the drives.

I've just tried the simplified method and it works fine. I'll stick with that on my end. Thanks for the help,

-Walt

Can't you fix the geometry from fdisk expert mode?

I've done that several times before, when otherwise like-sized disks appeared with vastly different geometry.

It is easiest to think that it is meaningless what you say (in this case). A disk does not have a geometry, and moreover you cannot change it with fdisk.

However, under some circumstances, some kernels will guess a (translated) geometry from a DOS-type partition table, so it is true that under some kernel versions you can use fdisk to change the kernel's ideas about disk geometry. A very fragile activity.

[Moreover, there are several kinds of geometry, and the present authors of ide-disk.c conveniently confused them all. Concerning pdcraid, I don't know for which of the possible ideas of geometry it is true that one needs the first sector of the last cylinder.]

Andries

I don't think this has any to do with the calculation, you probably just have to use:

CONFIG_PDC202XX_FORCE=y

(well, combined with...

CONFIG_BLK_DEV_PDC202XX_NEW=y
CONFIG_BLK_DEV_PDC202XX=y
CONFIG_BLK_DEV_ATARAID=y
CONFIG_BLK_DEV_ATARAID_PDC=y

...and a lot of other options...)

With 2.4.20 the FORCE option had to be set at "n", now it should be set at "y".

Avoiding floating-point precision, this would do the same as your calculation:

lba = ideinfo->capacity - ideinfo->sect;

However, I don't think the expression "fails". the way it is expressed now, it will divide something integer-wise and then multiply it with the same value (and then minus sect), thus I assume this was intended: To round the capacity value to the next multiple of (head * start), and then do a minus sect...

I don't think any hardcore programmer ever does a integerwise divide /before/ an integerwise multiply without a very good reason... (And don't give me that "you'd be surprised"!!!)