[Patch 1/2] cciss: fix for 2TB support

[Patch 1/2] cciss: fix for 2TB support

[Mike Miller (OS Dev)]

Patch 1/2

This patch changes the way we determine if a logical volume is larger than 2TB. The
original test looked for a total_size of 0. Originally we added 1 to the total_size.
That would make our read_capacity return size 0 for >2TB lv's. We assumed that we
could not have a lv size of 0 so it seemed OK until we were in a clustered system. The
backup node would see a size of 0 due to the reservation on the drive. That caused
the driver to switch to 16-byte CDB's which are not supported on older controllers.
After that everything was broken.
It may seem petty but I don't see the value in trying to determine if the LBA is
beyond the 2TB boundary. That's why when we switch we use 16-byte CDB's for all
read/write operations.
Please consider this for inclusion.

Signed-off-by: Mike Miller <mike.miller@hp.com>
------------------------------------------------------------------------------------------
diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 05dfe35..916aab0 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -1291,13 +1291,19 @@ static void cciss_update_drive_info(int 
 	if (inq_buff == NULL)
 		goto mem_msg;
 
+ 	/* testing to see if 16-byte CDBs are already being used */
+ 	if (h->cciss_read == CCISS_READ_16) {
+ 		cciss_read_capacity_16(h->ctlr, drv_index, 1,
+ 			&total_size, &block_size);
+ 		goto geo_inq;
+ 	}
+  
 	cciss_read_capacity(ctlr, drv_index, 1,
 			    &total_size, &block_size);
 
-	/* total size = last LBA + 1 */
-	/* FFFFFFFF + 1 = 0, cannot have a logical volume of size 0 */
-	/* so we assume this volume this must be >2TB in size */
-	if (total_size == (__u32) 0) {
+  	/* if read_capacity returns all F's this volume is >2TB in size */
+  	/* so we switch to 16-byte CDB's for all read/write ops */
+  	if (total_size == 0xFFFFFFFF) {
 		cciss_read_capacity_16(ctlr, drv_index, 1,
 		&total_size, &block_size);
 		h->cciss_read = CCISS_READ_16;
@@ -1306,6 +1312,7 @@ static void cciss_update_drive_info(int 
 		h->cciss_read = CCISS_READ_10;
 		h->cciss_write = CCISS_WRITE_10;
 	}
+geo_inq:
 	cciss_geometry_inquiry(ctlr, drv_index, 1, total_size, block_size,
 			       inq_buff, &h->drv[drv_index]);
 
@@ -1917,13 +1924,14 @@ static void cciss_geometry_inquiry(int c
 			drv->raid_level = inq_buff->data_byte[8];
 		}
 		drv->block_size = block_size;
-		drv->nr_blocks = total_size;
+		drv->nr_blocks = total_size + 1;
 		t = drv->heads * drv->sectors;
 		if (t > 1) {
-			unsigned rem = sector_div(total_size, t);
+			sector_t real_size = total_size + 1;
+			unsigned long rem = sector_div(real_size, t);
 			if (rem)
-				total_size++;
-			drv->cylinders = total_size;
+				real_size++;
+			drv->cylinders = real_size;
 		}
 	} else {		/* Get geometry failed */
 		printk(KERN_WARNING "cciss: reading geometry failed\n");
@@ -1953,16 +1961,16 @@ cciss_read_capacity(int ctlr, int logvol
 				ctlr, buf, sizeof(ReadCapdata_struct),
 					1, logvol, 0, NULL, TYPE_CMD);
 	if (return_code == IO_OK) {
-		*total_size = be32_to_cpu(*(__u32 *) buf->total_size)+1;
+		*total_size = be32_to_cpu(*(__u32 *) buf->total_size);
 		*block_size = be32_to_cpu(*(__u32 *) buf->block_size);
 	} else {		/* read capacity command failed */
 		printk(KERN_WARNING "cciss: read capacity failed\n");
 		*total_size = 0;
 		*block_size = BLOCK_SIZE;
 	}
-	if (*total_size != (__u32) 0)
+	if (*total_size != 0)
 		printk(KERN_INFO "      blocks= %llu block_size= %d\n",
-		(unsigned long long)*total_size, *block_size);
+		(unsigned long long)*total_size+1, *block_size);
 	kfree(buf);
 	return;
 }
@@ -1989,7 +1997,7 @@ cciss_read_capacity_16(int ctlr, int log
 				1, logvol, 0, NULL, TYPE_CMD);
 	}
 	if (return_code == IO_OK) {
-		*total_size = be64_to_cpu(*(__u64 *) buf->total_size)+1;
+		*total_size = be64_to_cpu(*(__u64 *) buf->total_size);
 		*block_size = be32_to_cpu(*(__u32 *) buf->block_size);
 	} else {		/* read capacity command failed */
 		printk(KERN_WARNING "cciss: read capacity failed\n");
@@ -1997,7 +2005,7 @@ cciss_read_capacity_16(int ctlr, int log
 		*block_size = BLOCK_SIZE;
 	}
 	printk(KERN_INFO "      blocks= %llu block_size= %d\n",
-	       (unsigned long long)*total_size, *block_size);
+	       (unsigned long long)*total_size+1, *block_size);
 	kfree(buf);
 	return;
 }
@@ -3119,8 +3127,9 @@ #endif				/* CCISS_DEBUG */
 		}
 		cciss_read_capacity(cntl_num, i, 0, &total_size, &block_size);
 
-		/* total_size = last LBA + 1 */
-		if(total_size == (__u32) 0) {
+		/* If read_capacity returns all F's the logical is >2TB */
+		/* so we switch to 16-byte CDBs for all read/write ops */
+		if(total_size == 0xFFFFFFFF) {
 			cciss_read_capacity_16(cntl_num, i, 0,
 			&total_size, &block_size);
 			hba[cntl_num]->cciss_read = CCISS_READ_16;

Re: [Patch 1/2] cciss: fix for 2TB support

[Andrew Morton]

On Wed, 21 Feb 2007 15:10:39 -0600 "Mike Miller (OS Dev)" <mikem@beardog.cca.cpqcorp.net> wrote:

> Patch 1/2
> 
> This patch changes the way we determine if a logical volume is larger than 2TB. The
> original test looked for a total_size of 0. Originally we added 1 to the total_size.
> That would make our read_capacity return size 0 for >2TB lv's. We assumed that we
> could not have a lv size of 0 so it seemed OK until we were in a clustered system. The
> backup node would see a size of 0 due to the reservation on the drive. That caused
> the driver to switch to 16-byte CDB's which are not supported on older controllers.
> After that everything was broken.
> It may seem petty but I don't see the value in trying to determine if the LBA is
> beyond the 2TB boundary. That's why when we switch we use 16-byte CDB's for all
> read/write operations.
> Please consider this for inclusion.
> 
> ...
>
> +  	if (total_size == 0xFFFFFFFF) {

I seem to remember having already questioned this.  total_size is sector_t, which
can be either 32-bit or 64-bit.  Are you sure that comparison works as
intended in both cases?


> +		if(total_size == 0xFFFFFFFF) {
>  			cciss_read_capacity_16(cntl_num, i, 0,
>  			&total_size, &block_size);
>  			hba[cntl_num]->cciss_read = CCISS_READ_16;

Here too.

Re: [Patch 1/2] cciss: fix for 2TB support

[Mike Miller (OS Dev)]

On Wed, Feb 21, 2007 at 07:14:27PM -0800, Andrew Morton wrote:
> On Wed, 21 Feb 2007 15:10:39 -0600 "Mike Miller (OS Dev)" <mikem@beardog.cca.cpqcorp.net> wrote:
> 
> > Patch 1/2
> > 
> > This patch changes the way we determine if a logical volume is larger than 2TB. The
> > original test looked for a total_size of 0. Originally we added 1 to the total_size.
> > That would make our read_capacity return size 0 for >2TB lv's. We assumed that we
> > could not have a lv size of 0 so it seemed OK until we were in a clustered system. The
> > backup node would see a size of 0 due to the reservation on the drive. That caused
> > the driver to switch to 16-byte CDB's which are not supported on older controllers.
> > After that everything was broken.
> > It may seem petty but I don't see the value in trying to determine if the LBA is
> > beyond the 2TB boundary. That's why when we switch we use 16-byte CDB's for all
> > read/write operations.
> > Please consider this for inclusion.
> > 
> > ...
> >
> > +  	if (total_size == 0xFFFFFFFF) {
> 
> I seem to remember having already questioned this.  total_size is sector_t, which
> can be either 32-bit or 64-bit.  Are you sure that comparison works as
> intended in both cases?
> 
> 
> > +		if(total_size == 0xFFFFFFFF) {
> >  			cciss_read_capacity_16(cntl_num, i, 0,
> >  			&total_size, &block_size);
> >  			hba[cntl_num]->cciss_read = CCISS_READ_16;
> 
> Here too.
It has worked in all of the configs I've tested. Should I change it from sector_t to a
__64? I have not tested all possible configs.

-- mikem

Re: [Patch 1/2] cciss: fix for 2TB support

[Andrew Morton]

> On Thu, 22 Feb 2007 10:51:23 -0600 "Mike Miller (OS Dev)" <mikem@beardog.cca.cpqcorp.net> wrote:
> On Wed, Feb 21, 2007 at 07:14:27PM -0800, Andrew Morton wrote:
> > On Wed, 21 Feb 2007 15:10:39 -0600 "Mike Miller (OS Dev)" <mikem@beardog.cca.cpqcorp.net> wrote:
> > 
> > > Patch 1/2
> > > +  	if (total_size == 0xFFFFFFFF) {
> > 
> > I seem to remember having already questioned this.  total_size is sector_t, which
> > can be either 32-bit or 64-bit.  Are you sure that comparison works as
> > intended in both cases?
> > 
> > 
> > > +		if(total_size == 0xFFFFFFFF) {
> > >  			cciss_read_capacity_16(cntl_num, i, 0,
> > >  			&total_size, &block_size);
> > >  			hba[cntl_num]->cciss_read = CCISS_READ_16;
> > 
> > Here too.
> It has worked in all of the configs I've tested. Should I change it from sector_t to a
> __64? I have not tested all possible configs.
> 

I'd suggest using -1: that just works.

Re: [Patch 1/2] cciss: fix for 2TB support

[James Bottomley]

On Thu, 2007-02-22 at 13:24 -0800, Andrew Morton wrote:
> > On Thu, 22 Feb 2007 10:51:23 -0600 "Mike Miller (OS Dev)" <mikem@beardog.cca.cpqcorp.net> wrote:
> > On Wed, Feb 21, 2007 at 07:14:27PM -0800, Andrew Morton wrote:
> > > On Wed, 21 Feb 2007 15:10:39 -0600 "Mike Miller (OS Dev)" <mikem@beardog.cca.cpqcorp.net> wrote:
> > > 
> > > > Patch 1/2
> > > > +  	if (total_size == 0xFFFFFFFF) {
> > > 
> > > I seem to remember having already questioned this.  total_size is sector_t, which
> > > can be either 32-bit or 64-bit.  Are you sure that comparison works as
> > > intended in both cases?
> > > 
> > > 
> > > > +		if(total_size == 0xFFFFFFFF) {
> > > >  			cciss_read_capacity_16(cntl_num, i, 0,
> > > >  			&total_size, &block_size);
> > > >  			hba[cntl_num]->cciss_read = CCISS_READ_16;
> > > 
> > > Here too.
> > It has worked in all of the configs I've tested. Should I change it from sector_t to a
> > __64? I have not tested all possible configs.
> > 
> 
> I'd suggest using -1: that just works.

Actually, no, that won't work.

This is a SCSI heuristic for determining when to use the 16 byte version
of the read capacity command.  The 10 byte command can only return 32
bits of information (this is in sectors, so it returns up to 2TB of
bytes).

The heuristic requirement is that if the size is exactly 0xffffffff then
you should try the 16 byte command (which can return 64 bits of
information).  If that fails then you assume the 0xfffffff is a real
size otherwize, you assume it was truncated and take the real result
from the 16 byte command.

You can see a far more elaborate version of this in operation in
sd.c:sd_read_capacity().

The only thing I'd suggest is to use 0xFFFFFFFFULL as the constant to
prevent sign extension issues.

James

Re: [Patch 1/2] cciss: fix for 2TB support

[Mike Miller (OS Dev)]

On Thu, Feb 22, 2007 at 03:41:24PM -0600, James Bottomley wrote:
> On Thu, 2007-02-22 at 13:24 -0800, Andrew Morton wrote:
> > > On Thu, 22 Feb 2007 10:51:23 -0600 "Mike Miller (OS Dev)" <mikem@beardog.cca.cpqcorp.net> wrote:
> > > On Wed, Feb 21, 2007 at 07:14:27PM -0800, Andrew Morton wrote:
> > > > On Wed, 21 Feb 2007 15:10:39 -0600 "Mike Miller (OS Dev)" <mikem@beardog.cca.cpqcorp.net> wrote:
> > > > 
> > > > > Patch 1/2
> > > > > +  	if (total_size == 0xFFFFFFFF) {
> > > > 
> > > > I seem to remember having already questioned this.  total_size is sector_t, which
> > > > can be either 32-bit or 64-bit.  Are you sure that comparison works as
> > > > intended in both cases?
> > > > 
> > > > 
> > > > > +		if(total_size == 0xFFFFFFFF) {
> > > > >  			cciss_read_capacity_16(cntl_num, i, 0,
> > > > >  			&total_size, &block_size);
> > > > >  			hba[cntl_num]->cciss_read = CCISS_READ_16;
> > > > 
> > > > Here too.
> > > It has worked in all of the configs I've tested. Should I change it from sector_t to a
> > > __64? I have not tested all possible configs.
> > > 
> > 
> > I'd suggest using -1: that just works.
> 
> Actually, no, that won't work.
> 
> This is a SCSI heuristic for determining when to use the 16 byte version
> of the read capacity command.  The 10 byte command can only return 32
> bits of information (this is in sectors, so it returns up to 2TB of
> bytes).
> 
> The heuristic requirement is that if the size is exactly 0xffffffff then
> you should try the 16 byte command (which can return 64 bits of
> information).  If that fails then you assume the 0xfffffff is a real
> size otherwize, you assume it was truncated and take the real result
> from the 16 byte command.
> 
> You can see a far more elaborate version of this in operation in
> sd.c:sd_read_capacity().
> 
> The only thing I'd suggest is to use 0xFFFFFFFFULL as the constant to
> prevent sign extension issues.
> 
> James
> 
> 
Will this patch for my patch work for now?


diff --git a/drivers/block/cciss.c b/drivers/block/cciss.c
index 1abf1f5..a1f1d9f 100644
--- a/drivers/block/cciss.c
+++ b/drivers/block/cciss.c
@@ -1303,7 +1303,7 @@ static void cciss_update_drive_info(int 
 
   	/* if read_capacity returns all F's this volume is >2TB in size */
   	/* so we switch to 16-byte CDB's for all read/write ops */
-  	if (total_size == 0xFFFFFFFF) {
+  	if (total_size == 0xFFFFFFFFULL) {
 		cciss_read_capacity_16(ctlr, drv_index, 1,
 		&total_size, &block_size);
 		h->cciss_read = CCISS_READ_16;
@@ -3129,7 +3129,7 @@ #endif				/* CCISS_DEBUG */
 
 		/* If read_capacity returns all F's the logical is >2TB */
 		/* so we switch to 16-byte CDBs for all read/write ops */
-		if(total_size == 0xFFFFFFFF) {
+		if(total_size == 0xFFFFFFFFULL) {
 			cciss_read_capacity_16(cntl_num, i, 0,
 			&total_size, &block_size);
 			hba[cntl_num]->cciss_read = CCISS_READ_16;

Re: [Patch 1/2] cciss: fix for 2TB support

[James Bottomley]

On Thu, 2007-02-22 at 16:02 -0600, Mike Miller (OS Dev) wrote:
> Will this patch for my patch work for now?

Yes, I think that should be fine ... it's only a theoretical worry; at
the moment sector_t is unsigned ... but just in case.

James

Re: [Patch 1/2] cciss: fix for 2TB support

[Mike Miller (OS Dev)]

On Thu, Feb 22, 2007 at 04:06:41PM -0600, James Bottomley wrote:
> On Thu, 2007-02-22 at 16:02 -0600, Mike Miller (OS Dev) wrote:
> > Will this patch for my patch work for now?
> 
> Yes, I think that should be fine ... it's only a theoretical worry; at
> the moment sector_t is unsigned ... but just in case.
> 
> James
> 
> 
Andrew,
Are you waiting for a new patch from me? Or is my patch's patch sufficient?

-- mikem

Re: [Patch 1/2] cciss: fix for 2TB support

[Andrew Morton]

> On Fri, 23 Feb 2007 14:52:29 -0600 "Mike Miller (OS Dev)" <mikem@beardog.cca.cpqcorp.net> wrote:
> On Thu, Feb 22, 2007 at 04:06:41PM -0600, James Bottomley wrote:
> > On Thu, 2007-02-22 at 16:02 -0600, Mike Miller (OS Dev) wrote:
> > > Will this patch for my patch work for now?
> > 
> > Yes, I think that should be fine ... it's only a theoretical worry; at
> > the moment sector_t is unsigned ... but just in case.
> > 
> > James
> > 
> > 
> Andrew,
> Are you waiting for a new patch from me? Or is my patch's patch sufficient?
> 

It looked OK.  But I'm travelling at present, will get back into things
late next week.

Re: [Patch 1/2] cciss: fix for 2TB support

[Mike Miller (OS Dev)]

On Wed, Feb 21, 2007 at 07:14:27PM -0800, Andrew Morton wrote:
> >
> > +  	if (total_size == 0xFFFFFFFF) {
> 
> I seem to remember having already questioned this.  total_size is sector_t, which
> can be either 32-bit or 64-bit.  Are you sure that comparison works as
> intended in both cases?
> 
> 
> > +		if(total_size == 0xFFFFFFFF) {
> >  			cciss_read_capacity_16(cntl_num, i, 0,
> >  			&total_size, &block_size);
> >  			hba[cntl_num]->cciss_read = CCISS_READ_16;
> 
> Here too.

Andrew,
Using this test program and changing the type of x to int, long, long long signed and
unsigned the comparison always worked on x86, x86_64, and ia64. It looks to me like
the comparsion will always do what we expect. Unless you see some other problem.


#include <stdio.h>

int main(int argc, char *argv[])
{
	unsigned long long x;
	
	x =  0x00000000ffffffff;

	printf(sizeof(x) == 8 ? 
		"x = %lld, sizeof(x) = %d\n" :
		"x = %ld, sizeof(x) = %d\n",  x, sizeof(x));
	if (x == 0xffffffff)
		printf("equal\n");
	else
		printf("not equal\n");
	
}

-- mikem

RE: [Patch 1/2] cciss: fix for 2TB support

[Miller, Mike (OS Dev)]

 

> -----Original Message-----
> From: Mike Miller (OS Dev) [mailto:mikem@beardog.cca.cpqcorp.net] 
> 
> Andrew,
> Using this test program and changing the type of x to int, 
> long, long long signed and unsigned the comparison always 
> worked on x86, x86_64, and ia64. It looks to me like the 
> comparsion will always do what we expect. Unless you see some 
> other problem.
> 
> 
> #include <stdio.h>
> 
> int main(int argc, char *argv[])
> {
> 	unsigned long long x;
> 	
> 	x =  0x00000000ffffffff;
> 
> 	printf(sizeof(x) == 8 ? 
> 		"x = %lld, sizeof(x) = %d\n" :
> 		"x = %ld, sizeof(x) = %d\n",  x, sizeof(x));
> 	if (x == 0xffffffff)
> 		printf("equal\n");
> 	else
> 		printf("not equal\n");
> 	
> }
> 
> -- mikem
> 
BTW: also changed x to be 8 f's, 16 f's, and 8 and 8 as shown.