pktcdvd: don't scribble over the bvec array

Hi Peter, hi Jens,

I've been looking over the multi page bio vec work again recently, and
one of the stumbling blocks is raw biovec access in the pktcdvd.

The first issue is that it directly sets up the page and offset pointers
in the biovec just before calling bio_add_page.  As bio_add_page already
does the setup it's trivial to just switch it to stack variables for the
arguments.

The second issue is the copy code in pkt_make_local_copy, which
effectively is an opencoded version of bio_copy_data except that it
skips pages that already are the same in the ѕource and destination.
But we look at the only calleer we just set up the bio using bio_add_page
to point exactly to the page array that pkt_make_local_copy compares,
so the pages will always be the same and we can just remove this function.

Note that all this is done based on code inspection, I don't have any
packet writing hardware myself.

Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Jens Axboe <axboe@fb.com>

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 7cf795e0fc8d638100cb55b73d2ad9757b8347d4..95c98de929714c123217cf6fc1ba3c5ca998dafe 100644 (file)
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -944,39 +944,6 @@ static int pkt_set_segment_merging(struct pktcdvd_device *pd, struct request_que
        }
 }
 
-/*
- * Copy all data for this packet to pkt->pages[], so that
- * a) The number of required segments for the write bio is minimized, which
- *    is necessary for some scsi controllers.
- * b) The data can be used as cache to avoid read requests if we receive a
- *    new write request for the same zone.
- */
-static void pkt_make_local_copy(struct packet_data *pkt, struct bio_vec *bvec)
-{
-       int f, p, offs;
-
-       /* Copy all data to pkt->pages[] */
-       p = 0;
-       offs = 0;
-       for (f = 0; f < pkt->frames; f++) {
-               if (bvec[f].bv_page != pkt->pages[p]) {
-                       void *vfrom = kmap_atomic(bvec[f].bv_page) + bvec[f].bv_offset;
-                       void *vto = page_address(pkt->pages[p]) + offs;
-                       memcpy(vto, vfrom, CD_FRAMESIZE);
-                       kunmap_atomic(vfrom);
-                       bvec[f].bv_page = pkt->pages[p];
-                       bvec[f].bv_offset = offs;
-               } else {
-                       BUG_ON(bvec[f].bv_offset != offs);
-               }
-               offs += CD_FRAMESIZE;
-               if (offs >= PAGE_SIZE) {
-                       offs = 0;
-                       p++;
-               }
-       }
-}
-
 static void pkt_end_io_read(struct bio *bio)
 {
        struct packet_data *pkt = bio->bi_private;
@@ -1298,7 +1265,6 @@ try_next_bio:
 static void pkt_start_write(struct pktcdvd_device *pd, struct packet_data *pkt)
 {
        int f;
-       struct bio_vec *bvec = pkt->w_bio->bi_io_vec;
 
        bio_reset(pkt->w_bio);
        pkt->w_bio->bi_iter.bi_sector = pkt->sector;
@@ -1308,9 +1274,10 @@ static void pkt_start_write(struct pktcdvd_device *pd, struct packet_data *pkt)
 
        /* XXX: locking? */
        for (f = 0; f < pkt->frames; f++) {
-               bvec[f].bv_page = pkt->pages[(f * CD_FRAMESIZE) / PAGE_SIZE];
-               bvec[f].bv_offset = (f * CD_FRAMESIZE) % PAGE_SIZE;
-               if (!bio_add_page(pkt->w_bio, bvec[f].bv_page, CD_FRAMESIZE, bvec[f].bv_offset))
+               struct page *page = pkt->pages[(f * CD_FRAMESIZE) / PAGE_SIZE];
+               unsigned offset = (f * CD_FRAMESIZE) % PAGE_SIZE;
+
+               if (!bio_add_page(pkt->w_bio, page, CD_FRAMESIZE, offset))
                        BUG();
        }
        pkt_dbg(2, pd, "vcnt=%d\n", pkt->w_bio->bi_vcnt);
@@ -1327,12 +1294,10 @@ static void pkt_start_write(struct pktcdvd_device *pd, struct packet_data *pkt)
        pkt_dbg(2, pd, "Writing %d frames for zone %llx\n",
                pkt->write_size, (unsigned long long)pkt->sector);
 
-       if (test_bit(PACKET_MERGE_SEGS, &pd->flags) || (pkt->write_size < pkt->frames)) {
-               pkt_make_local_copy(pkt, bvec);
+       if (test_bit(PACKET_MERGE_SEGS, &pd->flags) || (pkt->write_size < pkt->frames))
                pkt->cache_valid = 1;
-       } else {
+       else
                pkt->cache_valid = 0;
-       }
 
        /* Start the write request */
        atomic_set(&pkt->io_wait, 1);