allow bio code to unmap sg io requests and have blk_execute_rq_nowait bounce bios

RFC PATCH In review Mike Christie 1 participant 3 messages Jul 29, 2005 – Aug 6, 2005 1 message not in the archive

Subsystems (4)

VFS SCSI Block layer Uniform cdrom driver

lore

Share & tools

lore

Mike Christie

author

2005-07-29 09:20 UTC #0 lore raw

RFC PATCH 1/1 allow bio code to unmap sg io requests and have blk_execute_rq_nowait bounce bios

Hey Jens and James,

The inlined patch moves the bounce buffer handling to blk_execute_rq_nowait so the scsi, sg io and cdrom code does not have to handle it. To accomplish this I moved the bio_uncopy_user to a bi_end_io function and bio_unmap_user to a work struct that is schedule from a bi_end_io functions. Did you say you disliked the idea of calling bio_unmap_user from a work struct - don't remember and I lost my emails when I moved? :(

In my patch there is the possiblity of pages being marked dirty after the request has been returned to userspace instead of the pages being dirtied before in the bio_unmap_user. Is this OK?

And, as I work on the adding of BLKERR_* error values in replacement of the dm-multupath/bio sense patch, I was thinking about your comment about having one true make_request function. It seems like if we extended bios to handle more request stuff (like what is needed for SG IO) we could just pass the bio to __make_request - with some modifications to __make_request - instead of doing it request based and moving the blk_queue_bounce call to blk_execute_rq_nowait like I did in my patch. Is this what you were thinking when adding the bio sense code?

Patch was made against James scsi-block-2.6 tree.

Signed-off-by Mike Christie <[email protected]>

drivers/block/ll_rw_blk.c +3 ~~-25~~

@@ -2193,31 +2193,6 @@ int blk_rq_map_user_iov(request_queue_t 
 EXPORT_SYMBOL(blk_rq_map_user_iov);
 
 /**
- * blk_rq_unmap_user - unmap a request with user data
- * @rq:		request to be unmapped
- * @bio:	bio for the request
- * @ulen:	length of user buffer
- *
- * Description:
- *    Unmap a request previously mapped by blk_rq_map_user().
- */
-int blk_rq_unmap_user(struct bio *bio, unsigned int ulen)
-{
-	int ret = 0;
-
-	if (bio) {
-		if (bio_flagged(bio, BIO_USER_MAPPED))
-			bio_unmap_user(bio);
-		else
-			ret = bio_uncopy_user(bio);
-	}
-
-	return 0;
-}
-
-EXPORT_SYMBOL(blk_rq_unmap_user);
-
-/**
  * blk_rq_map_kern - map kernel data to a request, for REQ_BLOCK_PC usage
  * @q:		request queue where request should be inserted
  * @rw:		READ or WRITE data

@@ -2257,6 +2232,9 @@ void blk_execute_rq_nowait(request_queue
 {
 	int where = at_head ? ELEVATOR_INSERT_FRONT : ELEVATOR_INSERT_BACK;
 
+        if (rq->bio)
+                blk_queue_bounce(q, &rq->bio);
+
 	rq->rq_disk = bd_disk;
 	rq->flags |= REQ_NOMERGE;
 	rq->end_io = done;

drivers/block/scsi_ioctl.c +0 ~~-12~~

@@ -218,7 +218,6 @@ static int sg_io(struct file *file, requ
 	unsigned long start_time;
 	int writing = 0, ret = 0;
 	struct request *rq;
-	struct bio *bio;
 	char sense[SCSI_SENSE_BUFFERSIZE];
 	unsigned char cmd[BLK_MAX_CDB];

@@ -287,14 +286,6 @@ static int sg_io(struct file *file, requ
 	rq->sense_len = 0;
 
 	rq->flags |= REQ_BLOCK_PC;
-	bio = rq->bio;
-
-	/*
-	 * bounce this after holding a reference to the original bio, it's
-	 * needed for proper unmapping
-	 */
-	if (rq->bio)
-		blk_queue_bounce(q, &rq->bio);
 
 	rq->timeout = (hdr->timeout * HZ) / 1000;
 	if (!rq->timeout)

@@ -330,9 +321,6 @@ static int sg_io(struct file *file, requ
 			hdr->sb_len_wr = len;
 	}
 
-	if (blk_rq_unmap_user(bio, hdr->dxfer_len))
-		ret = -EFAULT;
-
 	/* may not have succeeded, but output values written to control
 	 * structure (struct sg_io_hdr).  */
 out:

drivers/cdrom/cdrom.c +0 -8

@@ -2090,7 +2090,6 @@ static int cdrom_read_cdda_bpc(struct cd
 {
 	request_queue_t *q = cdi->disk->queue;
 	struct request *rq;
-	struct bio *bio;
 	unsigned int len;
 	int nr, ret = 0;

@@ -2131,10 +2130,6 @@ static int cdrom_read_cdda_bpc(struct cd
 		rq->cmd_len = 12;
 		rq->flags |= REQ_BLOCK_PC;
 		rq->timeout = 60 * HZ;
-		bio = rq->bio;
-
-		if (rq->bio)
-			blk_queue_bounce(q, &rq->bio);
 
 		if (blk_execute_rq(q, cdi->disk, rq, 0)) {
 			struct request_sense *s = rq->sense;

@@ -2142,9 +2137,6 @@ static int cdrom_read_cdda_bpc(struct cd
 			cdi->last_sense = s->sense_key;
 		}
 
-		if (blk_rq_unmap_user(bio, len))
-			ret = -EFAULT;
-
 		if (ret)
 			break;

fs/bio.c +62 ~~-48~~

189 lines not on this page · raw message

include/linux/bio.h +0 -2

@@ -285,13 +285,11 @@ struct sg_iovec;
 extern struct bio *bio_map_user_iov(struct request_queue *,
 				    struct block_device *,
 				    struct sg_iovec *, int, int);
-extern void bio_unmap_user(struct bio *);
 extern struct bio *bio_map_kern(struct request_queue *, void *, unsigned int,
 				unsigned int);
 extern void bio_set_pages_dirty(struct bio *bio);
 extern void bio_check_pages_dirty(struct bio *bio);
 extern struct bio *bio_copy_user(struct request_queue *, unsigned long, unsigned int, int);
-extern int bio_uncopy_user(struct bio *);
 void zero_fill_bio(struct bio *bio);
 
 #ifdef CONFIG_HIGHMEM

include/linux/blkdev.h +0 -1

@@ -553,7 +553,6 @@ extern void __blk_stop_queue(request_que
 extern void blk_run_queue(request_queue_t *);
 extern void blk_queue_activity_fn(request_queue_t *, activity_fn *, void *);
 extern int blk_rq_map_user(request_queue_t *, struct request *, void __user *, unsigned int);
-extern int blk_rq_unmap_user(struct bio *, unsigned int);
 extern int blk_rq_map_kern(request_queue_t *, struct request *, void *, unsigned int, unsigned int);
 extern int blk_rq_map_user_iov(request_queue_t *, struct request *, struct sg_iovec *, int);
 extern int blk_execute_rq(request_queue_t *, struct gendisk *,

Mike Christie

author

2005-07-29 09:28 UTC #1 lore raw

Quoted 8 lines · Mike Christie

And, as I work on the adding of BLKERR_* error values in replacement of the dm-multupath/bio sense patch, I was thinking about your comment about having one true make_request function. It seems like if we extended bios to handle more request stuff (like what is needed for SG IO) we could just pass the bio to __make_request - with some modifications to __make_request - instead of doing it request based and moving the blk_queue_bounce call to blk_execute_rq_nowait like I did in my patch. Is this what you were thinking when adding the bio sense code?

I mean doing things like sg io and dm-multipath HW handler's manual failover with bios instead of requests could simplify those things, but I am guessing bios were not meant to handle some of the lower level details like SCSI things. Or maybe they are or should be done in a more generic way?

A message that is not in the archive (sent off-list or not yet received): [email protected]

Mike Christie

author

2005-08-06 16:25 UTC #3 lore raw

Quoted 12 lines · Jens Axboe

Quoted 8 lines · Mike Christie
Hey Jens and James,
The inlined patch moves the bounce buffer handling to blk_execute_rq_nowait so the scsi, sg io and cdrom code does not have to handle it. To accomplish this I moved the bio_uncopy_user to a bi_end_io function and bio_unmap_user to a work struct that is schedule from a bi_end_io functions. Did you say you disliked the idea of calling bio_unmap_user from a work struct - don't remember and I lost my emails when I moved? :(

Quoted 4 lines

It's probably alright, it cleans up the code a lot. It will cost some extra context switches, but I'm naively hoping that for busy io we will have good batching of the processing anyways.

Doh! I ported sg.c and noticed I messed up. copy_to_user must be called from user context so my patch does not work for that path.