[PATCH] pdcraid and weird IDE geometry

[PATCH] pdcraid and weird IDE geometry

[Walt H]

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

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



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

--- /usr/src/temp/linux-2.4.21/drivers/ide/raid/pdcraid.c       2003-06-13 07:51:34.000000000 -0700
+++ pdcraid.c   2003-07-16 19:03:15.000000000 -0700
@@ -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;
 }

Re: [PATCH] pdcraid and weird IDE geometry

[Arjan van de Ven]

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

On Thu, 2003-07-17 at 04:26, Walt H wrote:
> 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,

(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 ?

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] pdcraid and weird IDE geometry

[David Zaffiro]

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".


> 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. 


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"!!!)

Re: [PATCH] pdcraid and weird IDE geometry

[Walt H]

Arjan van de Ven wrote:
> On Thu, 2003-07-17 at 04:26, Walt H wrote:
> 
>>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,
> 
> 
> (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

Re: [PATCH] pdcraid and weird IDE geometry

[Alan Cox]

On Iau, 2003-07-17 at 15:37, Walt H wrote:

> 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

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

Re: [PATCH] pdcraid and weird IDE geometry

[Jeff Garzik]

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.

Re: [PATCH] pdcraid and weird IDE geometry

[Andries Brouwer]

On Thu, Jul 17, 2003 at 03:58:38PM +0100, Alan Cox wrote:
> On Iau, 2003-07-17 at 15:37, Walt H wrote:
> 
> > 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
> 
> 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

Re: [PATCH] pdcraid and weird IDE geometry

[Andries Brouwer]

On Thu, Jul 17, 2003 at 11:34:11AM -0400, Jeff Garzik wrote:

> 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

Re: [PATCH] pdcraid and weird IDE geometry

[Walt H]

Andries Brouwer wrote:
> On Thu, Jul 17, 2003 at 03:58:38PM +0100, Alan Cox wrote:
> 
>>On Iau, 2003-07-17 at 15:37, Walt H wrote:
>>
>>
>>>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
>>
>>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

Re: [PATCH] pdcraid and weird IDE geometry

[Andries Brouwer]

On Thu, Jul 17, 2003 at 07:33:00PM -0700, Walt H wrote:

> 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.

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

Re: [PATCH] pdcraid and weird IDE geometry

[Walt H]

Andries Brouwer wrote:
 >
> 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

Re: [PATCH] pdcraid and weird IDE geometry

[Walt H]

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

Just to finish this thread. In case anyone else has similar problems of
only detecting 1 drive of their array and you suspect strange geometry
is at play, here's the simple patch I used in the end which WFM :)

-Walt



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

--- /usr/src/temp/linux-2.4.21/drivers/ide/raid/pdcraid.c	2003-06-13 07:51:34.000000000 -0700
+++ pdcraid.c	2003-07-18 06:54:25.000000000 -0700
@@ -361,7 +361,8 @@
 	if (ideinfo->sect==0)
 		return 0;
-	lba = (ideinfo->capacity / (ideinfo->head*ideinfo->sect));
+/*	lba = (ideinfo->capacity / (ideinfo->head*ideinfo->sect));
 	lba = lba * (ideinfo->head*ideinfo->sect);
-	lba = lba - ideinfo->sect;
+	lba = lba - ideinfo->sect; */
+	lba = ideinfo->capacity - ideinfo->sect;
 
 	return lba;