Re: [Cbe-oss-dev] [PATCH] ps3rom: sector size should be 512 bytes

Re: [Cbe-oss-dev] [PATCH] ps3rom: sector size should be 512 bytes

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1792 bytes --]

On Tue, 29 Jan 2008, Aegis Lin wrote:
> Quite minor patch, but from the context it should be desired
> that 64KB is available for ATAPI transferrring.
> (Historically) in SCSI/block layer sector size is defined as
> 512 during sector-byte calculation. Originally in ps3rom.c
> CD_FRAMESIZE was used, which makes
> /sys/block/sr0/queue/max_sectors_kb
> to be limited to 16KB.

Can I please have a Signed-off-by?

> diff --git a/drivers/scsi/ps3rom.c b/drivers/scsi/ps3rom.c
> index 17b4a7c..cc7062b 100644
> --- a/drivers/scsi/ps3rom.c
> +++ b/drivers/scsi/ps3rom.c
> @@ -35,7 +35,7 @@
>  
>  #define BOUNCE_SIZE			(64*1024)
>  
> -#define PS3ROM_MAX_SECTORS		(BOUNCE_SIZE / CD_FRAMESIZE)
> +#define PS3ROM_MAX_SECTORS		(BOUNCE_SIZE / 512)
>  
>  
>  struct ps3rom_private {

Indeed, scsi_host_template.max_sectors seems to be just passed to the block
layer, so it concerns 512-byte sectors.

Furthermore, drivers/cdrom/cdrom.c:cdrom_read_cdda_bpc() seems to handle the
conversion from raw CD frame sizes to 512-byte sectors, and limit the requested
number of sectors if needed.

However, I didn't find where it handles conversion from CD data frame sizes to
512-byte sectors.

Am I missing something?

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [Cbe-oss-dev] [PATCH] ps3rom: sector size should be 512 bytes

[Nicholas A. Bellinger]

Greetings Geert and Co,

I have a related patch that I have been using with ps3rom.c for some
time that fixes a bug in fs/bio.c that assumes 512 byte sectors for
ATAPI operations.  This bug actually exists for all non 512 byte sector
devices go through this code path (I found it with
scsi_execute_async()), but I first ran into this issue with ps3rom.c
because max_sectors (32) is small enough to trigger the bug assuming 512
byte sectors during typical ATAPI READ_10 ops with iSCSI/HD.  Because
typical max_sector settings for libata and USB are much higher, I have
never ran into this issue outside of ps3rom.c, but the bug exists
nevertheless..

The current patch assumes 512 byte sectors, and adds a sector_size
parameter to drivers/scsi/scsi_lib.c:scsi_req_map_sg() to change it for
passed struct request.  I know that some folks talked about killing
scsi_execute_async() and fixing this problem elsewhere, but until then
please consider this patch.  Any input is also appreciated.

:-)

--nab

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a417a6f..caa399c 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -297,7 +297,8 @@ static int scsi_bi_endio(struct bio *bio, unsigned
int bytes_done, int error)
  * sent to use, as some ULDs use that struct to only organize the
pages.
  */
 static int scsi_req_map_sg(struct request *rq, struct scatterlist *sgl,
-                          int nsegs, unsigned bufflen, gfp_t gfp)
+                          int nsegs, unsigned bufflen, gfp_t gfp,
+                          unsigned int sector_size)
 {
        struct request_queue *q = rq->q;
        int nr_pages = (bufflen + sgl[0].offset + PAGE_SIZE - 1) >>
PAGE_SHIFT;
@@ -325,6 +326,8 @@ static int scsi_req_map_sg(struct request *rq,
struct scatterlist *sgl,
                                        goto free_bios;
                                }
                                bio->bi_end_io = scsi_bi_endio;
+                               if (sector_size != bio->bi_sector_size)
+                                       bio->bi_sector_size =
sector_size;
                        }
 
                        if (bio_add_pc_page(q, bio, page, bytes, off) !=
@@ -399,7 +402,8 @@ int scsi_execute_async(struct scsi_device *sdev,
const unsigned char *cmd,
        req->cmd_flags |= REQ_QUIET;
 
        if (use_sg)
-               err = scsi_req_map_sg(req, buffer, use_sg, bufflen,
gfp);
+               err = scsi_req_map_sg(req, buffer, use_sg, bufflen, gfp,
+                               sdev->sector_size);
        else if (bufflen)
                err = blk_rq_map_kern(req->q, req, buffer, bufflen,
gfp);
 

diff --git a/fs/bio.c b/fs/bio.c
index 29a44c1..00e0490 100644
--- a/fs/bio.c
+++ b/fs/bio.c
@@ -188,8 +188,10 @@ struct bio *bio_alloc(gfp_t gfp_mask, int
nr_iovecs)
 {
        struct bio *bio = bio_alloc_bioset(gfp_mask, nr_iovecs,
fs_bio_set);
 
-       if (bio)
+       if (bio) {
+               bio->bi_sector_size = 512;
                bio->bi_destructor = bio_fs_destructor;
+       }
 
        return bio;
 }
@@ -328,7 +330,7 @@ static int __bio_add_page(struct request_queue *q,
struct bio *bio, struct page
        if (unlikely(bio_flagged(bio, BIO_CLONED)))
                return 0;
 
-       if (((bio->bi_size + len) >> 9) > max_sectors)
+       if (((bio->bi_size + len) / bio->bi_sector_size) > max_sectors)
                return 0;
 
        /*
@@ -352,8 +354,9 @@ static int __bio_add_page(struct request_queue *q,
struct bio *bio, struct page
                }
        }
 
-       if (bio->bi_vcnt >= bio->bi_max_vecs)
+       if (bio->bi_vcnt >= bio->bi_max_vecs) {
                return 0;
+       }
 
        /*
         * we might lose a segment or two here, but rather that than
@@ -1026,7 +1029,7 @@ void bio_endio(struct bio *bio, unsigned int
bytes_done, int error)
        }
 
        bio->bi_size -= bytes_done;
-       bio->bi_sector += (bytes_done >> 9);
+       bio->bi_sector += (bytes_done / bio->bi_sector_size);
 
        if (bio->bi_end_io)
                bio->bi_end_io(bio, bytes_done, error);
diff --git a/include/linux/bio.h b/include/linux/bio.h
index 1ddef34..e9ed1d1 100644
--- a/include/linux/bio.h
+++ b/include/linux/bio.h
@@ -105,6 +105,7 @@ struct bio {
        unsigned int            bi_hw_back_size;
 
        unsigned int            bi_max_vecs;    /* max bvl_vecs we can
hold */
+       unsigned int            bi_sector_size; /* sector size */
 
        struct bio_vec          *bi_io_vec;     /* the actual vec list
*/



On Thu, 2008-01-31 at 14:28 +0100, Geert Uytterhoeven wrote:
> On Tue, 29 Jan 2008, Aegis Lin wrote:
> > Quite minor patch, but from the context it should be desired
> > that 64KB is available for ATAPI transferrring.
> > (Historically) in SCSI/block layer sector size is defined as
> > 512 during sector-byte calculation. Originally in ps3rom.c
> > CD_FRAMESIZE was used, which makes
> > /sys/block/sr0/queue/max_sectors_kb
> > to be limited to 16KB.
> 
> Can I please have a Signed-off-by?
> 
> > diff --git a/drivers/scsi/ps3rom.c b/drivers/scsi/ps3rom.c
> > index 17b4a7c..cc7062b 100644
> > --- a/drivers/scsi/ps3rom.c
> > +++ b/drivers/scsi/ps3rom.c
> > @@ -35,7 +35,7 @@
> >  
> >  #define BOUNCE_SIZE			(64*1024)
> >  
> > -#define PS3ROM_MAX_SECTORS		(BOUNCE_SIZE / CD_FRAMESIZE)
> > +#define PS3ROM_MAX_SECTORS		(BOUNCE_SIZE / 512)
> >  
> >  
> >  struct ps3rom_private {
> 
> Indeed, scsi_host_template.max_sectors seems to be just passed to the block
> layer, so it concerns 512-byte sectors.
> 
> Furthermore, drivers/cdrom/cdrom.c:cdrom_read_cdda_bpc() seems to handle the
> conversion from raw CD frame sizes to 512-byte sectors, and limit the requested
> number of sectors if needed.
> 
> However, I didn't find where it handles conversion from CD data frame sizes to
> 512-byte sectors.
> 
> Am I missing something?
> 
> With kind regards,
> 
> Geert Uytterhoeven
> Software Architect
> 
> Sony Network and Software Technology Center Europe
> The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
> 
> Phone:    +32 (0)2 700 8453
> Fax:      +32 (0)2 700 8622
> E-mail:   Geert.Uytterhoeven@sonycom.com
> Internet: http://www.sony-europe.com/
> 
> Sony Network and Software Technology Center Europe
> A division of Sony Service Centre (Europe) N.V.
> Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
> VAT BE 0413.825.160 · RPR Brussels
> Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

-
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

Re: [Cbe-oss-dev] [PATCH] ps3rom: sector size should be 512 bytes

[James Bottomley]

On Thu, 2008-01-31 at 06:10 -0800, Nicholas A. Bellinger wrote:
> Greetings Geert and Co,
> 
> I have a related patch that I have been using with ps3rom.c for some
> time that fixes a bug in fs/bio.c that assumes 512 byte sectors for
> ATAPI operations.  This bug actually exists for all non 512 byte sector
> devices go through this code path (I found it with
> scsi_execute_async()), but I first ran into this issue with ps3rom.c
> because max_sectors (32) is small enough to trigger the bug assuming 512
> byte sectors during typical ATAPI READ_10 ops with iSCSI/HD.  Because
> typical max_sector settings for libata and USB are much higher, I have
> never ran into this issue outside of ps3rom.c, but the bug exists
> nevertheless..
> 
> The current patch assumes 512 byte sectors, and adds a sector_size
> parameter to drivers/scsi/scsi_lib.c:scsi_req_map_sg() to change it for
> passed struct request.  I know that some folks talked about killing
> scsi_execute_async() and fixing this problem elsewhere, but until then
> please consider this patch.  Any input is also appreciated.

My first reaction is really, no; there's no way we should be doing such
a nasty layering violation.

The block code is set up to work with 512 byte sectors for its internal
counts.  Something like this will damage all non-512 byte sector devices
(and we do have several of those).

There are three quantities you need to understand when trying to do
something like this

     1. The actual internal block sector size for sector counts.  This
        is hard coded to 512
     2. The medium sector size.  This is stored in hardsect_size in the
        queue.  Block ensures that it always sends down enough 512 byte
        sectors to be a multiple of this
     3. The block size.  This represents the unit the user of the device
        wants to think in terms of (most often for filesystems, the fs
        block size). It's stored in various places including the
        block_device bd_block_size.

It really sounds like the problem with ps3rom is that hardsect_size
isn't being set correctly.  As I understand the problem, from the
incredibly cursory insight the emails give, so please correct if wrong,
you want the hard sector size to be the CD_FRAMESIZE?

James

Re: [Cbe-oss-dev] [PATCH] ps3rom: sector size should be 512 bytes

[Nicholas A. Bellinger]

On Thu, 2008-01-31 at 09:34 -0600, James Bottomley wrote:
> On Thu, 2008-01-31 at 06:10 -0800, Nicholas A. Bellinger wrote:
> > Greetings Geert and Co,
> > 
> > I have a related patch that I have been using with ps3rom.c for some
> > time that fixes a bug in fs/bio.c that assumes 512 byte sectors for
> > ATAPI operations.  This bug actually exists for all non 512 byte sector
> > devices go through this code path (I found it with
> > scsi_execute_async()), but I first ran into this issue with ps3rom.c
> > because max_sectors (32) is small enough to trigger the bug assuming 512
> > byte sectors during typical ATAPI READ_10 ops with iSCSI/HD.  Because
> > typical max_sector settings for libata and USB are much higher, I have
> > never ran into this issue outside of ps3rom.c, but the bug exists
> > nevertheless..
> > 
> > The current patch assumes 512 byte sectors, and adds a sector_size
> > parameter to drivers/scsi/scsi_lib.c:scsi_req_map_sg() to change it for
> > passed struct request.  I know that some folks talked about killing
> > scsi_execute_async() and fixing this problem elsewhere, but until then
> > please consider this patch.  Any input is also appreciated.
> 
> My first reaction is really, no; there's no way we should be doing such
> a nasty layering violation.
> 

I don't care for it either, but without this patch (or something
similar) all SCSI targets that use scsi_execute_async(), for non 512
byte requests are broken.  This causes a problem when a small max_sector
is correctly used by the LLD, and trips the check in
fs/bio.c:__bio_add_page()

	if (((bio->bi_size + len) >> 9) > max_sectors)

> The block code is set up to work with 512 byte sectors for its internal
> counts.  Something like this will damage all non-512 byte sector devices
> (and we do have several of those).

The purpose of the patch was to make none 512 byte sector struct
scsi_device (2048 byte sector size ATAPI packets received over IP
storage fabric, and queued into ps3rom.c in this particular case) work
with scsi_execute_async().  If there is another target mode SCSI
interface that does not have this problem existing or planned, I have no
problem moving to that one instead.

> 
> There are three quantities you need to understand when trying to do
> something like this
> 
>      1. The actual internal block sector size for sector counts.  This
>         is hard coded to 512
>      2. The medium sector size.  This is stored in hardsect_size in the
>         queue.  Block ensures that it always sends down enough 512 byte
>         sectors to be a multiple of this
>      3. The block size.  This represents the unit the user of the device
>         wants to think in terms of (most often for filesystems, the fs
>         block size). It's stored in various places including the
>         block_device bd_block_size.

Yes, the assumption of the two hardcoded locations of 512 byte sectors
in __bio_add_page() and bio_endio() is where I was orginally running
into problems. I know you have mentioned that no in-tree drivers
currently send non 512 byte sector requests into scsi_execute_async(),
but the issue exists for all current target mode code when going through
said SCSI target interface.

So what I am hearing is that the conditionals in __bio_add_page() and
bio_endio() that assume 512 byte sector size should be checking when the
medium sector size does not match the hardcoded block sector size,  byte
and use the non 512 sector value for the incoming and completion
paths..?

> It really sounds like the problem with ps3rom is that hardsect_size
> isn't being set correctly.  As I understand the problem, from the
> incredibly cursory insight the emails give, so please correct if wrong,
> you want the hard sector size to be the CD_FRAMESIZE?

Sorry, to confuse this.  These are two seperate issues.

--nab
  
> 
> James
> 
> 
> 

Re: [Cbe-oss-dev] [PATCH] ps3rom: sector size should be 512 bytes

[James Bottomley]

On Thu, 2008-01-31 at 08:10 -0800, Nicholas A. Bellinger wrote:
> On Thu, 2008-01-31 at 09:34 -0600, James Bottomley wrote:
> > On Thu, 2008-01-31 at 06:10 -0800, Nicholas A. Bellinger wrote:
> > > Greetings Geert and Co,
> > > 
> > > I have a related patch that I have been using with ps3rom.c for some
> > > time that fixes a bug in fs/bio.c that assumes 512 byte sectors for
> > > ATAPI operations.  This bug actually exists for all non 512 byte sector
> > > devices go through this code path (I found it with
> > > scsi_execute_async()), but I first ran into this issue with ps3rom.c
> > > because max_sectors (32) is small enough to trigger the bug assuming 512
> > > byte sectors during typical ATAPI READ_10 ops with iSCSI/HD.  Because
> > > typical max_sector settings for libata and USB are much higher, I have
> > > never ran into this issue outside of ps3rom.c, but the bug exists
> > > nevertheless..
> > > 
> > > The current patch assumes 512 byte sectors, and adds a sector_size
> > > parameter to drivers/scsi/scsi_lib.c:scsi_req_map_sg() to change it for
> > > passed struct request.  I know that some folks talked about killing
> > > scsi_execute_async() and fixing this problem elsewhere, but until then
> > > please consider this patch.  Any input is also appreciated.
> > 
> > My first reaction is really, no; there's no way we should be doing such
> > a nasty layering violation.
> > 
> 
> I don't care for it either, but without this patch (or something
> similar) all SCSI targets that use scsi_execute_async(), for non 512
> byte requests are broken.  This causes a problem when a small max_sector
> is correctly used by the LLD, and trips the check in
> fs/bio.c:__bio_add_page()
> 
> 	if (((bio->bi_size + len) >> 9) > max_sectors)
> 

Could we rewind this discussion back to an actual problem description
then, please?  Nothing in the standard path for a CD/DVD should be using
scsi_execute_async(), what's the actual problem use case?

James

Re: [Cbe-oss-dev] [PATCH] ps3rom: sector size should be 512 bytes

[Nicholas A. Bellinger]

On Thu, 2008-01-31 at 10:26 -0600, James Bottomley wrote:
> On Thu, 2008-01-31 at 08:10 -0800, Nicholas A. Bellinger wrote:
> > On Thu, 2008-01-31 at 09:34 -0600, James Bottomley wrote:
> > > On Thu, 2008-01-31 at 06:10 -0800, Nicholas A. Bellinger wrote:
> > > > Greetings Geert and Co,
> > > > 
> > > > I have a related patch that I have been using with ps3rom.c for some
> > > > time that fixes a bug in fs/bio.c that assumes 512 byte sectors for
> > > > ATAPI operations.  This bug actually exists for all non 512 byte sector
> > > > devices go through this code path (I found it with
> > > > scsi_execute_async()), but I first ran into this issue with ps3rom.c
> > > > because max_sectors (32) is small enough to trigger the bug assuming 512
> > > > byte sectors during typical ATAPI READ_10 ops with iSCSI/HD.  Because
> > > > typical max_sector settings for libata and USB are much higher, I have
> > > > never ran into this issue outside of ps3rom.c, but the bug exists
> > > > nevertheless..
> > > > 
> > > > The current patch assumes 512 byte sectors, and adds a sector_size
> > > > parameter to drivers/scsi/scsi_lib.c:scsi_req_map_sg() to change it for
> > > > passed struct request.  I know that some folks talked about killing
> > > > scsi_execute_async() and fixing this problem elsewhere, but until then
> > > > please consider this patch.  Any input is also appreciated.
> > > 
> > > My first reaction is really, no; there's no way we should be doing such
> > > a nasty layering violation.
> > > 
> > 
> > I don't care for it either, but without this patch (or something
> > similar) all SCSI targets that use scsi_execute_async(), for non 512
> > byte requests are broken.  This causes a problem when a small max_sector
> > is correctly used by the LLD, and trips the check in
> > fs/bio.c:__bio_add_page()
> > 
> > 	if (((bio->bi_size + len) >> 9) > max_sectors)
> > 
> 
> Could we rewind this discussion back to an actual problem description
> then, please?  Nothing in the standard path for a CD/DVD should be using
> scsi_execute_async(), what's the actual problem use case?
> 

The problem case is a SCSI Target Mode engine that receives a 2048 Byte
single sector ATAPI READ_10 request from the storage fabric, and uses
scsi_execute_async() (the only option >= 2.6.18) to issue said request
to the underlying struct scsi_device.  Because the underlying bio code
assumes 512 byte only sectors, the check in __bio_add_page() incorrectly
determines that max_sectors (max_sectors has to be low, as with 32 from
ps3rom.c) has been exceeded, and fails the request back up the stack.

--nab

> James
> 
> 
> 

Re: [Cbe-oss-dev] [PATCH] ps3rom: sector size should be 512 bytes

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3484 bytes --]

On Thu, 31 Jan 2008, Nicholas A. Bellinger wrote:
> On Thu, 2008-01-31 at 10:26 -0600, James Bottomley wrote:
> > On Thu, 2008-01-31 at 08:10 -0800, Nicholas A. Bellinger wrote:
> > > On Thu, 2008-01-31 at 09:34 -0600, James Bottomley wrote:
> > > > On Thu, 2008-01-31 at 06:10 -0800, Nicholas A. Bellinger wrote:
> > > > > Greetings Geert and Co,
> > > > > 
> > > > > I have a related patch that I have been using with ps3rom.c for some
> > > > > time that fixes a bug in fs/bio.c that assumes 512 byte sectors for
> > > > > ATAPI operations.  This bug actually exists for all non 512 byte sector
> > > > > devices go through this code path (I found it with
> > > > > scsi_execute_async()), but I first ran into this issue with ps3rom.c
> > > > > because max_sectors (32) is small enough to trigger the bug assuming 512
> > > > > byte sectors during typical ATAPI READ_10 ops with iSCSI/HD.  Because
> > > > > typical max_sector settings for libata and USB are much higher, I have
> > > > > never ran into this issue outside of ps3rom.c, but the bug exists
> > > > > nevertheless..
> > > > > 
> > > > > The current patch assumes 512 byte sectors, and adds a sector_size
> > > > > parameter to drivers/scsi/scsi_lib.c:scsi_req_map_sg() to change it for
> > > > > passed struct request.  I know that some folks talked about killing
> > > > > scsi_execute_async() and fixing this problem elsewhere, but until then
> > > > > please consider this patch.  Any input is also appreciated.
> > > > 
> > > > My first reaction is really, no; there's no way we should be doing such
> > > > a nasty layering violation.
> > > > 
> > > 
> > > I don't care for it either, but without this patch (or something
> > > similar) all SCSI targets that use scsi_execute_async(), for non 512
> > > byte requests are broken.  This causes a problem when a small max_sector
> > > is correctly used by the LLD, and trips the check in
> > > fs/bio.c:__bio_add_page()
> > > 
> > > 	if (((bio->bi_size + len) >> 9) > max_sectors)
> > > 
> > 
> > Could we rewind this discussion back to an actual problem description
> > then, please?  Nothing in the standard path for a CD/DVD should be using
> > scsi_execute_async(), what's the actual problem use case?
> > 
> 
> The problem case is a SCSI Target Mode engine that receives a 2048 Byte
> single sector ATAPI READ_10 request from the storage fabric, and uses
> scsi_execute_async() (the only option >= 2.6.18) to issue said request
> to the underlying struct scsi_device.  Because the underlying bio code
> assumes 512 byte only sectors, the check in __bio_add_page() incorrectly
> determines that max_sectors (max_sectors has to be low, as with 32 from
> ps3rom.c) has been exceeded, and fails the request back up the stack.

Thanks for unfocussing James ;-)

James, do you have any comment about the first email in this thread, increasing
scsi_host_template.max_sectors?

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [Cbe-oss-dev] [PATCH] ps3rom: sector size should be 512 bytes

[James Bottomley]

On Thu, 2008-01-31 at 09:28 -0800, Nicholas A. Bellinger wrote:
> On Thu, 2008-01-31 at 10:26 -0600, James Bottomley wrote:
> > On Thu, 2008-01-31 at 08:10 -0800, Nicholas A. Bellinger wrote:
> > > On Thu, 2008-01-31 at 09:34 -0600, James Bottomley wrote:
> > > > On Thu, 2008-01-31 at 06:10 -0800, Nicholas A. Bellinger wrote:
> > > > > Greetings Geert and Co,
> > > > > 
> > > > > I have a related patch that I have been using with ps3rom.c for some
> > > > > time that fixes a bug in fs/bio.c that assumes 512 byte sectors for
> > > > > ATAPI operations.  This bug actually exists for all non 512 byte sector
> > > > > devices go through this code path (I found it with
> > > > > scsi_execute_async()), but I first ran into this issue with ps3rom.c
> > > > > because max_sectors (32) is small enough to trigger the bug assuming 512
> > > > > byte sectors during typical ATAPI READ_10 ops with iSCSI/HD.  Because
> > > > > typical max_sector settings for libata and USB are much higher, I have
> > > > > never ran into this issue outside of ps3rom.c, but the bug exists
> > > > > nevertheless..
> > > > > 
> > > > > The current patch assumes 512 byte sectors, and adds a sector_size
> > > > > parameter to drivers/scsi/scsi_lib.c:scsi_req_map_sg() to change it for
> > > > > passed struct request.  I know that some folks talked about killing
> > > > > scsi_execute_async() and fixing this problem elsewhere, but until then
> > > > > please consider this patch.  Any input is also appreciated.
> > > > 
> > > > My first reaction is really, no; there's no way we should be doing such
> > > > a nasty layering violation.
> > > > 
> > > 
> > > I don't care for it either, but without this patch (or something
> > > similar) all SCSI targets that use scsi_execute_async(), for non 512
> > > byte requests are broken.  This causes a problem when a small max_sector
> > > is correctly used by the LLD, and trips the check in
> > > fs/bio.c:__bio_add_page()
> > > 
> > > 	if (((bio->bi_size + len) >> 9) > max_sectors)
> > > 
> > 
> > Could we rewind this discussion back to an actual problem description
> > then, please?  Nothing in the standard path for a CD/DVD should be using
> > scsi_execute_async(), what's the actual problem use case?
> > 
> 
> The problem case is a SCSI Target Mode engine that receives a 2048 Byte
> single sector ATAPI READ_10 request from the storage fabric, and uses
> scsi_execute_async() (the only option >= 2.6.18) to issue said request
> to the underlying struct scsi_device.  Because the underlying bio code
> assumes 512 byte only sectors, the check in __bio_add_page() incorrectly
> determines that max_sectors (max_sectors has to be low, as with 32 from
> ps3rom.c) has been exceeded, and fails the request back up the stack.

OK, so this is a totally separate issue from the one you actually posted
it as a patch to fix?

the queue max_sectors parameter is also counted in the block internal of
512 byte sectors.  If you set it to 32 that means you were only
expecting 16k of transfers per command maximum.  If that's not right,
then set the limit correctly.

In short, and to repeat: almost every internal size counter to block is
in units of 512 byte sectors ... that includes capacity, maximum etc ...

James

Re: [Cbe-oss-dev] [PATCH] ps3rom: sector size should be 512 bytes

[James Bottomley]

On Thu, 2008-01-31 at 18:41 +0100, Geert Uytterhoeven wrote:
> On Thu, 31 Jan 2008, Nicholas A. Bellinger wrote:
> > On Thu, 2008-01-31 at 10:26 -0600, James Bottomley wrote:
> > > On Thu, 2008-01-31 at 08:10 -0800, Nicholas A. Bellinger wrote:
> > > > On Thu, 2008-01-31 at 09:34 -0600, James Bottomley wrote:
> > > > > On Thu, 2008-01-31 at 06:10 -0800, Nicholas A. Bellinger wrote:
> > > > > > Greetings Geert and Co,
> > > > > > 
> > > > > > I have a related patch that I have been using with ps3rom.c for some
> > > > > > time that fixes a bug in fs/bio.c that assumes 512 byte sectors for
> > > > > > ATAPI operations.  This bug actually exists for all non 512 byte sector
> > > > > > devices go through this code path (I found it with
> > > > > > scsi_execute_async()), but I first ran into this issue with ps3rom.c
> > > > > > because max_sectors (32) is small enough to trigger the bug assuming 512
> > > > > > byte sectors during typical ATAPI READ_10 ops with iSCSI/HD.  Because
> > > > > > typical max_sector settings for libata and USB are much higher, I have
> > > > > > never ran into this issue outside of ps3rom.c, but the bug exists
> > > > > > nevertheless..
> > > > > > 
> > > > > > The current patch assumes 512 byte sectors, and adds a sector_size
> > > > > > parameter to drivers/scsi/scsi_lib.c:scsi_req_map_sg() to change it for
> > > > > > passed struct request.  I know that some folks talked about killing
> > > > > > scsi_execute_async() and fixing this problem elsewhere, but until then
> > > > > > please consider this patch.  Any input is also appreciated.
> > > > > 
> > > > > My first reaction is really, no; there's no way we should be doing such
> > > > > a nasty layering violation.
> > > > > 
> > > > 
> > > > I don't care for it either, but without this patch (or something
> > > > similar) all SCSI targets that use scsi_execute_async(), for non 512
> > > > byte requests are broken.  This causes a problem when a small max_sector
> > > > is correctly used by the LLD, and trips the check in
> > > > fs/bio.c:__bio_add_page()
> > > > 
> > > > 	if (((bio->bi_size + len) >> 9) > max_sectors)
> > > > 
> > > 
> > > Could we rewind this discussion back to an actual problem description
> > > then, please?  Nothing in the standard path for a CD/DVD should be using
> > > scsi_execute_async(), what's the actual problem use case?
> > > 
> > 
> > The problem case is a SCSI Target Mode engine that receives a 2048 Byte
> > single sector ATAPI READ_10 request from the storage fabric, and uses
> > scsi_execute_async() (the only option >= 2.6.18) to issue said request
> > to the underlying struct scsi_device.  Because the underlying bio code
> > assumes 512 byte only sectors, the check in __bio_add_page() incorrectly
> > determines that max_sectors (max_sectors has to be low, as with 32 from
> > ps3rom.c) has been exceeded, and fails the request back up the stack.
> 
> Thanks for unfocussing James ;-)
> 
> James, do you have any comment about the first email in this thread, increasing
> scsi_host_template.max_sectors?

Assuming your BOUNCE_SIZE is in bytes, and represents your largest
transfer, then yes, setting .max_sectors to BOUNCE_SIZE>>9 is exactly
the right way of phrasing this, since .max_sectors is counted in units
of the internal block sector (or 512 bytes).

Were you going to submit a full patch to linux-scsi?

James

Re: [Cbe-oss-dev] [PATCH] ps3rom: sector size should be 512 bytes

[Nicholas A. Bellinger]

On Thu, 2008-01-31 at 11:53 -0600, James Bottomley wrote:
> On Thu, 2008-01-31 at 09:28 -0800, Nicholas A. Bellinger wrote:
> > On Thu, 2008-01-31 at 10:26 -0600, James Bottomley wrote:
> > > On Thu, 2008-01-31 at 08:10 -0800, Nicholas A. Bellinger wrote:
> > > > On Thu, 2008-01-31 at 09:34 -0600, James Bottomley wrote:
> > > > > On Thu, 2008-01-31 at 06:10 -0800, Nicholas A. Bellinger wrote:
> > > > > > Greetings Geert and Co,
> > > > > > 
> > > > > > I have a related patch that I have been using with ps3rom.c for some
> > > > > > time that fixes a bug in fs/bio.c that assumes 512 byte sectors for
> > > > > > ATAPI operations.  This bug actually exists for all non 512 byte sector
> > > > > > devices go through this code path (I found it with
> > > > > > scsi_execute_async()), but I first ran into this issue with ps3rom.c
> > > > > > because max_sectors (32) is small enough to trigger the bug assuming 512
> > > > > > byte sectors during typical ATAPI READ_10 ops with iSCSI/HD.  Because
> > > > > > typical max_sector settings for libata and USB are much higher, I have
> > > > > > never ran into this issue outside of ps3rom.c, but the bug exists
> > > > > > nevertheless..
> > > > > > 
> > > > > > The current patch assumes 512 byte sectors, and adds a sector_size
> > > > > > parameter to drivers/scsi/scsi_lib.c:scsi_req_map_sg() to change it for
> > > > > > passed struct request.  I know that some folks talked about killing
> > > > > > scsi_execute_async() and fixing this problem elsewhere, but until then
> > > > > > please consider this patch.  Any input is also appreciated.
> > > > > 
> > > > > My first reaction is really, no; there's no way we should be doing such
> > > > > a nasty layering violation.
> > > > > 
> > > > 
> > > > I don't care for it either, but without this patch (or something
> > > > similar) all SCSI targets that use scsi_execute_async(), for non 512
> > > > byte requests are broken.  This causes a problem when a small max_sector
> > > > is correctly used by the LLD, and trips the check in
> > > > fs/bio.c:__bio_add_page()
> > > > 
> > > > 	if (((bio->bi_size + len) >> 9) > max_sectors)
> > > > 
> > > 
> > > Could we rewind this discussion back to an actual problem description
> > > then, please?  Nothing in the standard path for a CD/DVD should be using
> > > scsi_execute_async(), what's the actual problem use case?
> > > 
> > 
> > The problem case is a SCSI Target Mode engine that receives a 2048 Byte
> > single sector ATAPI READ_10 request from the storage fabric, and uses
> > scsi_execute_async() (the only option >= 2.6.18) to issue said request
> > to the underlying struct scsi_device.  Because the underlying bio code
> > assumes 512 byte only sectors, the check in __bio_add_page() incorrectly
> > determines that max_sectors (max_sectors has to be low, as with 32 from
> > ps3rom.c) has been exceeded, and fails the request back up the stack.
> 
> OK, so this is a totally separate issue from the one you actually posted
> it as a patch to fix?
> 
> the queue max_sectors parameter is also counted in the block internal of
> 512 byte sectors.  If you set it to 32 that means you were only
> expecting 16k of transfers per command maximum.  If that's not right,
> then set the limit correctly.
> 
> In short, and to repeat: almost every internal size counter to block is
> in units of 512 byte sectors ... that includes capacity, maximum etc ...
> 

Ok, after reading your followup with Geert I see that this looks like a
bug in ps3rom.c assuming 2048 byte sectors to calculate .max_sectors
(which was originally set to 32 as I mentioned).  Using the setting
BOUNCE_SIZE << 9 where BOUNCE_SIZE is the request size in bytes looks
like this will solve the issue.  My misunderstanding was
that .max_sectors was allowed to be calcuated in non 512 byte sectors,
so please disregard my patch.

Geert, .max_sectors for ps3rom.c using 512 byte sectors ends up being
128, yes.?

James, could we put something in the SCSI docs stating that .max_sectors
MUST be calculated against 512 byte sectors..?

Thanks again,

--nab

> James
> 
> 
> 

Re: [Cbe-oss-dev] [PATCH] ps3rom: sector size should be 512 bytes

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1185 bytes --]

On Thu, 31 Jan 2008, James Bottomley wrote:
> On Thu, 2008-01-31 at 18:41 +0100, Geert Uytterhoeven wrote:
> > James, do you have any comment about the first email in this thread, increasing
> > scsi_host_template.max_sectors?
> 
> Assuming your BOUNCE_SIZE is in bytes, and represents your largest
> transfer, then yes, setting .max_sectors to BOUNCE_SIZE>>9 is exactly
> the right way of phrasing this, since .max_sectors is counted in units
> of the internal block sector (or 512 bytes).

OK, thanks for the confirmation!

> Were you going to submit a full patch to linux-scsi?

Yes I will.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [Cbe-oss-dev] [PATCH] ps3rom: sector size should be 512 bytes

[Geert Uytterhoeven]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 2387 bytes --]

On Thu, 31 Jan 2008, Nicholas A. Bellinger wrote:
> On Thu, 2008-01-31 at 11:53 -0600, James Bottomley wrote:
> > On Thu, 2008-01-31 at 09:28 -0800, Nicholas A. Bellinger wrote:
> > > The problem case is a SCSI Target Mode engine that receives a 2048 Byte
> > > single sector ATAPI READ_10 request from the storage fabric, and uses
> > > scsi_execute_async() (the only option >= 2.6.18) to issue said request
> > > to the underlying struct scsi_device.  Because the underlying bio code
> > > assumes 512 byte only sectors, the check in __bio_add_page() incorrectly
> > > determines that max_sectors (max_sectors has to be low, as with 32 from
> > > ps3rom.c) has been exceeded, and fails the request back up the stack.
> > 
> > OK, so this is a totally separate issue from the one you actually posted
> > it as a patch to fix?
> > 
> > the queue max_sectors parameter is also counted in the block internal of
> > 512 byte sectors.  If you set it to 32 that means you were only
> > expecting 16k of transfers per command maximum.  If that's not right,
> > then set the limit correctly.
> > 
> > In short, and to repeat: almost every internal size counter to block is
> > in units of 512 byte sectors ... that includes capacity, maximum etc ...
> > 
> 
> Ok, after reading your followup with Geert I see that this looks like a
> bug in ps3rom.c assuming 2048 byte sectors to calculate .max_sectors
> (which was originally set to 32 as I mentioned).  Using the setting
> BOUNCE_SIZE << 9 where BOUNCE_SIZE is the request size in bytes looks
> like this will solve the issue.  My misunderstanding was
> that .max_sectors was allowed to be calcuated in non 512 byte sectors,
> so please disregard my patch.
> 
> Geert, .max_sectors for ps3rom.c using 512 byte sectors ends up being
> 128, yes.?

Yes.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Network and Software Technology Center Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

Sony Network and Software Technology Center Europe
A division of Sony Service Centre (Europe) N.V.
Registered office: Technologielaan 7 · B-1840 Londerzeel · Belgium
VAT BE 0413.825.160 · RPR Brussels
Fortis Bank Zaventem · Swift GEBABEBB08A · IBAN BE39001382358619

Re: [Cbe-oss-dev] [PATCH] ps3rom: sector size should be 512 bytes

[James Bottomley]

On Thu, 2008-01-31 at 10:44 -0800, Nicholas A. Bellinger wrote:
> > In short, and to repeat: almost every internal size counter to block is
> > in units of 512 byte sectors ... that includes capacity, maximum etc ...
> > 
> 
> Ok, after reading your followup with Geert I see that this looks like a
> bug in ps3rom.c assuming 2048 byte sectors to calculate .max_sectors
> (which was originally set to 32 as I mentioned).  Using the setting
> BOUNCE_SIZE << 9 where BOUNCE_SIZE is the request size in bytes looks
> like this will solve the issue.  My misunderstanding was
> that .max_sectors was allowed to be calcuated in non 512 byte sectors,
> so please disregard my patch.
> 
> Geert, .max_sectors for ps3rom.c using 512 byte sectors ends up being
> 128, yes.?
> 
> James, could we put something in the SCSI docs stating that .max_sectors
> MUST be calculated against 512 byte sectors..?

If that "we" is royal, then certainly you're welcome to submit a patch
(just get Randy's ack).  However, do take a look at the existing docs
first.  Certainly the block layer seems to make this very clear:

/**
 * blk_queue_max_sectors - set max sectors for a request for this queue
 * @q:  the request queue for the device
 * @max_sectors:  max sectors in the usual 512b unit
                              ^^^^^^^^^^^^^^^^^^^^^^
 *
 * Description:
 *    Enables a low level driver to set an upper limit on the size of
 *    received requests.
 **/

I suspect you just didn't read the docs anyway ... ?

James

Update SCSI documentation for 512 byte sector requirement with max_sectors

[Nicholas A. Bellinger]

On Thu, 2008-01-31 at 13:42 -0600, James Bottomley wrote:
> On Thu, 2008-01-31 at 10:44 -0800, Nicholas A. Bellinger wrote:
> > > In short, and to repeat: almost every internal size counter to block is
> > > in units of 512 byte sectors ... that includes capacity, maximum etc ...
> > > 
> > 
> > Ok, after reading your followup with Geert I see that this looks like a
> > bug in ps3rom.c assuming 2048 byte sectors to calculate .max_sectors
> > (which was originally set to 32 as I mentioned).  Using the setting
> > BOUNCE_SIZE << 9 where BOUNCE_SIZE is the request size in bytes looks
> > like this will solve the issue.  My misunderstanding was
> > that .max_sectors was allowed to be calcuated in non 512 byte sectors,
> > so please disregard my patch.
> > 
> > Geert, .max_sectors for ps3rom.c using 512 byte sectors ends up being
> > 128, yes.?
> > 
> > James, could we put something in the SCSI docs stating that .max_sectors
> > MUST be calculated against 512 byte sectors..?
> 
> If that "we" is royal, then certainly you're welcome to submit a patch
> (just get Randy's ack).  However, do take a look at the existing docs
> first.  Certainly the block layer seems to make this very clear:
> 
> /**
>  * blk_queue_max_sectors - set max sectors for a request for this queue
>  * @q:  the request queue for the device
>  * @max_sectors:  max sectors in the usual 512b unit
>                               ^^^^^^^^^^^^^^^^^^^^^^
>  *
>  * Description:
>  *    Enables a low level driver to set an upper limit on the size of
>  *    received requests.
>  **/
> 

Agreed, here is the patch to make this clear within SCSI.  Randy, does
this look OK..?

Thanks,

--nab

Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>

diff --git a/Documentation/scsi/scsi_mid_low_api.txt b/Documentation/scsi/scsi_mid_low_api.txt
index 6f70f2b..570f271 100644
--- a/Documentation/scsi/scsi_mid_low_api.txt
+++ b/Documentation/scsi/scsi_mid_low_api.txt
@@ -1244,13 +1244,12 @@ of interest:
     this_id      - scsi id of host (scsi initiator) or -1 if not known
     sg_tablesize - maximum scatter gather elements allowed by host.
                    0 implies scatter gather not supported by host
-    max_sectors  - maximum number of sectors (usually 512 bytes) allowed
-                   in a single SCSI command. The default value of 0 leads
-                   to a setting of SCSI_DEFAULT_MAX_SECTORS (defined in
-                   scsi_host.h) which is currently set to 1024. So for a
-                   disk the maximum transfer size is 512 KB when max_sectors
-                   is not defined. Note that this size may not be sufficient
-                   for disk firmware uploads.
+    max_sectors  - maximum number of 512 bytes sectors allowed in a single
+                   SCSI command. The default value of 0 leads to a setting
+                   of SCSI_DEFAULT_MAX_SECTORS (defined in scsi_host.h) which
+                   is currently set to 1024. So for a disk the maximum transfer
+                   size is 512 KB when max_sectors is not defined. Note that
+                   this size may not be sufficient for disk firmware uploads.
     cmd_per_lun  - maximum number of commands that can be queued on devices
                    controlled by the host. Overridden by LLD calls to
                    scsi_adjust_queue_depth().
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 5c58d59..84098e3 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -372,7 +372,10 @@ struct scsi_host_template {
        unsigned short sg_tablesize;
 
        /*
-        * If the host adapter has limitations beside segment count
+        * If the host adapter has limitations beside segment count.
+        * Note that this value MUST be calculated in 512 byte sectors,
+        * even if the attached struct scsi_device->sector_size is expected
+        * to use non 512 byte sectors.
         */
        unsigned short max_sectors;



> 

Re: Update SCSI documentation for 512 byte sector requirement with max_sectors

[Randy Dunlap]

On Thu, 31 Jan 2008 17:58:30 -0800 Nicholas A. Bellinger wrote:

> On Thu, 2008-01-31 at 13:42 -0600, James Bottomley wrote:
> > On Thu, 2008-01-31 at 10:44 -0800, Nicholas A. Bellinger wrote:
> > > > In short, and to repeat: almost every internal size counter to block is
> > > > in units of 512 byte sectors ... that includes capacity, maximum etc ...
> > > > 
> > > 
> > > Ok, after reading your followup with Geert I see that this looks like a
> > > bug in ps3rom.c assuming 2048 byte sectors to calculate .max_sectors
> > > (which was originally set to 32 as I mentioned).  Using the setting
> > > BOUNCE_SIZE << 9 where BOUNCE_SIZE is the request size in bytes looks
> > > like this will solve the issue.  My misunderstanding was
> > > that .max_sectors was allowed to be calcuated in non 512 byte sectors,
> > > so please disregard my patch.
> > > 
> > > Geert, .max_sectors for ps3rom.c using 512 byte sectors ends up being
> > > 128, yes.?
> > > 
> > > James, could we put something in the SCSI docs stating that .max_sectors
> > > MUST be calculated against 512 byte sectors..?
> > 
> > If that "we" is royal, then certainly you're welcome to submit a patch
> > (just get Randy's ack).  However, do take a look at the existing docs
> > first.  Certainly the block layer seems to make this very clear:
> > 
> > /**
> >  * blk_queue_max_sectors - set max sectors for a request for this queue
> >  * @q:  the request queue for the device
> >  * @max_sectors:  max sectors in the usual 512b unit
> >                               ^^^^^^^^^^^^^^^^^^^^^^
> >  *
> >  * Description:
> >  *    Enables a low level driver to set an upper limit on the size of
> >  *    received requests.
> >  **/
> > 
> 
> Agreed, here is the patch to make this clear within SCSI.  Randy, does
> this look OK..?
> 
> Thanks,
> 
> --nab
> 
> Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> 
> diff --git a/Documentation/scsi/scsi_mid_low_api.txt b/Documentation/scsi/scsi_mid_low_api.txt
> index 6f70f2b..570f271 100644
> --- a/Documentation/scsi/scsi_mid_low_api.txt
> +++ b/Documentation/scsi/scsi_mid_low_api.txt
> @@ -1244,13 +1244,12 @@ of interest:
>      this_id      - scsi id of host (scsi initiator) or -1 if not known
>      sg_tablesize - maximum scatter gather elements allowed by host.
>                     0 implies scatter gather not supported by host
> -    max_sectors  - maximum number of sectors (usually 512 bytes) allowed
> -                   in a single SCSI command. The default value of 0 leads
> -                   to a setting of SCSI_DEFAULT_MAX_SECTORS (defined in
> -                   scsi_host.h) which is currently set to 1024. So for a
> -                   disk the maximum transfer size is 512 KB when max_sectors
> -                   is not defined. Note that this size may not be sufficient
> -                   for disk firmware uploads.
> +    max_sectors  - maximum number of 512 bytes sectors allowed in a single
> +                   SCSI command. The default value of 0 leads to a setting
> +                   of SCSI_DEFAULT_MAX_SECTORS (defined in scsi_host.h) which
> +                   is currently set to 1024. So for a disk the maximum transfer
> +                   size is 512 KB when max_sectors is not defined. Note that
> +                   this size may not be sufficient for disk firmware uploads.
>      cmd_per_lun  - maximum number of commands that can be queued on devices
>                     controlled by the host. Overridden by LLD calls to
>                     scsi_adjust_queue_depth().
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 5c58d59..84098e3 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -372,7 +372,10 @@ struct scsi_host_template {
>         unsigned short sg_tablesize;
>  
>         /*
> -        * If the host adapter has limitations beside segment count
> +        * If the host adapter has limitations beside segment count.
> +        * Note that this value MUST be calculated in 512 byte sectors,
> +        * even if the attached struct scsi_device->sector_size is expected
> +        * to use non 512 byte sectors.

How about:
	   * to use a sector size other than 512 bytes.

and
Acked-by: Randy Dunlap <randy.dunlap@oracle.com>


>          */
>         unsigned short max_sectors;

---
~Randy

Re: Update SCSI documentation for 512 byte sector requirement with max_sectors

[Nicholas A. Bellinger]

On Thu, 2008-01-31 at 18:46 -0800, Randy Dunlap wrote:
> > Agreed, here is the patch to make this clear within SCSI.  Randy, does
> > this look OK..?
> > 
> > Thanks,
> > 
> > --nab
> > 
> > Signed-off-by: Nicholas A. Bellinger <nab@linux-iscsi.org>
> > 
> > diff --git a/Documentation/scsi/scsi_mid_low_api.txt b/Documentation/scsi/scsi_mid_low_api.txt
> > index 6f70f2b..570f271 100644
> > --- a/Documentation/scsi/scsi_mid_low_api.txt
> > +++ b/Documentation/scsi/scsi_mid_low_api.txt
> > @@ -1244,13 +1244,12 @@ of interest:
> >      this_id      - scsi id of host (scsi initiator) or -1 if not known
> >      sg_tablesize - maximum scatter gather elements allowed by host.
> >                     0 implies scatter gather not supported by host
> > -    max_sectors  - maximum number of sectors (usually 512 bytes) allowed
> > -                   in a single SCSI command. The default value of 0 leads
> > -                   to a setting of SCSI_DEFAULT_MAX_SECTORS (defined in
> > -                   scsi_host.h) which is currently set to 1024. So for a
> > -                   disk the maximum transfer size is 512 KB when max_sectors
> > -                   is not defined. Note that this size may not be sufficient
> > -                   for disk firmware uploads.
> > +    max_sectors  - maximum number of 512 bytes sectors allowed in a single
> > +                   SCSI command. The default value of 0 leads to a setting
> > +                   of SCSI_DEFAULT_MAX_SECTORS (defined in scsi_host.h) which
> > +                   is currently set to 1024. So for a disk the maximum transfer
> > +                   size is 512 KB when max_sectors is not defined. Note that
> > +                   this size may not be sufficient for disk firmware uploads.
> >      cmd_per_lun  - maximum number of commands that can be queued on devices
> >                     controlled by the host. Overridden by LLD calls to
> >                     scsi_adjust_queue_depth().
> > diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> > index 5c58d59..84098e3 100644
> > --- a/include/scsi/scsi_host.h
> > +++ b/include/scsi/scsi_host.h
> > @@ -372,7 +372,10 @@ struct scsi_host_template {
> >         unsigned short sg_tablesize;
> >  
> >         /*
> > -        * If the host adapter has limitations beside segment count
> > +        * If the host adapter has limitations beside segment count.
> > +        * Note that this value MUST be calculated in 512 byte sectors,
> > +        * even if the attached struct scsi_device->sector_size is expected
> > +        * to use non 512 byte sectors.
> 
> How about:
> 	   * to use a sector size other than 512 bytes.
> 

Sounds good.

> and
> Acked-by: Randy Dunlap <randy.dunlap@oracle.com>
> 
> 
> >          */
> >         unsigned short max_sectors;
> 

Thanks!

--nab

Re: [Cbe-oss-dev] [PATCH] ps3rom: sector size should be 512 bytes

[Nicholas A. Bellinger]

On Thu, 2008-01-31 at 20:14 +0100, Geert Uytterhoeven wrote:
> On Thu, 31 Jan 2008, Nicholas A. Bellinger wrote:
> > On Thu, 2008-01-31 at 11:53 -0600, James Bottomley wrote:
> > > On Thu, 2008-01-31 at 09:28 -0800, Nicholas A. Bellinger wrote:
> > > > The problem case is a SCSI Target Mode engine that receives a 2048 Byte
> > > > single sector ATAPI READ_10 request from the storage fabric, and uses
> > > > scsi_execute_async() (the only option >= 2.6.18) to issue said request
> > > > to the underlying struct scsi_device.  Because the underlying bio code
> > > > assumes 512 byte only sectors, the check in __bio_add_page() incorrectly
> > > > determines that max_sectors (max_sectors has to be low, as with 32 from
> > > > ps3rom.c) has been exceeded, and fails the request back up the stack.
> > > 
> > > OK, so this is a totally separate issue from the one you actually posted
> > > it as a patch to fix?
> > > 
> > > the queue max_sectors parameter is also counted in the block internal of
> > > 512 byte sectors.  If you set it to 32 that means you were only
> > > expecting 16k of transfers per command maximum.  If that's not right,
> > > then set the limit correctly.
> > > 
> > > In short, and to repeat: almost every internal size counter to block is
> > > in units of 512 byte sectors ... that includes capacity, maximum etc ...
> > > 
> > 
> > Ok, after reading your followup with Geert I see that this looks like a
> > bug in ps3rom.c assuming 2048 byte sectors to calculate .max_sectors
> > (which was originally set to 32 as I mentioned).  Using the setting
> > BOUNCE_SIZE << 9 where BOUNCE_SIZE is the request size in bytes looks
> > like this will solve the issue.  My misunderstanding was
> > that .max_sectors was allowed to be calcuated in non 512 byte sectors,
> > so please disregard my patch.
> > 
> > Geert, .max_sectors for ps3rom.c using 512 byte sectors ends up being
> > 128, yes.?
> 
> Yes.
> 
> With kind regards,
> 
> Geert Uytterhoeven
> Software Architect
> 

Great!  I will update my ps3 to the latest 2.6.24 from ps3-linux.git and
get some new Linux/iSCSI.org Target builds made this weekend once
ps3rom-use-128-max-sector.patch is committed.

Also, I will be updating my userspace to FC8 PPC, and would like to get
a target build for the current stable 2.6.23 PPC64 from the addon CDs.
I also would not mind providing a ps3rom.ko with said patch for
interested parties using the default FC-8 Ps3-Linux kernel who are not
adventurous enough to build their own kernel until the Addon CDs are
reved again.

--nab