[PATCH 000 of 6] A few block-layer tidy-up patches.

[PATCH 000 of 6] A few block-layer tidy-up patches.

[NeilBrown]

Following are 5 patches which - I think - clean up various bits and pieces
in the block layer.

The only part that might be seen as a function change rather than
simply rearranging code is in ps3disk where bvec_kmap_irq is used
instead of bio_kmap_atomic (so interrupts are disabled).

The only other user of bvec_kmap_irq is ide-floppy.c.  If that does
need to disable interrupts, and ps3disk doesn't, make the disabling of
interrupts should be separated from the kmapping??

Thanks,
NeilBrown


 [PATCH 001 of 6] Merge blk_recount_segments into blk_recalc_rq_segments
 [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio
 [PATCH 003 of 6] Fix various abuse of bio fields in umem.c
 [PATCH 004 of 6] New function blk_req_append_bio
 [PATCH 005 of 6] Stop exporting blk_rq_bio_prep
 [PATCH 006 of 6] Share code between init_request_from_bio and blk_rq_bio_prep

[PATCH 001 of 6] Merge blk_recount_segments into blk_recalc_rq_segments

[NeilBrown]

blk_recalc_rq_segments calls blk_recount_segments on each bio,
then does some extra calculations to handle segments that overlap
two bios.

If we merge the code from blk_recount_segments into
blk_recalc_rq_segments, we can process the whole request one bio_vec
at a time, and not need the messy cross-bio calculations.

Then blk_recount_segments can be implemented by calling
blk_recalc_rq_segments, passing it a simple on-stack request which
stores just the bio.

Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./block/ll_rw_blk.c |  102 ++++++++++++++++++++++------------------------------
 1 file changed, 44 insertions(+), 58 deletions(-)

diff .prev/block/ll_rw_blk.c ./block/ll_rw_blk.c
--- .prev/block/ll_rw_blk.c	2007-08-16 17:37:43.000000000 +1000
+++ ./block/ll_rw_blk.c	2007-08-16 17:43:27.000000000 +1000
@@ -42,6 +42,7 @@ static void drive_stat_acct(struct reque
 static void init_request_from_bio(struct request *req, struct bio *bio);
 static int __make_request(struct request_queue *q, struct bio *bio);
 static struct io_context *current_io_context(gfp_t gfp_flags, int node);
+static void blk_recalc_rq_segments(struct request *rq);
 
 /*
  * For the allocated request tables
@@ -1209,16 +1210,42 @@ EXPORT_SYMBOL(blk_dump_rq_flags);
 
 void blk_recount_segments(struct request_queue *q, struct bio *bio)
 {
+	struct request rq;
+	struct bio *nxt = bio->bi_next;
+	rq.q = q;
+	rq.bio = rq.biotail = bio;
+	bio->bi_next = NULL;
+	blk_recalc_rq_segments(&rq);
+	bio->bi_next = nxt;
+	bio->bi_phys_segments = rq.nr_phys_segments;
+	bio->bi_hw_segments = rq.nr_hw_segments;
+	bio->bi_flags |= (1 << BIO_SEG_VALID);
+}
+EXPORT_SYMBOL(blk_recount_segments);
+
+static void blk_recalc_rq_segments(struct request *rq)
+{
+	int nr_phys_segs;
+	int nr_hw_segs;
+	unsigned int phys_size;
+	unsigned int hw_size;
 	struct bio_vec *bv, *bvprv = NULL;
-	int i, nr_phys_segs, nr_hw_segs, seg_size, hw_seg_size, cluster;
+	int seg_size;
+	int hw_seg_size;
+	int cluster;
+	struct bio *bio;
+	int i;
 	int high, highprv = 1;
+	struct request_queue *q = rq->q;
 
-	if (unlikely(!bio->bi_io_vec))
+	if (!rq->bio)
 		return;
 
 	cluster = q->queue_flags & (1 << QUEUE_FLAG_CLUSTER);
-	hw_seg_size = seg_size = nr_phys_segs = nr_hw_segs = 0;
-	bio_for_each_segment(bv, bio, i) {
+	hw_seg_size = seg_size = 0;
+	phys_size = hw_size = nr_phys_segs = nr_hw_segs = 0;
+	rq_for_each_bio(bio, rq)
+	    bio_for_each_segment(bv, bio, i) {
 		/*
 		 * the trick here is making sure that a high page is never
 		 * considered part of another segment, since that might
@@ -1244,12 +1271,13 @@ void blk_recount_segments(struct request
 		}
 new_segment:
 		if (BIOVEC_VIRT_MERGEABLE(bvprv, bv) &&
-		    !BIOVEC_VIRT_OVERSIZE(hw_seg_size + bv->bv_len)) {
+		    !BIOVEC_VIRT_OVERSIZE(hw_seg_size + bv->bv_len))
 			hw_seg_size += bv->bv_len;
-		} else {
+		else {
 new_hw_segment:
-			if (hw_seg_size > bio->bi_hw_front_size)
-				bio->bi_hw_front_size = hw_seg_size;
+			if (nr_hw_segs == 1 &&
+			    hw_seg_size > rq->bio->bi_hw_front_size)
+				rq->bio->bi_hw_front_size = hw_seg_size;
 			hw_seg_size = BIOVEC_VIRT_START_SIZE(bv) + bv->bv_len;
 			nr_hw_segs++;
 		}
@@ -1259,15 +1287,15 @@ new_hw_segment:
 		seg_size = bv->bv_len;
 		highprv = high;
 	}
-	if (hw_seg_size > bio->bi_hw_back_size)
-		bio->bi_hw_back_size = hw_seg_size;
-	if (nr_hw_segs == 1 && hw_seg_size > bio->bi_hw_front_size)
-		bio->bi_hw_front_size = hw_seg_size;
-	bio->bi_phys_segments = nr_phys_segs;
-	bio->bi_hw_segments = nr_hw_segs;
-	bio->bi_flags |= (1 << BIO_SEG_VALID);
+
+	if (nr_hw_segs == 1 &&
+	    hw_seg_size > rq->bio->bi_hw_front_size)
+		rq->bio->bi_hw_front_size = hw_seg_size;
+	if (hw_seg_size > rq->biotail->bi_hw_back_size)
+		rq->biotail->bi_hw_back_size = hw_seg_size;
+	rq->nr_phys_segments = nr_phys_segs;
+	rq->nr_hw_segments = nr_hw_segs;
 }
-EXPORT_SYMBOL(blk_recount_segments);
 
 static int blk_phys_contig_segment(struct request_queue *q, struct bio *bio,
 				   struct bio *nxt)
@@ -3316,48 +3344,6 @@ void submit_bio(int rw, struct bio *bio)
 
 EXPORT_SYMBOL(submit_bio);
 
-static void blk_recalc_rq_segments(struct request *rq)
-{
-	struct bio *bio, *prevbio = NULL;
-	int nr_phys_segs, nr_hw_segs;
-	unsigned int phys_size, hw_size;
-	struct request_queue *q = rq->q;
-
-	if (!rq->bio)
-		return;
-
-	phys_size = hw_size = nr_phys_segs = nr_hw_segs = 0;
-	rq_for_each_bio(bio, rq) {
-		/* Force bio hw/phys segs to be recalculated. */
-		bio->bi_flags &= ~(1 << BIO_SEG_VALID);
-
-		nr_phys_segs += bio_phys_segments(q, bio);
-		nr_hw_segs += bio_hw_segments(q, bio);
-		if (prevbio) {
-			int pseg = phys_size + prevbio->bi_size + bio->bi_size;
-			int hseg = hw_size + prevbio->bi_size + bio->bi_size;
-
-			if (blk_phys_contig_segment(q, prevbio, bio) &&
-			    pseg <= q->max_segment_size) {
-				nr_phys_segs--;
-				phys_size += prevbio->bi_size + bio->bi_size;
-			} else
-				phys_size = 0;
-
-			if (blk_hw_contig_segment(q, prevbio, bio) &&
-			    hseg <= q->max_segment_size) {
-				nr_hw_segs--;
-				hw_size += prevbio->bi_size + bio->bi_size;
-			} else
-				hw_size = 0;
-		}
-		prevbio = bio;
-	}
-
-	rq->nr_phys_segments = nr_phys_segs;
-	rq->nr_hw_segments = nr_hw_segs;
-}
-
 static void blk_recalc_rq_sectors(struct request *rq, int nsect)
 {
 	if (blk_fs_request(rq)) {

[PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

[NeilBrown]

Every usage of rq_for_each_bio wraps a usage of
bio_for_each_segment, so these can be combined into
rq_for_each_segment.

We define "struct req_iterator" to hold the 'bio' and 'index' that
are needed for the double iteration.

Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./Documentation/block/biodoc.txt |   22 +++++++++++-----------
 ./block/ll_rw_blk.c              |   19 ++++++-------------
 ./drivers/block/floppy.c         |   14 ++++----------
 ./drivers/block/lguest_blk.c     |   10 ++++------
 ./drivers/block/nbd.c            |   22 +++++++++-------------
 ./drivers/block/ps3disk.c        |   29 +++++++++++++++--------------
 ./drivers/block/xen-blkfront.c   |    7 ++-----
 ./drivers/ide/ide-floppy.c       |   16 ++++++----------
 ./drivers/s390/block/dasd_diag.c |   11 +++--------
 ./drivers/s390/block/dasd_eckd.c |   15 ++++++---------
 ./drivers/s390/block/dasd_fba.c  |   15 ++++++---------
 ./drivers/s390/char/tape_34xx.c  |   15 +++++----------
 ./drivers/s390/char/tape_3590.c  |   16 ++++++----------
 ./include/linux/blkdev.h         |   15 ++++++++++++++-
 14 files changed, 97 insertions(+), 129 deletions(-)

diff .prev/block/ll_rw_blk.c ./block/ll_rw_blk.c
--- .prev/block/ll_rw_blk.c	2007-08-16 17:43:27.000000000 +1000
+++ ./block/ll_rw_blk.c	2007-08-16 20:53:44.000000000 +1000
@@ -1233,8 +1233,7 @@ static void blk_recalc_rq_segments(struc
 	int seg_size;
 	int hw_seg_size;
 	int cluster;
-	struct bio *bio;
-	int i;
+	struct req_iterator iter;
 	int high, highprv = 1;
 	struct request_queue *q = rq->q;
 
@@ -1244,8 +1243,7 @@ static void blk_recalc_rq_segments(struc
 	cluster = q->queue_flags & (1 << QUEUE_FLAG_CLUSTER);
 	hw_seg_size = seg_size = 0;
 	phys_size = hw_size = nr_phys_segs = nr_hw_segs = 0;
-	rq_for_each_bio(bio, rq)
-	    bio_for_each_segment(bv, bio, i) {
+	rq_for_each_segment(bv, rq, iter) {
 		/*
 		 * the trick here is making sure that a high page is never
 		 * considered part of another segment, since that might
@@ -1342,8 +1340,8 @@ int blk_rq_map_sg(struct request_queue *
 		  struct scatterlist *sg)
 {
 	struct bio_vec *bvec, *bvprv;
-	struct bio *bio;
-	int nsegs, i, cluster;
+	struct req_iterator iter;
+	int nsegs, cluster;
 
 	nsegs = 0;
 	cluster = q->queue_flags & (1 << QUEUE_FLAG_CLUSTER);
@@ -1352,11 +1350,7 @@ int blk_rq_map_sg(struct request_queue *
 	 * for each bio in rq
 	 */
 	bvprv = NULL;
-	rq_for_each_bio(bio, rq) {
-		/*
-		 * for each segment in bio
-		 */
-		bio_for_each_segment(bvec, bio, i) {
+	rq_for_each_segment(bvec, rq, iter) {
 			int nbytes = bvec->bv_len;
 
 			if (bvprv && cluster) {
@@ -1379,8 +1373,7 @@ new_segment:
 				nsegs++;
 			}
 			bvprv = bvec;
-		} /* segments in bio */
-	} /* bios in rq */
+	} /* segments in rq */
 
 	return nsegs;
 }

diff .prev/Documentation/block/biodoc.txt ./Documentation/block/biodoc.txt
--- .prev/Documentation/block/biodoc.txt	2007-08-16 20:51:10.000000000 +1000
+++ ./Documentation/block/biodoc.txt	2007-08-16 20:52:51.000000000 +1000
@@ -477,9 +477,9 @@ With this multipage bio design:
   the same bi_io_vec array, but with the index and size accordingly modified)
 - A linked list of bios is used as before for unrelated merges (*) - this
   avoids reallocs and makes independent completions easier to handle.
-- Code that traverses the req list needs to make a distinction between
-  segments of a request (bio_for_each_segment) and the distinct completion
-  units/bios (rq_for_each_bio).
+- Code that traverses the req list can find all the segments of a bio
+  by using rq_for_each_segment.  This handles the fact that a request
+  has multiple bios, each of which can have multiple segments.
 - Drivers which can't process a large bio in one shot can use the bi_idx
   field to keep track of the next bio_vec entry to process.
   (e.g a 1MB bio_vec needs to be handled in max 128kB chunks for IDE)
@@ -664,14 +664,14 @@ in lvm or md.
 
 3.2.1 Traversing segments and completion units in a request
 
-The macros bio_for_each_segment() and rq_for_each_bio() should be used for
-traversing the bios in the request list (drivers should avoid directly
-trying to do it themselves). Using these helpers should also make it easier
-to cope with block changes in the future.
-
-	rq_for_each_bio(bio, rq)
-		bio_for_each_segment(bio_vec, bio, i)
-			/* bio_vec is now current segment */
+The macro rq_for_each_segment() should be used for traversing the bios
+in the request list (drivers should avoid directly trying to do it
+themselves). Using these helpers should also make it easier to cope
+with block changes in the future.
+
+	struct req_iterator iter;
+	rq_for_each_segment(bio_vec, rq, iter)
+		/* bio_vec is now current segment */
 
 I/O completion callbacks are per-bio rather than per-segment, so drivers
 that traverse bio chains on completion need to keep that in mind. Drivers

diff .prev/drivers/block/floppy.c ./drivers/block/floppy.c
--- .prev/drivers/block/floppy.c	2007-08-16 17:18:38.000000000 +1000
+++ ./drivers/block/floppy.c	2007-08-16 17:55:58.000000000 +1000
@@ -2450,23 +2450,20 @@ static void rw_interrupt(void)
 /* Compute maximal contiguous buffer size. */
 static int buffer_chain_size(void)
 {
-	struct bio *bio;
 	struct bio_vec *bv;
 	int size;
-	int i;
+	struct req_iterator iter;
 	char *base;
 
 	base = bio_data(current_req->bio);
 	size = 0;
 
-	rq_for_each_bio(bio, current_req) {
-		bio_for_each_segment(bv, bio, i) {
+	rq_for_each_segment(bv, current_req, iter) {
 			if (page_address(bv->bv_page) + bv->bv_offset !=
 			    base + size)
 				break;
 
 			size += bv->bv_len;
-		}
 	}
 
 	return size >> 9;
@@ -2493,11 +2490,10 @@ static void copy_buffer(int ssize, int m
 {
 	int remaining;		/* number of transferred 512-byte sectors */
 	struct bio_vec *bv;
-	struct bio *bio;
 	char *buffer;
 	char *dma_buffer;
 	int size;
-	int i;
+	struct req_iterator iter;
 
 	max_sector = transfer_size(ssize,
 				   min(max_sector, max_sector_2),
@@ -2530,8 +2526,7 @@ static void copy_buffer(int ssize, int m
 
 	size = current_req->current_nr_sectors << 9;
 
-	rq_for_each_bio(bio, current_req) {
-		bio_for_each_segment(bv, bio, i) {
+	rq_for_each_segment(bv, current_req, iter) {
 			if (!remaining)
 				break;
 
@@ -2566,7 +2561,6 @@ static void copy_buffer(int ssize, int m
 
 			remaining -= size;
 			dma_buffer += size;
-		}
 	}
 #ifdef FLOPPY_SANITY_CHECK
 	if (remaining) {

diff .prev/drivers/block/lguest_blk.c ./drivers/block/lguest_blk.c
--- .prev/drivers/block/lguest_blk.c	2007-08-16 17:18:37.000000000 +1000
+++ ./drivers/block/lguest_blk.c	2007-08-16 17:58:14.000000000 +1000
@@ -142,12 +142,11 @@ static irqreturn_t lgb_irq(int irq, void
  * return the total length. */
 static unsigned int req_to_dma(struct request *req, struct lguest_dma *dma)
 {
-	unsigned int i = 0, idx, len = 0;
-	struct bio *bio;
+	unsigned int i = 0, len = 0;
+	struct req_iterator iter;
+	struct bio_vec *bvec;
 
-	rq_for_each_bio(bio, req) {
-		struct bio_vec *bvec;
-		bio_for_each_segment(bvec, bio, idx) {
+	rq_for_each_segment(bvec, req, iter) {
 			/* We told the block layer not to give us too many. */
 			BUG_ON(i == LGUEST_MAX_DMA_SECTIONS);
 			/* If we had a zero-length segment, it would look like
@@ -160,7 +159,6 @@ static unsigned int req_to_dma(struct re
 			dma->len[i] = bvec->bv_len;
 			len += bvec->bv_len;
 			i++;
-		}
 	}
 	/* If the array isn't full, we mark the end with a 0 length */
 	if (i < LGUEST_MAX_DMA_SECTIONS)

diff .prev/drivers/block/nbd.c ./drivers/block/nbd.c
--- .prev/drivers/block/nbd.c	2007-08-16 17:18:37.000000000 +1000
+++ ./drivers/block/nbd.c	2007-08-16 20:31:41.000000000 +1000
@@ -180,7 +180,7 @@ static inline int sock_send_bvec(struct 
 
 static int nbd_send_req(struct nbd_device *lo, struct request *req)
 {
-	int result, i, flags;
+	int result, flags;
 	struct nbd_request request;
 	unsigned long size = req->nr_sectors << 9;
 	struct socket *sock = lo->sock;
@@ -205,16 +205,15 @@ static int nbd_send_req(struct nbd_devic
 	}
 
 	if (nbd_cmd(req) == NBD_CMD_WRITE) {
-		struct bio *bio;
+		struct req_iterator iter;
+		struct bio_vec *bvec;
 		/*
 		 * we are really probing at internals to determine
 		 * whether to set MSG_MORE or not...
 		 */
-		rq_for_each_bio(bio, req) {
-			struct bio_vec *bvec;
-			bio_for_each_segment(bvec, bio, i) {
+		rq_for_each_segment(bvec, req, iter) {
 				flags = 0;
-				if ((i < (bio->bi_vcnt - 1)) || bio->bi_next)
+				if (!rq_iter_last(req, iter))
 					flags = MSG_MORE;
 				dprintk(DBG_TX, "%s: request %p: sending %d bytes data\n",
 						lo->disk->disk_name, req,
@@ -226,7 +225,6 @@ static int nbd_send_req(struct nbd_devic
 							result);
 					goto error_out;
 				}
-			}
 		}
 	}
 	return 0;
@@ -317,11 +315,10 @@ static struct request *nbd_read_stat(str
 	dprintk(DBG_RX, "%s: request %p: got reply\n",
 			lo->disk->disk_name, req);
 	if (nbd_cmd(req) == NBD_CMD_READ) {
-		int i;
-		struct bio *bio;
-		rq_for_each_bio(bio, req) {
-			struct bio_vec *bvec;
-			bio_for_each_segment(bvec, bio, i) {
+		struct req_iterator iter;
+		struct bio_vec *bvec;
+
+		rq_for_each_segment(bvec, req, iter) {
 				result = sock_recv_bvec(sock, bvec);
 				if (result <= 0) {
 					printk(KERN_ERR "%s: Receive data failed (result %d)\n",
@@ -332,7 +329,6 @@ static struct request *nbd_read_stat(str
 				}
 				dprintk(DBG_RX, "%s: request %p: got %d bytes data\n",
 					lo->disk->disk_name, req, bvec->bv_len);
-			}
 		}
 	}
 	return req;

diff .prev/drivers/block/ps3disk.c ./drivers/block/ps3disk.c
--- .prev/drivers/block/ps3disk.c	2007-08-16 20:37:26.000000000 +1000
+++ ./drivers/block/ps3disk.c	2007-08-16 20:50:07.000000000 +1000
@@ -91,30 +91,30 @@ static void ps3disk_scatter_gather(struc
 				   struct request *req, int gather)
 {
 	unsigned int offset = 0;
-	struct bio *bio;
-	sector_t sector;
+	struct req_iterator iter;
 	struct bio_vec *bvec;
-	unsigned int i = 0, j;
+	unsigned int i = 0;
 	size_t size;
 	void *buf;
 
-	rq_for_each_bio(bio, req) {
-		sector = bio->bi_sector;
+	rq_for_each_segment(bvec, req, iter) {
+		unsigned long flags;
 		dev_dbg(&dev->sbd.core,
 			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
-			__func__, __LINE__, i, bio_segments(bio),
-			bio_sectors(bio), sector);
-		bio_for_each_segment(bvec, bio, j) {
+			__func__, __LINE__, i, bio_segments(iter.bio),
+			bio_sectors(iter.bio),
+			(unsigned long)iter.bio->bi_sector);
+
 			size = bvec->bv_len;
-			buf = __bio_kmap_atomic(bio, j, KM_IRQ0);
+			buf = bvec_kmap_irq(bvec, flags);
 			if (gather)
 				memcpy(dev->bounce_buf+offset, buf, size);
 			else
 				memcpy(buf, dev->bounce_buf+offset, size);
 			offset += size;
 			flush_kernel_dcache_page(bio_iovec_idx(bio, j)->bv_page);
-			__bio_kunmap_atomic(bio, KM_IRQ0);
-		}
+			bio_kunmap_bvec(bvec, flags);
+
 		i++;
 	}
 }
@@ -130,12 +130,13 @@ static int ps3disk_submit_request_sg(str
 
 #ifdef DEBUG
 	unsigned int n = 0;
-	struct bio *bio;
+	struct bio_vec *bv;
+	struct req_iterator iter;
 
-	rq_for_each_bio(bio, req)
+	rq_for_each_segment(bv, req, iter)
 		n++;
 	dev_dbg(&dev->sbd.core,
-		"%s:%u: %s req has %u bios for %lu sectors %lu hard sectors\n",
+		"%s:%u: %s req has %u bvecs for %lu sectors %lu hard sectors\n",
 		__func__, __LINE__, op, n, req->nr_sectors,
 		req->hard_nr_sectors);
 #endif

diff .prev/drivers/block/xen-blkfront.c ./drivers/block/xen-blkfront.c
--- .prev/drivers/block/xen-blkfront.c	2007-08-16 17:18:37.000000000 +1000
+++ ./drivers/block/xen-blkfront.c	2007-08-16 18:03:12.000000000 +1000
@@ -150,9 +150,8 @@ static int blkif_queue_request(struct re
 	struct blkfront_info *info = req->rq_disk->private_data;
 	unsigned long buffer_mfn;
 	struct blkif_request *ring_req;
-	struct bio *bio;
+	struct req_iterator iter;
 	struct bio_vec *bvec;
-	int idx;
 	unsigned long id;
 	unsigned int fsect, lsect;
 	int ref;
@@ -186,8 +185,7 @@ static int blkif_queue_request(struct re
 		ring_req->operation = BLKIF_OP_WRITE_BARRIER;
 
 	ring_req->nr_segments = 0;
-	rq_for_each_bio (bio, req) {
-		bio_for_each_segment (bvec, bio, idx) {
+	rq_for_each_segment(bvec, req, iter) {
 			BUG_ON(ring_req->nr_segments
 			       == BLKIF_MAX_SEGMENTS_PER_REQUEST);
 			buffer_mfn = pfn_to_mfn(page_to_pfn(bvec->bv_page));
@@ -213,7 +211,6 @@ static int blkif_queue_request(struct re
 					.last_sect  = lsect };
 
 			ring_req->nr_segments++;
-		}
 	}
 
 	info->ring.req_prod_pvt++;

diff .prev/drivers/ide/ide-floppy.c ./drivers/ide/ide-floppy.c
--- .prev/drivers/ide/ide-floppy.c	2007-08-16 17:18:37.000000000 +1000
+++ ./drivers/ide/ide-floppy.c	2007-08-16 18:04:55.000000000 +1000
@@ -606,13 +606,12 @@ static void idefloppy_input_buffers (ide
 {
 	struct request *rq = pc->rq;
 	struct bio_vec *bvec;
-	struct bio *bio;
+	struct req_iterator iter;
 	unsigned long flags;
 	char *data;
-	int count, i, done = 0;
+	int count, done = 0;
 
-	rq_for_each_bio(bio, rq) {
-		bio_for_each_segment(bvec, bio, i) {
+	rq_for_each_segment(bvec, rq, iter) {
 			if (!bcount)
 				break;
 
@@ -625,7 +624,6 @@ static void idefloppy_input_buffers (ide
 			bcount -= count;
 			pc->b_count += count;
 			done += count;
-		}
 	}
 
 	idefloppy_do_end_request(drive, 1, done >> 9);
@@ -639,14 +637,13 @@ static void idefloppy_input_buffers (ide
 static void idefloppy_output_buffers (ide_drive_t *drive, idefloppy_pc_t *pc, unsigned int bcount)
 {
 	struct request *rq = pc->rq;
-	struct bio *bio;
+	struct req_iterator iter;
 	struct bio_vec *bvec;
 	unsigned long flags;
-	int count, i, done = 0;
+	int count, done = 0;
 	char *data;
 
-	rq_for_each_bio(bio, rq) {
-		bio_for_each_segment(bvec, bio, i) {
+	rq_for_each_segment(bvec, rq, iter) {
 			if (!bcount)
 				break;
 
@@ -659,7 +656,6 @@ static void idefloppy_output_buffers (id
 			bcount -= count;
 			pc->b_count += count;
 			done += count;
-		}
 	}
 
 	idefloppy_do_end_request(drive, 1, done >> 9);

diff .prev/drivers/s390/block/dasd_diag.c ./drivers/s390/block/dasd_diag.c
--- .prev/drivers/s390/block/dasd_diag.c	2007-08-16 17:18:37.000000000 +1000
+++ ./drivers/s390/block/dasd_diag.c	2007-08-16 18:06:21.000000000 +1000
@@ -471,14 +471,13 @@ dasd_diag_build_cp(struct dasd_device * 
 	struct dasd_ccw_req *cqr;
 	struct dasd_diag_req *dreq;
 	struct dasd_diag_bio *dbio;
-	struct bio *bio;
+	struct req_iterator iter;
 	struct bio_vec *bv;
 	char *dst;
 	unsigned int count, datasize;
 	sector_t recid, first_rec, last_rec;
 	unsigned int blksize, off;
 	unsigned char rw_cmd;
-	int i;
 
 	if (rq_data_dir(req) == READ)
 		rw_cmd = MDSK_READ_REQ;
@@ -492,13 +491,11 @@ dasd_diag_build_cp(struct dasd_device * 
 	last_rec = (req->sector + req->nr_sectors - 1) >> device->s2b_shift;
 	/* Check struct bio and count the number of blocks for the request. */
 	count = 0;
-	rq_for_each_bio(bio, req) {
-		bio_for_each_segment(bv, bio, i) {
+	rq_for_each_segment(bv, req, iter) {
 			if (bv->bv_len & (blksize - 1))
 				/* Fba can only do full blocks. */
 				return ERR_PTR(-EINVAL);
 			count += bv->bv_len >> (device->s2b_shift + 9);
-		}
 	}
 	/* Paranoia. */
 	if (count != last_rec - first_rec + 1)
@@ -515,8 +512,7 @@ dasd_diag_build_cp(struct dasd_device * 
 	dreq->block_count = count;
 	dbio = dreq->bio;
 	recid = first_rec;
-	rq_for_each_bio(bio, req) {
-		bio_for_each_segment(bv, bio, i) {
+	rq_for_each_segment(bv, req, iter) {
 			dst = page_address(bv->bv_page) + bv->bv_offset;
 			for (off = 0; off < bv->bv_len; off += blksize) {
 				memset(dbio, 0, sizeof (struct dasd_diag_bio));
@@ -527,7 +523,6 @@ dasd_diag_build_cp(struct dasd_device * 
 				dst += blksize;
 				recid++;
 			}
-		}
 	}
 	cqr->retries = DIAG_MAX_RETRIES;
 	cqr->buildclk = get_clock();

diff .prev/drivers/s390/block/dasd_eckd.c ./drivers/s390/block/dasd_eckd.c
--- .prev/drivers/s390/block/dasd_eckd.c	2007-08-16 17:18:37.000000000 +1000
+++ ./drivers/s390/block/dasd_eckd.c	2007-08-16 18:07:59.000000000 +1000
@@ -1176,7 +1176,7 @@ dasd_eckd_build_cp(struct dasd_device * 
 	struct LO_eckd_data *LO_data;
 	struct dasd_ccw_req *cqr;
 	struct ccw1 *ccw;
-	struct bio *bio;
+	struct req_iterator iter
 	struct bio_vec *bv;
 	char *dst;
 	unsigned int blksize, blk_per_trk, off;
@@ -1185,7 +1185,6 @@ dasd_eckd_build_cp(struct dasd_device * 
 	sector_t first_trk, last_trk;
 	unsigned int first_offs, last_offs;
 	unsigned char cmd, rcmd;
-	int i;
 
 	private = (struct dasd_eckd_private *) device->private;
 	if (rq_data_dir(req) == READ)
@@ -1206,8 +1205,7 @@ dasd_eckd_build_cp(struct dasd_device * 
 	/* Check struct bio and count the number of blocks for the request. */
 	count = 0;
 	cidaw = 0;
-	rq_for_each_bio(bio, req) {
-		bio_for_each_segment(bv, bio, i) {
+	rq_for_each_segment(bv, req, iter) {
 			if (bv->bv_len & (blksize - 1))
 				/* Eckd can only do full blocks. */
 				return ERR_PTR(-EINVAL);
@@ -1217,7 +1215,6 @@ dasd_eckd_build_cp(struct dasd_device * 
 					    bv->bv_len))
 				cidaw += bv->bv_len >> (device->s2b_shift + 9);
 #endif
-		}
 	}
 	/* Paranoia. */
 	if (count != last_rec - first_rec + 1)
@@ -1257,7 +1254,7 @@ dasd_eckd_build_cp(struct dasd_device * 
 		locate_record(ccw++, LO_data++, first_trk, first_offs + 1,
 			      last_rec - recid + 1, cmd, device, blksize);
 	}
-	rq_for_each_bio(bio, req) bio_for_each_segment(bv, bio, i) {
+	rq_for_each_segment(bv, req, iter) {
 		dst = page_address(bv->bv_page) + bv->bv_offset;
 		if (dasd_page_cache) {
 			char *copy = kmem_cache_alloc(dasd_page_cache,
@@ -1328,12 +1325,12 @@ dasd_eckd_free_cp(struct dasd_ccw_req *c
 {
 	struct dasd_eckd_private *private;
 	struct ccw1 *ccw;
-	struct bio *bio;
+	struct req_iterator iter;
 	struct bio_vec *bv;
 	char *dst, *cda;
 	unsigned int blksize, blk_per_trk, off;
 	sector_t recid;
-	int i, status;
+	int status;
 
 	if (!dasd_page_cache)
 		goto out;
@@ -1346,7 +1343,7 @@ dasd_eckd_free_cp(struct dasd_ccw_req *c
 	ccw++;
 	if (private->uses_cdl == 0 || recid > 2*blk_per_trk)
 		ccw++;
-	rq_for_each_bio(bio, req) bio_for_each_segment(bv, bio, i) {
+	rq_for_each_segment(bv, req, iter) {
 		dst = page_address(bv->bv_page) + bv->bv_offset;
 		for (off = 0; off < bv->bv_len; off += blksize) {
 			/* Skip locate record. */

diff .prev/drivers/s390/block/dasd_fba.c ./drivers/s390/block/dasd_fba.c
--- .prev/drivers/s390/block/dasd_fba.c	2007-08-16 17:18:37.000000000 +1000
+++ ./drivers/s390/block/dasd_fba.c	2007-08-16 18:09:43.000000000 +1000
@@ -234,14 +234,13 @@ dasd_fba_build_cp(struct dasd_device * d
 	struct LO_fba_data *LO_data;
 	struct dasd_ccw_req *cqr;
 	struct ccw1 *ccw;
-	struct bio *bio;
+	struct req_iterator iter;
 	struct bio_vec *bv;
 	char *dst;
 	int count, cidaw, cplength, datasize;
 	sector_t recid, first_rec, last_rec;
 	unsigned int blksize, off;
 	unsigned char cmd;
-	int i;
 
 	private = (struct dasd_fba_private *) device->private;
 	if (rq_data_dir(req) == READ) {
@@ -257,8 +256,7 @@ dasd_fba_build_cp(struct dasd_device * d
 	/* Check struct bio and count the number of blocks for the request. */
 	count = 0;
 	cidaw = 0;
-	rq_for_each_bio(bio, req) {
-		bio_for_each_segment(bv, bio, i) {
+	rq_for_each_segment(bv, req, iter) {
 			if (bv->bv_len & (blksize - 1))
 				/* Fba can only do full blocks. */
 				return ERR_PTR(-EINVAL);
@@ -268,7 +266,6 @@ dasd_fba_build_cp(struct dasd_device * d
 					    bv->bv_len))
 				cidaw += bv->bv_len / blksize;
 #endif
-		}
 	}
 	/* Paranoia. */
 	if (count != last_rec - first_rec + 1)
@@ -304,7 +301,7 @@ dasd_fba_build_cp(struct dasd_device * d
 		locate_record(ccw++, LO_data++, rq_data_dir(req), 0, count);
 	}
 	recid = first_rec;
-	rq_for_each_bio(bio, req) bio_for_each_segment(bv, bio, i) {
+	rq_for_each_segment(bv, req, iter) {
 		dst = page_address(bv->bv_page) + bv->bv_offset;
 		if (dasd_page_cache) {
 			char *copy = kmem_cache_alloc(dasd_page_cache,
@@ -359,11 +356,11 @@ dasd_fba_free_cp(struct dasd_ccw_req *cq
 {
 	struct dasd_fba_private *private;
 	struct ccw1 *ccw;
-	struct bio *bio;
+	struct req_iterator iter;
 	struct bio_vec *bv;
 	char *dst, *cda;
 	unsigned int blksize, off;
-	int i, status;
+	int status;
 
 	if (!dasd_page_cache)
 		goto out;
@@ -374,7 +371,7 @@ dasd_fba_free_cp(struct dasd_ccw_req *cq
 	ccw++;
 	if (private->rdc_data.mode.bits.data_chain != 0)
 		ccw++;
-	rq_for_each_bio(bio, req) bio_for_each_segment(bv, bio, i) {
+	rq_for_each_segment(bv, req, iter) {
 		dst = page_address(bv->bv_page) + bv->bv_offset;
 		for (off = 0; off < bv->bv_len; off += blksize) {
 			/* Skip locate record. */

diff .prev/drivers/s390/char/tape_34xx.c ./drivers/s390/char/tape_34xx.c
--- .prev/drivers/s390/char/tape_34xx.c	2007-08-16 17:18:37.000000000 +1000
+++ ./drivers/s390/char/tape_34xx.c	2007-08-16 18:10:50.000000000 +1000
@@ -1134,21 +1134,18 @@ tape_34xx_bread(struct tape_device *devi
 {
 	struct tape_request *request;
 	struct ccw1 *ccw;
-	int count = 0, i;
+	int count = 0;
 	unsigned off;
 	char *dst;
 	struct bio_vec *bv;
-	struct bio *bio;
+	struct req_iterator iter;
 	struct tape_34xx_block_id *	start_block;
 
 	DBF_EVENT(6, "xBREDid:");
 
 	/* Count the number of blocks for the request. */
-	rq_for_each_bio(bio, req) {
-		bio_for_each_segment(bv, bio, i) {
-			count += bv->bv_len >> (TAPEBLOCK_HSEC_S2B + 9);
-		}
-	}
+	rq_for_each_segment(bv, req, iter)
+		count += bv->bv_len >> (TAPEBLOCK_HSEC_S2B + 9);
 
 	/* Allocate the ccw request. */
 	request = tape_alloc_request(3+count+1, 8);
@@ -1175,8 +1172,7 @@ tape_34xx_bread(struct tape_device *devi
 	ccw = tape_ccw_cc(ccw, NOP, 0, NULL);
 	ccw = tape_ccw_cc(ccw, NOP, 0, NULL);
 
-	rq_for_each_bio(bio, req) {
-		bio_for_each_segment(bv, bio, i) {
+	rq_for_each_segment(bv, req, iter) {
 			dst = kmap(bv->bv_page) + bv->bv_offset;
 			for (off = 0; off < bv->bv_len;
 			     off += TAPEBLOCK_HSEC_SIZE) {
@@ -1187,7 +1183,6 @@ tape_34xx_bread(struct tape_device *devi
 				ccw++;
 				dst += TAPEBLOCK_HSEC_SIZE;
 			}
-		}
 	}
 
 	ccw = tape_ccw_end(ccw, NOP, 0, NULL);

diff .prev/drivers/s390/char/tape_3590.c ./drivers/s390/char/tape_3590.c
--- .prev/drivers/s390/char/tape_3590.c	2007-08-16 17:18:37.000000000 +1000
+++ ./drivers/s390/char/tape_3590.c	2007-08-16 20:30:31.000000000 +1000
@@ -623,21 +623,19 @@ tape_3590_bread(struct tape_device *devi
 {
 	struct tape_request *request;
 	struct ccw1 *ccw;
-	int count = 0, start_block, i;
+	int count = 0, start_block;
 	unsigned off;
 	char *dst;
 	struct bio_vec *bv;
-	struct bio *bio;
+	struct req_iterator iter;
 
 	DBF_EVENT(6, "xBREDid:");
 	start_block = req->sector >> TAPEBLOCK_HSEC_S2B;
 	DBF_EVENT(6, "start_block = %i\n", start_block);
 
-	rq_for_each_bio(bio, req) {
-		bio_for_each_segment(bv, bio, i) {
-			count += bv->bv_len >> (TAPEBLOCK_HSEC_S2B + 9);
-		}
-	}
+	rq_for_each_segment(bv, req, iter)
+		count += bv->bv_len >> (TAPEBLOCK_HSEC_S2B + 9);
+
 	request = tape_alloc_request(2 + count + 1, 4);
 	if (IS_ERR(request))
 		return request;
@@ -653,8 +651,7 @@ tape_3590_bread(struct tape_device *devi
 	 */
 	ccw = tape_ccw_cc(ccw, NOP, 0, NULL);
 
-	rq_for_each_bio(bio, req) {
-		bio_for_each_segment(bv, bio, i) {
+	rq_for_each_segment(bv, req, iter) {
 			dst = page_address(bv->bv_page) + bv->bv_offset;
 			for (off = 0; off < bv->bv_len;
 			     off += TAPEBLOCK_HSEC_SIZE) {
@@ -667,7 +664,6 @@ tape_3590_bread(struct tape_device *devi
 			}
 			if (off > bv->bv_len)
 				BUG();
-		}
 	}
 	ccw = tape_ccw_end(ccw, NOP, 0, NULL);
 	DBF_EVENT(6, "xBREDccwg\n");

diff .prev/include/linux/blkdev.h ./include/linux/blkdev.h
--- .prev/include/linux/blkdev.h	2007-08-16 17:39:13.000000000 +1000
+++ ./include/linux/blkdev.h	2007-08-16 20:54:26.000000000 +1000
@@ -637,10 +637,23 @@ static inline void blk_queue_bounce(stru
 }
 #endif /* CONFIG_MMU */
 
-#define rq_for_each_bio(_bio, rq)	\
+struct req_iterator {
+	int i;
+	struct bio *bio;
+};
+
+/* This should not be used directly - use rq_for_each_segment */
+#define __rq_for_each_bio(_bio, rq)	\
 	if ((rq->bio))			\
 		for (_bio = (rq)->bio; _bio; _bio = _bio->bi_next)
 
+#define rq_for_each_segment(bvl, _rq, _iter)			\
+	__rq_for_each_bio(_iter.bio, _rq)			\
+		bio_for_each_segment(bvl, _iter.bio, _iter.i)
+
+#define rq_iter_last(rq, _iter)					\
+		(_iter.bio->bi_next == NULL && _iter.i == _iter.bio->bi_vcnt-1)
+
 extern int blk_register_queue(struct gendisk *disk);
 extern void blk_unregister_queue(struct gendisk *disk);
 extern void register_disk(struct gendisk *dev);

[PATCH 003 of 6] Fix various abuse of bio fields in umem.c

[NeilBrown]

umem.c:
  advances bi_idx and bi_sector to track where it is up to.
   But it is only ever doing this on one bio, so the updated
   fields can easily be kept elsewhere (current_*).
  updates bi_size, but never uses the updated values, so
   this isn't needed.
  reuses bi_phys_segments to count how many iovecs have been
   completely.  As the completion happens sequentiually, we
   can store this information outside the bio too.


Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./drivers/block/umem.c |   38 +++++++++++++++++++++++++-------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff .prev/drivers/block/umem.c ./drivers/block/umem.c
--- .prev/drivers/block/umem.c	2007-08-16 21:10:00.000000000 +1000
+++ ./drivers/block/umem.c	2007-08-16 21:10:12.000000000 +1000
@@ -113,6 +113,8 @@ struct cardinfo {
 				    * have been written
 				    */
 	struct bio	*bio, *currentbio, **biotail;
+	int		current_idx;
+	sector_t	current_sector;
 
 	struct request_queue *queue;
 
@@ -121,6 +123,7 @@ struct cardinfo {
 		struct mm_dma_desc	*desc;
 		int	 		cnt, headcnt;
 		struct bio		*bio, **biotail;
+		int			idx;
 	} mm_pages[2];
 #define DESC_PER_PAGE ((PAGE_SIZE*2)/sizeof(struct mm_dma_desc))
 
@@ -380,12 +383,16 @@ static int add_bio(struct cardinfo *card
 	dma_addr_t dma_handle;
 	int offset;
 	struct bio *bio;
+	struct bio_vec *vec;
+	int idx;
 	int rw;
 	int len;
 
 	bio = card->currentbio;
 	if (!bio && card->bio) {
 		card->currentbio = card->bio;
+		card->current_idx = card->bio->bi_idx;
+		card->current_sector = card->bio->bi_sector;
 		card->bio = card->bio->bi_next;
 		if (card->bio == NULL)
 			card->biotail = &card->bio;
@@ -394,15 +401,17 @@ static int add_bio(struct cardinfo *card
 	}
 	if (!bio)
 		return 0;
+	idx = card->current_idx;
 
 	rw = bio_rw(bio);
 	if (card->mm_pages[card->Ready].cnt >= DESC_PER_PAGE)
 		return 0;
 
-	len = bio_iovec(bio)->bv_len;
-	dma_handle = pci_map_page(card->dev, 
-				  bio_page(bio),
-				  bio_offset(bio),
+	vec = bio_iovec_idx(bio, idx);
+	len = vec->bv_len;
+	dma_handle = pci_map_page(card->dev,
+				  vec->bv_page,
+				  vec->bv_offset,
 				  len,
 				  (rw==READ) ?
 				  PCI_DMA_FROMDEVICE : PCI_DMA_TODEVICE);
@@ -410,6 +419,8 @@ static int add_bio(struct cardinfo *card
 	p = &card->mm_pages[card->Ready];
 	desc = &p->desc[p->cnt];
 	p->cnt++;
+	if (p->bio == NULL)
+		p->idx = idx;
 	if ((p->biotail) != &bio->bi_next) {
 		*(p->biotail) = bio;
 		p->biotail = &(bio->bi_next);
@@ -419,7 +430,7 @@ static int add_bio(struct cardinfo *card
 	desc->data_dma_handle = dma_handle;
 
 	desc->pci_addr = cpu_to_le64((u64)desc->data_dma_handle);
-	desc->local_addr= cpu_to_le64(bio->bi_sector << 9);
+	desc->local_addr = cpu_to_le64(card->current_sector << 9);
 	desc->transfer_size = cpu_to_le32(len);
 	offset = ( ((char*)&desc->sem_control_bits) - ((char*)p->desc));
 	desc->sem_addr = cpu_to_le64((u64)(p->page_dma+offset));
@@ -435,10 +446,10 @@ static int add_bio(struct cardinfo *card
 		desc->control_bits |= cpu_to_le32(DMASCR_TRANSFER_READ);
 	desc->sem_control_bits = desc->control_bits;
 
-	bio->bi_sector += (len>>9);
-	bio->bi_size -= len;
-	bio->bi_idx++;
-	if (bio->bi_idx >= bio->bi_vcnt) 
+	card->current_sector += (len >> 9);
+	idx++;
+	card->current_idx = idx;
+	if (idx >= bio->bi_vcnt)
 		card->currentbio = NULL;
 
 	return 1;
@@ -474,10 +485,12 @@ static void process_page(unsigned long d
 			last=1; 
 		}
 		page->headcnt++;
-		idx = bio->bi_phys_segments;
-		bio->bi_phys_segments++;
-		if (bio->bi_phys_segments >= bio->bi_vcnt)
+		idx = page->idx;
+		page->idx++;
+		if (page->idx >= bio->bi_vcnt) {
 			page->bio = bio->bi_next;
+			page->idx = page->bio->bi_idx;
+		}
 
 		pci_unmap_page(card->dev, desc->data_dma_handle, 
 			       bio_iovec_idx(bio,idx)->bv_len,
@@ -547,7 +560,6 @@ static int mm_make_request(struct reques
 	pr_debug("mm_make_request %llu %u\n",
 		 (unsigned long long)bio->bi_sector, bio->bi_size);
 
-	bio->bi_phys_segments = bio->bi_idx; /* count of completed segments*/
 	spin_lock_irq(&card->lock);
 	*card->biotail = bio;
 	bio->bi_next = NULL;

[PATCH 004 of 6] New function blk_req_append_bio

[NeilBrown]

ll_back_merge_fn is currently exported to SCSI where is it used,
together with blk_rq_bio_prep, in exactly the same way these
functions are used in __blk_rq_map_user.

So move the common code into a new function (blk_rq_append_bio), and
don't export ll_back_merge_fn any longer.

Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./block/ll_rw_blk.c       |   38 ++++++++++++++++++++++----------------
 ./drivers/scsi/scsi_lib.c |   11 +----------
 ./include/linux/blkdev.h  |    4 ++--
 3 files changed, 25 insertions(+), 28 deletions(-)

diff .prev/block/ll_rw_blk.c ./block/ll_rw_blk.c
--- .prev/block/ll_rw_blk.c	2007-08-16 21:10:01.000000000 +1000
+++ ./block/ll_rw_blk.c	2007-08-16 21:10:16.000000000 +1000
@@ -1430,7 +1430,8 @@ static inline int ll_new_hw_segment(stru
 	return 1;
 }
 
-int ll_back_merge_fn(struct request_queue *q, struct request *req, struct bio *bio)
+static int ll_back_merge_fn(struct request_queue *q, struct request *req,
+			    struct bio *bio)
 {
 	unsigned short max_sectors;
 	int len;
@@ -1466,7 +1467,6 @@ int ll_back_merge_fn(struct request_queu
 
 	return ll_new_hw_segment(q, req, bio);
 }
-EXPORT_SYMBOL(ll_back_merge_fn);
 
 static int ll_front_merge_fn(struct request_queue *q, struct request *req, 
 			     struct bio *bio)
@@ -2358,6 +2358,23 @@ static int __blk_rq_unmap_user(struct bi
 	return ret;
 }
 
+int blk_rq_append_bio(struct request_queue *q, struct request *rq,
+		      struct bio *bio)
+{
+	if (!rq->bio)
+		blk_rq_bio_prep(q, rq, bio);
+	else if (!ll_back_merge_fn(q, rq, bio))
+		return -EINVAL;
+	else {
+		rq->biotail->bi_next = bio;
+		rq->biotail = bio;
+
+		rq->data_len += bio->bi_size;
+	}
+	return 0;
+}
+EXPORT_SYMBOL(blk_rq_append_bio);
+
 static int __blk_rq_map_user(struct request_queue *q, struct request *rq,
 			     void __user *ubuf, unsigned int len)
 {
@@ -2389,21 +2406,10 @@ static int __blk_rq_map_user(struct requ
 	 */
 	bio_get(bio);
 
-	if (!rq->bio)
-		blk_rq_bio_prep(q, rq, bio);
-	else if (!ll_back_merge_fn(q, rq, bio)) {
-		ret = -EINVAL;
-		goto unmap_bio;
-	} else {
-		rq->biotail->bi_next = bio;
-		rq->biotail = bio;
-
-		rq->data_len += bio->bi_size;
-	}
-
-	return bio->bi_size;
+	ret = blk_rq_append_bio(q, rq, bio);
+	if (!ret)
+		return bio->bi_size;
 
-unmap_bio:
 	/* if it was boucned we must call the end io function */
 	bio_endio(bio, bio->bi_size, 0);
 	__blk_rq_unmap_user(orig_bio);

diff .prev/drivers/scsi/scsi_lib.c ./drivers/scsi/scsi_lib.c
--- .prev/drivers/scsi/scsi_lib.c	2007-08-16 21:10:00.000000000 +1000
+++ ./drivers/scsi/scsi_lib.c	2007-08-16 21:10:16.000000000 +1000
@@ -268,16 +268,7 @@ static int scsi_merge_bio(struct request
 		bio->bi_rw |= (1 << BIO_RW);
 	blk_queue_bounce(q, &bio);
 
-	if (!rq->bio)
-		blk_rq_bio_prep(q, rq, bio);
-	else if (!ll_back_merge_fn(q, rq, bio))
-		return -EINVAL;
-	else {
-		rq->biotail->bi_next = bio;
-		rq->biotail = bio;
-	}
-
-	return 0;
+	return blk_rq_append_bio(q, rq, bio);
 }
 
 static int scsi_bi_endio(struct bio *bio, unsigned int bytes_done, int error)

diff .prev/include/linux/blkdev.h ./include/linux/blkdev.h
--- .prev/include/linux/blkdev.h	2007-08-16 21:10:00.000000000 +1000
+++ ./include/linux/blkdev.h	2007-08-16 21:10:16.000000000 +1000
@@ -675,8 +675,8 @@ extern int sg_scsi_ioctl(struct file *, 
 /*
  * Temporary export, until SCSI gets fixed up.
  */
-extern int ll_back_merge_fn(struct request_queue *, struct request *,
-		struct bio *);
+extern int blk_rq_append_bio(struct request_queue *q, struct request *rq,
+			     struct bio *bio);
 
 /*
  * A queue has just exitted congestion.  Note this in the global counter of

[PATCH 005 of 6] Stop exporting blk_rq_bio_prep

[NeilBrown]

blk_rq_bio_prep is exported for use in exactly
one place.  That place can benefit from using
the new blk_rq_append_bio instead. 
So
  - change dm-emc to call blk_rq_append_bio
  - stop exporting blk_rq_bio_prep, and
  - initialise rq_disk in blk_rq_bio_prep,
       as dm-emc needs it.

Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./block/ll_rw_blk.c      |   11 +++++++----
 ./drivers/md/dm-emc.c    |   10 +---------
 ./include/linux/blkdev.h |    1 -
 3 files changed, 8 insertions(+), 14 deletions(-)

diff .prev/block/ll_rw_blk.c ./block/ll_rw_blk.c
--- .prev/block/ll_rw_blk.c	2007-08-16 21:10:16.000000000 +1000
+++ ./block/ll_rw_blk.c	2007-08-16 21:10:55.000000000 +1000
@@ -43,6 +43,8 @@ static void init_request_from_bio(struct
 static int __make_request(struct request_queue *q, struct bio *bio);
 static struct io_context *current_io_context(gfp_t gfp_flags, int node);
 static void blk_recalc_rq_segments(struct request *rq);
+static void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
+			    struct bio *bio);
 
 /*
  * For the allocated request tables
@@ -3652,8 +3654,8 @@ void end_request(struct request *req, in
 
 EXPORT_SYMBOL(end_request);
 
-void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
-		     struct bio *bio)
+static void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
+			    struct bio *bio)
 {
 	/* first two bits are identical in rq->cmd_flags and bio->bi_rw */
 	rq->cmd_flags |= (bio->bi_rw & 3);
@@ -3667,9 +3669,10 @@ void blk_rq_bio_prep(struct request_queu
 	rq->data_len = bio->bi_size;
 
 	rq->bio = rq->biotail = bio;
-}
 
-EXPORT_SYMBOL(blk_rq_bio_prep);
+	if (bio->bi_bdev)
+		rq->rq_disk = bio->bi_bdev->bd_disk;
+}
 
 int kblockd_schedule_work(struct work_struct *work)
 {

diff .prev/drivers/md/dm-emc.c ./drivers/md/dm-emc.c
--- .prev/drivers/md/dm-emc.c	2007-08-16 17:37:43.000000000 +1000
+++ ./drivers/md/dm-emc.c	2007-08-16 21:10:30.000000000 +1000
@@ -109,15 +109,7 @@ static struct request *get_failover_req(
 		return NULL;
 	}
 
-	rq->bio = rq->biotail = bio;
-	blk_rq_bio_prep(q, rq, bio);
-
-	rq->rq_disk = bdev->bd_contains->bd_disk;
-
-	/* bio backed don't set data */
-	rq->buffer = rq->data = NULL;
-	/* rq data_len used for pc cmd's request_bufflen */
-	rq->data_len = bio->bi_size;
+	blk_rq_append_bio(q, rq, bio);
 
 	rq->sense = h->sense;
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);

diff .prev/include/linux/blkdev.h ./include/linux/blkdev.h
--- .prev/include/linux/blkdev.h	2007-08-16 21:10:16.000000000 +1000
+++ ./include/linux/blkdev.h	2007-08-16 21:10:30.000000000 +1000
@@ -823,7 +823,6 @@ static inline struct request *blk_map_qu
 	return bqt->tag_index[tag];
 }
 
-extern void blk_rq_bio_prep(struct request_queue *, struct request *, struct bio *);
 extern int blkdev_issue_flush(struct block_device *, sector_t *);
 
 #define MAX_PHYS_SEGMENTS 128

[PATCH 006 of 6] Share code between init_request_from_bio and blk_rq_bio_prep

[NeilBrown]

These have very similar functions and should share code where
possible.

Signed-off-by: Neil Brown <neilb@suse.de>

### Diffstat output
 ./block/ll_rw_blk.c |    8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff .prev/block/ll_rw_blk.c ./block/ll_rw_blk.c
--- .prev/block/ll_rw_blk.c	2007-08-16 21:10:55.000000000 +1000
+++ ./block/ll_rw_blk.c	2007-08-16 21:11:03.000000000 +1000
@@ -2932,15 +2932,9 @@ static void init_request_from_bio(struct
 
 	req->errors = 0;
 	req->hard_sector = req->sector = bio->bi_sector;
-	req->hard_nr_sectors = req->nr_sectors = bio_sectors(bio);
-	req->current_nr_sectors = req->hard_cur_sectors = bio_cur_sectors(bio);
-	req->nr_phys_segments = bio_phys_segments(req->q, bio);
-	req->nr_hw_segments = bio_hw_segments(req->q, bio);
-	req->buffer = bio_data(bio);	/* see ->buffer comment above */
-	req->bio = req->biotail = bio;
 	req->ioprio = bio_prio(bio);
-	req->rq_disk = bio->bi_bdev->bd_disk;
 	req->start_time = jiffies;
+	blk_rq_bio_prep(req->q, req, bio);
 }
 
 static int __make_request(struct request_queue *q, struct bio *bio)

Re: [PATCH 000 of 6] A few block-layer tidy-up patches.

[Jens Axboe]

On Thu, Aug 16 2007, NeilBrown wrote:
> Following are 5 patches which - I think - clean up various bits and pieces
> in the block layer.
> 
> The only part that might be seen as a function change rather than
> simply rearranging code is in ps3disk where bvec_kmap_irq is used
> instead of bio_kmap_atomic (so interrupts are disabled).
> 
> The only other user of bvec_kmap_irq is ide-floppy.c.  If that does
> need to disable interrupts, and ps3disk doesn't, make the disabling of
> interrupts should be separated from the kmapping??

Applied 1-6, thanks! BTW, your patch #2 doesn't apply cleanly on
floppy.c in current -git, and there has been no changes there since July
24th. So you must be diffing against something else?

You also don't re-indent and remove one nesting when removing
bio_for_each_segment(). It makes it easier to review, but I formatted
that as well since I had to hand-apply patch #2.

Please inspect the #block-2.6.24 branch to see the result.

-- 
Jens Axboe

Re: [PATCH 000 of 6] A few block-layer tidy-up patches.

[Neil Brown]

On Thursday August 16, jens.axboe@oracle.com wrote:
> On Thu, Aug 16 2007, NeilBrown wrote:
> > Following are 5 patches which - I think - clean up various bits and pieces
> > in the block layer.
> > 
> > The only part that might be seen as a function change rather than
> > simply rearranging code is in ps3disk where bvec_kmap_irq is used
> > instead of bio_kmap_atomic (so interrupts are disabled).
> > 
> > The only other user of bvec_kmap_irq is ide-floppy.c.  If that does
> > need to disable interrupts, and ps3disk doesn't, make the disabling of
> > interrupts should be separated from the kmapping??
> 
> Applied 1-6, thanks! BTW, your patch #2 doesn't apply cleanly on
> floppy.c in current -git, and there has been no changes there since July
> 24th. So you must be diffing against something else?

Yes, current -mm.  That is where I usually work.  I'll make sure
future patches are against -linus or -block.

> 
> You also don't re-indent and remove one nesting when removing
> bio_for_each_segment(). It makes it easier to review, but I formatted
> that as well since I had to hand-apply patch #2.

I meant to mention that in the changelog comment.  I deliberately
didn't re-indent most of the loops to ease review.  I was going to
send an indent-only patch it the first was accepted.  No need now :-)

> 
> Please inspect the #block-2.6.24 branch to see the result.

I don't know where to look for this.  I checked
  http://git.kernel.org/?p=linux/kernel/git/axboe/linux-2.6-block.git
but they don't seem to be there.
??

Thanks,
NeilBrown

Re: [PATCH 000 of 6] A few block-layer tidy-up patches.

[Jens Axboe]

On Fri, Aug 17 2007, Neil Brown wrote:
> On Thursday August 16, jens.axboe@oracle.com wrote:
> > On Thu, Aug 16 2007, NeilBrown wrote:
> > > Following are 5 patches which - I think - clean up various bits and pieces
> > > in the block layer.
> > > 
> > > The only part that might be seen as a function change rather than
> > > simply rearranging code is in ps3disk where bvec_kmap_irq is used
> > > instead of bio_kmap_atomic (so interrupts are disabled).
> > > 
> > > The only other user of bvec_kmap_irq is ide-floppy.c.  If that does
> > > need to disable interrupts, and ps3disk doesn't, make the disabling of
> > > interrupts should be separated from the kmapping??
> > 
> > Applied 1-6, thanks! BTW, your patch #2 doesn't apply cleanly on
> > floppy.c in current -git, and there has been no changes there since July
> > 24th. So you must be diffing against something else?
> 
> Yes, current -mm.  That is where I usually work.  I'll make sure
> future patches are against -linus or -block.

OK

> > You also don't re-indent and remove one nesting when removing
> > bio_for_each_segment(). It makes it easier to review, but I formatted
> > that as well since I had to hand-apply patch #2.
> 
> I meant to mention that in the changelog comment.  I deliberately
> didn't re-indent most of the loops to ease review.  I was going to
> send an indent-only patch it the first was accepted.  No need now :-)

Once I started going through them, it seemed so. No problem, it does
make it easier to verify that no little extra edit crept in.

> > Please inspect the #block-2.6.24 branch to see the result.
> 
> I don't know where to look for this.  I checked
>   http://git.kernel.org/?p=linux/kernel/git/axboe/linux-2.6-block.git
> but they don't seem to be there.
> ??

That's where it is, but the kernel.org mirroring is just horrible slow.
Just checked now and it's there.

-- 
Jens Axboe

Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

[Geert Uytterhoeven]

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

On Thu, 16 Aug 2007, NeilBrown wrote:
> Every usage of rq_for_each_bio wraps a usage of
> bio_for_each_segment, so these can be combined into
> rq_for_each_segment.
> 
> We define "struct req_iterator" to hold the 'bio' and 'index' that
> are needed for the double iteration.

> --- .prev/drivers/block/ps3disk.c	2007-08-16 20:37:26.000000000 +1000
> +++ ./drivers/block/ps3disk.c	2007-08-16 20:50:07.000000000 +1000
> @@ -91,30 +91,30 @@ static void ps3disk_scatter_gather(struc
>  				   struct request *req, int gather)
>  {
>  	unsigned int offset = 0;
> -	struct bio *bio;
> -	sector_t sector;
> +	struct req_iterator iter;
>  	struct bio_vec *bvec;
> -	unsigned int i = 0, j;
> +	unsigned int i = 0;
>  	size_t size;
>  	void *buf;
>  
> -	rq_for_each_bio(bio, req) {
> -		sector = bio->bi_sector;
> +	rq_for_each_segment(bvec, req, iter) {
> +		unsigned long flags;
>  		dev_dbg(&dev->sbd.core,
>  			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
> -			__func__, __LINE__, i, bio_segments(bio),
> -			bio_sectors(bio), sector);
> -		bio_for_each_segment(bvec, bio, j) {
> +			__func__, __LINE__, i, bio_segments(iter.bio),
> +			bio_sectors(iter.bio),
> +			(unsigned long)iter.bio->bi_sector);
                        ^^^^^^^^^^^^^^^
Superfluous cast: PS3 is 64-bit only, and casts are evil.

> +
>  			size = bvec->bv_len;
> -			buf = __bio_kmap_atomic(bio, j, KM_IRQ0);
> +			buf = bvec_kmap_irq(bvec, flags);

As you already mentioned in the preample of the patch series, this changes
functionality.

>  			if (gather)
>  				memcpy(dev->bounce_buf+offset, buf, size);
>  			else
>  				memcpy(buf, dev->bounce_buf+offset, size);
>  			offset += size;
>  			flush_kernel_dcache_page(bio_iovec_idx(bio, j)->bv_page);
> -			__bio_kunmap_atomic(bio, KM_IRQ0);
> -		}
> +			bio_kunmap_bvec(bvec, flags);
> +
>  		i++;
>  	}
>  }

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: [PATCH 000 of 6] A few block-layer tidy-up patches.

[Neil Brown]

On Friday August 17, jens.axboe@oracle.com wrote:
> > > Please inspect the #block-2.6.24 branch to see the result.
> > 
> > I don't know where to look for this.  I checked
> >   http://git.kernel.org/?p=linux/kernel/git/axboe/linux-2.6-block.git
> > but they don't seem to be there.
> > ??
> 
> That's where it is, but the kernel.org mirroring is just horrible slow.
> Just checked now and it's there.

I discovered I was looking in the wrong place - not being very
familiar with git terminology.
I found them and it looks right.

I had a bit of a look at removing bio_data and ->buffer ...  the
usages outside of drivers/ide are fairly easy to deal with - I might
send a patch for that.  The drivers/ide stuff looks like a fairly
substantial rewrite is in order.
e.g.  idefloppy_packet_command_s seems to duplicate a lot of 
  fields from 'struct request', and it should probably use the request
  struct directly.

But a number of times ->buffer points to ->cmd, and there is no bio.
I guess we should use bio_map_kern to make a bio?

I'll see if I can come up with something.... testing it might be
awkward.  I have an ide cdrom I can test on.  Maybe an ide disk,, but
not an ide floppy :-)

NeilBrown

Re: [PATCH 000 of 6] A few block-layer tidy-up patches.

[Jens Axboe]

On Fri, Aug 17 2007, Neil Brown wrote:
> On Friday August 17, jens.axboe@oracle.com wrote:
> > > > Please inspect the #block-2.6.24 branch to see the result.
> > > 
> > > I don't know where to look for this.  I checked
> > >   http://git.kernel.org/?p=linux/kernel/git/axboe/linux-2.6-block.git
> > > but they don't seem to be there.
> > > ??
> > 
> > That's where it is, but the kernel.org mirroring is just horrible slow.
> > Just checked now and it's there.
> 
> I discovered I was looking in the wrong place - not being very
> familiar with git terminology.
> I found them and it looks right.

Ah, good.

> I had a bit of a look at removing bio_data and ->buffer ...  the
> usages outside of drivers/ide are fairly easy to deal with - I might
> send a patch for that.  The drivers/ide stuff looks like a fairly
> substantial rewrite is in order.
> e.g.  idefloppy_packet_command_s seems to duplicate a lot of 
>   fields from 'struct request', and it should probably use the request
>   struct directly.

We can do it bits at the time, the triviel ones first. I have some
experience with drivers/ide/ myself, so I can attempt to tackle some of
that myself.

> But a number of times ->buffer points to ->cmd, and there is no bio.
> I guess we should use bio_map_kern to make a bio?

It points to ->cmd?! But yes, generally things just need to be
converted to map the data to a bio with bio_map_kern() (or
bio_map_user(), where appropriate).

> I'll see if I can come up with something.... testing it might be
> awkward.  I have an ide cdrom I can test on.  Maybe an ide disk,, but
> not an ide floppy :-)

I can help with that as well :-)

-- 
Jens Axboe

Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

[Jan Engelhardt]

On Aug 18 2007 20:07, Satyam Sharma wrote:
>On Fri, 17 Aug 2007, Geert Uytterhoeven wrote:
>
>> On Thu, 16 Aug 2007, NeilBrown wrote:
>> [...]
>> >  		dev_dbg(&dev->sbd.core,
>> >  			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
>> > -			__func__, __LINE__, i, bio_segments(bio),
>> > -			bio_sectors(bio), sector);
>> > -		bio_for_each_segment(bvec, bio, j) {
>> > +			__func__, __LINE__, i, bio_segments(iter.bio),
>> > +			bio_sectors(iter.bio),
>> > +			(unsigned long)iter.bio->bi_sector);
>>                         ^^^^^^^^^^^^^^^
>> Superfluous cast: PS3 is 64-bit only, and casts are evil.

bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully
so since sector_t may just change its shape underneath. It would not be so much
of a problem if printf() was not a varargs function, but it is, and hence,
passing an object bigger than the format specifier can make problems.


	Jan
-- 

Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

[Satyam Sharma]

On Fri, 17 Aug 2007, Geert Uytterhoeven wrote:

> On Thu, 16 Aug 2007, NeilBrown wrote:
> [...]
> >  		dev_dbg(&dev->sbd.core,
> >  			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
> > -			__func__, __LINE__, i, bio_segments(bio),
> > -			bio_sectors(bio), sector);
> > -		bio_for_each_segment(bvec, bio, j) {
> > +			__func__, __LINE__, i, bio_segments(iter.bio),
> > +			bio_sectors(iter.bio),
> > +			(unsigned long)iter.bio->bi_sector);
>                         ^^^^^^^^^^^^^^^
> Superfluous cast: PS3 is 64-bit only, and casts are evil.

(Sorry for butting in), but I wonder if relying on that in code here
(granted, a PS3-only driver, but it's in drivers/ and not arch/) would
be good style. Why not just print it out as: "%llu" and use an (unsigned
long long) cast, considering that's the largest type sector_t can ever
have (irrespective of arch) ... I can see most of the other generic places
in the kernel (such as in drivers/md/) also using the latter (%llu with
unsigned long long cast) to get bi_sector printed.

Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

[Satyam Sharma]

On Sat, 18 Aug 2007, Jan Engelhardt wrote:

> On Aug 18 2007 20:07, Satyam Sharma wrote:
> >On Fri, 17 Aug 2007, Geert Uytterhoeven wrote:
> >
> >> On Thu, 16 Aug 2007, NeilBrown wrote:
> >> [...]
> >> >  		dev_dbg(&dev->sbd.core,
> >> >  			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
> >> > -			__func__, __LINE__, i, bio_segments(bio),
> >> > -			bio_sectors(bio), sector);
> >> > -		bio_for_each_segment(bvec, bio, j) {
> >> > +			__func__, __LINE__, i, bio_segments(iter.bio),
> >> > +			bio_sectors(iter.bio),
> >> > +			(unsigned long)iter.bio->bi_sector);
> >>                         ^^^^^^^^^^^^^^^
> >> Superfluous cast: PS3 is 64-bit only, and casts are evil.
> 
> bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully
> so since sector_t may just change its shape underneath. It would not be so much
> of a problem if printf() was not a varargs function, but it is, and hence,
> passing an object bigger than the format specifier can make problems.

Oh yeah, that's why the _cast_ _is_ needed in the first place, by the way.
I was mentioning why the cast itself should be (unsigned long long) otoh.

Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

[Geert Uytterhoeven]

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

On Sat, 18 Aug 2007, Satyam Sharma wrote:
> On Sat, 18 Aug 2007, Jan Engelhardt wrote:
> > On Aug 18 2007 20:07, Satyam Sharma wrote:
> > >On Fri, 17 Aug 2007, Geert Uytterhoeven wrote:
> > >> On Thu, 16 Aug 2007, NeilBrown wrote:
> > >> [...]
> > >> >  		dev_dbg(&dev->sbd.core,
> > >> >  			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
> > >> > -			__func__, __LINE__, i, bio_segments(bio),
> > >> > -			bio_sectors(bio), sector);
> > >> > -		bio_for_each_segment(bvec, bio, j) {
> > >> > +			__func__, __LINE__, i, bio_segments(iter.bio),
> > >> > +			bio_sectors(iter.bio),
> > >> > +			(unsigned long)iter.bio->bi_sector);
> > >>                         ^^^^^^^^^^^^^^^
> > >> Superfluous cast: PS3 is 64-bit only, and casts are evil.
> > 
> > bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully
> > so since sector_t may just change its shape underneath. It would not be so much
> > of a problem if printf() was not a varargs function, but it is, and hence,
> > passing an object bigger than the format specifier can make problems.
> 
> Oh yeah, that's why the _cast_ _is_ needed in the first place, by the way.
> I was mentioning why the cast itself should be (unsigned long long) otoh.

On 64-bit platforms, sector_t (which is either u64 or unsigned long, depending
on CONFIG_LBD) is unsigned long, so there's no need for a cast. Hence there's
no compiler warning to be silenced by adding the cast.

On the other hand, adding a cast may hide bugs:
  - cast to unsigned long: When reusing parts of this code for a new 32-bit
    driver, the sector_t will be silently truncated,
  - cast to unsigned long long: When sector_t is changed to an even larger
    type, it will be silently truncated as well.
Without the cast, these would cause compiler warnings.

BTW, the resulting code didn't even compile, because of 3 bugs:

| linux-2.6/drivers/block/ps3disk.c: In function ‘ps3disk_scatter_gather’:
| linux-2.6/drivers/block/ps3disk.c:115: error: ‘bio’ undeclared (first use in this function)
| linux-2.6/drivers/block/ps3disk.c:115: error: (Each undeclared identifier is reported only once
| linux-2.6/drivers/block/ps3disk.c:115: error: for each function it appears in.)
| linux-2.6/drivers/block/ps3disk.c:115: error: ‘j’ undeclared (first use in this function)
| linux-2.6/drivers/block/ps3disk.c:116: error: implicit declaration of function ‘bio_kunmap_bvec’
| make[3]: *** [drivers/block/ps3disk.o] Error 1

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: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

[Jan Engelhardt]

On Aug 20 2007 13:21, Geert Uytterhoeven wrote:
>> > >> >  		dev_dbg(&dev->sbd.core,
>> > >> >  			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
>> > >> > -			__func__, __LINE__, i, bio_segments(bio),
>> > >> > -			bio_sectors(bio), sector);
>> > >> > -		bio_for_each_segment(bvec, bio, j) {
>> > >> > +			__func__, __LINE__, i, bio_segments(iter.bio),
>> > >> > +			bio_sectors(iter.bio),
>> > >> > +			(unsigned long)iter.bio->bi_sector);
>> > >>                         ^^^^^^^^^^^^^^^
>> > >> Superfluous cast: PS3 is 64-bit only, and casts are evil.
>> > 
>> > bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully
>> > so since sector_t may just change its shape underneath. It would not be so much
>> > of a problem if printf() was not a varargs function, but it is, and hence,
>> > passing an object bigger than the format specifier can make problems.
>> 
>> Oh yeah, that's why the _cast_ _is_ needed in the first place, by the way.
>> I was mentioning why the cast itself should be (unsigned long long) otoh.
>
>On 64-bit platforms, sector_t (which is either u64 or unsigned long, depending
>on CONFIG_LBD) is unsigned long, so there's no need for a cast. Hence there's
>no compiler warning to be silenced by adding the cast.

Note actually.. I was accidentally referring to printf rather than the
actual ^^^-marked cast. Sorry for the inconvenience.


	Jan
-- 

Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

[Satyam Sharma]

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

Hi Geert,


On Mon, 20 Aug 2007, Geert Uytterhoeven wrote:

> On Sat, 18 Aug 2007, Satyam Sharma wrote:
> > On Sat, 18 Aug 2007, Jan Engelhardt wrote:
> > > On Aug 18 2007 20:07, Satyam Sharma wrote:
> > > >On Fri, 17 Aug 2007, Geert Uytterhoeven wrote:
> > > >> On Thu, 16 Aug 2007, NeilBrown wrote:
> > > >> [...]
> > > >> >  		dev_dbg(&dev->sbd.core,
> > > >> >  			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
                                                                        ^^^

> > > >> > -			__func__, __LINE__, i, bio_segments(bio),
> > > >> > -			bio_sectors(bio), sector);
> > > >> > -		bio_for_each_segment(bvec, bio, j) {
> > > >> > +			__func__, __LINE__, i, bio_segments(iter.bio),
> > > >> > +			bio_sectors(iter.bio),
> > > >> > +			(unsigned long)iter.bio->bi_sector);
                                ^^^^^^^^^^^^^^^          ^^^^^^^^^

> > > >> Superfluous cast: PS3 is 64-bit only, and casts are evil.
> > > 
> > > bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully
> > > so since sector_t may just change its shape underneath.
> > 
> > Oh yeah, that's why the _cast_ _is_ needed in the first place, by the way.
> > I was mentioning why the cast itself should be (unsigned long long) otoh.
> 
> On 64-bit platforms, sector_t (which is either u64 or unsigned long, depending
> on CONFIG_LBD) is unsigned long, so there's no need for a cast. Hence there's
> no compiler warning to be silenced by adding the cast.

This is turning rather funny :-)

* Why the printk() conversion specifier must be "%llu"?

When reusing parts of this code (such as this debug message) for another
32-bit driver (we certainly seem to care about this, as per your reply),
the largest size of sector_t can be "unsigned long long", thereby causing
truncated output, and the following warning:

warning: format ‘%lu’ expects type ‘long unsigned int’, but argument 2 has
type ‘sector_t’

Therefore, let us not depend on the fact that this driver is being used
only for 64-bit platforms as of now (especially since this is in drivers/
and not in arch/) and rather make the printk() specifier as "%llu".

[ Sadly, I had to repeat most of my previous mail, which Jan Engelhardt
appears to have snipped. ]

* Why we require an explicit (unsigned long long) cast?

Having made the above change (and say we don't have an explicit cast
there), that line now becomes:

	printk("... %llu\n", ..., iter.bio->bi_sector);

Now if we build this code with CONFIG_LBD=n, sector_t becomes just an
"unsigned long" (whereas printk specifier is the larger "%llu") thereby
causing:

warning: format ‘%llu’ expects type ‘long long unsigned int’, but argument
2 has type ‘sector_t’

Therefore, let us shut this up with an explicit (unsigned long long) cast,
as is done in most other existing code in the kernel where we want to get
bi_sector printed out, which would correctly convert the value to the
larger integer type (even negative ones, due to sign-extension) and print
it out.


> On the other hand, adding a cast may hide bugs:
>   - cast to unsigned long long: When sector_t is changed to an even larger
>     type, it will be silently truncated as well.

IMHO, this is not a valid enough reason, given the above (BTW if/when that
happens, we'll have to update the printk conversion specifer as well).

Personally, I'd say code that assumes "sizeof(unsigned long long) ==
sizeof(unsigned long)", and also that assumes "sizeof(unsigned long) ==
sizeof(sector_t)" -- both assumptions _are_ made by the above code --
is not good taste, even if both may be true for PS3 platform. So unless
we decide "nobody except that platform would ever build this driver
anyway", I'd suggest to make the printk specifier as "%llu" and use an
explicit (unsigned long long) cast. Therefore (on top of this series):

Signed-off-by: Satyam Sharma <satyam@infradead.org>

---

diff -ruNp a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
--- a/drivers/block/ps3disk.c	2007-08-21 06:52:34.000000000 +0530
+++ b/drivers/block/ps3disk.c	2007-08-21 06:56:07.000000000 +0530
@@ -100,10 +100,10 @@ static void ps3disk_scatter_gather(struc
 	rq_for_each_segment(bvec, req, iter) {
 		unsigned long flags;
 		dev_dbg(&dev->sbd.core,
-			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
+			"%s:%u: bio %u: %u segs %u sectors from %llu\n",
 			__func__, __LINE__, i, bio_segments(iter.bio),
 			bio_sectors(iter.bio),
-			(unsigned long)iter.bio->bi_sector);
+			(unsigned long long)iter.bio->bi_sector);
 
 		size = bvec->bv_len;
 		buf = bvec_kmap_irq(bvec, flags);
@@ -142,8 +142,9 @@ static int ps3disk_submit_request_sg(str
 
 	start_sector = req->sector * priv->blocking_factor;
 	sectors = req->nr_sectors * priv->blocking_factor;
-	dev_dbg(&dev->sbd.core, "%s:%u: %s %lu sectors starting at %lu\n",
-		__func__, __LINE__, op, sectors, start_sector);
+	dev_dbg(&dev->sbd.core, "%s:%u: %s %llu sectors starting at %llu\n",
+		__func__, __LINE__, op, (unsigned long long)sectors,
+		(unsigned long long)start_sector);
 
 	if (write) {
 		ps3disk_scatter_gather(dev, req, 1);

Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

[Geert Uytterhoeven]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; CHARSET=UTF-8, Size: 6200 bytes --]

On Tue, 21 Aug 2007, Satyam Sharma wrote:
> On Mon, 20 Aug 2007, Geert Uytterhoeven wrote:
> > On Sat, 18 Aug 2007, Satyam Sharma wrote:
> > > On Sat, 18 Aug 2007, Jan Engelhardt wrote:
> > > > On Aug 18 2007 20:07, Satyam Sharma wrote:
> > > > >On Fri, 17 Aug 2007, Geert Uytterhoeven wrote:
> > > > >> On Thu, 16 Aug 2007, NeilBrown wrote:
> > > > >> [...]
> > > > >> >  		dev_dbg(&dev->sbd.core,
> > > > >> >  			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
>                                                                         ^^^
> 
> > > > >> > -			__func__, __LINE__, i, bio_segments(bio),
> > > > >> > -			bio_sectors(bio), sector);
> > > > >> > -		bio_for_each_segment(bvec, bio, j) {
> > > > >> > +			__func__, __LINE__, i, bio_segments(iter.bio),
> > > > >> > +			bio_sectors(iter.bio),
> > > > >> > +			(unsigned long)iter.bio->bi_sector);
>                                 ^^^^^^^^^^^^^^^          ^^^^^^^^^
> 
> > > > >> Superfluous cast: PS3 is 64-bit only, and casts are evil.
> > > > 
> > > > bi_sector is sector_t. The cast is ok, because printf will warn, and rightfully
> > > > so since sector_t may just change its shape underneath.
> > > 
> > > Oh yeah, that's why the _cast_ _is_ needed in the first place, by the way.
> > > I was mentioning why the cast itself should be (unsigned long long) otoh.
> > 
> > On 64-bit platforms, sector_t (which is either u64 or unsigned long, depending
> > on CONFIG_LBD) is unsigned long, so there's no need for a cast. Hence there's
> > no compiler warning to be silenced by adding the cast.
> 
> This is turning rather funny :-)
> 
> * Why the printk() conversion specifier must be "%llu"?
> 
> When reusing parts of this code (such as this debug message) for another
> 32-bit driver (we certainly seem to care about this, as per your reply),
> the largest size of sector_t can be "unsigned long long", thereby causing
> truncated output, and the following warning:
> 
> warning: format ¡Æ%lu¡Ç expects type ¡Ælong unsigned int¡Ç, but argument 2 has
> type ¡Æsector_t¡Ç

You will get a warning, so you will know.

> Therefore, let us not depend on the fact that this driver is being used
> only for 64-bit platforms as of now (especially since this is in drivers/
> and not in arch/) and rather make the printk() specifier as "%llu".
> 
> [ Sadly, I had to repeat most of my previous mail, which Jan Engelhardt
> appears to have snipped. ]

[ and you dropped the cell mailing list from the CC list ]

> * Why we require an explicit (unsigned long long) cast?
> 
> Having made the above change (and say we don't have an explicit cast
> there), that line now becomes:
> 
> 	printk("... %llu\n", ..., iter.bio->bi_sector);
> 
> Now if we build this code with CONFIG_LBD=n, sector_t becomes just an
> "unsigned long" (whereas printk specifier is the larger "%llu") thereby
> causing:
> 
> warning: format ¡Æ%llu¡Ç expects type ¡Ælong long unsigned int¡Ç, but argument
> 2 has type ¡Æsector_t¡Ç
> 
> Therefore, let us shut this up with an explicit (unsigned long long) cast,
> as is done in most other existing code in the kernel where we want to get
> bi_sector printed out, which would correctly convert the value to the
> larger integer type (even negative ones, due to sign-extension) and print
> it out.
> 
> 
> > On the other hand, adding a cast may hide bugs:
> >   - cast to unsigned long long: When sector_t is changed to an even larger
> >     type, it will be silently truncated as well.
> 
> IMHO, this is not a valid enough reason, given the above (BTW if/when that
> happens, we'll have to update the printk conversion specifer as well).
> 
> Personally, I'd say code that assumes "sizeof(unsigned long long) ==
> sizeof(unsigned long)", and also that assumes "sizeof(unsigned long) ==
> sizeof(sector_t)" -- both assumptions _are_ made by the above code --
> is not good taste, even if both may be true for PS3 platform. So unless
> we decide "nobody except that platform would ever build this driver
> anyway", I'd suggest to make the printk specifier as "%llu" and use an
> explicit (unsigned long long) cast. Therefore (on top of this series):

Adding such a cast makes it impossible for the compiler to detect (and warn us)
if something has changed.

> Signed-off-by: Satyam Sharma <satyam@infradead.org>

NAK. Do not add casts that are not needed. Casts are evil.

> diff -ruNp a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
> --- a/drivers/block/ps3disk.c	2007-08-21 06:52:34.000000000 +0530
> +++ b/drivers/block/ps3disk.c	2007-08-21 06:56:07.000000000 +0530
> @@ -100,10 +100,10 @@ static void ps3disk_scatter_gather(struc
>  	rq_for_each_segment(bvec, req, iter) {
>  		unsigned long flags;
>  		dev_dbg(&dev->sbd.core,
> -			"%s:%u: bio %u: %u segs %u sectors from %lu\n",
> +			"%s:%u: bio %u: %u segs %u sectors from %llu\n",
>  			__func__, __LINE__, i, bio_segments(iter.bio),
>  			bio_sectors(iter.bio),
> -			(unsigned long)iter.bio->bi_sector);
> +			(unsigned long long)iter.bio->bi_sector);
>  
>  		size = bvec->bv_len;
>  		buf = bvec_kmap_irq(bvec, flags);
> @@ -142,8 +142,9 @@ static int ps3disk_submit_request_sg(str
>  
>  	start_sector = req->sector * priv->blocking_factor;
>  	sectors = req->nr_sectors * priv->blocking_factor;
> -	dev_dbg(&dev->sbd.core, "%s:%u: %s %lu sectors starting at %lu\n",
> -		__func__, __LINE__, op, sectors, start_sector);
> +	dev_dbg(&dev->sbd.core, "%s:%u: %s %llu sectors starting at %llu\n",
> +		__func__, __LINE__, op, (unsigned long long)sectors,
> +		(unsigned long long)start_sector);
>  
>  	if (write) {
>  		ps3disk_scatter_gather(dev, req, 1);

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: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

[Satyam Sharma]

On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:

> On Tue, 21 Aug 2007, Satyam Sharma wrote:
> > 
> > This is turning rather funny :-)
> > 
> > * Why the printk() conversion specifier must be "%llu"?
> > 
> > When reusing parts of this code (such as this debug message) for another
> > 32-bit driver (we certainly seem to care about this, as per your reply),
> > the largest size of sector_t can be "unsigned long long", thereby causing
> > truncated output, and the following warning:
> > 
> > warning: format ???lu???expects type ???ong unsigned int??? but argument 2 has
> > type ???ector_t?
> 
> You will get a warning, so you will know.

Ah. So you'd much rather prefer that code in drivers/ make assumptions:
sizeof(long) == sizeof(long long), and, sizeof(long) == sizeof(sector_t),
hmmm?


> > Therefore, let us not depend on the fact that this driver is being used
> > only for 64-bit platforms as of now (especially since this is in drivers/
> > and not in arch/) and rather make the printk() specifier as "%llu".
> > 
> > [ Sadly, I had to repeat most of my previous mail, which Jan Engelhardt
> > appears to have snipped. ]
> 
> [ and you dropped the cell mailing list from the CC list ]

I don't enjoy getting "this is a subscriber-only list" notifications,
thank you.


> > * Why we require an explicit (unsigned long long) cast?
> > 
> > Having made the above change (and say we don't have an explicit cast
> > there), that line now becomes:
> > 
> > 	printk("... %llu\n", ..., iter.bio->bi_sector);
> > 
> > Now if we build this code with CONFIG_LBD=n, sector_t becomes just an
> > "unsigned long" (whereas printk specifier is the larger "%llu") thereby
> > causing:
> > 
> > warning: format ???llu???expects type ???ong long unsigned int??? but argument
> > 2 has type ???ector_t?
> > 
> > Therefore, let us shut this up with an explicit (unsigned long long) cast,
> > as is done in most other existing code in the kernel where we want to get
> > bi_sector printed out, which would correctly convert the value to the
> > larger integer type (even negative ones, due to sign-extension) and print
> > it out.
> > 
> > > On the other hand, adding a cast may hide bugs:
> > >   - cast to unsigned long long: When sector_t is changed to an even larger
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > >     type, it will be silently truncated as well.
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > 
> > IMHO, this is not a valid enough reason, given the above (BTW if/when that
> > happens, we'll have to update the printk conversion specifer as well).
> > 
> > Personally, I'd say code that assumes "sizeof(unsigned long long) ==
> > sizeof(unsigned long)", and also that assumes "sizeof(unsigned long) ==
> > sizeof(sector_t)" -- both assumptions _are_ made by the above code --
> > is not good taste, even if both may be true for PS3 platform. So unless
> > we decide "nobody except that platform would ever build this driver
> > anyway", I'd suggest to make the printk specifier as "%llu" and use an
> > explicit (unsigned long long) cast. Therefore (on top of this series):
> 
> Adding such a cast makes it impossible for the compiler to detect (and warn us)
> if something has changed.

Change as in increase in size, right? Did you even read the mail? The only
argument you had given against the cast to (unsigned long long) was what
has been underlined above, which was bogus, considering we would have to
change the "%lu" specifier in the printk() also, when that happens
(otherwise suffer silent truncation).

But looks like you /are/ deciding "nobody except PS3 platform would ever
build this driver anyway" ... so okay, the current code, will all it's
non-portable assumptions, is okay.

Cheers,

Satyam

Re: [PATCH 002 of 6] Introduce rq_for_each_segment replacing rq_for_each_bio

[Geert Uytterhoeven]

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

On Tue, 21 Aug 2007, Satyam Sharma wrote:
> On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:
> > On Tue, 21 Aug 2007, Satyam Sharma wrote:
> > > 
> > > This is turning rather funny :-)
> > > 
> > > * Why the printk() conversion specifier must be "%llu"?
> > > 
> > > When reusing parts of this code (such as this debug message) for another
> > > 32-bit driver (we certainly seem to care about this, as per your reply),
> > > the largest size of sector_t can be "unsigned long long", thereby causing
> > > truncated output, and the following warning:
> > > 
> > > warning: format ???lu???expects type ???ong unsigned int??? but argument 2 has
> > > type ???ector_t?
> > 
> > You will get a warning, so you will know.
> 
> Ah. So you'd much rather prefer that code in drivers/ make assumptions:
> sizeof(long) == sizeof(long long), and, sizeof(long) == sizeof(sector_t),
> hmmm?

If it's code for a PS3-specific driver that cannot work in 32-bit mode, yes.

> > > Therefore, let us not depend on the fact that this driver is being used
> > > only for 64-bit platforms as of now (especially since this is in drivers/
> > > and not in arch/) and rather make the printk() specifier as "%llu".
> > > 
> > > [ Sadly, I had to repeat most of my previous mail, which Jan Engelhardt
> > > appears to have snipped. ]
> > 
> > [ and you dropped the cell mailing list from the CC list ]
> 
> I don't enjoy getting "this is a subscriber-only list" notifications,
> thank you.

IC.

> > > * Why we require an explicit (unsigned long long) cast?
> > > 
> > > Having made the above change (and say we don't have an explicit cast
> > > there), that line now becomes:
> > > 
> > > 	printk("... %llu\n", ..., iter.bio->bi_sector);
> > > 
> > > Now if we build this code with CONFIG_LBD=n, sector_t becomes just an
> > > "unsigned long" (whereas printk specifier is the larger "%llu") thereby
> > > causing:
> > > 
> > > warning: format ???llu???expects type ???ong long unsigned int??? but argument
> > > 2 has type ???ector_t?
> > > 
> > > Therefore, let us shut this up with an explicit (unsigned long long) cast,
> > > as is done in most other existing code in the kernel where we want to get
> > > bi_sector printed out, which would correctly convert the value to the
> > > larger integer type (even negative ones, due to sign-extension) and print
> > > it out.
> > > 
> > > > On the other hand, adding a cast may hide bugs:
> > > >   - cast to unsigned long long: When sector_t is changed to an even larger
>           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > >     type, it will be silently truncated as well.
>           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> > > 
> > > IMHO, this is not a valid enough reason, given the above (BTW if/when that
> > > happens, we'll have to update the printk conversion specifer as well).
> > > 
> > > Personally, I'd say code that assumes "sizeof(unsigned long long) ==
> > > sizeof(unsigned long)", and also that assumes "sizeof(unsigned long) ==
> > > sizeof(sector_t)" -- both assumptions _are_ made by the above code --
> > > is not good taste, even if both may be true for PS3 platform. So unless
> > > we decide "nobody except that platform would ever build this driver
> > > anyway", I'd suggest to make the printk specifier as "%llu" and use an
> > > explicit (unsigned long long) cast. Therefore (on top of this series):
> > 
> > Adding such a cast makes it impossible for the compiler to detect (and warn us)
> > if something has changed.
> 
> Change as in increase in size, right? Did you even read the mail? The only

Yes. Yes.

> argument you had given against the cast to (unsigned long long) was what
> has been underlined above, which was bogus, considering we would have to
> change the "%lu" specifier in the printk() also, when that happens
> (otherwise suffer silent truncation).

Not silent truncation. Without a cast, the compiler will warn.

> But looks like you /are/ deciding "nobody except PS3 platform would ever
> build this driver anyway" ... so okay, the current code, will all it's
> non-portable assumptions, is okay.

OK, thx.
The driver uses the PS3-specific hypervisor interface, which is 64-bit.

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

[PATCH] PS3: Update MAINTAINERS

[Satyam Sharma]

On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:

> On Tue, 21 Aug 2007, Satyam Sharma wrote:
> > On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:
> > > 
> > > [ and you dropped the cell mailing list from the CC list ]
> > 
> > I don't enjoy getting "this is a subscriber-only list" notifications,
> > thank you.
> 
> IC.

[PATCH] PS3: Update MAINTAINERS

Cell Broadband Engine OSS Development <cbe-oss-dev@ozlabs.org> is an
subscribers-only mailing list.

Signed-off-by: Satyam Sharma <satyam@infradead.org>

---

 MAINTAINERS |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 371fe67..ec1bc77 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -3029,14 +3029,14 @@ PS3 NETWORK SUPPORT
 P:	Masakazu Mokuno
 M:	mokuno@sm.sony.co.jp
 L:	netdev@vger.kernel.org
-L:	cbe-oss-dev@ozlabs.org
+L:	cbe-oss-dev@ozlabs.org	(subscribers-only)
 S:	Supported
 
 PS3 PLATFORM SUPPORT
 P:	Geoff Levand
 M:	geoffrey.levand@am.sony.com
 L:	linuxppc-dev@ozlabs.org
-L:	cbe-oss-dev@ozlabs.org
+L:	cbe-oss-dev@ozlabs.org	(subscribers-only)
 S:	Supported
 
 PVRUSB2 VIDEO4LINUX DRIVER

Re: [PATCH] PS3: Update MAINTAINERS

[Geoff Levand]

Satyam Sharma wrote:
> 
> On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:
> 
>> On Tue, 21 Aug 2007, Satyam Sharma wrote:
>> > On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:
>> > > 
>> > > [ and you dropped the cell mailing list from the CC list ]
>> > 
>> > I don't enjoy getting "this is a subscriber-only list" notifications,
>> > thank you.
>> 
>> IC.
> 
> [PATCH] PS3: Update MAINTAINERS
> 
> Cell Broadband Engine OSS Development <cbe-oss-dev@ozlabs.org> is an
> subscribers-only mailing list.
> 
> Signed-off-by: Satyam Sharma <satyam@infradead.org>
> 
> ---
> 
>  MAINTAINERS |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 371fe67..ec1bc77 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3029,14 +3029,14 @@ PS3 NETWORK SUPPORT
>  P:	Masakazu Mokuno
>  M:	mokuno@sm.sony.co.jp
>  L:	netdev@vger.kernel.org
> -L:	cbe-oss-dev@ozlabs.org
> +L:	cbe-oss-dev@ozlabs.org	(subscribers-only)
>  S:	Supported
>  
>  PS3 PLATFORM SUPPORT
>  P:	Geoff Levand
>  M:	geoffrey.levand@am.sony.com
>  L:	linuxppc-dev@ozlabs.org
> -L:	cbe-oss-dev@ozlabs.org
> +L:	cbe-oss-dev@ozlabs.org	(subscribers-only)
>  S:	Supported

Hi Hugh,

Could you verify that cbe-oss-dev is a subscriber's
only list?  I didn't see anything that said it
specifically in the listinfo.

-Geoff

Re: [PATCH] PS3: Update MAINTAINERS

[Joe Perches]

On Tue, 2007-08-21 at 12:01 -0700, Geoff Levand wrote:
> Could you verify that cbe-oss-dev is a subscriber's
> only list?  I didn't see anything that said it
> specifically in the listinfo.

This is the "hold" message I got on posting to that list.

Your mail to 'cbe-oss-dev' with the subject

    Re: [PATCH] [392/2many] MAINTAINERS - PS3 PLATFORM SUPPORT

Is being held until the list moderator can review it for approval.

The reason it is being held:

    Post by non-member to a members-only list

Re: [PATCH] PS3: Update MAINTAINERS

[Rene Herman]

On 08/21/2007 10:01 PM, Joe Perches wrote:

> On Tue, 2007-08-21 at 12:01 -0700, Geoff Levand wrote:

>> Could you verify that cbe-oss-dev is a subscriber's
>> only list?  I didn't see anything that said it
>> specifically in the listinfo.
> 
> This is the "hold" message I got on posting to that list.
> 
> Your mail to 'cbe-oss-dev' with the subject
> 
>     Re: [PATCH] [392/2many] MAINTAINERS - PS3 PLATFORM SUPPORT
> 
> Is being held until the list moderator can review it for approval.
> 
> The reason it is being held:
> 
>     Post by non-member to a members-only list

That means it's not a subcriber-only list -- the message wasn't rejected, 
just subjected to moderation.

Rene.

Re: [PATCH] PS3: Update MAINTAINERS

[Geoff Levand]

Satyam Sharma wrote:
> 
> On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:
> 
>> On Tue, 21 Aug 2007, Satyam Sharma wrote:
>> > On Tue, 21 Aug 2007, Geert Uytterhoeven wrote:
>> > > 
>> > > [ and you dropped the cell mailing list from the CC list ]
>> > 
>> > I don't enjoy getting "this is a subscriber-only list" notifications,
>> > thank you.
>> 
>> IC.
> 
> [PATCH] PS3: Update MAINTAINERS
> 
> Cell Broadband Engine OSS Development <cbe-oss-dev@ozlabs.org> is an
> subscribers-only mailing list.
> 
> Signed-off-by: Satyam Sharma <satyam@infradead.org>
> 
> ---
> 
>  MAINTAINERS |    4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 371fe67..ec1bc77 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -3029,14 +3029,14 @@ PS3 NETWORK SUPPORT
>  P:	Masakazu Mokuno
>  M:	mokuno@sm.sony.co.jp
>  L:	netdev@vger.kernel.org
> -L:	cbe-oss-dev@ozlabs.org
> +L:	cbe-oss-dev@ozlabs.org	(subscribers-only)
>  S:	Supported
>  
>  PS3 PLATFORM SUPPORT
>  P:	Geoff Levand
>  M:	geoffrey.levand@am.sony.com
>  L:	linuxppc-dev@ozlabs.org
> -L:	cbe-oss-dev@ozlabs.org
> +L:	cbe-oss-dev@ozlabs.org	(subscribers-only)
>  S:	Supported

Sorry for any inconvenience this may have caused to any
non-subscribers who posted to that list.

Acked-by: Geoff Levand <geoffrey.levand@am.sony.com>

Re: [PATCH] PS3: Update MAINTAINERS

[Geoff Levand]

Rene Herman wrote:
> On 08/21/2007 10:01 PM, Joe Perches wrote:
> 
>> On Tue, 2007-08-21 at 12:01 -0700, Geoff Levand wrote:
> 
>>> Could you verify that cbe-oss-dev is a subscriber's
>>> only list?  I didn't see anything that said it
>>> specifically in the listinfo.
>> 
>> This is the "hold" message I got on posting to that list.
>> 
>> Your mail to 'cbe-oss-dev' with the subject
>> 
>>     Re: [PATCH] [392/2many] MAINTAINERS - PS3 PLATFORM SUPPORT
>> 
>> Is being held until the list moderator can review it for approval.
>> 
>> The reason it is being held:
>> 
>>     Post by non-member to a members-only list
> 
> That means it's not a subcriber-only list -- the message wasn't rejected, 
> just subjected to moderation.
> 

So maybe it would be more precise to have something like this:

-L:	cbe-oss-dev@ozlabs.org
+L:	cbe-oss-dev@ozlabs.org	(moderated)

Re: [PATCH] PS3: Update MAINTAINERS

[Rene Herman]

On 08/21/2007 10:17 PM, Geoff Levand wrote:

> Rene Herman wrote:
>> On 08/21/2007 10:01 PM, Joe Perches wrote:

>>> The reason it is being held:
>>>
>>>     Post by non-member to a members-only list
>> That means it's not a subcriber-only list -- the message wasn't rejected, 
>> just subjected to moderation.
>>
> 
> So maybe it would be more precise to have something like this:
> 
> -L:	cbe-oss-dev@ozlabs.org
> +L:	cbe-oss-dev@ozlabs.org	(moderated)

Perhaps. In these spamridden days, mailinglists that don't have the kind of 
(human and other) resources behind them that linux-kernel has basically have 
the choice between drowning in spam, becoming subscriber only or moderate 
non-subscribers and of these, that third option is "best among the bad".

alsa-devel for example also went this route -- the spam levels simply 
weren't tolerable anymore for any subscriber and the list was dying as a 
result. Moderation sucks, but subcriber-only sucks even worse (generally, 
and/but even more so for lists that expect crossposts from linux-kernel) so 
what's a small-time list to do.

Moderation takes some effort so the lists that moderate have made the 
explicit choice to not become subscriber-only. While subscriber-only 
certainly is useful to mention alongside the list address itself, I'm not 
too sure mentioning moderation makes a great deal of sense actually -- if 
all's well, the moderator will simply approve it and you don't have to deal 
with it other than that.

Rene.

Re: [PATCH] PS3: Update MAINTAINERS

[Rene Herman]

On 08/21/2007 10:48 PM, Satyam Sharma wrote:

>>> That means it's not a subcriber-only list -- the message wasn't rejected, 
>>> just subjected to moderation.
> 
> That's precisely a subscribers-only list. At least as per MAINTAINER's
> definition of that term. It'd be surprising to know of a mailing list
> mentioned in MAINTAINERS that _drops_ messages posted to it entirely!

Then its definition does not make any sense whatsoever -- as said just now 
as well, moderation takes some effort so lists that do have made the 
explicit choice to _not_ become subscriber-only.

Moderation is furthermore not something the poster needs to concern himself 
with, other than through being aware of a possible delay. It's mostly those 
poor moderators that wade through bucket-loads of spam that care... ;-)

Rene.

Re: [PATCH] PS3: Update MAINTAINERS

[Joe Perches]

On Wed, 2007-08-22 at 02:18 +0530, Satyam Sharma wrote:
> That's precisely a subscribers-only list. At least as per MAINTAINER's
> definition of that term. It'd be surprising to know of a mailing list
> mentioned in MAINTAINERS that _drops_ messages posted to it entirely!

Here's one possibility, though I did sent messages that
some might have considered spam:

usbb2k-api-dev@nongnu.org

This one isn't marked as subscriber only either.

On Mon, 2007-08-13 at 14:00 -0400, usbb2k-api-dev-owner@nongnu.org
wrote:
> Vous n'êtes pas autorisé à envoyer des messages sur cette liste, votre
> message a été automatiquement rejeté. Si vous pensez que votre message
> a été rejeté par erreur, veuillez contacter le gestionnaire de la
> liste à l'adresse usbb2k-api-dev-owner@nongnu.org.

Re: [PATCH] PS3: Update MAINTAINERS

[Satyam Sharma]

Hi,


On Tue, 21 Aug 2007, Geoff Levand wrote:

> Rene Herman wrote:
> > On 08/21/2007 10:01 PM, Joe Perches wrote:
> > 
> >> On Tue, 2007-08-21 at 12:01 -0700, Geoff Levand wrote:
> > 
> >>> Could you verify that cbe-oss-dev is a subscriber's
> >>> only list?  I didn't see anything that said it
> >>> specifically in the listinfo.
> >> 
> >> This is the "hold" message I got on posting to that list.
> >> 
> >> Your mail to 'cbe-oss-dev' with the subject
> >> 
> >>     Re: [PATCH] [392/2many] MAINTAINERS - PS3 PLATFORM SUPPORT
> >> 
> >> Is being held until the list moderator can review it for approval.
> >> 
> >> The reason it is being held:
> >> 
> >>     Post by non-member to a members-only list
> > 
> > That means it's not a subcriber-only list -- the message wasn't rejected, 
> > just subjected to moderation.

That's precisely a subscribers-only list. At least as per MAINTAINER's
definition of that term. It'd be surprising to know of a mailing list
mentioned in MAINTAINERS that _drops_ messages posted to it entirely!

> So maybe it would be more precise to have something like this:
> 
> -L:	cbe-oss-dev@ozlabs.org
> +L:	cbe-oss-dev@ozlabs.org	(moderated)

Exactly a similar message greets those who post to linux-arm-kernel@
and it (and other such lists) is referred to everywhere in that file
as (subscribers-only), for example. The patch is correct, IMO.

Thanks,

Satyam

Re: [PATCH] PS3: Update MAINTAINERS

[Hugh Blemings]

Hi Geoff,

> Could you verify that cbe-oss-dev is a subscriber's
> only list?  I didn't see anything that said it
> specifically in the listinfo.

Yes, the list is configured as subscriber only, I make sure I add people to the accept filter as soon as they hit the list. 

Fell behind a bit yesterday so apologies for the inconvenience this caused you Satyam.

Rationale is it keeps the SPAM down quite a bit but I acknowledge it has it drawbacks if I don't get to the held messages quickly.

Cheers,
Hugh

Re: [PATCH] PS3: Update MAINTAINERS

[Matt Mackall]

On Wed, Aug 22, 2007 at 07:58:13AM +1000, Hugh Blemings wrote:
> Hi Geoff,
> 
> > Could you verify that cbe-oss-dev is a subscriber's
> > only list?  I didn't see anything that said it
> > specifically in the listinfo.
> 
> Yes, the list is configured as subscriber only, I make sure I add people to the accept filter as soon as they hit the list. 
> 
> Fell behind a bit yesterday so apologies for the inconvenience this caused you Satyam.
> 
> Rationale is it keeps the SPAM down quite a bit but I acknowledge it has it drawbacks if I don't get to the held messages quickly.

The drawback is mostly in getting the rejection/moderation message.

Whitelisting is fine. If you configure the list to not send the
moderation warning to legitimate non-subscribers, you'll eliminate 99%
of the annoyance.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH] PS3: Update MAINTAINERS

[Hugh Blemings]

Hi Matt, All,

> The drawback is mostly in getting the rejection/moderation message.
> 
> Whitelisting is fine. If you configure the list to not send the
> moderation warning to legitimate non-subscribers, you'll eliminate 99%
> of the annoyance.

Good call, have changed the configuration accordingly.

Thanks for the feedback all :)

Cheers,
Hugh

Re: [Cbe-oss-dev] [PATCH] PS3: Update MAINTAINERS

[Hugh Blemings]

Hi Satyam, All,

> [PATCH] PS3: Update MAINTAINERS
> 
> Cell Broadband Engine OSS Development <cbe-oss-dev@ozlabs.org> is an
> subscribers-only mailing list.
> 
> Signed-off-by: Satyam Sharma <satyam@infradead.org>
>
> [...]

As per recent discussions...

The list is moderated for SPAM prevention, messages from valid senders
will be forwarded manually and their email added to the whitelist.

I've changed the list config so that it no longer generates the
"subscriber-only" notification.

On this basis I recommended the patch be NAKd so that people aren't put
off copying the list where appropriate.

Thanks,

Regards,
Hugh

Re: [Cbe-oss-dev] [PATCH] PS3: Update MAINTAINERS

[Satyam Sharma]

On Fri, 24 Aug 2007, Hugh Blemings wrote:

> Hi Satyam, All,
> 
> > [PATCH] PS3: Update MAINTAINERS
> > 
> > Cell Broadband Engine OSS Development <cbe-oss-dev@ozlabs.org> is an
> > subscribers-only mailing list.
> > 
> > Signed-off-by: Satyam Sharma <satyam@infradead.org>
> >
> > [...]
> 
> As per recent discussions...
> 
> The list is moderated for SPAM prevention, messages from valid senders
> will be forwarded manually and their email added to the whitelist.
> 
> I've changed the list config so that it no longer generates the
> "subscriber-only" notification.

So ... if someone (!subscriber && !whitelisted) sends a message, it stays
put somewhere till you get to it and determine whether it is relevant or
not, and either (1) forward it on to the list if appropriate, or (2) drop
it if detected as spam. And no "notifications" are ever sent out to
genuine / well-meaning posters during this process. Right?

> On this basis I recommended the patch be NAKd so that people aren't put
> off copying the list where appropriate.

Fair enough, if my understanding (as mentioned above) is correct, then I
believe the patch achieved a greater purpose than what it was originally
sent out for!

Thanks,

Satyam

Re: [Cbe-oss-dev] [PATCH] PS3: Update MAINTAINERS

[Hugh Blemings]

Hiya Satyam, All,

> > > [PATCH] PS3: Update MAINTAINERS
> > > 
> > > Cell Broadband Engine OSS Development <cbe-oss-dev@ozlabs.org> is an
> > > subscribers-only mailing list.
> > > 
> > > Signed-off-by: Satyam Sharma <satyam@infradead.org>
> > >
> > > [...]
> > 
> > As per recent discussions...
> > 
> > The list is moderated for SPAM prevention, messages from valid senders
> > will be forwarded manually and their email added to the whitelist.
> > 
> > I've changed the list config so that it no longer generates the
> > "subscriber-only" notification.
> 
> So ... if someone (!subscriber && !whitelisted) sends a message, it stays
> put somewhere till you get to it and determine whether it is relevant or
> not, and either (1) forward it on to the list if appropriate, or (2) drop
> it if detected as spam. And no "notifications" are ever sent out to
> genuine / well-meaning posters during this process. Right?

Correct.

I aim to check messages at least once a day, typically more like three or four times.  

There is a (increasingly brief it seems) period where I sleep so things may languish during that period, or if I've gone bush or some such :)

Jokes aside, am in the process of getting someone in Europe to co-moderate so we've some redundancy.

> > On this basis I recommended the patch be NAKd so that people aren't put
> > off copying the list where appropriate.
> 
> Fair enough, if my understanding (as mentioned above) is correct, then I
> believe the patch achieved a greater purpose than what it was originally
> sent out for!

No prob, good result all round it seems.

Cheers,
Hugh