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

["Nicholas A. Bellinger" <nab@linux-iscsi.org>]

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