[PATCH 1/1] scsicam_getgeo_odd_sector_size

[PATCH 1/1] scsicam_getgeo_odd_sector_size

[brking]

The following BUG() was observed when a scsi disk formatted to 522 bytes/sector
was discovered attached to a sym2 adapter.

<2>kernel BUG in grow_buffers at fs/buffer.c:1271!

which corresponds to:

static inline int
grow_buffers(struct block_device *bdev, sector_t block, int size)
{
        struct page *page;
        pgoff_t index;
        int sizebits;

        /* Size must be multiple of hard sectorsize */
        if (size & (bdev_hardsect_size(bdev)-1))
                BUG();
        if (size < 512 || size > PAGE_SIZE)
                BUG();                      <----- line 1271 is here


The following patch fixes this by not attempting to read the partition table
of a device with an unsupported sector size.


Signed-off-by: Brian King <brking@us.ibm.com>
---

 linux-2.6.8.1-bjking1/drivers/scsi/scsicam.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletion(-)

diff -puN drivers/scsi/scsicam.c~scsicam_getgeo_odd_sector_size drivers/scsi/scsicam.c
--- linux-2.6.8.1/drivers/scsi/scsicam.c~scsicam_getgeo_odd_sector_size	2004-08-17 11:28:31.000000000 -0500
+++ linux-2.6.8.1-bjking1/drivers/scsi/scsicam.c	2004-08-17 11:33:13.000000000 -0500
@@ -29,7 +29,10 @@ unsigned char *scsi_bios_ptable(struct b
 	unsigned char *res = kmalloc(66, GFP_KERNEL);
 	if (res) {
 		struct block_device *bdev = dev->bd_contains;
-		struct buffer_head *bh = __bread(bdev, 0, block_size(bdev));
+		struct buffer_head *bh = NULL;
+
+		if (get_capacity(bdev->bd_disk))
+			bh = __bread(bdev, 0, block_size(bdev));
 		if (bh) {
 			memcpy(res, bh->b_data + 0x1be, 66);
 			brelse(bh);
_

Re: [PATCH 1/1] scsicam_getgeo_odd_sector_size

[Brian King]

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

This patch appears to have been dropped. Resending.

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

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


The following BUG() was observed when a scsi disk formatted to 522 bytes/sector
was discovered attached to a sym2 adapter.

<2>kernel BUG in grow_buffers at fs/buffer.c:1271!

which corresponds to:

static inline int
grow_buffers(struct block_device *bdev, sector_t block, int size)
{
        struct page *page;
        pgoff_t index;
        int sizebits;

        /* Size must be multiple of hard sectorsize */
        if (size & (bdev_hardsect_size(bdev)-1))
                BUG();
        if (size < 512 || size > PAGE_SIZE)
                BUG();                      <----- line 1271 is here


The following patch fixes this by not attempting to read the partition table
of a device with an unsupported sector size.


Signed-off-by: Brian King <brking@us.ibm.com>
---

 linux-2.6.9-rc4-bk1-bjking1/drivers/scsi/scsicam.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletion(-)

diff -puN drivers/scsi/scsicam.c~scsicam_getgeo_odd_sector_size drivers/scsi/scsicam.c
--- linux-2.6.9-rc4-bk1/drivers/scsi/scsicam.c~scsicam_getgeo_odd_sector_size	2004-10-13 08:57:46.000000000 -0500
+++ linux-2.6.9-rc4-bk1-bjking1/drivers/scsi/scsicam.c	2004-10-13 08:57:46.000000000 -0500
@@ -29,7 +29,10 @@ unsigned char *scsi_bios_ptable(struct b
 	unsigned char *res = kmalloc(66, GFP_KERNEL);
 	if (res) {
 		struct block_device *bdev = dev->bd_contains;
-		struct buffer_head *bh = __bread(bdev, 0, block_size(bdev));
+		struct buffer_head *bh = NULL;
+
+		if (get_capacity(bdev->bd_disk))
+			bh = __bread(bdev, 0, block_size(bdev));
 		if (bh) {
 			memcpy(res, bh->b_data + 0x1be, 66);
 			brelse(bh);
_

Re: [PATCH 1/1] scsicam_getgeo_odd_sector_size

[Matthew Wilcox]

On Wed, Oct 13, 2004 at 09:01:26AM -0500, Brian King wrote:
> grow_buffers(struct block_device *bdev, sector_t block, int size)
> {
>         struct page *page;
>         pgoff_t index;
>         int sizebits;
> 
>         /* Size must be multiple of hard sectorsize */
>         if (size & (bdev_hardsect_size(bdev)-1))
>                 BUG();
>         if (size < 512 || size > PAGE_SIZE)
>                 BUG();                      <----- line 1271 is here

What tree is this in?  Neither of these BUG()s appear in 2.6.9-rc4.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: [PATCH 1/1] scsicam_getgeo_odd_sector_size

[Brian King]

Looks like the code in fs/buffer.c has changed since I originally submitted this.
The BUG()s appear to have been moved directly into __getblk_slow. The patch is
still appropriate and needed, but the text at the beginning is now incorrect.
Here was the backtrace of the original failure.


GPR24: C0000000CCC9BB00 C0000000CF7B3C20 0000000000001050 0000000000001050
GPR28: 0000000000000000 C0000000CCC9BB00 C0000000005E1480 C0000000CCC9BB00
NIP [c0000000000c479c] .__getblk_slow+0xa8/0x484
LR [c0000000000c474c] .__getblk_slow+0x58/0x484
Call Trace:
[c0000000cf7b3520] [c0000000000c474c] .__getblk_slow+0x58/0x484 (unreliable)
[c0000000cf7b35e0] [c0000000000c4bdc] .__getblk+0x64/0x6c
[c0000000cf7b3680] [c0000000000c7b3c] .__bread+0x20/0x14c
[c0000000cf7b3710] [d000000000098044] .scsi_bios_ptable+0x50/0xd4 [scsi_mod]
[c0000000cf7b37a0] [d0000000000980f8] .scsicam_bios_param+0x30/0x18c [scsi_mod]
[c0000000cf7b3840] [d000000000063118] .sd_ioctl+0x240/0x274 [sd_mod]
[c0000000cf7b38f0] [c000000000258a64] .blkdev_ioctl+0xd0/0x9e4
[c0000000cf7b3a50] [c0000000000cbbc8] .block_ioctl+0x18/0x2c
[c0000000cf7b3ad0] [c0000000000dd3d4] .sys_ioctl+0x450/0x690
[c0000000cf7b3bb0] [c00000000002715c] .hdio_getgeo+0x40/0xcc
[c0000000cf7b3c50] [c0000000000fc57c] .compat_sys_ioctl+0x178/0x32c
[c0000000cf7b3d10] [c00000000001045c] .ret_from_syscall_1+0x0/0xa4

-Brian

Matthew Wilcox wrote:
> On Wed, Oct 13, 2004 at 09:01:26AM -0500, Brian King wrote:
> 
>>grow_buffers(struct block_device *bdev, sector_t block, int size)
>>{
>>        struct page *page;
>>        pgoff_t index;
>>        int sizebits;
>>
>>        /* Size must be multiple of hard sectorsize */
>>        if (size & (bdev_hardsect_size(bdev)-1))
>>                BUG();
>>        if (size < 512 || size > PAGE_SIZE)
>>                BUG();                      <----- line 1271 is here
> 
> 
> What tree is this in?  Neither of these BUG()s appear in 2.6.9-rc4.
> 

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH 1/1] scsicam_getgeo_odd_sector_size

[James Bottomley]

On Wed, 2004-10-13 at 09:25, Brian King wrote:
> Looks like the code in fs/buffer.c has changed since I originally submitted this.
> The BUG()s appear to have been moved directly into __getblk_slow. The patch is
> still appropriate and needed, but the text at the beginning is now incorrect.
> Here was the backtrace of the original failure.
> 
> 
> GPR24: C0000000CCC9BB00 C0000000CF7B3C20 0000000000001050 0000000000001050
> GPR28: 0000000000000000 C0000000CCC9BB00 C0000000005E1480 C0000000CCC9BB00
> NIP [c0000000000c479c] .__getblk_slow+0xa8/0x484
> LR [c0000000000c474c] .__getblk_slow+0x58/0x484
> Call Trace:
> [c0000000cf7b3520] [c0000000000c474c] .__getblk_slow+0x58/0x484 (unreliable)
> [c0000000cf7b35e0] [c0000000000c4bdc] .__getblk+0x64/0x6c
> [c0000000cf7b3680] [c0000000000c7b3c] .__bread+0x20/0x14c
> [c0000000cf7b3710] [d000000000098044] .scsi_bios_ptable+0x50/0xd4 [scsi_mod]
> [c0000000cf7b37a0] [d0000000000980f8] .scsicam_bios_param+0x30/0x18c [scsi_mod]
> [c0000000cf7b3840] [d000000000063118] .sd_ioctl+0x240/0x274 [sd_mod]
> [c0000000cf7b38f0] [c000000000258a64] .blkdev_ioctl+0xd0/0x9e4
> [c0000000cf7b3a50] [c0000000000cbbc8] .block_ioctl+0x18/0x2c
> [c0000000cf7b3ad0] [c0000000000dd3d4] .sys_ioctl+0x450/0x690
> [c0000000cf7b3bb0] [c00000000002715c] .hdio_getgeo+0x40/0xcc
> [c0000000cf7b3c50] [c0000000000fc57c] .compat_sys_ioctl+0x178/0x32c
> [c0000000cf7b3d10] [c00000000001045c] .ret_from_syscall_1+0x0/0xa4

But this isn't a BUG().  It should be a warning followed by a stack
dump.  However, it looks like the correct fix is to get rid of the
__bread() in the cam code.

How does the attached work for you?

James

===== drivers/scsi/scsicam.c 1.16 vs edited =====
--- 1.16/drivers/scsi/scsicam.c	2004-06-19 09:45:02 -05:00
+++ edited/drivers/scsi/scsicam.c	2004-10-13 10:38:25 -05:00
@@ -29,10 +29,11 @@
 	unsigned char *res = kmalloc(66, GFP_KERNEL);
 	if (res) {
 		struct block_device *bdev = dev->bd_contains;
-		struct buffer_head *bh = __bread(bdev, 0, block_size(bdev));
-		if (bh) {
-			memcpy(res, bh->b_data + 0x1be, 66);
-			brelse(bh);
+		Sector sect;
+		void *data = read_dev_sector(bdev, 0, &sect);
+		if (data) {
+			memcpy(res, data + 0x1be, 66);
+			put_dev_sector(sect);
 		} else {
 			kfree(res);
 			res = NULL;

Re: [PATCH 1/1] scsicam_getgeo_odd_sector_size

[Brian King]

James Bottomley wrote:
> But this isn't a BUG().  It should be a warning followed by a stack
> dump.  However, it looks like the correct fix is to get rid of the
> __bread() in the cam code.

Looks like the BUG() was removed since I originally submitted this patch. I
recreated the original problem with a 2.6.9-rc2 kernel, which has the
change you mention above. It looks like we now survive the __getblk call
from __bread, but __bread promptly dereferences the NULL bh returned by
__getblk by calling buffer_uptodate and we end up oopsing all the same.

> How does the attached work for you?

Not very well. It looks like it blows up in create_buffers() with this patch.
Since the block size (522) is not a multiple of the host page size, we end up
decrementing offset to a negative number in create_buffers, which then hits a BUG()
in set_bh_page. It looks to me like the block layer assumes the device block size
is a multiple of PAGE_SIZE, in more than one place. Here is the latest backtrace:

cpu 0x7: Vector: 300 (Data Access) at [c00000003480b240]
     pc: c0000000000aba20: .create_empty_buffers+0x30/0x10c
     lr: c0000000000aba18: .create_empty_buffers+0x28/0x10c
     sp: c00000003480b4c0
    msr: 8000000000009032
    dar: 0
  dsisr: 40000000
   current = 0xc0000000345982c0
   paca    = 0xc000000000465600
     pid   = 9012, comm = hwscan
7:mon> t
[c00000003480b550] c0000000000afbb0 .block_read_full_page+0x2d4/0x3f8
[c00000003480b670] c0000000000b3bf0 .blkdev_readpage+0x20/0x38
[c00000003480b6f0] c00000000007cf0c .read_cache_page+0xf4/0x25c
[c00000003480b7a0] c0000000000f7d84 .read_dev_sector+0x44/0x148
[c00000003480b830] d000000000080f38 .scsi_bios_ptable+0x50/0xd0 [scsi_mod]
[c00000003480b8d0] d000000000080fe8 .scsicam_bios_param+0x30/0x18c [scsi_mod]
[c00000003480b970] d00000000004b170 .sd_ioctl+0x258/0x28c [sd_mod]
[c00000003480ba30] c000000000249eac .blkdev_ioctl+0xd0/0x9b8
[c00000003480bb90] c0000000000b3c88 .block_ioctl+0x18/0x2c
[c00000003480bc10] c0000000000c3e34 .sys_ioctl+0x3c0/0x5c4
[c00000003480bcd0] c000000000021bb0 .hdio_getgeo+0x40/0xd8
[c00000003480bd70] c0000000000e3438 .compat_sys_ioctl+0x148/0x3dc
[c00000003480be30] c000000000010980 syscall_exit+0x0/0x18


> 
> James
> 
> ===== drivers/scsi/scsicam.c 1.16 vs edited =====
> --- 1.16/drivers/scsi/scsicam.c	2004-06-19 09:45:02 -05:00
> +++ edited/drivers/scsi/scsicam.c	2004-10-13 10:38:25 -05:00
> @@ -29,10 +29,11 @@
>  	unsigned char *res = kmalloc(66, GFP_KERNEL);
>  	if (res) {
>  		struct block_device *bdev = dev->bd_contains;
> -		struct buffer_head *bh = __bread(bdev, 0, block_size(bdev));
> -		if (bh) {
> -			memcpy(res, bh->b_data + 0x1be, 66);
> -			brelse(bh);
> +		Sector sect;
> +		void *data = read_dev_sector(bdev, 0, &sect);
> +		if (data) {
> +			memcpy(res, data + 0x1be, 66);
> +			put_dev_sector(sect);
>  		} else {
>  			kfree(res);
>  			res = NULL;
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center

Re: [PATCH 1/1] scsicam_getgeo_odd_sector_size

[James Bottomley]

On Wed, 2004-10-13 at 14:55, Brian King wrote:
> James Bottomley wrote:
> > How does the attached work for you?
> 
> Not very well. It looks like it blows up in create_buffers() with this patch.
> Since the block size (522) is not a multiple of the host page size, we end up
> decrementing offset to a negative number in create_buffers, which then hits a BUG()
> in set_bh_page. It looks to me like the block layer assumes the device block size
> is a multiple of PAGE_SIZE, in more than one place. Here is the latest backtrace:

OK it seems that we get to bogus block size confusion before we see the
device's zero size.  The least nasty fix seems to be to set a power of
two blocksize as well as zeroing the capacity for this problem.

How does the attached fare?

James

===== drivers/scsi/sd.c 1.160 vs edited =====
--- 1.160/drivers/scsi/sd.c	2004-09-19 11:55:03 -05:00
+++ edited/drivers/scsi/sd.c	2004-10-13 16:29:37 -05:00
@@ -1125,6 +1125,13 @@
 		 * For this reason, we leave the thing in the table.
 		 */
 		sdkp->capacity = 0;
+		/*
+		 * set a bogus sector size so the normal read/write
+		 * logic in the block layer will eventually refuse any
+		 * request on this device without tripping over power
+		 * of two sector size assumptions
+		 */
+		sector_size = 512;
 	}
 	{
 		/*
===== drivers/scsi/scsicam.c 1.16 vs edited =====
--- 1.16/drivers/scsi/scsicam.c	2004-06-19 09:45:02 -05:00
+++ edited/drivers/scsi/scsicam.c	2004-10-13 10:38:25 -05:00
@@ -29,10 +29,11 @@
 	unsigned char *res = kmalloc(66, GFP_KERNEL);
 	if (res) {
 		struct block_device *bdev = dev->bd_contains;
-		struct buffer_head *bh = __bread(bdev, 0, block_size(bdev));
-		if (bh) {
-			memcpy(res, bh->b_data + 0x1be, 66);
-			brelse(bh);
+		Sector sect;
+		void *data = read_dev_sector(bdev, 0, &sect);
+		if (data) {
+			memcpy(res, data + 0x1be, 66);
+			put_dev_sector(sect);
 		} else {
 			kfree(res);
 			res = NULL;

Re: [PATCH 1/1] scsicam_getgeo_odd_sector_size

[Brian King]

James Bottomley wrote:
> On Wed, 2004-10-13 at 14:55, Brian King wrote:
> 
>>James Bottomley wrote:
>>
>>>How does the attached work for you?
>>
>>Not very well. It looks like it blows up in create_buffers() with this patch.
>>Since the block size (522) is not a multiple of the host page size, we end up
>>decrementing offset to a negative number in create_buffers, which then hits a BUG()
>>in set_bh_page. It looks to me like the block layer assumes the device block size
>>is a multiple of PAGE_SIZE, in more than one place. Here is the latest backtrace:
> 
> 
> OK it seems that we get to bogus block size confusion before we see the
> device's zero size.  The least nasty fix seems to be to set a power of
> two blocksize as well as zeroing the capacity for this problem.
> 
> How does the attached fare?

Much better. I no longer get an oops. However, I do get the following confusing
console output since we are lying to the layers above regarding the sector size:

sdl : unsupported sector size 522.
SCSI device sdl: 0 512-byte hdwr sectors (0 MB)

As far as I am concerned, this isn't a showstopper for this patch,
just an observation.

Thanks

-Brian

> 
> James
> 
> ===== drivers/scsi/sd.c 1.160 vs edited =====
> --- 1.160/drivers/scsi/sd.c	2004-09-19 11:55:03 -05:00
> +++ edited/drivers/scsi/sd.c	2004-10-13 16:29:37 -05:00
> @@ -1125,6 +1125,13 @@
>  		 * For this reason, we leave the thing in the table.
>  		 */
>  		sdkp->capacity = 0;
> +		/*
> +		 * set a bogus sector size so the normal read/write
> +		 * logic in the block layer will eventually refuse any
> +		 * request on this device without tripping over power
> +		 * of two sector size assumptions
> +		 */
> +		sector_size = 512;
>  	}
>  	{
>  		/*
> ===== drivers/scsi/scsicam.c 1.16 vs edited =====
> --- 1.16/drivers/scsi/scsicam.c	2004-06-19 09:45:02 -05:00
> +++ edited/drivers/scsi/scsicam.c	2004-10-13 10:38:25 -05:00
> @@ -29,10 +29,11 @@
>  	unsigned char *res = kmalloc(66, GFP_KERNEL);
>  	if (res) {
>  		struct block_device *bdev = dev->bd_contains;
> -		struct buffer_head *bh = __bread(bdev, 0, block_size(bdev));
> -		if (bh) {
> -			memcpy(res, bh->b_data + 0x1be, 66);
> -			brelse(bh);
> +		Sector sect;
> +		void *data = read_dev_sector(bdev, 0, &sect);
> +		if (data) {
> +			memcpy(res, data + 0x1be, 66);
> +			put_dev_sector(sect);
>  		} else {
>  			kfree(res);
>  			res = NULL;
> 
> -
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Brian King
eServer Storage I/O
IBM Linux Technology Center