[PATCH 1/2] block: export __make_request

[PATCH 1/2] block: export __make_request

[Christoph Hellwig]

Avoid the hacks need for request based device mappers currently by simply
exporting the symbol instead of trying to get it through the back door.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/block/blk-core.c
===================================================================
--- linux-2.6.orig/block/blk-core.c	2011-09-08 12:02:11.575274440 +0200
+++ linux-2.6/block/blk-core.c	2011-09-08 12:03:25.422774199 +0200
@@ -38,8 +38,6 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(block_bio_r
 EXPORT_TRACEPOINT_SYMBOL_GPL(block_rq_remap);
 EXPORT_TRACEPOINT_SYMBOL_GPL(block_bio_complete);
 
-static int __make_request(struct request_queue *q, struct bio *bio);
-
 /*
  * For the allocated request tables
  */
@@ -1213,7 +1211,7 @@ void init_request_from_bio(struct reques
 	blk_rq_bio_prep(req->q, req, bio);
 }
 
-static int __make_request(struct request_queue *q, struct bio *bio)
+int __make_request(struct request_queue *q, struct bio *bio)
 {
 	const bool sync = !!(bio->bi_rw & REQ_SYNC);
 	struct blk_plug *plug;
@@ -1317,6 +1315,7 @@ out_unlock:
 out:
 	return 0;
 }
+EXPORT_SYMBOL_GPL(__make_request);	/* for device mapper only */
 
 /*
  * If bio->bi_dev is a partition, remap the location
Index: linux-2.6/drivers/md/dm.c
===================================================================
--- linux-2.6.orig/drivers/md/dm.c	2011-09-08 12:03:31.587273831 +0200
+++ linux-2.6/drivers/md/dm.c	2011-09-08 12:05:02.306774593 +0200
@@ -180,9 +180,6 @@ struct mapped_device {
 	/* forced geometry settings */
 	struct hd_geometry geometry;
 
-	/* For saving the address of __make_request for request based dm */
-	make_request_fn *saved_make_request_fn;
-
 	/* sysfs handle */
 	struct kobject kobj;
 
@@ -1420,13 +1417,6 @@ static int _dm_request(struct request_qu
 	return 0;
 }
 
-static int dm_make_request(struct request_queue *q, struct bio *bio)
-{
-	struct mapped_device *md = q->queuedata;
-
-	return md->saved_make_request_fn(q, bio); /* call __make_request() */
-}
-
 static int dm_request_based(struct mapped_device *md)
 {
 	return blk_queue_stackable(md->queue);
@@ -1437,7 +1427,7 @@ static int dm_request(struct request_que
 	struct mapped_device *md = q->queuedata;
 
 	if (dm_request_based(md))
-		return dm_make_request(q, bio);
+		return __make_request(q, bio);
 
 	return _dm_request(q, bio);
 }
@@ -2172,7 +2162,6 @@ static int dm_init_request_based_queue(s
 		return 0;
 
 	md->queue = q;
-	md->saved_make_request_fn = md->queue->make_request_fn;
 	dm_init_md_queue(md);
 	blk_queue_softirq_done(md->queue, dm_softirq_done);
 	blk_queue_prep_rq(md->queue, dm_prep_fn);
Index: linux-2.6/include/linux/blkdev.h
===================================================================
--- linux-2.6.orig/include/linux/blkdev.h	2011-09-08 12:02:11.595273510 +0200
+++ linux-2.6/include/linux/blkdev.h	2011-09-08 12:02:33.038774679 +0200
@@ -680,6 +680,8 @@ extern int scsi_cmd_ioctl(struct request
 extern int sg_scsi_ioctl(struct request_queue *, struct gendisk *, fmode_t,
 			 struct scsi_ioctl_command __user *);
 
+extern int __make_request(struct request_queue *q, struct bio *bio);
+
 /*
  * A queue has just exitted congestion.  Note this in the global counter of
  * congested queues, and wake up anyone who was waiting for requests to be

[PATCH 2/2] block: remove support for bio remapping from ->make_request

[Christoph Hellwig]

There is very little benefit in allowing to let a ->make_request instance update
the bios device and sector and loop around it in __generic_make_request when
we can archive the same through calling generic_make_request from the driver
and letting the loop in generic_make_request handle it.

Note that various drivers got the return value from ->make_request and returned
non-zero values for errors.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/arch/m68k/emu/nfblock.c
===================================================================
--- linux-2.6.orig/arch/m68k/emu/nfblock.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/arch/m68k/emu/nfblock.c	2011-09-08 13:46:07.993865436 +0200
@@ -59,7 +59,7 @@ struct nfhd_device {
 	struct gendisk *disk;
 };
 
-static int nfhd_make_request(struct request_queue *queue, struct bio *bio)
+static void nfhd_make_request(struct request_queue *queue, struct bio *bio)
 {
 	struct nfhd_device *dev = queue->queuedata;
 	struct bio_vec *bvec;
@@ -76,7 +76,6 @@ static int nfhd_make_request(struct requ
 		sec += len;
 	}
 	bio_endio(bio, 0);
-	return 0;
 }
 
 static int nfhd_getgeo(struct block_device *bdev, struct hd_geometry *geo)
Index: linux-2.6/arch/powerpc/sysdev/axonram.c
===================================================================
--- linux-2.6.orig/arch/powerpc/sysdev/axonram.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/arch/powerpc/sysdev/axonram.c	2011-09-08 13:46:07.985864438 +0200
@@ -104,7 +104,7 @@ axon_ram_irq_handler(int irq, void *dev)
  * axon_ram_make_request - make_request() method for block device
  * @queue, @bio: see blk_queue_make_request()
  */
-static int
+static void
 axon_ram_make_request(struct request_queue *queue, struct bio *bio)
 {
 	struct axon_ram_bank *bank = bio->bi_bdev->bd_disk->private_data;
@@ -113,7 +113,6 @@ axon_ram_make_request(struct request_que
 	struct bio_vec *vec;
 	unsigned int transfered;
 	unsigned short idx;
-	int rc = 0;
 
 	phys_mem = bank->io_addr + (bio->bi_sector << AXON_RAM_SECTOR_SHIFT);
 	phys_end = bank->io_addr + bank->size;
@@ -121,8 +120,7 @@ axon_ram_make_request(struct request_que
 	bio_for_each_segment(vec, bio, idx) {
 		if (unlikely(phys_mem + vec->bv_len > phys_end)) {
 			bio_io_error(bio);
-			rc = -ERANGE;
-			break;
+			return;
 		}
 
 		user_mem = page_address(vec->bv_page) + vec->bv_offset;
@@ -135,8 +133,6 @@ axon_ram_make_request(struct request_que
 		transfered += vec->bv_len;
 	}
 	bio_endio(bio, 0);
-
-	return rc;
 }
 
 /**
Index: linux-2.6/drivers/block/aoe/aoeblk.c
===================================================================
--- linux-2.6.orig/drivers/block/aoe/aoeblk.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/block/aoe/aoeblk.c	2011-09-08 13:46:08.009865290 +0200
@@ -159,7 +159,7 @@ aoeblk_release(struct gendisk *disk, fmo
 	return 0;
 }
 
-static int
+static void
 aoeblk_make_request(struct request_queue *q, struct bio *bio)
 {
 	struct sk_buff_head queue;
@@ -172,25 +172,25 @@ aoeblk_make_request(struct request_queue
 	if (bio == NULL) {
 		printk(KERN_ERR "aoe: bio is NULL\n");
 		BUG();
-		return 0;
+		return;
 	}
 	d = bio->bi_bdev->bd_disk->private_data;
 	if (d == NULL) {
 		printk(KERN_ERR "aoe: bd_disk->private_data is NULL\n");
 		BUG();
 		bio_endio(bio, -ENXIO);
-		return 0;
+		return;
 	} else if (bio->bi_io_vec == NULL) {
 		printk(KERN_ERR "aoe: bi_io_vec is NULL\n");
 		BUG();
 		bio_endio(bio, -ENXIO);
-		return 0;
+		return;
 	}
 	buf = mempool_alloc(d->bufpool, GFP_NOIO);
 	if (buf == NULL) {
 		printk(KERN_INFO "aoe: buf allocation failure\n");
 		bio_endio(bio, -ENOMEM);
-		return 0;
+		return;
 	}
 	memset(buf, 0, sizeof(*buf));
 	INIT_LIST_HEAD(&buf->bufs);
@@ -211,7 +211,7 @@ aoeblk_make_request(struct request_queue
 		spin_unlock_irqrestore(&d->lock, flags);
 		mempool_free(buf, d->bufpool);
 		bio_endio(bio, -ENXIO);
-		return 0;
+		return;
 	}
 
 	list_add_tail(&buf->bufs, &d->bufq);
@@ -222,8 +222,6 @@ aoeblk_make_request(struct request_queue
 
 	spin_unlock_irqrestore(&d->lock, flags);
 	aoenet_xmit(&queue);
-
-	return 0;
 }
 
 static int
Index: linux-2.6/drivers/block/brd.c
===================================================================
--- linux-2.6.orig/drivers/block/brd.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/block/brd.c	2011-09-08 13:46:08.017865926 +0200
@@ -323,7 +323,7 @@ out:
 	return err;
 }
 
-static int brd_make_request(struct request_queue *q, struct bio *bio)
+static void brd_make_request(struct request_queue *q, struct bio *bio)
 {
 	struct block_device *bdev = bio->bi_bdev;
 	struct brd_device *brd = bdev->bd_disk->private_data;
@@ -359,8 +359,6 @@ static int brd_make_request(struct reque
 
 out:
 	bio_endio(bio, err);
-
-	return 0;
 }
 
 #ifdef CONFIG_BLK_DEV_XIP
Index: linux-2.6/drivers/block/drbd/drbd_int.h
===================================================================
--- linux-2.6.orig/drivers/block/drbd/drbd_int.h	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/block/drbd/drbd_int.h	2011-09-08 12:05:58.560289507 +0200
@@ -1507,7 +1507,7 @@ extern void drbd_free_mdev(struct drbd_c
 extern int proc_details;
 
 /* drbd_req */
-extern int drbd_make_request(struct request_queue *q, struct bio *bio);
+extern void drbd_make_request(struct request_queue *q, struct bio *bio);
 extern int drbd_read_remote(struct drbd_conf *mdev, struct drbd_request *req);
 extern int drbd_merge_bvec(struct request_queue *q, struct bvec_merge_data *bvm, struct bio_vec *bvec);
 extern int is_valid_ar_handle(struct drbd_request *, sector_t);
Index: linux-2.6/drivers/block/drbd/drbd_req.c
===================================================================
--- linux-2.6.orig/drivers/block/drbd/drbd_req.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/block/drbd/drbd_req.c	2011-09-08 12:05:58.564273023 +0200
@@ -1073,7 +1077,7 @@ static int drbd_fail_request_early(struc
 	return 0;
 }
 
-int drbd_make_request(struct request_queue *q, struct bio *bio)
+void drbd_make_request(struct request_queue *q, struct bio *bio)
 {
 	unsigned int s_enr, e_enr;
 	struct drbd_conf *mdev = (struct drbd_conf *) q->queuedata;
@@ -1081,7 +1085,7 @@ int drbd_make_request(struct request_que
 
 	if (drbd_fail_request_early(mdev, bio_data_dir(bio) & WRITE)) {
 		bio_endio(bio, -EPERM);
-		return 0;
+		return;
 	}
 
 	start_time = jiffies;
@@ -1100,7 +1104,8 @@ int drbd_make_request(struct request_que
 
 	if (likely(s_enr == e_enr)) {
 		inc_ap_bio(mdev, 1);
-		return drbd_make_request_common(mdev, bio, start_time);
+		drbd_make_request_common(mdev, bio, start_time);
+		return;
 	}
 
 	/* can this bio be split generically?
@@ -1148,7 +1153,6 @@ int drbd_make_request(struct request_que
 
 		bio_pair_release(bp);
 	}
-	return 0;
 }
 
 /* This is called by bio_add_page().  With this function we reduce
Index: linux-2.6/drivers/block/loop.c
===================================================================
--- linux-2.6.orig/drivers/block/loop.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/block/loop.c	2011-09-08 13:46:08.037866440 +0200
@@ -514,7 +514,7 @@ static struct bio *loop_get_bio(struct l
 	return bio_list_pop(&lo->lo_bio_list);
 }
 
-static int loop_make_request(struct request_queue *q, struct bio *old_bio)
+static void loop_make_request(struct request_queue *q, struct bio *old_bio)
 {
 	struct loop_device *lo = q->queuedata;
 	int rw = bio_rw(old_bio);
@@ -532,12 +532,11 @@ static int loop_make_request(struct requ
 	loop_add_bio(lo, old_bio);
 	wake_up(&lo->lo_event);
 	spin_unlock_irq(&lo->lo_lock);
-	return 0;
+	return;
 
 out:
 	spin_unlock_irq(&lo->lo_lock);
 	bio_io_error(old_bio);
-	return 0;
 }
 
 struct switch_request {
Index: linux-2.6/drivers/block/pktcdvd.c
===================================================================
--- linux-2.6.orig/drivers/block/pktcdvd.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/block/pktcdvd.c	2011-09-08 13:46:08.041865904 +0200
@@ -2444,7 +2444,7 @@ static void pkt_end_io_read_cloned(struc
 	pkt_bio_finished(pd);
 }
 
-static int pkt_make_request(struct request_queue *q, struct bio *bio)
+static void pkt_make_request(struct request_queue *q, struct bio *bio)
 {
 	struct pktcdvd_device *pd;
 	char b[BDEVNAME_SIZE];
@@ -2473,7 +2473,7 @@ static int pkt_make_request(struct reque
 		cloned_bio->bi_end_io = pkt_end_io_read_cloned;
 		pd->stats.secs_r += bio->bi_size >> 9;
 		pkt_queue_bio(pd, cloned_bio);
-		return 0;
+		return;
 	}
 
 	if (!test_bit(PACKET_WRITABLE, &pd->flags)) {
@@ -2509,7 +2509,7 @@ static int pkt_make_request(struct reque
 			pkt_make_request(q, &bp->bio1);
 			pkt_make_request(q, &bp->bio2);
 			bio_pair_release(bp);
-			return 0;
+			return;
 		}
 	}
 
@@ -2533,7 +2533,7 @@ static int pkt_make_request(struct reque
 				}
 				spin_unlock(&pkt->lock);
 				spin_unlock(&pd->cdrw.active_list_lock);
-				return 0;
+				return;
 			} else {
 				blocked_bio = 1;
 			}
@@ -2584,10 +2584,9 @@ static int pkt_make_request(struct reque
 		 */
 		wake_up(&pd->wqueue);
 	}
-	return 0;
+	return;
 end_io:
 	bio_io_error(bio);
-	return 0;
 }
 
 
Index: linux-2.6/drivers/block/ps3vram.c
===================================================================
--- linux-2.6.orig/drivers/block/ps3vram.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/block/ps3vram.c	2011-09-08 13:46:08.049864488 +0200
@@ -596,7 +596,7 @@ out:
 	return next;
 }
 
-static int ps3vram_make_request(struct request_queue *q, struct bio *bio)
+static void ps3vram_make_request(struct request_queue *q, struct bio *bio)
 {
 	struct ps3_system_bus_device *dev = q->queuedata;
 	struct ps3vram_priv *priv = ps3_system_bus_get_drvdata(dev);
@@ -610,13 +610,11 @@ static int ps3vram_make_request(struct r
 	spin_unlock_irq(&priv->lock);
 
 	if (busy)
-		return 0;
+		return;
 
 	do {
 		bio = ps3vram_do_bio(dev, bio);
 	} while (bio);
-
-	return 0;
 }
 
 static int __devinit ps3vram_probe(struct ps3_system_bus_device *dev)
Index: linux-2.6/drivers/block/umem.c
===================================================================
--- linux-2.6.orig/drivers/block/umem.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/block/umem.c	2011-09-08 13:46:08.065865586 +0200
@@ -513,7 +513,7 @@ static void process_page(unsigned long d
 	}
 }
 
-static int mm_make_request(struct request_queue *q, struct bio *bio)
+static void mm_make_request(struct request_queue *q, struct bio *bio)
 {
 	struct cardinfo *card = q->queuedata;
 	pr_debug("mm_make_request %llu %u\n",
@@ -525,7 +525,7 @@ static int mm_make_request(struct reques
 	card->biotail = &bio->bi_next;
 	spin_unlock_irq(&card->lock);
 
-	return 0;
+	return;
 }
 
 static irqreturn_t mm_interrupt(int irq, void *__card)
Index: linux-2.6/drivers/md/dm.c
===================================================================
--- linux-2.6.orig/drivers/md/dm.c	2011-09-08 12:05:02.000000000 +0200
+++ linux-2.6/drivers/md/dm.c	2011-09-08 13:46:08.101867184 +0200
@@ -1388,7 +1388,7 @@ out:
  * The request function that just remaps the bio built up by
  * dm_merge_bvec.
  */
-static int _dm_request(struct request_queue *q, struct bio *bio)
+static void _dm_request(struct request_queue *q, struct bio *bio)
 {
 	int rw = bio_data_dir(bio);
 	struct mapped_device *md = q->queuedata;
@@ -1409,12 +1409,12 @@ static int _dm_request(struct request_qu
 			queue_io(md, bio);
 		else
 			bio_io_error(bio);
-		return 0;
+		return;
 	}
 
 	__split_and_process_bio(md, bio);
 	up_read(&md->io_lock);
-	return 0;
+	return;
 }
 
 static int dm_request_based(struct mapped_device *md)
@@ -1422,14 +1422,14 @@ static int dm_request_based(struct mappe
 	return blk_queue_stackable(md->queue);
 }
 
-static int dm_request(struct request_queue *q, struct bio *bio)
+static void dm_request(struct request_queue *q, struct bio *bio)
 {
 	struct mapped_device *md = q->queuedata;
 
 	if (dm_request_based(md))
-		return __make_request(q, bio);
-
-	return _dm_request(q, bio);
+		__make_request(q, bio);
+	else
+		_dm_request(q, bio);
 }
 
 void dm_dispatch_request(struct request *rq)
Index: linux-2.6/drivers/s390/block/dcssblk.c
===================================================================
--- linux-2.6.orig/drivers/s390/block/dcssblk.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/s390/block/dcssblk.c	2011-09-08 13:46:08.073864338 +0200
@@ -27,7 +27,7 @@
 
 static int dcssblk_open(struct block_device *bdev, fmode_t mode);
 static int dcssblk_release(struct gendisk *disk, fmode_t mode);
-static int dcssblk_make_request(struct request_queue *q, struct bio *bio);
+static void dcssblk_make_request(struct request_queue *q, struct bio *bio);
 static int dcssblk_direct_access(struct block_device *bdev, sector_t secnum,
 				 void **kaddr, unsigned long *pfn);
 
@@ -814,7 +814,7 @@ out:
 	return rc;
 }
 
-static int
+static void
 dcssblk_make_request(struct request_queue *q, struct bio *bio)
 {
 	struct dcssblk_dev_info *dev_info;
@@ -871,10 +871,9 @@ dcssblk_make_request(struct request_queu
 		bytes_done += bvec->bv_len;
 	}
 	bio_endio(bio, 0);
-	return 0;
+	return;
 fail:
 	bio_io_error(bio);
-	return 0;
 }
 
 static int
Index: linux-2.6/drivers/s390/block/xpram.c
===================================================================
--- linux-2.6.orig/drivers/s390/block/xpram.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/s390/block/xpram.c	2011-09-08 13:46:08.081866518 +0200
@@ -181,7 +181,7 @@ static unsigned long xpram_highest_page_
 /*
  * Block device make request function.
  */
-static int xpram_make_request(struct request_queue *q, struct bio *bio)
+static void xpram_make_request(struct request_queue *q, struct bio *bio)
 {
 	xpram_device_t *xdev = bio->bi_bdev->bd_disk->private_data;
 	struct bio_vec *bvec;
@@ -221,10 +221,9 @@ static int xpram_make_request(struct req
 	}
 	set_bit(BIO_UPTODATE, &bio->bi_flags);
 	bio_endio(bio, 0);
-	return 0;
+	return;
 fail:
 	bio_io_error(bio);
-	return 0;
 }
 
 static int xpram_getgeo(struct block_device *bdev, struct hd_geometry *geo)
Index: linux-2.6/drivers/staging/zram/zram_drv.c
===================================================================
--- linux-2.6.orig/drivers/staging/zram/zram_drv.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/staging/zram/zram_drv.c	2011-09-08 13:46:08.109866400 +0200
@@ -556,24 +556,22 @@ static inline int valid_io_request(struc
 /*
  * Handler function for all zram I/O requests.
  */
-static int zram_make_request(struct request_queue *queue, struct bio *bio)
+static void zram_make_request(struct request_queue *queue, struct bio *bio)
 {
 	struct zram *zram = queue->queuedata;
 
 	if (!valid_io_request(zram, bio)) {
 		zram_stat64_inc(zram, &zram->stats.invalid_io);
 		bio_io_error(bio);
-		return 0;
+		return;
 	}
 
 	if (unlikely(!zram->init_done) && zram_init_device(zram)) {
 		bio_io_error(bio);
-		return 0;
+		return;
 	}
 
 	__zram_make_request(zram, bio, bio_data_dir(bio));
-
-	return 0;
 }
 
 void zram_reset_device(struct zram *zram)
Index: linux-2.6/include/linux/blkdev.h
===================================================================
--- linux-2.6.orig/include/linux/blkdev.h	2011-09-08 12:02:33.000000000 +0200
+++ linux-2.6/include/linux/blkdev.h	2011-09-08 13:46:08.117866175 +0200
@@ -195,7 +195,7 @@ struct request_pm_state
 #include <linux/elevator.h>
 
 typedef void (request_fn_proc) (struct request_queue *q);
-typedef int (make_request_fn) (struct request_queue *q, struct bio *bio);
+typedef void (make_request_fn) (struct request_queue *q, struct bio *bio);
 typedef int (prep_rq_fn) (struct request_queue *, struct request *);
 typedef void (unprep_rq_fn) (struct request_queue *, struct request *);
 
@@ -680,7 +680,7 @@ extern int scsi_cmd_ioctl(struct request
 extern int sg_scsi_ioctl(struct request_queue *, struct gendisk *, fmode_t,
 			 struct scsi_ioctl_command __user *);
 
-extern int __make_request(struct request_queue *q, struct bio *bio);
+extern void __make_request(struct request_queue *q, struct bio *bio);
 
 /*
  * A queue has just exitted congestion.  Note this in the global counter of
Index: linux-2.6/drivers/md/faulty.c
===================================================================
--- linux-2.6.orig/drivers/md/faulty.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/md/faulty.c	2011-09-08 12:05:58.602773802 +0200
@@ -169,7 +169,7 @@ static void add_sector(conf_t *conf, sec
 		conf->nfaults = n+1;
 }
 
-static int make_request(mddev_t *mddev, struct bio *bio)
+static void make_request(mddev_t *mddev, struct bio *bio)
 {
 	conf_t *conf = mddev->private;
 	int failit = 0;
@@ -181,7 +181,7 @@ static int make_request(mddev_t *mddev,
 			 * just fail immediately
 			 */
 			bio_endio(bio, -EIO);
-			return 0;
+			return;
 		}
 
 		if (check_sector(conf, bio->bi_sector, bio->bi_sector+(bio->bi_size>>9),
@@ -214,12 +214,11 @@ static int make_request(mddev_t *mddev,
 		b->bi_bdev = conf->rdev->bdev;
 		b->bi_private = bio;
 		b->bi_end_io = faulty_fail;
-		generic_make_request(b);
-		return 0;
 	} else {
 		bio->bi_bdev = conf->rdev->bdev;
-		return 1;
 	}
+
+	generic_make_request(b);
 }
 
 static void status(struct seq_file *seq, mddev_t *mddev)
Index: linux-2.6/drivers/md/linear.c
===================================================================
--- linux-2.6.orig/drivers/md/linear.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/md/linear.c	2011-09-08 12:05:58.602773802 +0200
@@ -264,14 +264,14 @@ static int linear_stop (mddev_t *mddev)
 	return 0;
 }
 
-static int linear_make_request (mddev_t *mddev, struct bio *bio)
+static void linear_make_request (mddev_t *mddev, struct bio *bio)
 {
 	dev_info_t *tmp_dev;
 	sector_t start_sector;
 
 	if (unlikely(bio->bi_rw & REQ_FLUSH)) {
 		md_flush_request(mddev, bio);
-		return 0;
+		return;
 	}
 
 	rcu_read_lock();
@@ -293,7 +293,7 @@ static int linear_make_request (mddev_t
 		       (unsigned long long)start_sector);
 		rcu_read_unlock();
 		bio_io_error(bio);
-		return 0;
+		return;
 	}
 	if (unlikely(bio->bi_sector + (bio->bi_size >> 9) >
 		     tmp_dev->end_sector)) {
@@ -307,20 +307,17 @@ static int linear_make_request (mddev_t
 
 		bp = bio_split(bio, end_sector - bio->bi_sector);
 
-		if (linear_make_request(mddev, &bp->bio1))
-			generic_make_request(&bp->bio1);
-		if (linear_make_request(mddev, &bp->bio2))
-			generic_make_request(&bp->bio2);
+		linear_make_request(mddev, &bp->bio1);
+		linear_make_request(mddev, &bp->bio2);
 		bio_pair_release(bp);
-		return 0;
+		return;
 	}
 		    
 	bio->bi_bdev = tmp_dev->rdev->bdev;
 	bio->bi_sector = bio->bi_sector - start_sector
 		+ tmp_dev->rdev->data_offset;
 	rcu_read_unlock();
-
-	return 1;
+	generic_make_request(bio);
 }
 
 static void linear_status (struct seq_file *seq, mddev_t *mddev)
Index: linux-2.6/drivers/md/multipath.c
===================================================================
--- linux-2.6.orig/drivers/md/multipath.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/md/multipath.c	2011-09-08 12:05:58.606775620 +0200
@@ -106,7 +106,7 @@ static void multipath_end_request(struct
 	rdev_dec_pending(rdev, conf->mddev);
 }
 
-static int multipath_make_request(mddev_t *mddev, struct bio * bio)
+static void multipath_make_request(mddev_t *mddev, struct bio * bio)
 {
 	multipath_conf_t *conf = mddev->private;
 	struct multipath_bh * mp_bh;
@@ -114,7 +114,7 @@ static int multipath_make_request(mddev_
 
 	if (unlikely(bio->bi_rw & REQ_FLUSH)) {
 		md_flush_request(mddev, bio);
-		return 0;
+		return;
 	}
 
 	mp_bh = mempool_alloc(conf->pool, GFP_NOIO);
@@ -126,7 +126,7 @@ static int multipath_make_request(mddev_
 	if (mp_bh->path < 0) {
 		bio_endio(bio, -EIO);
 		mempool_free(mp_bh, conf->pool);
-		return 0;
+		return;
 	}
 	multipath = conf->multipaths + mp_bh->path;
 
@@ -137,7 +137,7 @@ static int multipath_make_request(mddev_
 	mp_bh->bio.bi_end_io = multipath_end_request;
 	mp_bh->bio.bi_private = mp_bh;
 	generic_make_request(&mp_bh->bio);
-	return 0;
+	return;
 }
 
 static void multipath_status (struct seq_file *seq, mddev_t *mddev)
Index: linux-2.6/drivers/md/raid0.c
===================================================================
--- linux-2.6.orig/drivers/md/raid0.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/md/raid0.c	2011-09-08 12:05:58.614775973 +0200
@@ -466,7 +466,7 @@ static inline int is_io_in_chunk_boundar
 	}
 }
 
-static int raid0_make_request(mddev_t *mddev, struct bio *bio)
+static void raid0_make_request(mddev_t *mddev, struct bio *bio)
 {
 	unsigned int chunk_sects;
 	sector_t sector_offset;
@@ -475,7 +475,7 @@ static int raid0_make_request(mddev_t *m
 
 	if (unlikely(bio->bi_rw & REQ_FLUSH)) {
 		md_flush_request(mddev, bio);
-		return 0;
+		return;
 	}
 
 	chunk_sects = mddev->chunk_sectors;
@@ -495,13 +495,10 @@ static int raid0_make_request(mddev_t *m
 		else
 			bp = bio_split(bio, chunk_sects -
 				       sector_div(sector, chunk_sects));
-		if (raid0_make_request(mddev, &bp->bio1))
-			generic_make_request(&bp->bio1);
-		if (raid0_make_request(mddev, &bp->bio2))
-			generic_make_request(&bp->bio2);
-
+		raid0_make_request(mddev, &bp->bio1);
+		raid0_make_request(mddev, &bp->bio2);
 		bio_pair_release(bp);
-		return 0;
+		return;
 	}
 
 	sector_offset = bio->bi_sector;
@@ -511,10 +508,9 @@ static int raid0_make_request(mddev_t *m
 	bio->bi_bdev = tmp_dev->bdev;
 	bio->bi_sector = sector_offset + zone->dev_start +
 		tmp_dev->data_offset;
-	/*
-	 * Let the main block layer submit the IO and resolve recursion:
-	 */
-	return 1;
+
+	generic_make_request(bio);
+	return;
 
 bad_map:
 	printk("md/raid0:%s: make_request bug: can't convert block across chunks"
@@ -523,7 +519,7 @@ bad_map:
 	       (unsigned long long)bio->bi_sector, bio->bi_size >> 10);
 
 	bio_io_error(bio);
-	return 0;
+	return;
 }
 
 static void raid0_status(struct seq_file *seq, mddev_t *mddev)
Index: linux-2.6/drivers/md/raid1.c
===================================================================
--- linux-2.6.orig/drivers/md/raid1.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/md/raid1.c	2011-09-08 12:05:58.614775973 +0200
@@ -785,7 +785,7 @@ do_sync_io:
 	PRINTK("%dB behind alloc failed, doing sync I/O\n", bio->bi_size);
 }
 
-static int make_request(mddev_t *mddev, struct bio * bio)
+static void make_request(mddev_t *mddev, struct bio * bio)
 {
 	conf_t *conf = mddev->private;
 	mirror_info_t *mirror;
@@ -870,7 +870,7 @@ read_again:
 		if (rdisk < 0) {
 			/* couldn't find anywhere to read from */
 			raid_end_bio_io(r1_bio);
-			return 0;
+			return;
 		}
 		mirror = conf->mirrors + rdisk;
 
@@ -928,7 +928,7 @@ read_again:
 			goto read_again;
 		} else
 			generic_make_request(read_bio);
-		return 0;
+		return;
 	}
 
 	/*
@@ -1119,8 +1119,6 @@ read_again:
 
 	if (do_sync || !bitmap || !plugged)
 		md_wakeup_thread(mddev->thread);
-
-	return 0;
 }
 
 static void status(struct seq_file *seq, mddev_t *mddev)
Index: linux-2.6/drivers/md/raid10.c
===================================================================
--- linux-2.6.orig/drivers/md/raid10.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/md/raid10.c	2011-09-08 12:05:58.618773608 +0200
@@ -825,7 +825,7 @@ static void unfreeze_array(conf_t *conf)
 	spin_unlock_irq(&conf->resync_lock);
 }
 
-static int make_request(mddev_t *mddev, struct bio * bio)
+static void make_request(mddev_t *mddev, struct bio * bio)
 {
 	conf_t *conf = mddev->private;
 	mirror_info_t *mirror;
@@ -844,7 +844,7 @@ static int make_request(mddev_t *mddev,
 
 	if (unlikely(bio->bi_rw & REQ_FLUSH)) {
 		md_flush_request(mddev, bio);
-		return 0;
+		return;
 	}
 
 	/* If this request crosses a chunk boundary, we need to
@@ -876,10 +876,8 @@ static int make_request(mddev_t *mddev,
 		conf->nr_waiting++;
 		spin_unlock_irq(&conf->resync_lock);
 
-		if (make_request(mddev, &bp->bio1))
-			generic_make_request(&bp->bio1);
-		if (make_request(mddev, &bp->bio2))
-			generic_make_request(&bp->bio2);
+		make_request(mddev, &bp->bio1);
+		make_request(mddev, &bp->bio2);
 
 		spin_lock_irq(&conf->resync_lock);
 		conf->nr_waiting--;
@@ -887,14 +885,14 @@ static int make_request(mddev_t *mddev,
 		spin_unlock_irq(&conf->resync_lock);
 
 		bio_pair_release(bp);
-		return 0;
+		return;
 	bad_map:
 		printk("md/raid10:%s: make_request bug: can't convert block across chunks"
 		       " or bigger than %dk %llu %d\n", mdname(mddev), chunk_sects/2,
 		       (unsigned long long)bio->bi_sector, bio->bi_size >> 10);
 
 		bio_io_error(bio);
-		return 0;
+		return;
 	}
 
 	md_write_start(mddev, bio);
@@ -937,7 +935,7 @@ read_again:
 		slot = r10_bio->read_slot;
 		if (disk < 0) {
 			raid_end_bio_io(r10_bio);
-			return 0;
+			return;
 		}
 		mirror = conf->mirrors + disk;
 
@@ -985,7 +983,7 @@ read_again:
 			goto read_again;
 		} else
 			generic_make_request(read_bio);
-		return 0;
+		return;
 	}
 
 	/*
@@ -1157,7 +1155,6 @@ retry_write:
 
 	if (do_sync || !mddev->bitmap || !plugged)
 		md_wakeup_thread(mddev->thread);
-	return 0;
 }
 
 static void status(struct seq_file *seq, mddev_t *mddev)
Index: linux-2.6/drivers/md/raid5.c
===================================================================
--- linux-2.6.orig/drivers/md/raid5.c	2011-09-08 12:01:53.502775855 +0200
+++ linux-2.6/drivers/md/raid5.c	2011-09-08 12:05:58.622774752 +0200
@@ -3695,7 +3695,7 @@ static struct stripe_head *__get_priorit
 	return sh;
 }
 
-static int make_request(mddev_t *mddev, struct bio * bi)
+static void make_request(mddev_t *mddev, struct bio * bi)
 {
 	raid5_conf_t *conf = mddev->private;
 	int dd_idx;
@@ -3708,7 +3708,7 @@ static int make_request(mddev_t *mddev,
 
 	if (unlikely(bi->bi_rw & REQ_FLUSH)) {
 		md_flush_request(mddev, bi);
-		return 0;
+		return;
 	}
 
 	md_write_start(mddev, bi);
@@ -3716,7 +3716,7 @@ static int make_request(mddev_t *mddev,
 	if (rw == READ &&
 	     mddev->reshape_position == MaxSector &&
 	     chunk_aligned_read(mddev,bi))
-		return 0;
+		return;
 
 	logical_sector = bi->bi_sector & ~((sector_t)STRIPE_SECTORS-1);
 	last_sector = bi->bi_sector + (bi->bi_size>>9);
@@ -3851,8 +3851,6 @@ static int make_request(mddev_t *mddev,
 
 		bio_endio(bi, 0);
 	}
-
-	return 0;
 }
 
 static sector_t raid5_size(mddev_t *mddev, sector_t sectors, int raid_disks);
Index: linux-2.6/block/blk-core.c
===================================================================
--- linux-2.6.orig/block/blk-core.c	2011-09-08 12:03:25.422774199 +0200
+++ linux-2.6/block/blk-core.c	2011-09-08 13:46:08.165866669 +0200
@@ -1211,7 +1211,7 @@ void init_request_from_bio(struct reques
 	blk_rq_bio_prep(req->q, req, bio);
 }
 
-int __make_request(struct request_queue *q, struct bio *bio)
+void __make_request(struct request_queue *q, struct bio *bio)
 {
 	const bool sync = !!(bio->bi_rw & REQ_SYNC);
 	struct blk_plug *plug;
@@ -1236,7 +1236,7 @@ int __make_request(struct request_queue
 	 * any locks.
 	 */
 	if (attempt_plug_merge(current, q, bio))
-		goto out;
+		return;
 
 	spin_lock_irq(q->queue_lock);
 
@@ -1312,8 +1312,6 @@ get_rq:
 out_unlock:
 		spin_unlock_irq(q->queue_lock);
 	}
-out:
-	return 0;
 }
 EXPORT_SYMBOL_GPL(__make_request);	/* for device mapper only */
 
@@ -1441,112 +1439,85 @@ static inline int bio_check_eod(struct b
 static inline void __generic_make_request(struct bio *bio)
 {
 	struct request_queue *q;
-	sector_t old_sector;
-	int ret, nr_sectors = bio_sectors(bio);
-	dev_t old_dev;
+	int nr_sectors = bio_sectors(bio);
 	int err = -EIO;
+	char b[BDEVNAME_SIZE];
+	struct hd_struct *part;
 
 	might_sleep();
 
 	if (bio_check_eod(bio, nr_sectors))
 		goto end_io;
 
-	/*
-	 * Resolve the mapping until finished. (drivers are
-	 * still free to implement/resolve their own stacking
-	 * by explicitly returning 0)
-	 *
-	 * NOTE: we don't repeat the blk_size check for each new device.
-	 * Stacking drivers are expected to know what they are doing.
-	 */
-	old_sector = -1;
-	old_dev = 0;
-	do {
-		char b[BDEVNAME_SIZE];
-		struct hd_struct *part;
-
-		q = bdev_get_queue(bio->bi_bdev);
-		if (unlikely(!q)) {
-			printk(KERN_ERR
-			       "generic_make_request: Trying to access "
-				"nonexistent block-device %s (%Lu)\n",
-				bdevname(bio->bi_bdev, b),
-				(long long) bio->bi_sector);
-			goto end_io;
-		}
-
-		if (unlikely(!(bio->bi_rw & REQ_DISCARD) &&
-			     nr_sectors > queue_max_hw_sectors(q))) {
-			printk(KERN_ERR "bio too big device %s (%u > %u)\n",
-			       bdevname(bio->bi_bdev, b),
-			       bio_sectors(bio),
-			       queue_max_hw_sectors(q));
-			goto end_io;
-		}
-
-		if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)))
-			goto end_io;
-
-		part = bio->bi_bdev->bd_part;
-		if (should_fail_request(part, bio->bi_size) ||
-		    should_fail_request(&part_to_disk(part)->part0,
-					bio->bi_size))
-			goto end_io;
+	q = bdev_get_queue(bio->bi_bdev);
+	if (unlikely(!q)) {
+		printk(KERN_ERR
+		       "generic_make_request: Trying to access "
+			"nonexistent block-device %s (%Lu)\n",
+			bdevname(bio->bi_bdev, b),
+			(long long) bio->bi_sector);
+		goto end_io;
+	}
 
-		/*
-		 * If this device has partitions, remap block n
-		 * of partition p to block n+start(p) of the disk.
-		 */
-		blk_partition_remap(bio);
+	if (unlikely(!(bio->bi_rw & REQ_DISCARD) &&
+		     nr_sectors > queue_max_hw_sectors(q))) {
+		printk(KERN_ERR "bio too big device %s (%u > %u)\n",
+		       bdevname(bio->bi_bdev, b),
+		       bio_sectors(bio),
+		       queue_max_hw_sectors(q));
+		goto end_io;
+	}
 
-		if (bio_integrity_enabled(bio) && bio_integrity_prep(bio))
-			goto end_io;
+	if (unlikely(test_bit(QUEUE_FLAG_DEAD, &q->queue_flags)))
+		goto end_io;
 
-		if (old_sector != -1)
-			trace_block_bio_remap(q, bio, old_dev, old_sector);
+	part = bio->bi_bdev->bd_part;
+	if (should_fail_request(part, bio->bi_size) ||
+	    should_fail_request(&part_to_disk(part)->part0,
+				bio->bi_size))
+		goto end_io;
 
-		old_sector = bio->bi_sector;
-		old_dev = bio->bi_bdev->bd_dev;
+	/*
+	 * If this device has partitions, remap block n
+	 * of partition p to block n+start(p) of the disk.
+	 */
+	blk_partition_remap(bio);
 
-		if (bio_check_eod(bio, nr_sectors))
-			goto end_io;
+	if (bio_integrity_enabled(bio) && bio_integrity_prep(bio))
+		goto end_io;
 
-		/*
-		 * Filter flush bio's early so that make_request based
-		 * drivers without flush support don't have to worry
-		 * about them.
-		 */
-		if ((bio->bi_rw & (REQ_FLUSH | REQ_FUA)) && !q->flush_flags) {
-			bio->bi_rw &= ~(REQ_FLUSH | REQ_FUA);
-			if (!nr_sectors) {
-				err = 0;
-				goto end_io;
-			}
-		}
+	if (bio_check_eod(bio, nr_sectors))
+		goto end_io;
 
-		if ((bio->bi_rw & REQ_DISCARD) &&
-		    (!blk_queue_discard(q) ||
-		     ((bio->bi_rw & REQ_SECURE) &&
-		      !blk_queue_secdiscard(q)))) {
-			err = -EOPNOTSUPP;
+	/*
+	 * Filter flush bio's early so that make_request based
+	 * drivers without flush support don't have to worry
+	 * about them.
+	 */
+	if ((bio->bi_rw & (REQ_FLUSH | REQ_FUA)) && !q->flush_flags) {
+		bio->bi_rw &= ~(REQ_FLUSH | REQ_FUA);
+		if (!nr_sectors) {
+			err = 0;
 			goto end_io;
 		}
+	}
 
-		if (blk_throtl_bio(q, &bio))
-			goto end_io;
-
-		/*
-		 * If bio = NULL, bio has been throttled and will be submitted
-		 * later.
-		 */
-		if (!bio)
-			break;
-
-		trace_block_bio_queue(q, bio);
+	if ((bio->bi_rw & REQ_DISCARD) &&
+	    (!blk_queue_discard(q) ||
+	     ((bio->bi_rw & REQ_SECURE) &&
+	      !blk_queue_secdiscard(q)))) {
+		err = -EOPNOTSUPP;
+		goto end_io;
+	}
 
-		ret = q->make_request_fn(q, bio);
-	} while (ret);
+	if (blk_throtl_bio(q, &bio))
+		goto end_io;
 
+	/* if bio = NULL, bio has been throttled and will be submitted later. */
+	if (!bio)
+		return;
+	trace_block_bio_queue(q, bio);
+	q->make_request_fn(q, bio);
 	return;
 
 end_io:
Index: linux-2.6/drivers/md/md.h
===================================================================
--- linux-2.6.orig/drivers/md/md.h	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/md/md.h	2011-09-08 12:05:58.630773689 +0200
@@ -424,7 +424,7 @@ struct mdk_personality
 	int level;
 	struct list_head list;
 	struct module *owner;
-	int (*make_request)(mddev_t *mddev, struct bio *bio);
+	void (*make_request)(mddev_t *mddev, struct bio *bio);
 	int (*run)(mddev_t *mddev);
 	int (*stop)(mddev_t *mddev);
 	void (*status)(struct seq_file *seq, mddev_t *mddev);
Index: linux-2.6/drivers/md/md.c
===================================================================
--- linux-2.6.orig/drivers/md/md.c	2011-09-08 12:01:53.000000000 +0200
+++ linux-2.6/drivers/md/md.c	2011-09-08 13:46:08.089865876 +0200
@@ -330,18 +330,17 @@ static DEFINE_SPINLOCK(all_mddevs_lock);
  * call has finished, the bio has been linked into some internal structure
  * and so is visible to ->quiesce(), so we don't need the refcount any more.
  */
-static int md_make_request(struct request_queue *q, struct bio *bio)
+static void md_make_request(struct request_queue *q, struct bio *bio)
 {
 	const int rw = bio_data_dir(bio);
 	mddev_t *mddev = q->queuedata;
-	int rv;
 	int cpu;
 	unsigned int sectors;
 
 	if (mddev == NULL || mddev->pers == NULL
 	    || !mddev->ready) {
 		bio_io_error(bio);
-		return 0;
+		return;
 	}
 	smp_rmb(); /* Ensure implications of  'active' are visible */
 	rcu_read_lock();
@@ -366,7 +365,7 @@ static int md_make_request(struct reques
 	 * go away inside make_request
 	 */
 	sectors = bio_sectors(bio);
-	rv = mddev->pers->make_request(mddev, bio);
+	mddev->pers->make_request(mddev, bio);
 
 	cpu = part_stat_lock();
 	part_stat_inc(cpu, &mddev->gendisk->part0, ios[rw]);
@@ -375,8 +374,6 @@ static int md_make_request(struct reques
 
 	if (atomic_dec_and_test(&mddev->active_io) && mddev->suspended)
 		wake_up(&mddev->sb_wait);
-
-	return rv;
 }
 
 /* mddev_suspend makes sure no new requests are submitted
@@ -475,8 +472,7 @@ static void md_submit_flush_data(struct
 		bio_endio(bio, 0);
 	else {
 		bio->bi_rw &= ~REQ_FLUSH;
-		if (mddev->pers->make_request(mddev, bio))
-			generic_make_request(bio);
+		mddev->pers->make_request(mddev, bio);
 	}
 
 	mddev->flush_bio = NULL;

Re: [PATCH 2/2] block: remove support for bio remapping from ->make_request

[NeilBrown]

On Sun, 11 Sep 2011 10:51:29 -0400 Christoph Hellwig <hch@infradead.org>
wrote:

> There is very little benefit in allowing to let a ->make_request instance update
> the bios device and sector and loop around it in __generic_make_request when
> we can archive the same through calling generic_make_request from the driver
> and letting the loop in generic_make_request handle it.

Completely agree - I like this change!

> 
> Note that various drivers got the return value from ->make_request and returned
> non-zero values for errors.

:-(

A diff-stat would have been nice... I think I've found all 'my' files though.



> Index: linux-2.6/drivers/md/faulty.c
> ===================================================================
> --- linux-2.6.orig/drivers/md/faulty.c	2011-09-08 12:01:53.000000000 +0200
> +++ linux-2.6/drivers/md/faulty.c	2011-09-08 12:05:58.602773802 +0200
> @@ -169,7 +169,7 @@ static void add_sector(conf_t *conf, sec
>  		conf->nfaults = n+1;
>  }
>  
> -static int make_request(mddev_t *mddev, struct bio *bio)
> +static void make_request(mddev_t *mddev, struct bio *bio)
>  {
>  	conf_t *conf = mddev->private;
>  	int failit = 0;
> @@ -181,7 +181,7 @@ static int make_request(mddev_t *mddev,
>  			 * just fail immediately
>  			 */
>  			bio_endio(bio, -EIO);
> -			return 0;
> +			return;
>  		}
>  
>  		if (check_sector(conf, bio->bi_sector, bio->bi_sector+(bio->bi_size>>9),
> @@ -214,12 +214,11 @@ static int make_request(mddev_t *mddev,
>  		b->bi_bdev = conf->rdev->bdev;
>  		b->bi_private = bio;
>  		b->bi_end_io = faulty_fail;
> -		generic_make_request(b);
> -		return 0;
>  	} else {
>  		bio->bi_bdev = conf->rdev->bdev;
> -		return 1;
>  	}
> +
> +	generic_make_request(b);
>  }

If the 'else' branch was taken, we need to generic_make_request(bio).
And 'b' isn't even declared at this level.
So at the end of the 'then' branch, add
        bio = b;
and make the final call
        generic_make_request(bio);


Changes to these files:
> Index: linux-2.6/drivers/md/linear.c
> Index: linux-2.6/drivers/md/multipath.c
> Index: linux-2.6/drivers/md/raid0.c
> Index: linux-2.6/drivers/md/raid1.c
> Index: linux-2.6/drivers/md/raid10.c
> Index: linux-2.6/drivers/md/raid5.c
> Index: linux-2.6/drivers/md/md.h
> Index: linux-2.6/drivers/md/md.c

Acked-by: NeilBrown <neilb@suse.de>

Thanks,
NeilBrown

Re: [PATCH 1/2] block: export __make_request

[Jens Axboe]

On 2011-09-11 16:50, Christoph Hellwig wrote:
> Avoid the hacks need for request based device mappers currently by simply
> exporting the symbol instead of trying to get it through the back door.

Good idea, lets rename it to blk_make_request() at the same time though.
I'll merge it as such.

-- 
Jens Axboe

Re: [PATCH 2/2] block: remove support for bio remapping from ->make_request

[Jens Axboe]

On 2011-09-11 16:51, Christoph Hellwig wrote:
> There is very little benefit in allowing to let a ->make_request
> instance update the bios device and sector and loop around it in
> __generic_make_request when we can archive the same through calling
> generic_make_request from the driver and letting the loop in
> generic_make_request handle it.

Agree, this looks good! Will apply for 3.2.

> Note that various drivers got the return value from ->make_request and
> returned non-zero values for errors.

Hadn't noticed that, I guess the return values are easy to get wrong in
that case. Never a good sign.


-- 
Jens Axboe

Re: [PATCH 1/2] block: export __make_request

[Christoph Hellwig]

On Mon, Sep 12, 2011 at 11:59:12AM +0200, Jens Axboe wrote:
> On 2011-09-11 16:50, Christoph Hellwig wrote:
> > Avoid the hacks need for request based device mappers currently by simply
> > exporting the symbol instead of trying to get it through the back door.
> 
> Good idea, lets rename it to blk_make_request() at the same time though.
> I'll merge it as such.

What happens to the existing blk_make_request?

Re: [PATCH 2/2] block: remove support for bio remapping from ->make_request

[Christoph Hellwig]

On Mon, Sep 12, 2011 at 11:59:52AM +0200, Jens Axboe wrote:
> On 2011-09-11 16:51, Christoph Hellwig wrote:
> > There is very little benefit in allowing to let a ->make_request
> > instance update the bios device and sector and loop around it in
> > __generic_make_request when we can archive the same through calling
> > generic_make_request from the driver and letting the loop in
> > generic_make_request handle it.
> 
> Agree, this looks good! Will apply for 3.2.

Are you going to fix the issue Neil pointed out beforehand?

Re: [PATCH 2/2] block: remove support for bio remapping from ->make_request

[Jens Axboe]

On 2011-09-12 14:25, Christoph Hellwig wrote:
> On Mon, Sep 12, 2011 at 11:59:52AM +0200, Jens Axboe wrote:
>> On 2011-09-11 16:51, Christoph Hellwig wrote:
>>> There is very little benefit in allowing to let a ->make_request
>>> instance update the bios device and sector and loop around it in
>>> __generic_make_request when we can archive the same through calling
>>> generic_make_request from the driver and letting the loop in
>>> generic_make_request handle it.
>>
>> Agree, this looks good! Will apply for 3.2.
> 
> Are you going to fix the issue Neil pointed out beforehand?

Yep, I merged that one in with the 2/2 patch and added his Acked-by as
well.

-- 
Jens Axboe

Re: [PATCH 1/2] block: export __make_request

[Jens Axboe]

On 2011-09-12 14:25, Christoph Hellwig wrote:
> On Mon, Sep 12, 2011 at 11:59:12AM +0200, Jens Axboe wrote:
>> On 2011-09-11 16:50, Christoph Hellwig wrote:
>>> Avoid the hacks need for request based device mappers currently by simply
>>> exporting the symbol instead of trying to get it through the back door.
>>
>> Good idea, lets rename it to blk_make_request() at the same time though.
>> I'll merge it as such.
> 
> What happens to the existing blk_make_request?

I ended up naming __make_request() blk_queue_bio() instead.

-- 
Jens Axboe

Re: [PATCH 1/2] block: export __make_request

[Christoph Hellwig]

On Mon, Sep 12, 2011 at 02:26:52PM +0200, Jens Axboe wrote:
> > What happens to the existing blk_make_request?
> 
> I ended up naming __make_request() blk_queue_bio() instead.

I really hate naming things different from the method they are
implementing.  I've tried to figure out what the point of the
old blk_make_request is - why would we not go through
generic_make_request for this?

Boaz, any idea?

[PATCH 3/2] block: refactor generic_make_request

[Christoph Hellwig]

Move all the checks performed on a bio into a new helper, and call it as
soon as bio is submitted even if it is a re-submission from ->make_request.

We explicitly mark the new helper as beeing non-inlined as the stack
usage for printing the block device name in the failure case is quite
high and this a patch where we have to be extremely conservative about
stack usage.

Signed-off-by: Christoph Hellwig <hch@lst.de>

Index: linux-2.6/block/blk-core.c
===================================================================
--- linux-2.6.orig/block/blk-core.c	2011-09-08 12:07:09.943065855 +0200
+++ linux-2.6/block/blk-core.c	2011-09-08 12:08:23.463277870 +0200
@@ -1412,31 +1412,8 @@ static inline int bio_check_eod(struct b
 	return 0;
 }
 
-/**
- * generic_make_request - hand a buffer to its device driver for I/O
- * @bio:  The bio describing the location in memory and on the device.
- *
- * generic_make_request() is used to make I/O requests of block
- * devices. It is passed a &struct bio, which describes the I/O that needs
- * to be done.
- *
- * generic_make_request() does not return any status.  The
- * success/failure status of the request, along with notification of
- * completion, is delivered asynchronously through the bio->bi_end_io
- * function described (one day) else where.
- *
- * The caller of generic_make_request must make sure that bi_io_vec
- * are set to describe the memory buffer, and that bi_dev and bi_sector are
- * set to describe the device address, and the
- * bi_end_io and optionally bi_private are set to describe how
- * completion notification should be signaled.
- *
- * generic_make_request and the drivers it calls may use bi_next if this
- * bio happens to be merged with someone else, and may change bi_dev and
- * bi_sector for remaps as it sees fit.  So the values of these fields
- * should NOT be depended on after the call to generic_make_request.
- */
-static inline void __generic_make_request(struct bio *bio)
+static noinline_for_stack bool
+generic_make_request_checks(struct bio *bio)
 {
 	struct request_queue *q;
 	int nr_sectors = bio_sectors(bio);
@@ -1515,35 +1492,62 @@ static inline void __generic_make_reques
 
 	/* if bio = NULL, bio has been throttled and will be submitted later. */
 	if (!bio)
-		return;
+		return false;
+
 	trace_block_bio_queue(q, bio);
-	q->make_request_fn(q, bio);
-	return;
+	return true;
 
 end_io:
 	bio_endio(bio, err);
+	return false;
 }
 
-/*
- * We only want one ->make_request_fn to be active at a time,
- * else stack usage with stacked devices could be a problem.
- * So use current->bio_list to keep a list of requests
- * submited by a make_request_fn function.
- * current->bio_list is also used as a flag to say if
- * generic_make_request is currently active in this task or not.
- * If it is NULL, then no make_request is active.  If it is non-NULL,
- * then a make_request is active, and new requests should be added
- * at the tail
+/**
+ * generic_make_request - hand a buffer to its device driver for I/O
+ * @bio:  The bio describing the location in memory and on the device.
+ *
+ * generic_make_request() is used to make I/O requests of block
+ * devices. It is passed a &struct bio, which describes the I/O that needs
+ * to be done.
+ *
+ * generic_make_request() does not return any status.  The
+ * success/failure status of the request, along with notification of
+ * completion, is delivered asynchronously through the bio->bi_end_io
+ * function described (one day) else where.
+ *
+ * The caller of generic_make_request must make sure that bi_io_vec
+ * are set to describe the memory buffer, and that bi_dev and bi_sector are
+ * set to describe the device address, and the
+ * bi_end_io and optionally bi_private are set to describe how
+ * completion notification should be signaled.
+ *
+ * generic_make_request and the drivers it calls may use bi_next if this
+ * bio happens to be merged with someone else, and may resubmit the bio to
+ * a lower device by calling into generic_make_request recursively, which
+ * means the bio should NOT be touched after the call to ->make_request_fn.
  */
 void generic_make_request(struct bio *bio)
 {
 	struct bio_list bio_list_on_stack;
 
+	if (!generic_make_request_checks(bio))
+		return;
+
+	/*
+	 * We only want one ->make_request_fn to be active at a time, else
+	 * stack usage with stacked devices could be a problem.  So use
+	 * current->bio_list to keep a list of requests submited by a
+	 * make_request_fn function.  current->bio_list is also used as a
+	 * flag to say if generic_make_request is currently active in this
+	 * task or not.  If it is NULL, then no make_request is active.  If
+	 * it is non-NULL, then a make_request is active, and new requests
+	 * should be added at the tail
+	 */
 	if (current->bio_list) {
-		/* make_request is active */
 		bio_list_add(current->bio_list, bio);
 		return;
 	}
+
 	/* following loop may be a bit non-obvious, and so deserves some
 	 * explanation.
 	 * Before entering the loop, bio->bi_next is NULL (as all callers
@@ -1557,16 +1561,15 @@ void generic_make_request(struct bio *bi
 	 * from the top.  In this case we really did just take the bio
 	 * of the top of the list (no pretending) and so remove it from
 	 * bio_list, and call into __generic_make_request again.
-	 *
-	 * The loop was structured like this to make only one call to
-	 * __generic_make_request (which is important as it is large and
-	 * inlined) and to keep the structure simple.
 	 */
 	BUG_ON(bio->bi_next);
 	bio_list_init(&bio_list_on_stack);
 	current->bio_list = &bio_list_on_stack;
 	do {
-		__generic_make_request(bio);
+		struct request_queue *q = bdev_get_queue(bio->bi_bdev);
+
+		q->make_request_fn(q, bio);
+
 		bio = bio_list_pop(current->bio_list);
 	} while (bio);
 	current->bio_list = NULL; /* deactivate */

Re: [PATCH 3/2] block: refactor generic_make_request

[Vivek Goyal]

On Mon, Sep 12, 2011 at 09:42:02AM -0400, Christoph Hellwig wrote:

[..]
> +
>  	/* following loop may be a bit non-obvious, and so deserves some
>  	 * explanation.
>  	 * Before entering the loop, bio->bi_next is NULL (as all callers
> @@ -1557,16 +1561,15 @@ void generic_make_request(struct bio *bi
>  	 * from the top.  In this case we really did just take the bio
>  	 * of the top of the list (no pretending) and so remove it from
>  	 * bio_list, and call into __generic_make_request again.
                                   ^^^^^^^^^^
You got rid of __generic_make_request(). Now above comment has stale
reference to it.

Thanks
Vivek

Re: [PATCH 3/2] block: refactor generic_make_request

[Christoph Hellwig]

On Mon, Sep 12, 2011 at 11:09:35AM -0400, Vivek Goyal wrote:
> On Mon, Sep 12, 2011 at 09:42:02AM -0400, Christoph Hellwig wrote:
> 
> [..]
> > +
> >  	/* following loop may be a bit non-obvious, and so deserves some
> >  	 * explanation.
> >  	 * Before entering the loop, bio->bi_next is NULL (as all callers
> > @@ -1557,16 +1561,15 @@ void generic_make_request(struct bio *bi
> >  	 * from the top.  In this case we really did just take the bio
> >  	 * of the top of the list (no pretending) and so remove it from
> >  	 * bio_list, and call into __generic_make_request again.
>                                    ^^^^^^^^^^
> You got rid of __generic_make_request(). Now above comment has stale
> reference to it.

Yeah, this should be ->make_request now.

Re: [PATCH 1/2] block: export __make_request

[Jens Axboe]

On 2011-09-12 15:38, Christoph Hellwig wrote:
> On Mon, Sep 12, 2011 at 02:26:52PM +0200, Jens Axboe wrote:
>>> What happens to the existing blk_make_request?
>>
>> I ended up naming __make_request() blk_queue_bio() instead.
> 
> I really hate naming things different from the method they are
> implementing.  I've tried to figure out what the point of the
> old blk_make_request is - why would we not go through
> generic_make_request for this?
> 
> Boaz, any idea?

I tend to agree, we could rename the existing blk_make_request(). It
could be blk_make_request_from_bio() or something like that, since
that's what it does.

-- 
Jens Axboe

Re: [PATCH 1/2] block: export __make_request

[Christoph Hellwig]

On Mon, Sep 12, 2011 at 08:04:46PM +0200, Jens Axboe wrote:
> > I really hate naming things different from the method they are
> > implementing.  I've tried to figure out what the point of the
> > old blk_make_request is - why would we not go through
> > generic_make_request for this?
> > 
> > Boaz, any idea?
> 
> I tend to agree, we could rename the existing blk_make_request(). It
> could be blk_make_request_from_bio() or something like that, since
> that's what it does.

It should at very least be renamed.  But I still can't figure out what
it is for exactly.

There are three users:

 (1) virtio_blk::virtblk_get_id():
	This looks like it really should just use blk_rq_map_kern.
 (2) osd_initiator::_make_request():
	This one looks like it should just use the same scheme as
	sg_io(), as it's doing the same thing.
 (3) target_core_pscsi::__pscsi_map_SG():
	Same as (2).

Re: [PATCH 1/2] block: export __make_request

[Boaz Harrosh]

On 9/14/2011 12:19 AM, Christoph Hellwig wrote:
> On Mon, Sep 12, 2011 at 08:04:46PM +0200, Jens Axboe wrote:
>>> I really hate naming things different from the method they are
>>> implementing.  I've tried to figure out what the point of the
>>> old blk_make_request is - why would we not go through
>>> generic_make_request for this?
>>>
>>> Boaz, any idea?
>>
>> I tend to agree, we could rename the existing blk_make_request(). It
>> could be blk_make_request_from_bio() or something like that, since
>> that's what it does.
>
> It should at very least be renamed.  But I still can't figure out what
> it is for exactly.
>
> There are three users:
>
>   (1) virtio_blk::virtblk_get_id():
> 	This looks like it really should just use blk_rq_map_kern.
>   (2) osd_initiator::_make_request():
> 	This one looks like it should just use the same scheme as
> 	sg_io(), as it's doing the same thing.

Good god what sg_io? That broken pointr+length from user-mode that sg.c
and bsg.c are using? no can do it's not user-mode pointers, and it's not
pointer+length it's pages pointers of a bio. The only other structure
that could carry the same information is struct sg, but we work very 
hard to get rid of this contraption. (scsi_execute_async or something 
that it was)

blk_make_request() was made to be the parallel of __make_request, to be
used from filesystem level users. But with two differences.
1. Mainly support for none-FS BLOCK_PC requests
2. Also support chained bios. (was added later)

>   (3) target_core_pscsi::__pscsi_map_SG():
> 	Same as (2).
>

There is no better suitable structure in current Kernel to carry a list
of pages, with optional offset and length, then bio struct. Given a bio
at hand. how do you make a block request out of it? (If it's not an 
FS_PC type IO?)

As I remember target_core had their own pages-linked-list structure, and
how do you make a request out of that? again best at hand is bio.

bio is for a long time a page-pointers-carrier-structure and is out of 
private block-level use. The filesystem level is full of it.

Thanks
Boaz

Re: [PATCH 1/2] block: export __make_request

[Boaz Harrosh]

On 9/14/2011 12:19 AM, Christoph Hellwig wrote:
> On Mon, Sep 12, 2011 at 08:04:46PM +0200, Jens Axboe wrote:
>>> I really hate naming things different from the method they are
>>> implementing.  I've tried to figure out what the point of the
>>> old blk_make_request is - why would we not go through
>>> generic_make_request for this?
>>>
>>> Boaz, any idea?
>>
>> I tend to agree, we could rename the existing blk_make_request(). It
>> could be blk_make_request_from_bio() or something like that, since
>> that's what it does.
>
> It should at very least be renamed.  But I still can't figure out what
> it is for exactly.
>
> There are three users:
>
>   (1) virtio_blk::virtblk_get_id():
> 	This looks like it really should just use blk_rq_map_kern.
>   (2) osd_initiator::_make_request():
> 	This one looks like it should just use the same scheme as
> 	sg_io(), as it's doing the same thing.

Good god what sg_io? That broken pointr+length from user-mode that sg.c
and bsg.c are using? no can do it's not user-mode pointers, and it's not
pointer+length it's pages pointers of a bio. The only other structure
that could carry the same information is struct sg, but we work very 
hard to get rid of this contraption. (scsi_execute_async or something 
that it was)

blk_make_request() was made to be the parallel of __make_request, to be
used from filesystem level users. But with two differences.
1. Mainly support for none-FS BLOCK_PC requests
2. Also support chained bios. (was added later)

>   (3) target_core_pscsi::__pscsi_map_SG():
> 	Same as (2).
>

There is no better suitable structure in current Kernel to carry a list
of pages, with optional offset and length, then bio struct. Given a bio
at hand. how do you make a block request out of it? (If it's not an 
FS_PC type IO?)

As I remember target_core had their own pages-linked-list structure, and
how do you make a request out of that? again best at hand is bio.

bio is for a long time a page-pointers-carrier-structure and is out of 
private block-level use. The filesystem level is full of it.

Thanks
Boaz

Re: [PATCH 1/2] block: export __make_request

[Doug Dumitru]

It would be great if this function were exported, but I would
encourage everyone to consider exporting it generally, not just for
GPL usage.

The kernel seems to be pretty clean that some subsystems are GPL only
(things like sysfs), and some are open (things like the device-mapper
interfaces).  When a subsystem is part GPL and part open, it gets very
confusing for developers of commercial modules to know what to use and
what not to use.

The goal of GPL versus commercial exports should be to encourage GPL
code, not to punish commercial code by requiring stupid work arounds
that only waste time and hurt performance.

Again, most of the block IO layer is plain exports.  Adding random GPL
exports seems somewhat arbitrary.

Then again, this is your code, and If you believe it should be GPL
linkable only, then that is fine.  Just consider that there are
projects out there that are strange enough to be impacted and some
degree of symmetry is desirable.

As a side node, I would vote for longer, more descriptive names.
"__anything" should never be exported.

Doug Dumitru
EasyCo LLC



On Wed, Sep 14, 2011 at 10:17 AM, Boaz Harrosh <bharrosh@panasas.com> wrote:
> On 9/14/2011 12:19 AM, Christoph Hellwig wrote:
>>
>> On Mon, Sep 12, 2011 at 08:04:46PM +0200, Jens Axboe wrote:
>>>>
>>>> I really hate naming things different from the method they are
>>>> implementing.  I've tried to figure out what the point of the
>>>> old blk_make_request is - why would we not go through
>>>> generic_make_request for this?
>>>>
>>>> Boaz, any idea?
>>>
>>> I tend to agree, we could rename the existing blk_make_request(). It
>>> could be blk_make_request_from_bio() or something like that, since
>>> that's what it does.
>>
>> It should at very least be renamed.  But I still can't figure out what
>> it is for exactly.
>>
>> There are three users:
>>
>>  (1) virtio_blk::virtblk_get_id():
>>        This looks like it really should just use blk_rq_map_kern.
>>  (2) osd_initiator::_make_request():
>>        This one looks like it should just use the same scheme as
>>        sg_io(), as it's doing the same thing.
>
> Good god what sg_io? That broken pointr+length from user-mode that sg.c
> and bsg.c are using? no can do it's not user-mode pointers, and it's not
> pointer+length it's pages pointers of a bio. The only other structure
> that could carry the same information is struct sg, but we work very hard to
> get rid of this contraption. (scsi_execute_async or something that it was)
>
> blk_make_request() was made to be the parallel of __make_request, to be
> used from filesystem level users. But with two differences.
> 1. Mainly support for none-FS BLOCK_PC requests
> 2. Also support chained bios. (was added later)
>
>>  (3) target_core_pscsi::__pscsi_map_SG():
>>        Same as (2).
>>
>
> There is no better suitable structure in current Kernel to carry a list
> of pages, with optional offset and length, then bio struct. Given a bio
> at hand. how do you make a block request out of it? (If it's not an FS_PC
> type IO?)
>
> As I remember target_core had their own pages-linked-list structure, and
> how do you make a request out of that? again best at hand is bio.
>
> bio is for a long time a page-pointers-carrier-structure and is out of
> private block-level use. The filesystem level is full of it.
>
> Thanks
> Boaz
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Doug Dumitru
EasyCo LLC
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] block: export __make_request

[Doug Dumitru]

It would be great if this function were exported, but I would
encourage everyone to consider exporting it generally, not just for
GPL usage.

The kernel seems to be pretty clean that some subsystems are GPL only
(things like sysfs), and some are open (things like the device-mapper
interfaces).  When a subsystem is part GPL and part open, it gets very
confusing for developers of commercial modules to know what to use and
what not to use.

The goal of GPL versus commercial exports should be to encourage GPL
code, not to punish commercial code by requiring stupid work arounds
that only waste time and hurt performance.

Again, most of the block IO layer is plain exports.  Adding random GPL
exports seems somewhat arbitrary.

Then again, this is your code, and If you believe it should be GPL
linkable only, then that is fine.  Just consider that there are
projects out there that are strange enough to be impacted and some
degree of symmetry is desirable.

As a side node, I would vote for longer, more descriptive names.
"__anything" should never be exported.

Doug Dumitru
EasyCo LLC



On Wed, Sep 14, 2011 at 10:17 AM, Boaz Harrosh <bharrosh@panasas.com> wrote:
> On 9/14/2011 12:19 AM, Christoph Hellwig wrote:
>>
>> On Mon, Sep 12, 2011 at 08:04:46PM +0200, Jens Axboe wrote:
>>>>
>>>> I really hate naming things different from the method they are
>>>> implementing.  I've tried to figure out what the point of the
>>>> old blk_make_request is - why would we not go through
>>>> generic_make_request for this?
>>>>
>>>> Boaz, any idea?
>>>
>>> I tend to agree, we could rename the existing blk_make_request(). It
>>> could be blk_make_request_from_bio() or something like that, since
>>> that's what it does.
>>
>> It should at very least be renamed.  But I still can't figure out what
>> it is for exactly.
>>
>> There are three users:
>>
>>  (1) virtio_blk::virtblk_get_id():
>>        This looks like it really should just use blk_rq_map_kern.
>>  (2) osd_initiator::_make_request():
>>        This one looks like it should just use the same scheme as
>>        sg_io(), as it's doing the same thing.
>
> Good god what sg_io? That broken pointr+length from user-mode that sg.c
> and bsg.c are using? no can do it's not user-mode pointers, and it's not
> pointer+length it's pages pointers of a bio. The only other structure
> that could carry the same information is struct sg, but we work very hard to
> get rid of this contraption. (scsi_execute_async or something that it was)
>
> blk_make_request() was made to be the parallel of __make_request, to be
> used from filesystem level users. But with two differences.
> 1. Mainly support for none-FS BLOCK_PC requests
> 2. Also support chained bios. (was added later)
>
>>  (3) target_core_pscsi::__pscsi_map_SG():
>>        Same as (2).
>>
>
> There is no better suitable structure in current Kernel to carry a list
> of pages, with optional offset and length, then bio struct. Given a bio
> at hand. how do you make a block request out of it? (If it's not an FS_PC
> type IO?)
>
> As I remember target_core had their own pages-linked-list structure, and
> how do you make a request out of that? again best at hand is bio.
>
> bio is for a long time a page-pointers-carrier-structure and is out of
> private block-level use. The filesystem level is full of it.
>
> Thanks
> Boaz
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-raid" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Doug Dumitru
EasyCo LLC

Re: [PATCH 1/2] block: export __make_request

[Alan Cox]

On Wed, 14 Sep 2011 10:53:22 -0700
Doug Dumitru <doug@easyco.com> wrote:

> It would be great if this function were exported, but I would
> encourage everyone to consider exporting it generally, not just for
> GPL usage.

That would require a licensing change for the kernel. The kernel is a GPL
work. Some of us have not licensed it in any other way.

> 
> The kernel seems to be pretty clean that some subsystems are GPL only
> (things like sysfs), and some are open 

The _GPL is meant to make it clear they are internal symbols. The lack of
an _GPL does not make them usable by non GPL code, it merely indicates
that its perhaps more likely that you could in some cases shown something
was not a derivative work.

The licensing is determined by the license document, and that very
clearly says the boundary is derivative works. The rest is just vague
guidance.

In addition btw please be careful when trying to do this even with non
derivative code your lawyer is happy with. The kernel contains algorithms
which are subject to various US software patent mess where GPL use has
been granted, that may impact you if you use such code indirectly in non
GPL code.

Alan

Re: [PATCH 1/2] block: export __make_request

[Nicholas A. Bellinger]

On Wed, 2011-09-14 at 20:17 +0300, Boaz Harrosh wrote:
> On 9/14/2011 12:19 AM, Christoph Hellwig wrote:
> > On Mon, Sep 12, 2011 at 08:04:46PM +0200, Jens Axboe wrote:
> >>> I really hate naming things different from the method they are
> >>> implementing.  I've tried to figure out what the point of the
> >>> old blk_make_request is - why would we not go through
> >>> generic_make_request for this?
> >>>
> >>> Boaz, any idea?
> >>
> >> I tend to agree, we could rename the existing blk_make_request(). It
> >> could be blk_make_request_from_bio() or something like that, since
> >> that's what it does.
> >
> > It should at very least be renamed.  But I still can't figure out what
> > it is for exactly.
> >
> > There are three users:
> >
> >   (1) virtio_blk::virtblk_get_id():
> > 	This looks like it really should just use blk_rq_map_kern.
> >   (2) osd_initiator::_make_request():
> > 	This one looks like it should just use the same scheme as
> > 	sg_io(), as it's doing the same thing.
> 
> Good god what sg_io? That broken pointr+length from user-mode that sg.c
> and bsg.c are using? no can do it's not user-mode pointers, and it's not
> pointer+length it's pages pointers of a bio. The only other structure
> that could carry the same information is struct sg, but we work very 
> hard to get rid of this contraption. (scsi_execute_async or something 
> that it was)
> 
> blk_make_request() was made to be the parallel of __make_request, to be
> used from filesystem level users. But with two differences.
> 1. Mainly support for none-FS BLOCK_PC requests
> 2. Also support chained bios. (was added later)
> 
> >   (3) target_core_pscsi::__pscsi_map_SG():
> > 	Same as (2).
> >
> 
> There is no better suitable structure in current Kernel to carry a list
> of pages, with optional offset and length, then bio struct. Given a bio
> at hand. how do you make a block request out of it? (If it's not an 
> FS_PC type IO?)
> 
> As I remember target_core had their own pages-linked-list structure, and
> how do you make a request out of that? again best at hand is bio.
> 

FYI, for v3.1-rc code, target core has been converted to use native
struct scatterlist for everything, and the legacy struct se_mem that
existed before the advent of modern >= .24 SGL linking, et al. has been
completely removed.

--nab

Re: [PATCH 1/2] block: export __make_request

[Doug Dumitru]

On Wed, Sep 14, 2011 at 11:40 AM, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Wed, 14 Sep 2011 10:53:22 -0700
> Doug Dumitru <doug@easyco.com> wrote:
>
>> It would be great if this function were exported, but I would
>> encourage everyone to consider exporting it generally, not just for
>> GPL usage.
>
> That would require a licensing change for the kernel. The kernel is a GPL
> work. Some of us have not licensed it in any other way.

Alan,

I agree with you, in part.  What you are saying implies that no new
EXPORT can ever happen.  If you wrote this function, then obviously,
you can choose to export it GPL only.  Likewise, if you wrote this
function, considering it's location in the block IO layer, I would ask
that you consider exporting it generally.

This does not change the GPL nature of the kernel.  It does change how
non GPL modules can interact with the kernel.  There are some (and I
am not sure if this includes you), that believe that all commercial
license kernel modules violate the GPL.  Other believe that they are
legal, but perhaps not desirable.  Others believe there is no issue.

Again, my request was just a polite request to consider the sub-system
when deciding on whether an EXPORT or EXPORT_GPL was more appropriate.

>> The kernel seems to be pretty clean that some subsystems are GPL only
>> (things like sysfs), and some are open
>
> The _GPL is meant to make it clear they are internal symbols. The lack of
> an _GPL does not make them usable by non GPL code, it merely indicates
> that its perhaps more likely that you could in some cases shown something
> was not a derivative work.

I don't think this is true, at least from a practical point.  If you
develop a commercial module, you can link to the kernel through public
symbols.  These symbols, and the kernels header files, define a public
API that is usable from commercial modules.  Provided that the
techniques used inside of the commercial module were not "derived
from" the rest of the kernel, using the API is acceptable.

When a symbol is not exported publicly, then, pretty much by
definition, it is not a part of the public API.  This creates kernel
functionality that is unreachable from a commercial module, regardless
of whether the commercial module is derived or not.  As such, the
EXPORT_GPL has the effect of being even more stringent than the GPL
license itself.  With an EXPORT_GPL, the author is saying that this
API is not available to commercial modules even if the commercial
software is not a derivative work.

> The licensing is determined by the license document, and that very
> clearly says the boundary is derivative works. The rest is just vague
> guidance.

I am very careful about what is a derivative work.  I am also careful
about honoring author's "desires" as to how stuff gets exported.
Right now, the block IO layer has a pretty clean, published, API with
adequate public symbols to get real work done, either from GPL or
commercial licensed modules.  If useful functions are walled off,
then this does not change the derivative nature, but just degrades the
usefulness of the kernel.  This would be like exporting kmalloc() as
an open symbol, but exporting kfree() as GPL only.

I guess another way of saying this is that if you are creating a new
entry into an existing, published API, and the API keeps caller/called
at "arms length" (which the block IO layer does), then it is possible
to create a work that uses the API but is not a derivative, and
hopefully the new symbol will be declared accessible to everyone.

> In addition btw please be careful when trying to do this even with non
> derivative code your lawyer is happy with. The kernel contains algorithms
> which are subject to various US software patent mess where GPL use has
> been granted, that may impact you if you use such code indirectly in non
> GPL code.

Understood, but, in all fairness, the US patent mess is far worse and
I suspect that breathing violates someones claim.

> Alan

Again, I was not trying to create a contentious issue.  I was just
asking that new exports consider what is around them, and try to
maintain some consistency of GPL vs open exports.  A function that
submits a block IO request seems to be a good candidate for an open
API which implies an open export.

-- 
Doug Dumitru
EasyCo LLC

Re: [PATCH 1/2] block: export __make_request

[Alan Cox]

> I agree with you, in part.  What you are saying implies that no new
> EXPORT can ever happen.  If you wrote this function, then obviously,

No.

> you can choose to export it GPL only.  Likewise, if you wrote this
> function, considering it's location in the block IO layer, I would ask
> that you consider exporting it generally.

The kernel is GPL, any work that is derivative is subject to the licence,
any work which is not is generally not subject to the licence.

> I don't think this is true, at least from a practical point.  If you
> develop a commercial module, you can link to the kernel through public
> symbols.  These symbols, and the kernels header files, define a public

You can create commercial modules but their licence must be GPL
compatible including source etc if they are derivative in any way. At
least that is what my lawyer advises me.

Possibly you mean "proprietary" here. The GPL welcomes people making
money out of things, it just rejects theft of freedoms given by other
parties.

> API that is usable from commercial modules.  Provided that the
> techniques used inside of the commercial module were not "derived
> from" the rest of the kernel, using the API is acceptable.

I'm not sure derivative works law is quite so clear cut, but then
'provide a clear concise definition of derivative works' appears to be
the legal version of The Goldbach Conjecture.

> When a symbol is not exported publicly, then, pretty much by
> definition, it is not a part of the public API.  This creates kernel

Not the view of the legal advice I have

> Again, I was not trying to create a contentious issue.  I was just
> asking that new exports consider what is around them, and try to
> maintain some consistency of GPL vs open exports.  A function that
> submits a block IO request seems to be a good candidate for an open
> API which implies an open export.

It's a GPL kernel, so to me the question is really moot.

I'm not trying to create contention. The reason I reply now and then to
these kinds of discussions is that I've been advised by my lawyer to do
so, because anyone who then produces a violating module who has seen this
message either as a recipient or if it turns up in a trawl can them be
subject to triple damages for wilful infringement - should it turn out
that the work is violating and they do not in turn have a good legal
assessment with which to show they did in fact take good care (and which
of course might well happen because derivative works are such a grey
fuzzy mess)

Whether it says EXPORT_SYMBOL, EXPORT_SYMBOL_GPL or RUBBERIZED_BANANA is
to my understanding irrelevant. If it does turn out to be relevant
however you are correct I'd make all my symbols _GPL....

Alan

Re: [PATCH 1/2] block: export __make_request

[Doug Dumitru]

Alan,

I appreciate your position and thank you for your continuing work on the Kernel.

Doug Dumitru
EasyCo LLC

Re: [PATCH 1/2] block: export __make_request

[Bernd Petrovitsch]

Hi!

On Mit, 2011-09-14 at 14:34 -0700, Doug Dumitru wrote:
[....]
> This does not change the GPL nature of the kernel.  It does change how
> non GPL modules can interact with the kernel.  There are some (and I
> am not sure if this includes you), that believe that all commercial
> license kernel modules violate the GPL.  Other believe that they are

You wrote "commercial" but actually meant "proprietary".

And yes, there are fully GPLv2 modules out there which are
"commercial" (whatever that actually means) - even in the sense that
people and companies live (also) directly from it.

You may rewrite the whole mail with s/commercial/proprietary/. It is
then (more) correct and IMHO very probably also more clear where you are
heading.
And please don't mix license issues (what can I do with it and what not)
with commercial issues (how and how much money can I make out of it) -
these are two totally different and independent things.

[...]
> entry into an existing, published API, and the API keeps caller/called

Is there a "published API" apart from the pragmatic one: look into
the .h (and .c) files and find it but beware, it may change with the
next release?
I don't think so. And no, generated (or other) documentation does not
change that.

> at "arms length" (which the block IO layer does), then it is possible
> to create a work that uses the API but is not a derivative, and
> hopefully the new symbol will be declared accessible to everyone.

That's the big wish of the proprietary folks - have an in-kernel API
which allows proprietary kernel modules.
Google for it to find lots of discussions about this topic and opinions
on what's OK and what's not OK.

[...]
> maintain some consistency of GPL vs open exports.  A function that
> submits a block IO request seems to be a good candidate for an open
> API which implies an open export. 

That has IMHO next to nothing to do if the module is a derivative work
of the kernel or not if you are a lawyer.
And there are several companies which play this game so you might want
to look how they try to handle that.

	Bernd
-- 
Bernd Petrovitsch                  Email : bernd@petrovitsch.priv.at
                     LUGA : http://www.luga.at

Re: [PATCH 3/2] block: refactor generic_make_request

[Christoph Hellwig]

On Mon, Sep 12, 2011 at 11:13:10AM -0400, Christoph Hellwig wrote:
> > > @@ -1557,16 +1561,15 @@ void generic_make_request(struct bio *bi
> > >  	 * from the top.  In this case we really did just take the bio
> > >  	 * of the top of the list (no pretending) and so remove it from
> > >  	 * bio_list, and call into __generic_make_request again.
> >                                    ^^^^^^^^^^
> > You got rid of __generic_make_request(). Now above comment has stale
> > reference to it.
> 
> Yeah, this should be ->make_request now.

Jens, do you want me to resend with that fix or can you fix it up when
applying?

Re: [PATCH 3/2] block: refactor generic_make_request

[Jens Axboe]

On 2011-09-15 13:55, Christoph Hellwig wrote:
> On Mon, Sep 12, 2011 at 11:13:10AM -0400, Christoph Hellwig wrote:
>>>> @@ -1557,16 +1561,15 @@ void generic_make_request(struct bio *bi
>>>>  	 * from the top.  In this case we really did just take the bio
>>>>  	 * of the top of the list (no pretending) and so remove it from
>>>>  	 * bio_list, and call into __generic_make_request again.
>>>                                    ^^^^^^^^^^
>>> You got rid of __generic_make_request(). Now above comment has stale
>>> reference to it.
>>
>> Yeah, this should be ->make_request now.
> 
> Jens, do you want me to resend with that fix or can you fix it up when
> applying?

I'll just fix it up, applying it now.

-- 
Jens Axboe