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@xxxxxxxxxxxxxxx>
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@xxxxxxxxxxx
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
