[RFC 0/5] block large commands support continue

[RFC 0/5] block large commands support continue

[Boaz Harrosh]

The support for large commands was dropped from the for-2.6.26 branch
and will probably not get accepted into next kernel.

I have tried to take all comments from Jens and Bart. and incorporate
it into a new patchset. This is basically Tomo's patchset but with
proposed changes.

They are based on current linux-block/master. They will probably conflict with
latest patch sent by Tomo for the blk_get_request(). Once those patches
get accepted at some git tree, (Where will that be?), I will rebase these
on top of them. Please CC me of any progress.

[PATCH 1/5] block: no need to initialize rq->cmd
  This is 2 of Tomo's patches squashed together as they are
  small and do the same. Tomo is this OK?

[PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB
  Tomos patch rebased to here

[PATCH 3/5] block: Export rq_init, rename to blk_init_rq
[PATCH 4/5] block: Use new blk_init_rq
  These patches are basically what Jens and Bart has suggested, that with
  a small code change to blk-core.c we can memset at rq_init() and only set
  none zero members. We can also export that initializer and use it all over
  the ide tree where ever requests don't come from a request queue. (OK also
  at scsi_error.c)

[PATCH 5/5] block: add large command support
  Now that all initialization goes through one place Tomos large command support
  is trivial.

Bart. This is mostly ide changes, so please if you can test it. I do not have
any legacy IDE devices here at the office, it is all new sata stuff.

I hope we can put this, and later the scsi stuff, on some tree that can be tested
at -mm and hopefully be ready for 2.6.27

Thanks
Boaz

[PATCH 1/5] block: no need to initialize rq->cmd

[Boaz Harrosh]

blk_get_request and queue_flush initializes rq->cmd (rq_init does)
so the users don't need to do that.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
Cc: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
---
 block/scsi_ioctl.c          |    3 ---
 drivers/block/pktcdvd.c     |    2 --
 drivers/block/ps3disk.c     |    1 -
 drivers/cdrom/cdrom.c       |    1 -
 drivers/md/dm-emc.c         |    2 --
 drivers/md/dm-mpath-hp-sw.c |    1 -
 drivers/md/dm-mpath-rdac.c  |    1 -
 drivers/scsi/sd.c           |    1 -
 8 files changed, 0 insertions(+), 12 deletions(-)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index a2c3a93..ffa3720 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -217,8 +217,6 @@ EXPORT_SYMBOL_GPL(blk_verify_command);
 static int blk_fill_sghdr_rq(struct request_queue *q, struct request *rq,
 			     struct sg_io_hdr *hdr, int has_write_perm)
 {
-	memset(rq->cmd, 0, BLK_MAX_CDB); /* ATAPI hates garbage after CDB */
-
 	if (copy_from_user(rq->cmd, hdr->cmdp, hdr->cmd_len))
 		return -EFAULT;
 	if (blk_verify_command(rq->cmd, has_write_perm))
@@ -531,7 +529,6 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
 	rq->data_len = 0;
 	rq->extra_len = 0;
 	rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
-	memset(rq->cmd, 0, sizeof(rq->cmd));
 	rq->cmd[0] = cmd;
 	rq->cmd[4] = data;
 	rq->cmd_len = 6;
diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index 18feb1c..3b806c9 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -776,8 +776,6 @@ static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *
 
 	rq->cmd_len = COMMAND_SIZE(cgc->cmd[0]);
 	memcpy(rq->cmd, cgc->cmd, CDROM_PACKET_SIZE);
-	if (sizeof(rq->cmd) > CDROM_PACKET_SIZE)
-		memset(rq->cmd + CDROM_PACKET_SIZE, 0, sizeof(rq->cmd) - CDROM_PACKET_SIZE);
 
 	rq->timeout = 60*HZ;
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
diff --git a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
index 7483f94..9c32b75 100644
--- a/drivers/block/ps3disk.c
+++ b/drivers/block/ps3disk.c
@@ -406,7 +406,6 @@ static void ps3disk_prepare_flush(struct request_queue *q, struct request *req)
 
 	dev_dbg(&dev->sbd.core, "%s:%u\n", __func__, __LINE__);
 
-	memset(req->cmd, 0, sizeof(req->cmd));
 	req->cmd_type = REQ_TYPE_FLUSH;
 }
 
diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
index ac38290..69f26eb 100644
--- a/drivers/cdrom/cdrom.c
+++ b/drivers/cdrom/cdrom.c
@@ -2194,7 +2194,6 @@ static int cdrom_read_cdda_bpc(struct cdrom_device_info *cdi, __u8 __user *ubuf,
 		if (ret)
 			break;
 
-		memset(rq->cmd, 0, sizeof(rq->cmd));
 		rq->cmd[0] = GPCMD_READ_CD;
 		rq->cmd[1] = 1 << 2;
 		rq->cmd[2] = (lba >> 24) & 0xff;
diff --git a/drivers/md/dm-emc.c b/drivers/md/dm-emc.c
index 6b91b9a..3ea5ad4 100644
--- a/drivers/md/dm-emc.c
+++ b/drivers/md/dm-emc.c
@@ -110,8 +110,6 @@ static struct request *get_failover_req(struct emc_handler *h,
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
 	rq->sense_len = 0;
 
-	memset(&rq->cmd, 0, BLK_MAX_CDB);
-
 	rq->timeout = EMC_FAILOVER_TIMEOUT;
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 	rq->cmd_flags |= REQ_FAILFAST | REQ_NOMERGE;
diff --git a/drivers/md/dm-mpath-hp-sw.c b/drivers/md/dm-mpath-hp-sw.c
index 204bf42..b63a0ab 100644
--- a/drivers/md/dm-mpath-hp-sw.c
+++ b/drivers/md/dm-mpath-hp-sw.c
@@ -137,7 +137,6 @@ static struct request *hp_sw_get_request(struct dm_path *path)
 	req->sense = h->sense;
 	memset(req->sense, 0, SCSI_SENSE_BUFFERSIZE);
 
-	memset(&req->cmd, 0, BLK_MAX_CDB);
 	req->cmd[0] = START_STOP;
 	req->cmd[4] = 1;
 	req->cmd_len = COMMAND_SIZE(req->cmd[0]);
diff --git a/drivers/md/dm-mpath-rdac.c b/drivers/md/dm-mpath-rdac.c
index e04eb5c..95e7773 100644
--- a/drivers/md/dm-mpath-rdac.c
+++ b/drivers/md/dm-mpath-rdac.c
@@ -284,7 +284,6 @@ static struct request *get_rdac_req(struct rdac_handler *h,
 		return NULL;
 	}
 
- 	memset(&rq->cmd, 0, BLK_MAX_CDB);
 	rq->sense = h->sense;
 	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
 	rq->sense_len = 0;
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 3cea17d..01cefbb 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -860,7 +860,6 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
 
 static void sd_prepare_flush(struct request_queue *q, struct request *rq)
 {
-	memset(rq->cmd, 0, sizeof(rq->cmd));
 	rq->cmd_type = REQ_TYPE_BLOCK_PC;
 	rq->timeout = SD_TIMEOUT;
 	rq->cmd[0] = SYNCHRONIZE_CACHE;
-- 
1.5.3.3

Re: [PATCH 1/5] block: no need to initialize rq->cmd

[Boaz Harrosh]

From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

Tomo is the author of this patch
On Wed, Apr 23 2008 at 17:57 +0300, Boaz Harrosh <bharrosh@panasas.com> wrote:
> blk_get_request and queue_flush initializes rq->cmd (rq_init does)
> so the users don't need to do that.
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
> Cc: Geert Uytterhoeven <Geert.Uytterhoeven@sonycom.com>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> ---
>  block/scsi_ioctl.c          |    3 ---
>  drivers/block/pktcdvd.c     |    2 --
>  drivers/block/ps3disk.c     |    1 -
>  drivers/cdrom/cdrom.c       |    1 -
>  drivers/md/dm-emc.c         |    2 --
>  drivers/md/dm-mpath-hp-sw.c |    1 -
>  drivers/md/dm-mpath-rdac.c  |    1 -
>  drivers/scsi/sd.c           |    1 -
>  8 files changed, 0 insertions(+), 12 deletions(-)
> 
> diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
> index a2c3a93..ffa3720 100644
> --- a/block/scsi_ioctl.c
> +++ b/block/scsi_ioctl.c
> @@ -217,8 +217,6 @@ EXPORT_SYMBOL_GPL(blk_verify_command);
>  static int blk_fill_sghdr_rq(struct request_queue *q, struct request *rq,
>  			     struct sg_io_hdr *hdr, int has_write_perm)
>  {
> -	memset(rq->cmd, 0, BLK_MAX_CDB); /* ATAPI hates garbage after CDB */
> -
>  	if (copy_from_user(rq->cmd, hdr->cmdp, hdr->cmd_len))
>  		return -EFAULT;
>  	if (blk_verify_command(rq->cmd, has_write_perm))
> @@ -531,7 +529,6 @@ static int __blk_send_generic(struct request_queue *q, struct gendisk *bd_disk,
>  	rq->data_len = 0;
>  	rq->extra_len = 0;
>  	rq->timeout = BLK_DEFAULT_SG_TIMEOUT;
> -	memset(rq->cmd, 0, sizeof(rq->cmd));
>  	rq->cmd[0] = cmd;
>  	rq->cmd[4] = data;
>  	rq->cmd_len = 6;
> diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
> index 18feb1c..3b806c9 100644
> --- a/drivers/block/pktcdvd.c
> +++ b/drivers/block/pktcdvd.c
> @@ -776,8 +776,6 @@ static int pkt_generic_packet(struct pktcdvd_device *pd, struct packet_command *
>  
>  	rq->cmd_len = COMMAND_SIZE(cgc->cmd[0]);
>  	memcpy(rq->cmd, cgc->cmd, CDROM_PACKET_SIZE);
> -	if (sizeof(rq->cmd) > CDROM_PACKET_SIZE)
> -		memset(rq->cmd + CDROM_PACKET_SIZE, 0, sizeof(rq->cmd) - CDROM_PACKET_SIZE);
>  
>  	rq->timeout = 60*HZ;
>  	rq->cmd_type = REQ_TYPE_BLOCK_PC;
> diff --git a/drivers/block/ps3disk.c b/drivers/block/ps3disk.c
> index 7483f94..9c32b75 100644
> --- a/drivers/block/ps3disk.c
> +++ b/drivers/block/ps3disk.c
> @@ -406,7 +406,6 @@ static void ps3disk_prepare_flush(struct request_queue *q, struct request *req)
>  
>  	dev_dbg(&dev->sbd.core, "%s:%u\n", __func__, __LINE__);
>  
> -	memset(req->cmd, 0, sizeof(req->cmd));
>  	req->cmd_type = REQ_TYPE_FLUSH;
>  }
>  
> diff --git a/drivers/cdrom/cdrom.c b/drivers/cdrom/cdrom.c
> index ac38290..69f26eb 100644
> --- a/drivers/cdrom/cdrom.c
> +++ b/drivers/cdrom/cdrom.c
> @@ -2194,7 +2194,6 @@ static int cdrom_read_cdda_bpc(struct cdrom_device_info *cdi, __u8 __user *ubuf,
>  		if (ret)
>  			break;
>  
> -		memset(rq->cmd, 0, sizeof(rq->cmd));
>  		rq->cmd[0] = GPCMD_READ_CD;
>  		rq->cmd[1] = 1 << 2;
>  		rq->cmd[2] = (lba >> 24) & 0xff;
> diff --git a/drivers/md/dm-emc.c b/drivers/md/dm-emc.c
> index 6b91b9a..3ea5ad4 100644
> --- a/drivers/md/dm-emc.c
> +++ b/drivers/md/dm-emc.c
> @@ -110,8 +110,6 @@ static struct request *get_failover_req(struct emc_handler *h,
>  	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
>  	rq->sense_len = 0;
>  
> -	memset(&rq->cmd, 0, BLK_MAX_CDB);
> -
>  	rq->timeout = EMC_FAILOVER_TIMEOUT;
>  	rq->cmd_type = REQ_TYPE_BLOCK_PC;
>  	rq->cmd_flags |= REQ_FAILFAST | REQ_NOMERGE;
> diff --git a/drivers/md/dm-mpath-hp-sw.c b/drivers/md/dm-mpath-hp-sw.c
> index 204bf42..b63a0ab 100644
> --- a/drivers/md/dm-mpath-hp-sw.c
> +++ b/drivers/md/dm-mpath-hp-sw.c
> @@ -137,7 +137,6 @@ static struct request *hp_sw_get_request(struct dm_path *path)
>  	req->sense = h->sense;
>  	memset(req->sense, 0, SCSI_SENSE_BUFFERSIZE);
>  
> -	memset(&req->cmd, 0, BLK_MAX_CDB);
>  	req->cmd[0] = START_STOP;
>  	req->cmd[4] = 1;
>  	req->cmd_len = COMMAND_SIZE(req->cmd[0]);
> diff --git a/drivers/md/dm-mpath-rdac.c b/drivers/md/dm-mpath-rdac.c
> index e04eb5c..95e7773 100644
> --- a/drivers/md/dm-mpath-rdac.c
> +++ b/drivers/md/dm-mpath-rdac.c
> @@ -284,7 +284,6 @@ static struct request *get_rdac_req(struct rdac_handler *h,
>  		return NULL;
>  	}
>  
> - 	memset(&rq->cmd, 0, BLK_MAX_CDB);
>  	rq->sense = h->sense;
>  	memset(rq->sense, 0, SCSI_SENSE_BUFFERSIZE);
>  	rq->sense_len = 0;
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 3cea17d..01cefbb 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -860,7 +860,6 @@ static int sd_sync_cache(struct scsi_disk *sdkp)
>  
>  static void sd_prepare_flush(struct request_queue *q, struct request *rq)
>  {
> -	memset(rq->cmd, 0, sizeof(rq->cmd));
>  	rq->cmd_type = REQ_TYPE_BLOCK_PC;
>  	rq->timeout = SD_TIMEOUT;
>  	rq->cmd[0] = SYNCHRONIZE_CACHE;

Boaz

[PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB

[Boaz Harrosh]

From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

This is a preparation for changing rq->cmd from the static array to a
pointer.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 block/blk-core.c             |    4 ++--
 drivers/ide/ide-cd.c         |    4 ++--
 drivers/ide/ide-cd_verbose.c |    2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index 2a438a9..6669238 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -132,7 +132,7 @@ void rq_init(struct request_queue *q, struct request *rq)
 	rq->errors = 0;
 	rq->ref_count = 1;
 	rq->cmd_len = 0;
-	memset(rq->cmd, 0, sizeof(rq->cmd));
+	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->data_len = 0;
 	rq->extra_len = 0;
 	rq->sense_len = 0;
@@ -194,7 +194,7 @@ void blk_dump_rq_flags(struct request *rq, char *msg)
 
 	if (blk_pc_request(rq)) {
 		printk(KERN_INFO "  cdb: ");
-		for (bit = 0; bit < sizeof(rq->cmd); bit++)
+		for (bit = 0; bit < BLK_MAX_CDB; bit++)
 			printk("%02x ", rq->cmd[bit]);
 		printk("\n");
 	}
diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index fe5aefb..dab4525 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -880,7 +880,7 @@ static ide_startstop_t cdrom_start_seek_continuation (ide_drive_t *drive)
 
 	sector_div(frame, queue_hardsect_size(drive->queue) >> SECTOR_BITS);
 
-	memset(rq->cmd, 0, sizeof(rq->cmd));
+	memset(rq->cmd, 0, BLK_MAX_CDB);
 	rq->cmd[0] = GPCMD_SEEK;
 	put_unaligned(cpu_to_be32(frame), (unsigned int *) &rq->cmd[2]);
 
@@ -1814,7 +1814,7 @@ static int ide_cdrom_prep_fs(struct request_queue *q, struct request *rq)
 	long block = (long)rq->hard_sector / (hard_sect >> 9);
 	unsigned long blocks = rq->hard_nr_sectors / (hard_sect >> 9);
 
-	memset(rq->cmd, 0, sizeof(rq->cmd));
+	memset(rq->cmd, 0, BLK_MAX_CDB);
 
 	if (rq_data_dir(rq) == READ)
 		rq->cmd[0] = GPCMD_READ_10;
diff --git a/drivers/ide/ide-cd_verbose.c b/drivers/ide/ide-cd_verbose.c
index 6ed7ca0..6490a2d 100644
--- a/drivers/ide/ide-cd_verbose.c
+++ b/drivers/ide/ide-cd_verbose.c
@@ -326,7 +326,7 @@ void ide_cd_log_error(const char *name, struct request *failed_command,
 
 		printk(KERN_ERR "  The failed \"%s\" packet command "
 				"was: \n  \"", s);
-		for (i = 0; i < sizeof(failed_command->cmd); i++)
+		for (i = 0; i < BLK_MAX_CDB; i++)
 			printk(KERN_CONT "%02x ", failed_command->cmd[i]);
 		printk(KERN_CONT "\"\n");
 	}
-- 
1.5.3.3

[RFC PATCH 3/5] block: Export rq_init, rename to blk_init_rq

[Boaz Harrosh]

Export rq_init so code that allocates requests on stack or in private
structures can call it, and not mess with block internals.

Rename rq_init => blk_init_rq to be more consistent with blk's API
naming convention.

Also memset the full request structure so new members that are zero
need not be initialized at init function separately.

It used to be that cmd_flags was set before the call to rq_init()
so pass cmd_flags to blk_init_req, to simplify user code.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 block/blk-barrier.c    |    6 ++----
 block/blk-core.c       |   37 +++++--------------------------------
 block/blk.h            |    1 -
 include/linux/blkdev.h |    1 +
 4 files changed, 8 insertions(+), 37 deletions(-)

diff --git a/block/blk-barrier.c b/block/blk-barrier.c
index 55c5f1f..9eb5bd0 100644
--- a/block/blk-barrier.c
+++ b/block/blk-barrier.c
@@ -143,8 +143,7 @@ static void queue_flush(struct request_queue *q, unsigned which)
 		end_io = post_flush_end_io;
 	}
 
-	rq->cmd_flags = REQ_HARDBARRIER;
-	rq_init(q, rq);
+	blk_init_rq(q, rq, REQ_HARDBARRIER);
 	rq->elevator_private = NULL;
 	rq->elevator_private2 = NULL;
 	rq->rq_disk = q->bar_rq.rq_disk;
@@ -167,8 +166,7 @@ static inline struct request *start_ordered(struct request_queue *q,
 	blkdev_dequeue_request(rq);
 	q->orig_bar_rq = rq;
 	rq = &q->bar_rq;
-	rq->cmd_flags = 0;
-	rq_init(q, rq);
+	blk_init_rq(q, rq, 0);
 	if (bio_data_dir(q->orig_bar_rq->bio) == WRITE)
 		rq->cmd_flags |= REQ_RW;
 	if (q->ordered & QUEUE_ORDERED_FUA)
diff --git a/block/blk-core.c b/block/blk-core.c
index 6669238..ca103d1 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -107,41 +107,20 @@ struct backing_dev_info *blk_get_backing_dev_info(struct block_device *bdev)
 }
 EXPORT_SYMBOL(blk_get_backing_dev_info);
 
-/*
- * We can't just memset() the structure, since the allocation path
- * already stored some information in the request.
- */
-void rq_init(struct request_queue *q, struct request *rq)
+void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
 {
+	memset(rq, 0, sizeof(*rq));
+	rq->cmd_flags = cmd_flags;
 	INIT_LIST_HEAD(&rq->queuelist);
 	INIT_LIST_HEAD(&rq->donelist);
 	rq->q = q;
 	rq->sector = rq->hard_sector = (sector_t) -1;
-	rq->nr_sectors = rq->hard_nr_sectors = 0;
-	rq->current_nr_sectors = rq->hard_cur_sectors = 0;
-	rq->bio = rq->biotail = NULL;
 	INIT_HLIST_NODE(&rq->hash);
 	RB_CLEAR_NODE(&rq->rb_node);
-	rq->rq_disk = NULL;
-	rq->nr_phys_segments = 0;
-	rq->nr_hw_segments = 0;
-	rq->ioprio = 0;
-	rq->special = NULL;
-	rq->buffer = NULL;
 	rq->tag = -1;
-	rq->errors = 0;
 	rq->ref_count = 1;
-	rq->cmd_len = 0;
-	memset(rq->cmd, 0, BLK_MAX_CDB);
-	rq->data_len = 0;
-	rq->extra_len = 0;
-	rq->sense_len = 0;
-	rq->data = NULL;
-	rq->sense = NULL;
-	rq->end_io = NULL;
-	rq->end_io_data = NULL;
-	rq->next_rq = NULL;
 }
+EXPORT_SYMBOL(blk_init_rq);
 
 static void req_bio_endio(struct request *rq, struct bio *bio,
 			  unsigned int nbytes, int error)
@@ -607,11 +586,7 @@ blk_alloc_request(struct request_queue *q, int rw, int priv, gfp_t gfp_mask)
 	if (!rq)
 		return NULL;
 
-	/*
-	 * first three bits are identical in rq->cmd_flags and bio->bi_rw,
-	 * see bio.h and blkdev.h
-	 */
-	rq->cmd_flags = rw | REQ_ALLOCED;
+	blk_init_rq(q, rq, rw | REQ_ALLOCED);
 
 	if (priv) {
 		if (unlikely(elv_set_request(q, rq, gfp_mask))) {
@@ -789,8 +764,6 @@ rq_starved:
 	if (ioc_batching(q, ioc))
 		ioc->nr_batch_requests--;
 
-	rq_init(q, rq);
-
 	blk_add_trace_generic(q, bio, rw, BLK_TA_GETRQ);
 out:
 	return rq;
diff --git a/block/blk.h b/block/blk.h
index ec9120f..59776ab 100644
--- a/block/blk.h
+++ b/block/blk.h
@@ -10,7 +10,6 @@
 extern struct kmem_cache *blk_requestq_cachep;
 extern struct kobj_type blk_queue_ktype;
 
-void rq_init(struct request_queue *q, struct request *rq);
 void init_request_from_bio(struct request *req, struct bio *bio);
 void blk_rq_bio_prep(struct request_queue *q, struct request *rq,
 			struct bio *bio);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c5065e3..7121783 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -596,6 +596,7 @@ extern int scsi_cmd_ioctl(struct file *, struct request_queue *,
 			  struct gendisk *, unsigned int, void __user *);
 extern int sg_scsi_ioctl(struct file *, struct request_queue *,
 		struct gendisk *, struct scsi_ioctl_command __user *);
+extern void blk_init_rq(struct request_queue *, struct request *, int);
 
 /*
  * Temporary export, until SCSI gets fixed up.
-- 
1.5.3.3

[RFC PATCH 4/5] block: Use new blk_init_rq

[Boaz Harrosh]

Places that allocate struct request on the stack or in private area
can use blk_init_rq, to zero everything out.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
 drivers/block/nbd.c        |    2 ++
 drivers/block/paride/pd.c  |    2 +-
 drivers/ide/ide-io.c       |    4 +---
 drivers/ide/ide-tape.c     |    2 +-
 drivers/ide/ide-taskfile.c |    3 +--
 drivers/ide/ide.c          |    4 ++--
 drivers/scsi/scsi_error.c  |    1 +
 7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 60cc543..dce9ffe 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -534,6 +534,8 @@ static int nbd_ioctl(struct inode *inode, struct file *file,
 	dprintk(DBG_IOCTL, "%s: nbd_ioctl cmd=%s(0x%x) arg=%lu\n",
 			lo->disk->disk_name, ioctl_cmd_to_ascii(cmd), cmd, arg);
 
+	blk_init_rq(NULL, &sreq, 0);
+
 	switch (cmd) {
 	case NBD_DISCONNECT:
 	        printk(KERN_INFO "%s: NBD_DISCONNECT\n", lo->disk->disk_name);
diff --git a/drivers/block/paride/pd.c b/drivers/block/paride/pd.c
index df819f8..2cd0cc4 100644
--- a/drivers/block/paride/pd.c
+++ b/drivers/block/paride/pd.c
@@ -716,7 +716,7 @@ static int pd_special_command(struct pd_unit *disk,
 	struct request rq;
 	int err = 0;
 
-	memset(&rq, 0, sizeof(rq));
+	blk_init_rq(NULL, &rq, 0);
 	rq.errors = 0;
 	rq.rq_disk = disk->gd;
 	rq.ref_count = 1;
diff --git a/drivers/ide/ide-io.c b/drivers/ide/ide-io.c
index 31e5afa..5661950 100644
--- a/drivers/ide/ide-io.c
+++ b/drivers/ide/ide-io.c
@@ -1601,10 +1601,8 @@ irqreturn_t ide_intr (int irq, void *dev_id)
 
 void ide_init_drive_cmd (struct request *rq)
 {
-	memset(rq, 0, sizeof(*rq));
-	rq->ref_count = 1;
+	blk_init_rq(NULL, rq, 0);
 }
-
 EXPORT_SYMBOL(ide_init_drive_cmd);
 
 /**
diff --git a/drivers/ide/ide-tape.c b/drivers/ide/ide-tape.c
index f43fd07..be61b86 100644
--- a/drivers/ide/ide-tape.c
+++ b/drivers/ide/ide-tape.c
@@ -899,7 +899,7 @@ static void idetape_create_request_sense_cmd(struct ide_atapi_pc *pc)
 
 static void idetape_init_rq(struct request *rq, u8 cmd)
 {
-	memset(rq, 0, sizeof(*rq));
+	blk_init_rq(NULL, rq, 0);
 	rq->cmd_type = REQ_TYPE_SPECIAL;
 	rq->cmd[0] = cmd;
 }
diff --git a/drivers/ide/ide-taskfile.c b/drivers/ide/ide-taskfile.c
index 155cc90..56ee99a 100644
--- a/drivers/ide/ide-taskfile.c
+++ b/drivers/ide/ide-taskfile.c
@@ -532,8 +532,7 @@ int ide_raw_taskfile(ide_drive_t *drive, ide_task_t *task, u8 *buf, u16 nsect)
 {
 	struct request rq;
 
-	memset(&rq, 0, sizeof(rq));
-	rq.ref_count = 1;
+	ide_init_drive_cmd(&rq);
 	rq.cmd_type = REQ_TYPE_ATA_TASKFILE;
 	rq.buffer = buf;
 
diff --git a/drivers/ide/ide.c b/drivers/ide/ide.c
index 917c72d..b812b7d 100644
--- a/drivers/ide/ide.c
+++ b/drivers/ide/ide.c
@@ -689,7 +689,7 @@ static int generic_ide_suspend(struct device *dev, pm_message_t mesg)
 	if (!(drive->dn % 2))
 		ide_acpi_get_timing(hwif);
 
-	memset(&rq, 0, sizeof(rq));
+	blk_init_rq(NULL, &rq, 0);
 	memset(&rqpm, 0, sizeof(rqpm));
 	memset(&args, 0, sizeof(args));
 	rq.cmd_type = REQ_TYPE_PM_SUSPEND;
@@ -727,7 +727,7 @@ static int generic_ide_resume(struct device *dev)
 
 	ide_acpi_exec_tfs(drive);
 
-	memset(&rq, 0, sizeof(rq));
+	blk_init_rq(NULL, &rq, 0);
 	memset(&rqpm, 0, sizeof(rqpm));
 	memset(&args, 0, sizeof(args));
 	rq.cmd_type = REQ_TYPE_PM_RESUME;
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 221f31e..ff14386 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -1771,6 +1771,7 @@ scsi_reset_provider(struct scsi_device *dev, int flag)
 	unsigned long flags;
 	int rtn;
 
+	blk_init_rq(dev->request_queue, &req, 0);
 	scmd->request = &req;
 	memset(&scmd->eh_timeout, 0, sizeof(scmd->eh_timeout));
 
-- 
1.5.3.3

[PATCH 5/5] block: add large command support

[Boaz Harrosh]

From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>

This patch changes rq->cmd from the static array to a pointer to
support large commands.

We rarely handle large commands. So for optimization, a struct request
still has a static array for a command. blk_init_rq sets rq->cmd pointer
to the static array.

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 block/blk-core.c       |    1 +
 include/linux/blkdev.h |   12 ++++++++++--
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/block/blk-core.c b/block/blk-core.c
index ca103d1..4c97fe2 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -119,6 +119,7 @@ void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
 	RB_CLEAR_NODE(&rq->rb_node);
 	rq->tag = -1;
 	rq->ref_count = 1;
+	rq->cmd = rq->__cmd;
 }
 EXPORT_SYMBOL(blk_init_rq);
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 7121783..32ae32b 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -215,8 +215,9 @@ struct request {
 	/*
 	 * when request is used as a packet command carrier
 	 */
-	unsigned int cmd_len;
-	unsigned char cmd[BLK_MAX_CDB];
+	unsigned short cmd_len;
+	unsigned char __cmd[BLK_MAX_CDB];
+	unsigned char *cmd;
 
 	unsigned int data_len;
 	unsigned int extra_len;	/* length of alignment and padding */
@@ -818,6 +819,13 @@ static inline void put_dev_sector(Sector p)
 	page_cache_release(p.v);
 }
 
+static inline void rq_set_cmd(struct request *rq, unsigned char *cmd,
+			      unsigned short cmd_len)
+{
+	rq->cmd = cmd;
+	rq->cmd_len = cmd_len;
+}
+
 struct work_struct;
 int kblockd_schedule_work(struct work_struct *work);
 void kblockd_flush_work(struct work_struct *work);
-- 
1.5.3.3

Re: [RFC 0/5] block large commands support continue

[FUJITA Tomonori]

On Wed, 23 Apr 2008 17:50:42 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> 
> The support for large commands was dropped from the for-2.6.26 branch
> and will probably not get accepted into next kernel.
> 
> I have tried to take all comments from Jens and Bart. and incorporate
> it into a new patchset. This is basically Tomo's patchset but with
> proposed changes.

Have you seen the patchset to remove request on the stack?

http://marc.info/?l=linux-ide&m=120882410712466&w=2


> They are based on current linux-block/master. They will probably conflict with
> latest patch sent by Tomo for the blk_get_request(). Once those patches
> get accepted at some git tree, (Where will that be?), I will rebase these
> on top of them. Please CC me of any progress.
> 
> [PATCH 1/5] block: no need to initialize rq->cmd
>   This is 2 of Tomo's patches squashed together as they are
>   small and do the same. Tomo is this OK?
> 
> [PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB
>   Tomos patch rebased to here
> 
> [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
> [PATCH 4/5] block: Use new blk_init_rq
>   These patches are basically what Jens and Bart has suggested, that with
>   a small code change to blk-core.c we can memset at rq_init() and only set
>   none zero members. We can also export that initializer and use it all over
>   the ide tree where ever requests don't come from a request queue. (OK also
>   at scsi_error.c)

+void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)

Hmm, would it be better to modify the block layer to let rq_init just
memset() the request structure?


> [PATCH 5/5] block: add large command support
>   Now that all initialization goes through one place Tomos large command support
>   is trivial.
> 
> Bart. This is mostly ide changes, so please if you can test it. I do not have
> any legacy IDE devices here at the office, it is all new sata stuff.
> 
> I hope we can put this, and later the scsi stuff, on some tree that can be tested
> at -mm and hopefully be ready for 2.6.27
> 
> Thanks
> Boaz
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 0/5] block large commands support continue

[FUJITA Tomonori]

On Thu, 24 Apr 2008 13:31:21 +0900
FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:

> On Wed, 23 Apr 2008 17:50:42 +0300
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
> > 
> > The support for large commands was dropped from the for-2.6.26 branch
> > and will probably not get accepted into next kernel.
> > 
> > I have tried to take all comments from Jens and Bart. and incorporate
> > it into a new patchset. This is basically Tomo's patchset but with
> > proposed changes.
> 
> Have you seen the patchset to remove request on the stack?
> 
> http://marc.info/?l=linux-ide&m=120882410712466&w=2
> 
> 
> > They are based on current linux-block/master. They will probably conflict with
> > latest patch sent by Tomo for the blk_get_request(). Once those patches
> > get accepted at some git tree, (Where will that be?), I will rebase these
> > on top of them. Please CC me of any progress.
> > 
> > [PATCH 1/5] block: no need to initialize rq->cmd
> >   This is 2 of Tomo's patches squashed together as they are
> >   small and do the same. Tomo is this OK?
> > 
> > [PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB
> >   Tomos patch rebased to here
> > 
> > [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
> > [PATCH 4/5] block: Use new blk_init_rq
> >   These patches are basically what Jens and Bart has suggested, that with
> >   a small code change to blk-core.c we can memset at rq_init() and only set
> >   none zero members. We can also export that initializer and use it all over
> >   the ide tree where ever requests don't come from a request queue. (OK also
> >   at scsi_error.c)
> 
> +void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
> 
> Hmm, would it be better to modify the block layer to let rq_init just
> memset() the request structure?

I think, if we move rq_init to blk_alloc_request from get_request,
rq_init can just memset() the structure.

Then we can export rq_init and rq_init works for everyone.


diff --git a/block/blk-barrier.c b/block/blk-barrier.c
index 55c5f1f..722140a 100644
--- a/block/blk-barrier.c
+++ b/block/blk-barrier.c
@@ -143,10 +143,8 @@ static void queue_flush(struct request_queue *q, unsigned which)
 		end_io = post_flush_end_io;
 	}
 
-	rq->cmd_flags = REQ_HARDBARRIER;
 	rq_init(q, rq);
-	rq->elevator_private = NULL;
-	rq->elevator_private2 = NULL;
+	rq->cmd_flags = REQ_HARDBARRIER;
 	rq->rq_disk = q->bar_rq.rq_disk;
 	rq->end_io = end_io;
 	q->prepare_flush_fn(q, rq);
@@ -167,14 +165,11 @@ static inline struct request *start_ordered(struct request_queue *q,
 	blkdev_dequeue_request(rq);
 	q->orig_bar_rq = rq;
 	rq = &q->bar_rq;
-	rq->cmd_flags = 0;
 	rq_init(q, rq);
 	if (bio_data_dir(q->orig_bar_rq->bio) == WRITE)
 		rq->cmd_flags |= REQ_RW;
 	if (q->ordered & QUEUE_ORDERED_FUA)
 		rq->cmd_flags |= REQ_FUA;
-	rq->elevator_private = NULL;
-	rq->elevator_private2 = NULL;
 	init_request_from_bio(rq, q->orig_bar_rq->bio);
 	rq->end_io = bar_end_io;
 
diff --git a/block/blk-core.c b/block/blk-core.c
index 2a438a9..e447799 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -107,40 +107,18 @@ struct backing_dev_info *blk_get_backing_dev_info(struct block_device *bdev)
 }
 EXPORT_SYMBOL(blk_get_backing_dev_info);
 
-/*
- * We can't just memset() the structure, since the allocation path
- * already stored some information in the request.
- */
 void rq_init(struct request_queue *q, struct request *rq)
 {
+	memset(rq, 0, sizeof(*rq));
+
 	INIT_LIST_HEAD(&rq->queuelist);
 	INIT_LIST_HEAD(&rq->donelist);
 	rq->q = q;
 	rq->sector = rq->hard_sector = (sector_t) -1;
-	rq->nr_sectors = rq->hard_nr_sectors = 0;
-	rq->current_nr_sectors = rq->hard_cur_sectors = 0;
-	rq->bio = rq->biotail = NULL;
 	INIT_HLIST_NODE(&rq->hash);
 	RB_CLEAR_NODE(&rq->rb_node);
-	rq->rq_disk = NULL;
-	rq->nr_phys_segments = 0;
-	rq->nr_hw_segments = 0;
-	rq->ioprio = 0;
-	rq->special = NULL;
-	rq->buffer = NULL;
 	rq->tag = -1;
-	rq->errors = 0;
 	rq->ref_count = 1;
-	rq->cmd_len = 0;
-	memset(rq->cmd, 0, sizeof(rq->cmd));
-	rq->data_len = 0;
-	rq->extra_len = 0;
-	rq->sense_len = 0;
-	rq->data = NULL;
-	rq->sense = NULL;
-	rq->end_io = NULL;
-	rq->end_io_data = NULL;
-	rq->next_rq = NULL;
 }
 
 static void req_bio_endio(struct request *rq, struct bio *bio,
@@ -607,6 +585,8 @@ blk_alloc_request(struct request_queue *q, int rw, int priv, gfp_t gfp_mask)
 	if (!rq)
 		return NULL;
 
+	rq_init(q, rq);
+
 	/*
 	 * first three bits are identical in rq->cmd_flags and bio->bi_rw,
 	 * see bio.h and blkdev.h
@@ -789,8 +769,6 @@ rq_starved:
 	if (ioc_batching(q, ioc))
 		ioc->nr_batch_requests--;
 
-	rq_init(q, rq);
-
 	blk_add_trace_generic(q, bio, rw, BLK_TA_GETRQ);
 out:
 	return rq;

Re: [RFC 0/5] block large commands support continue

[Jens Axboe]

On Thu, Apr 24 2008, FUJITA Tomonori wrote:
> On Thu, 24 Apr 2008 13:31:21 +0900
> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> 
> > On Wed, 23 Apr 2008 17:50:42 +0300
> > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > 
> > > 
> > > The support for large commands was dropped from the for-2.6.26 branch
> > > and will probably not get accepted into next kernel.
> > > 
> > > I have tried to take all comments from Jens and Bart. and incorporate
> > > it into a new patchset. This is basically Tomo's patchset but with
> > > proposed changes.
> > 
> > Have you seen the patchset to remove request on the stack?
> > 
> > http://marc.info/?l=linux-ide&m=120882410712466&w=2
> > 
> > 
> > > They are based on current linux-block/master. They will probably conflict with
> > > latest patch sent by Tomo for the blk_get_request(). Once those patches
> > > get accepted at some git tree, (Where will that be?), I will rebase these
> > > on top of them. Please CC me of any progress.
> > > 
> > > [PATCH 1/5] block: no need to initialize rq->cmd
> > >   This is 2 of Tomo's patches squashed together as they are
> > >   small and do the same. Tomo is this OK?
> > > 
> > > [PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB
> > >   Tomos patch rebased to here
> > > 
> > > [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
> > > [PATCH 4/5] block: Use new blk_init_rq
> > >   These patches are basically what Jens and Bart has suggested, that with
> > >   a small code change to blk-core.c we can memset at rq_init() and only set
> > >   none zero members. We can also export that initializer and use it all over
> > >   the ide tree where ever requests don't come from a request queue. (OK also
> > >   at scsi_error.c)
> > 
> > +void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
> > 
> > Hmm, would it be better to modify the block layer to let rq_init just
> > memset() the request structure?
> 
> I think, if we move rq_init to blk_alloc_request from get_request,
> rq_init can just memset() the structure.
> 
> Then we can export rq_init and rq_init works for everyone.

Wont work, see the io scheduler set_request() functions.

> 
> 
> diff --git a/block/blk-barrier.c b/block/blk-barrier.c
> index 55c5f1f..722140a 100644
> --- a/block/blk-barrier.c
> +++ b/block/blk-barrier.c
> @@ -143,10 +143,8 @@ static void queue_flush(struct request_queue *q, unsigned which)
>  		end_io = post_flush_end_io;
>  	}
>  
> -	rq->cmd_flags = REQ_HARDBARRIER;
>  	rq_init(q, rq);
> -	rq->elevator_private = NULL;
> -	rq->elevator_private2 = NULL;
> +	rq->cmd_flags = REQ_HARDBARRIER;
>  	rq->rq_disk = q->bar_rq.rq_disk;
>  	rq->end_io = end_io;
>  	q->prepare_flush_fn(q, rq);
> @@ -167,14 +165,11 @@ static inline struct request *start_ordered(struct request_queue *q,
>  	blkdev_dequeue_request(rq);
>  	q->orig_bar_rq = rq;
>  	rq = &q->bar_rq;
> -	rq->cmd_flags = 0;
>  	rq_init(q, rq);
>  	if (bio_data_dir(q->orig_bar_rq->bio) == WRITE)
>  		rq->cmd_flags |= REQ_RW;
>  	if (q->ordered & QUEUE_ORDERED_FUA)
>  		rq->cmd_flags |= REQ_FUA;
> -	rq->elevator_private = NULL;
> -	rq->elevator_private2 = NULL;
>  	init_request_from_bio(rq, q->orig_bar_rq->bio);
>  	rq->end_io = bar_end_io;
>  
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 2a438a9..e447799 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -107,40 +107,18 @@ struct backing_dev_info *blk_get_backing_dev_info(struct block_device *bdev)
>  }
>  EXPORT_SYMBOL(blk_get_backing_dev_info);
>  
> -/*
> - * We can't just memset() the structure, since the allocation path
> - * already stored some information in the request.
> - */
>  void rq_init(struct request_queue *q, struct request *rq)
>  {
> +	memset(rq, 0, sizeof(*rq));
> +
>  	INIT_LIST_HEAD(&rq->queuelist);
>  	INIT_LIST_HEAD(&rq->donelist);
>  	rq->q = q;
>  	rq->sector = rq->hard_sector = (sector_t) -1;
> -	rq->nr_sectors = rq->hard_nr_sectors = 0;
> -	rq->current_nr_sectors = rq->hard_cur_sectors = 0;
> -	rq->bio = rq->biotail = NULL;
>  	INIT_HLIST_NODE(&rq->hash);
>  	RB_CLEAR_NODE(&rq->rb_node);
> -	rq->rq_disk = NULL;
> -	rq->nr_phys_segments = 0;
> -	rq->nr_hw_segments = 0;
> -	rq->ioprio = 0;
> -	rq->special = NULL;
> -	rq->buffer = NULL;
>  	rq->tag = -1;
> -	rq->errors = 0;
>  	rq->ref_count = 1;
> -	rq->cmd_len = 0;
> -	memset(rq->cmd, 0, sizeof(rq->cmd));
> -	rq->data_len = 0;
> -	rq->extra_len = 0;
> -	rq->sense_len = 0;
> -	rq->data = NULL;
> -	rq->sense = NULL;
> -	rq->end_io = NULL;
> -	rq->end_io_data = NULL;
> -	rq->next_rq = NULL;
>  }
>  
>  static void req_bio_endio(struct request *rq, struct bio *bio,
> @@ -607,6 +585,8 @@ blk_alloc_request(struct request_queue *q, int rw, int priv, gfp_t gfp_mask)
>  	if (!rq)
>  		return NULL;
>  
> +	rq_init(q, rq);
> +
>  	/*
>  	 * first three bits are identical in rq->cmd_flags and bio->bi_rw,
>  	 * see bio.h and blkdev.h
> @@ -789,8 +769,6 @@ rq_starved:
>  	if (ioc_batching(q, ioc))
>  		ioc->nr_batch_requests--;
>  
> -	rq_init(q, rq);
> -
>  	blk_add_trace_generic(q, bio, rw, BLK_TA_GETRQ);
>  out:
>  	return rq;

-- 
Jens Axboe

Re: [RFC 0/5] block large commands support continue

[FUJITA Tomonori]

On Thu, 24 Apr 2008 12:49:30 +0200
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Thu, Apr 24 2008, FUJITA Tomonori wrote:
> > On Thu, 24 Apr 2008 13:31:21 +0900
> > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > 
> > > On Wed, 23 Apr 2008 17:50:42 +0300
> > > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > > 
> > > > 
> > > > The support for large commands was dropped from the for-2.6.26 branch
> > > > and will probably not get accepted into next kernel.
> > > > 
> > > > I have tried to take all comments from Jens and Bart. and incorporate
> > > > it into a new patchset. This is basically Tomo's patchset but with
> > > > proposed changes.
> > > 
> > > Have you seen the patchset to remove request on the stack?
> > > 
> > > http://marc.info/?l=linux-ide&m=120882410712466&w=2
> > > 
> > > 
> > > > They are based on current linux-block/master. They will probably conflict with
> > > > latest patch sent by Tomo for the blk_get_request(). Once those patches
> > > > get accepted at some git tree, (Where will that be?), I will rebase these
> > > > on top of them. Please CC me of any progress.
> > > > 
> > > > [PATCH 1/5] block: no need to initialize rq->cmd
> > > >   This is 2 of Tomo's patches squashed together as they are
> > > >   small and do the same. Tomo is this OK?
> > > > 
> > > > [PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB
> > > >   Tomos patch rebased to here
> > > > 
> > > > [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
> > > > [PATCH 4/5] block: Use new blk_init_rq
> > > >   These patches are basically what Jens and Bart has suggested, that with
> > > >   a small code change to blk-core.c we can memset at rq_init() and only set
> > > >   none zero members. We can also export that initializer and use it all over
> > > >   the ide tree where ever requests don't come from a request queue. (OK also
> > > >   at scsi_error.c)
> > > 
> > > +void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
> > > 
> > > Hmm, would it be better to modify the block layer to let rq_init just
> > > memset() the request structure?
> > 
> > I think, if we move rq_init to blk_alloc_request from get_request,
> > rq_init can just memset() the structure.
> > 
> > Then we can export rq_init and rq_init works for everyone.
> 
> Wont work, see the io scheduler set_request() functions.

Sorry, can you be more specific?

Only cfq uses set_request for now. cfq_set_request uses rq->cmd_flags
and stores information at rq->elevator_private and
rq->elevator_private2.

The patch does memset() the request structure and sets up
rq->cmd_flags, and then elv_set_request. Doesn't it work?

Re: [RFC 0/5] block large commands support continue

[Jens Axboe]

On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> On Thu, 24 Apr 2008 12:49:30 +0200
> Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > On Thu, Apr 24 2008, FUJITA Tomonori wrote:
> > > On Thu, 24 Apr 2008 13:31:21 +0900
> > > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > > 
> > > > On Wed, 23 Apr 2008 17:50:42 +0300
> > > > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > > > 
> > > > > 
> > > > > The support for large commands was dropped from the for-2.6.26 branch
> > > > > and will probably not get accepted into next kernel.
> > > > > 
> > > > > I have tried to take all comments from Jens and Bart. and incorporate
> > > > > it into a new patchset. This is basically Tomo's patchset but with
> > > > > proposed changes.
> > > > 
> > > > Have you seen the patchset to remove request on the stack?
> > > > 
> > > > http://marc.info/?l=linux-ide&m=120882410712466&w=2
> > > > 
> > > > 
> > > > > They are based on current linux-block/master. They will probably conflict with
> > > > > latest patch sent by Tomo for the blk_get_request(). Once those patches
> > > > > get accepted at some git tree, (Where will that be?), I will rebase these
> > > > > on top of them. Please CC me of any progress.
> > > > > 
> > > > > [PATCH 1/5] block: no need to initialize rq->cmd
> > > > >   This is 2 of Tomo's patches squashed together as they are
> > > > >   small and do the same. Tomo is this OK?
> > > > > 
> > > > > [PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB
> > > > >   Tomos patch rebased to here
> > > > > 
> > > > > [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
> > > > > [PATCH 4/5] block: Use new blk_init_rq
> > > > >   These patches are basically what Jens and Bart has suggested, that with
> > > > >   a small code change to blk-core.c we can memset at rq_init() and only set
> > > > >   none zero members. We can also export that initializer and use it all over
> > > > >   the ide tree where ever requests don't come from a request queue. (OK also
> > > > >   at scsi_error.c)
> > > > 
> > > > +void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
> > > > 
> > > > Hmm, would it be better to modify the block layer to let rq_init just
> > > > memset() the request structure?
> > > 
> > > I think, if we move rq_init to blk_alloc_request from get_request,
> > > rq_init can just memset() the structure.
> > > 
> > > Then we can export rq_init and rq_init works for everyone.
> > 
> > Wont work, see the io scheduler set_request() functions.
> 
> Sorry, can you be more specific?
> 
> Only cfq uses set_request for now. cfq_set_request uses rq->cmd_flags
> and stores information at rq->elevator_private and
> rq->elevator_private2.
> 
> The patch does memset() the request structure and sets up
> rq->cmd_flags, and then elv_set_request. Doesn't it work?

Sorry, with the moved rq_init() it should work, didn't look closely
enough.

-- 
Jens Axboe

Re: [RFC 0/5] block large commands support continue

[FUJITA Tomonori]

On Fri, 25 Apr 2008 11:22:04 +0200
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> > On Thu, 24 Apr 2008 12:49:30 +0200
> > Jens Axboe <jens.axboe@oracle.com> wrote:
> > 
> > > On Thu, Apr 24 2008, FUJITA Tomonori wrote:
> > > > On Thu, 24 Apr 2008 13:31:21 +0900
> > > > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > > > 
> > > > > On Wed, 23 Apr 2008 17:50:42 +0300
> > > > > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > > > > 
> > > > > > 
> > > > > > The support for large commands was dropped from the for-2.6.26 branch
> > > > > > and will probably not get accepted into next kernel.
> > > > > > 
> > > > > > I have tried to take all comments from Jens and Bart. and incorporate
> > > > > > it into a new patchset. This is basically Tomo's patchset but with
> > > > > > proposed changes.
> > > > > 
> > > > > Have you seen the patchset to remove request on the stack?
> > > > > 
> > > > > http://marc.info/?l=linux-ide&m=120882410712466&w=2
> > > > > 
> > > > > 
> > > > > > They are based on current linux-block/master. They will probably conflict with
> > > > > > latest patch sent by Tomo for the blk_get_request(). Once those patches
> > > > > > get accepted at some git tree, (Where will that be?), I will rebase these
> > > > > > on top of them. Please CC me of any progress.
> > > > > > 
> > > > > > [PATCH 1/5] block: no need to initialize rq->cmd
> > > > > >   This is 2 of Tomo's patches squashed together as they are
> > > > > >   small and do the same. Tomo is this OK?
> > > > > > 
> > > > > > [PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB
> > > > > >   Tomos patch rebased to here
> > > > > > 
> > > > > > [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
> > > > > > [PATCH 4/5] block: Use new blk_init_rq
> > > > > >   These patches are basically what Jens and Bart has suggested, that with
> > > > > >   a small code change to blk-core.c we can memset at rq_init() and only set
> > > > > >   none zero members. We can also export that initializer and use it all over
> > > > > >   the ide tree where ever requests don't come from a request queue. (OK also
> > > > > >   at scsi_error.c)
> > > > > 
> > > > > +void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
> > > > > 
> > > > > Hmm, would it be better to modify the block layer to let rq_init just
> > > > > memset() the request structure?
> > > > 
> > > > I think, if we move rq_init to blk_alloc_request from get_request,
> > > > rq_init can just memset() the structure.
> > > > 
> > > > Then we can export rq_init and rq_init works for everyone.
> > > 
> > > Wont work, see the io scheduler set_request() functions.
> > 
> > Sorry, can you be more specific?
> > 
> > Only cfq uses set_request for now. cfq_set_request uses rq->cmd_flags
> > and stores information at rq->elevator_private and
> > rq->elevator_private2.
> > 
> > The patch does memset() the request structure and sets up
> > rq->cmd_flags, and then elv_set_request. Doesn't it work?
> 
> Sorry, with the moved rq_init() it should work, didn't look closely
> enough.

No problem.

So are you ok with the patch? If so, I'll re-send it with a proper
description and the signed-off.

Re: [RFC 0/5] block large commands support continue

[Jens Axboe]

On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> On Fri, 25 Apr 2008 11:22:04 +0200
> Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> > > On Thu, 24 Apr 2008 12:49:30 +0200
> > > Jens Axboe <jens.axboe@oracle.com> wrote:
> > > 
> > > > On Thu, Apr 24 2008, FUJITA Tomonori wrote:
> > > > > On Thu, 24 Apr 2008 13:31:21 +0900
> > > > > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > > > > 
> > > > > > On Wed, 23 Apr 2008 17:50:42 +0300
> > > > > > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > > > > > 
> > > > > > > 
> > > > > > > The support for large commands was dropped from the for-2.6.26 branch
> > > > > > > and will probably not get accepted into next kernel.
> > > > > > > 
> > > > > > > I have tried to take all comments from Jens and Bart. and incorporate
> > > > > > > it into a new patchset. This is basically Tomo's patchset but with
> > > > > > > proposed changes.
> > > > > > 
> > > > > > Have you seen the patchset to remove request on the stack?
> > > > > > 
> > > > > > http://marc.info/?l=linux-ide&m=120882410712466&w=2
> > > > > > 
> > > > > > 
> > > > > > > They are based on current linux-block/master. They will probably conflict with
> > > > > > > latest patch sent by Tomo for the blk_get_request(). Once those patches
> > > > > > > get accepted at some git tree, (Where will that be?), I will rebase these
> > > > > > > on top of them. Please CC me of any progress.
> > > > > > > 
> > > > > > > [PATCH 1/5] block: no need to initialize rq->cmd
> > > > > > >   This is 2 of Tomo's patches squashed together as they are
> > > > > > >   small and do the same. Tomo is this OK?
> > > > > > > 
> > > > > > > [PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB
> > > > > > >   Tomos patch rebased to here
> > > > > > > 
> > > > > > > [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
> > > > > > > [PATCH 4/5] block: Use new blk_init_rq
> > > > > > >   These patches are basically what Jens and Bart has suggested, that with
> > > > > > >   a small code change to blk-core.c we can memset at rq_init() and only set
> > > > > > >   none zero members. We can also export that initializer and use it all over
> > > > > > >   the ide tree where ever requests don't come from a request queue. (OK also
> > > > > > >   at scsi_error.c)
> > > > > > 
> > > > > > +void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
> > > > > > 
> > > > > > Hmm, would it be better to modify the block layer to let rq_init just
> > > > > > memset() the request structure?
> > > > > 
> > > > > I think, if we move rq_init to blk_alloc_request from get_request,
> > > > > rq_init can just memset() the structure.
> > > > > 
> > > > > Then we can export rq_init and rq_init works for everyone.
> > > > 
> > > > Wont work, see the io scheduler set_request() functions.
> > > 
> > > Sorry, can you be more specific?
> > > 
> > > Only cfq uses set_request for now. cfq_set_request uses rq->cmd_flags
> > > and stores information at rq->elevator_private and
> > > rq->elevator_private2.
> > > 
> > > The patch does memset() the request structure and sets up
> > > rq->cmd_flags, and then elv_set_request. Doesn't it work?
> > 
> > Sorry, with the moved rq_init() it should work, didn't look closely
> > enough.
> 
> No problem.
> 
> So are you ok with the patch? If so, I'll re-send it with a proper
> description and the signed-off.

Please do - I actually already merged it, but do resend with a full
description and signed-off etc.

-- 
Jens Axboe

Re: [RFC 0/5] block large commands support continue

[FUJITA Tomonori]

On Fri, 25 Apr 2008 11:31:41 +0200
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> > On Fri, 25 Apr 2008 11:22:04 +0200
> > Jens Axboe <jens.axboe@oracle.com> wrote:
> > 
> > > On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> > > > On Thu, 24 Apr 2008 12:49:30 +0200
> > > > Jens Axboe <jens.axboe@oracle.com> wrote:
> > > > 
> > > > > On Thu, Apr 24 2008, FUJITA Tomonori wrote:
> > > > > > On Thu, 24 Apr 2008 13:31:21 +0900
> > > > > > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > > > > > 
> > > > > > > On Wed, 23 Apr 2008 17:50:42 +0300
> > > > > > > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > > > > > > 
> > > > > > > > 
> > > > > > > > The support for large commands was dropped from the for-2.6.26 branch
> > > > > > > > and will probably not get accepted into next kernel.
> > > > > > > > 
> > > > > > > > I have tried to take all comments from Jens and Bart. and incorporate
> > > > > > > > it into a new patchset. This is basically Tomo's patchset but with
> > > > > > > > proposed changes.
> > > > > > > 
> > > > > > > Have you seen the patchset to remove request on the stack?
> > > > > > > 
> > > > > > > http://marc.info/?l=linux-ide&m=120882410712466&w=2
> > > > > > > 
> > > > > > > 
> > > > > > > > They are based on current linux-block/master. They will probably conflict with
> > > > > > > > latest patch sent by Tomo for the blk_get_request(). Once those patches
> > > > > > > > get accepted at some git tree, (Where will that be?), I will rebase these
> > > > > > > > on top of them. Please CC me of any progress.
> > > > > > > > 
> > > > > > > > [PATCH 1/5] block: no need to initialize rq->cmd
> > > > > > > >   This is 2 of Tomo's patches squashed together as they are
> > > > > > > >   small and do the same. Tomo is this OK?
> > > > > > > > 
> > > > > > > > [PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB
> > > > > > > >   Tomos patch rebased to here
> > > > > > > > 
> > > > > > > > [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
> > > > > > > > [PATCH 4/5] block: Use new blk_init_rq
> > > > > > > >   These patches are basically what Jens and Bart has suggested, that with
> > > > > > > >   a small code change to blk-core.c we can memset at rq_init() and only set
> > > > > > > >   none zero members. We can also export that initializer and use it all over
> > > > > > > >   the ide tree where ever requests don't come from a request queue. (OK also
> > > > > > > >   at scsi_error.c)
> > > > > > > 
> > > > > > > +void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
> > > > > > > 
> > > > > > > Hmm, would it be better to modify the block layer to let rq_init just
> > > > > > > memset() the request structure?
> > > > > > 
> > > > > > I think, if we move rq_init to blk_alloc_request from get_request,
> > > > > > rq_init can just memset() the structure.
> > > > > > 
> > > > > > Then we can export rq_init and rq_init works for everyone.
> > > > > 
> > > > > Wont work, see the io scheduler set_request() functions.
> > > > 
> > > > Sorry, can you be more specific?
> > > > 
> > > > Only cfq uses set_request for now. cfq_set_request uses rq->cmd_flags
> > > > and stores information at rq->elevator_private and
> > > > rq->elevator_private2.
> > > > 
> > > > The patch does memset() the request structure and sets up
> > > > rq->cmd_flags, and then elv_set_request. Doesn't it work?
> > > 
> > > Sorry, with the moved rq_init() it should work, didn't look closely
> > > enough.
> > 
> > No problem.
> > 
> > So are you ok with the patch? If so, I'll re-send it with a proper
> > description and the signed-off.
> 
> Please do - I actually already merged it, but do resend with a full
> description and signed-off etc.

I just stole your description and added my signed-off.

Will you merge the large command support for 2.6.26? Or only this
clean-up patch?

=
From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Subject: [PATCH] block: make rq_init() do a full memset()

This requires moving rq_init() from get_request() to blk_alloc_request().
The upside is that we can now require an rq_init() from any path that
wishes to hand the request to the block layer.

rq_init() will be exported for the code that uses struct request
without blk_get_request.

This is a preparation for large command support, which needs to
initialize struct request in a proper way (that is, just doing a
memset() will not work).

Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
---
 block/blk-barrier.c |    7 +------
 block/blk-core.c    |   30 ++++--------------------------
 2 files changed, 5 insertions(+), 32 deletions(-)

diff --git a/block/blk-barrier.c b/block/blk-barrier.c
index 55c5f1f..722140a 100644
--- a/block/blk-barrier.c
+++ b/block/blk-barrier.c
@@ -143,10 +143,8 @@ static void queue_flush(struct request_queue *q, unsigned which)
 		end_io = post_flush_end_io;
 	}
 
-	rq->cmd_flags = REQ_HARDBARRIER;
 	rq_init(q, rq);
-	rq->elevator_private = NULL;
-	rq->elevator_private2 = NULL;
+	rq->cmd_flags = REQ_HARDBARRIER;
 	rq->rq_disk = q->bar_rq.rq_disk;
 	rq->end_io = end_io;
 	q->prepare_flush_fn(q, rq);
@@ -167,14 +165,11 @@ static inline struct request *start_ordered(struct request_queue *q,
 	blkdev_dequeue_request(rq);
 	q->orig_bar_rq = rq;
 	rq = &q->bar_rq;
-	rq->cmd_flags = 0;
 	rq_init(q, rq);
 	if (bio_data_dir(q->orig_bar_rq->bio) == WRITE)
 		rq->cmd_flags |= REQ_RW;
 	if (q->ordered & QUEUE_ORDERED_FUA)
 		rq->cmd_flags |= REQ_FUA;
-	rq->elevator_private = NULL;
-	rq->elevator_private2 = NULL;
 	init_request_from_bio(rq, q->orig_bar_rq->bio);
 	rq->end_io = bar_end_io;
 
diff --git a/block/blk-core.c b/block/blk-core.c
index 2a438a9..e447799 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -107,40 +107,18 @@ struct backing_dev_info *blk_get_backing_dev_info(struct block_device *bdev)
 }
 EXPORT_SYMBOL(blk_get_backing_dev_info);
 
-/*
- * We can't just memset() the structure, since the allocation path
- * already stored some information in the request.
- */
 void rq_init(struct request_queue *q, struct request *rq)
 {
+	memset(rq, 0, sizeof(*rq));
+
 	INIT_LIST_HEAD(&rq->queuelist);
 	INIT_LIST_HEAD(&rq->donelist);
 	rq->q = q;
 	rq->sector = rq->hard_sector = (sector_t) -1;
-	rq->nr_sectors = rq->hard_nr_sectors = 0;
-	rq->current_nr_sectors = rq->hard_cur_sectors = 0;
-	rq->bio = rq->biotail = NULL;
 	INIT_HLIST_NODE(&rq->hash);
 	RB_CLEAR_NODE(&rq->rb_node);
-	rq->rq_disk = NULL;
-	rq->nr_phys_segments = 0;
-	rq->nr_hw_segments = 0;
-	rq->ioprio = 0;
-	rq->special = NULL;
-	rq->buffer = NULL;
 	rq->tag = -1;
-	rq->errors = 0;
 	rq->ref_count = 1;
-	rq->cmd_len = 0;
-	memset(rq->cmd, 0, sizeof(rq->cmd));
-	rq->data_len = 0;
-	rq->extra_len = 0;
-	rq->sense_len = 0;
-	rq->data = NULL;
-	rq->sense = NULL;
-	rq->end_io = NULL;
-	rq->end_io_data = NULL;
-	rq->next_rq = NULL;
 }
 
 static void req_bio_endio(struct request *rq, struct bio *bio,
@@ -607,6 +585,8 @@ blk_alloc_request(struct request_queue *q, int rw, int priv, gfp_t gfp_mask)
 	if (!rq)
 		return NULL;
 
+	rq_init(q, rq);
+
 	/*
 	 * first three bits are identical in rq->cmd_flags and bio->bi_rw,
 	 * see bio.h and blkdev.h
@@ -789,8 +769,6 @@ rq_starved:
 	if (ioc_batching(q, ioc))
 		ioc->nr_batch_requests--;
 
-	rq_init(q, rq);
-
 	blk_add_trace_generic(q, bio, rw, BLK_TA_GETRQ);
 out:
 	return rq;
-- 
1.5.4.2

Re: [RFC 0/5] block large commands support continue

[Jens Axboe]

On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> On Fri, 25 Apr 2008 11:31:41 +0200
> Jens Axboe <jens.axboe@oracle.com> wrote:
> 
> > On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> > > On Fri, 25 Apr 2008 11:22:04 +0200
> > > Jens Axboe <jens.axboe@oracle.com> wrote:
> > > 
> > > > On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> > > > > On Thu, 24 Apr 2008 12:49:30 +0200
> > > > > Jens Axboe <jens.axboe@oracle.com> wrote:
> > > > > 
> > > > > > On Thu, Apr 24 2008, FUJITA Tomonori wrote:
> > > > > > > On Thu, 24 Apr 2008 13:31:21 +0900
> > > > > > > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > > > > > > 
> > > > > > > > On Wed, 23 Apr 2008 17:50:42 +0300
> > > > > > > > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > > > > > > > 
> > > > > > > > > 
> > > > > > > > > The support for large commands was dropped from the for-2.6.26 branch
> > > > > > > > > and will probably not get accepted into next kernel.
> > > > > > > > > 
> > > > > > > > > I have tried to take all comments from Jens and Bart. and incorporate
> > > > > > > > > it into a new patchset. This is basically Tomo's patchset but with
> > > > > > > > > proposed changes.
> > > > > > > > 
> > > > > > > > Have you seen the patchset to remove request on the stack?
> > > > > > > > 
> > > > > > > > http://marc.info/?l=linux-ide&m=120882410712466&w=2
> > > > > > > > 
> > > > > > > > 
> > > > > > > > > They are based on current linux-block/master. They will probably conflict with
> > > > > > > > > latest patch sent by Tomo for the blk_get_request(). Once those patches
> > > > > > > > > get accepted at some git tree, (Where will that be?), I will rebase these
> > > > > > > > > on top of them. Please CC me of any progress.
> > > > > > > > > 
> > > > > > > > > [PATCH 1/5] block: no need to initialize rq->cmd
> > > > > > > > >   This is 2 of Tomo's patches squashed together as they are
> > > > > > > > >   small and do the same. Tomo is this OK?
> > > > > > > > > 
> > > > > > > > > [PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB
> > > > > > > > >   Tomos patch rebased to here
> > > > > > > > > 
> > > > > > > > > [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
> > > > > > > > > [PATCH 4/5] block: Use new blk_init_rq
> > > > > > > > >   These patches are basically what Jens and Bart has suggested, that with
> > > > > > > > >   a small code change to blk-core.c we can memset at rq_init() and only set
> > > > > > > > >   none zero members. We can also export that initializer and use it all over
> > > > > > > > >   the ide tree where ever requests don't come from a request queue. (OK also
> > > > > > > > >   at scsi_error.c)
> > > > > > > > 
> > > > > > > > +void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
> > > > > > > > 
> > > > > > > > Hmm, would it be better to modify the block layer to let rq_init just
> > > > > > > > memset() the request structure?
> > > > > > > 
> > > > > > > I think, if we move rq_init to blk_alloc_request from get_request,
> > > > > > > rq_init can just memset() the structure.
> > > > > > > 
> > > > > > > Then we can export rq_init and rq_init works for everyone.
> > > > > > 
> > > > > > Wont work, see the io scheduler set_request() functions.
> > > > > 
> > > > > Sorry, can you be more specific?
> > > > > 
> > > > > Only cfq uses set_request for now. cfq_set_request uses rq->cmd_flags
> > > > > and stores information at rq->elevator_private and
> > > > > rq->elevator_private2.
> > > > > 
> > > > > The patch does memset() the request structure and sets up
> > > > > rq->cmd_flags, and then elv_set_request. Doesn't it work?
> > > > 
> > > > Sorry, with the moved rq_init() it should work, didn't look closely
> > > > enough.
> > > 
> > > No problem.
> > > 
> > > So are you ok with the patch? If so, I'll re-send it with a proper
> > > description and the signed-off.
> > 
> > Please do - I actually already merged it, but do resend with a full
> > description and signed-off etc.
> 
> I just stole your description and added my signed-off.

OK, will update it.

> Will you merge the large command support for 2.6.26? Or only this
> clean-up patch?

It's both, we can merge large command support for 2.6.26.

-- 
Jens Axboe

Re: [RFC 0/5] block large commands support continue

[FUJITA Tomonori]

On Fri, 25 Apr 2008 12:25:56 +0200
Jens Axboe <jens.axboe@oracle.com> wrote:

> On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> > On Fri, 25 Apr 2008 11:31:41 +0200
> > Jens Axboe <jens.axboe@oracle.com> wrote:
> > 
> > > On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> > > > On Fri, 25 Apr 2008 11:22:04 +0200
> > > > Jens Axboe <jens.axboe@oracle.com> wrote:
> > > > 
> > > > > On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> > > > > > On Thu, 24 Apr 2008 12:49:30 +0200
> > > > > > Jens Axboe <jens.axboe@oracle.com> wrote:
> > > > > > 
> > > > > > > On Thu, Apr 24 2008, FUJITA Tomonori wrote:
> > > > > > > > On Thu, 24 Apr 2008 13:31:21 +0900
> > > > > > > > FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > > > > > > > 
> > > > > > > > > On Wed, 23 Apr 2008 17:50:42 +0300
> > > > > > > > > Boaz Harrosh <bharrosh@panasas.com> wrote:
> > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > The support for large commands was dropped from the for-2.6.26 branch
> > > > > > > > > > and will probably not get accepted into next kernel.
> > > > > > > > > > 
> > > > > > > > > > I have tried to take all comments from Jens and Bart. and incorporate
> > > > > > > > > > it into a new patchset. This is basically Tomo's patchset but with
> > > > > > > > > > proposed changes.
> > > > > > > > > 
> > > > > > > > > Have you seen the patchset to remove request on the stack?
> > > > > > > > > 
> > > > > > > > > http://marc.info/?l=linux-ide&m=120882410712466&w=2
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > > They are based on current linux-block/master. They will probably conflict with
> > > > > > > > > > latest patch sent by Tomo for the blk_get_request(). Once those patches
> > > > > > > > > > get accepted at some git tree, (Where will that be?), I will rebase these
> > > > > > > > > > on top of them. Please CC me of any progress.
> > > > > > > > > > 
> > > > > > > > > > [PATCH 1/5] block: no need to initialize rq->cmd
> > > > > > > > > >   This is 2 of Tomo's patches squashed together as they are
> > > > > > > > > >   small and do the same. Tomo is this OK?
> > > > > > > > > > 
> > > > > > > > > > [PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB
> > > > > > > > > >   Tomos patch rebased to here
> > > > > > > > > > 
> > > > > > > > > > [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
> > > > > > > > > > [PATCH 4/5] block: Use new blk_init_rq
> > > > > > > > > >   These patches are basically what Jens and Bart has suggested, that with
> > > > > > > > > >   a small code change to blk-core.c we can memset at rq_init() and only set
> > > > > > > > > >   none zero members. We can also export that initializer and use it all over
> > > > > > > > > >   the ide tree where ever requests don't come from a request queue. (OK also
> > > > > > > > > >   at scsi_error.c)
> > > > > > > > > 
> > > > > > > > > +void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
> > > > > > > > > 
> > > > > > > > > Hmm, would it be better to modify the block layer to let rq_init just
> > > > > > > > > memset() the request structure?
> > > > > > > > 
> > > > > > > > I think, if we move rq_init to blk_alloc_request from get_request,
> > > > > > > > rq_init can just memset() the structure.
> > > > > > > > 
> > > > > > > > Then we can export rq_init and rq_init works for everyone.
> > > > > > > 
> > > > > > > Wont work, see the io scheduler set_request() functions.
> > > > > > 
> > > > > > Sorry, can you be more specific?
> > > > > > 
> > > > > > Only cfq uses set_request for now. cfq_set_request uses rq->cmd_flags
> > > > > > and stores information at rq->elevator_private and
> > > > > > rq->elevator_private2.
> > > > > > 
> > > > > > The patch does memset() the request structure and sets up
> > > > > > rq->cmd_flags, and then elv_set_request. Doesn't it work?
> > > > > 
> > > > > Sorry, with the moved rq_init() it should work, didn't look closely
> > > > > enough.
> > > > 
> > > > No problem.
> > > > 
> > > > So are you ok with the patch? If so, I'll re-send it with a proper
> > > > description and the signed-off.
> > > 
> > > Please do - I actually already merged it, but do resend with a full
> > > description and signed-off etc.
> > 
> > I just stole your description and added my signed-off.
> 
> OK, will update it.
> 
> > Will you merge the large command support for 2.6.26? Or only this
> > clean-up patch?
> 
> It's both, we can merge large command support for 2.6.26.

Great, I'll update and send the previous patchset shortly.

Re: [RFC 0/5] block large commands support continue

[Boaz Harrosh]

On Fri, Apr 25 2008 at 13:03 +0300, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> On Fri, 25 Apr 2008 11:31:41 +0200
> Jens Axboe <jens.axboe@oracle.com> wrote:
> 
>> On Fri, Apr 25 2008, FUJITA Tomonori wrote:
>>> On Fri, 25 Apr 2008 11:22:04 +0200
>>> Jens Axboe <jens.axboe@oracle.com> wrote:
>>>
>>>> On Fri, Apr 25 2008, FUJITA Tomonori wrote:
>>>>> On Thu, 24 Apr 2008 12:49:30 +0200
>>>>> Jens Axboe <jens.axboe@oracle.com> wrote:
>>>>>
>>>>>> On Thu, Apr 24 2008, FUJITA Tomonori wrote:
>>>>>>> On Thu, 24 Apr 2008 13:31:21 +0900
>>>>>>> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>>>>>>>
>>>>>>>> On Wed, 23 Apr 2008 17:50:42 +0300
>>>>>>>> Boaz Harrosh <bharrosh@panasas.com> wrote:
>>>>>>>>
>>>>>>>>> The support for large commands was dropped from the for-2.6.26 branch
>>>>>>>>> and will probably not get accepted into next kernel.
>>>>>>>>>
>>>>>>>>> I have tried to take all comments from Jens and Bart. and incorporate
>>>>>>>>> it into a new patchset. This is basically Tomo's patchset but with
>>>>>>>>> proposed changes.
>>>>>>>> Have you seen the patchset to remove request on the stack?
>>>>>>>>
>>>>>>>> http://marc.info/?l=linux-ide&m=120882410712466&w=2
>>>>>>>>
>>>>>>>>
>>>>>>>>> They are based on current linux-block/master. They will probably conflict with
>>>>>>>>> latest patch sent by Tomo for the blk_get_request(). Once those patches
>>>>>>>>> get accepted at some git tree, (Where will that be?), I will rebase these
>>>>>>>>> on top of them. Please CC me of any progress.
>>>>>>>>>
>>>>>>>>> [PATCH 1/5] block: no need to initialize rq->cmd
>>>>>>>>>   This is 2 of Tomo's patches squashed together as they are
>>>>>>>>>   small and do the same. Tomo is this OK?
>>>>>>>>>
>>>>>>>>> [PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB
>>>>>>>>>   Tomos patch rebased to here
>>>>>>>>>
>>>>>>>>> [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
>>>>>>>>> [PATCH 4/5] block: Use new blk_init_rq
>>>>>>>>>   These patches are basically what Jens and Bart has suggested, that with
>>>>>>>>>   a small code change to blk-core.c we can memset at rq_init() and only set
>>>>>>>>>   none zero members. We can also export that initializer and use it all over
>>>>>>>>>   the ide tree where ever requests don't come from a request queue. (OK also
>>>>>>>>>   at scsi_error.c)
>>>>>>>> +void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
>>>>>>>>
>>>>>>>> Hmm, would it be better to modify the block layer to let rq_init just
>>>>>>>> memset() the request structure?
>>>>>>> I think, if we move rq_init to blk_alloc_request from get_request,
>>>>>>> rq_init can just memset() the structure.
>>>>>>>
>>>>>>> Then we can export rq_init and rq_init works for everyone.
>>>>>> Wont work, see the io scheduler set_request() functions.
>>>>> Sorry, can you be more specific?
>>>>>
>>>>> Only cfq uses set_request for now. cfq_set_request uses rq->cmd_flags
>>>>> and stores information at rq->elevator_private and
>>>>> rq->elevator_private2.
>>>>>
>>>>> The patch does memset() the request structure and sets up
>>>>> rq->cmd_flags, and then elv_set_request. Doesn't it work?
>>>> Sorry, with the moved rq_init() it should work, didn't look closely
>>>> enough.
>>> No problem.
>>>
>>> So are you ok with the patch? If so, I'll re-send it with a proper
>>> description and the signed-off.
>> Please do - I actually already merged it, but do resend with a full
>> description and signed-off etc.
> 
> I just stole your description and added my signed-off.
> 
> Will you merge the large command support for 2.6.26? Or only this
> clean-up patch?
> 
> =
> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Subject: [PATCH] block: make rq_init() do a full memset()
> 
> This requires moving rq_init() from get_request() to blk_alloc_request().
> The upside is that we can now require an rq_init() from any path that
> wishes to hand the request to the block layer.
> 
> rq_init() will be exported for the code that uses struct request
> without blk_get_request.
> 
> This is a preparation for large command support, which needs to
> initialize struct request in a proper way (that is, just doing a
> memset() will not work).
> 
> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> ---
<snip>

Sorry for the late response. Those hebrew holidays on they way of Linux
coding ;-).

I don't mind as long as these things get accepted. But how is that any
different then the patch I sent?
  [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
It does exactly 100% the same move of rq_init to blk_alloc_request and the memset and
all, Have you looked at the patches at all? I feel like a mute person ;-(

If you are going to export the rq_init function then I think the name is very
wrong. And you have not exported it?

Boaz

Re: [RFC 0/5] block large commands support continue

[FUJITA Tomonori]

On Sun, 27 Apr 2008 11:26:20 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On Fri, Apr 25 2008 at 13:03 +0300, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > On Fri, 25 Apr 2008 11:31:41 +0200
> > Jens Axboe <jens.axboe@oracle.com> wrote:
> > 
> >> On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> >>> On Fri, 25 Apr 2008 11:22:04 +0200
> >>> Jens Axboe <jens.axboe@oracle.com> wrote:
> >>>
> >>>> On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> >>>>> On Thu, 24 Apr 2008 12:49:30 +0200
> >>>>> Jens Axboe <jens.axboe@oracle.com> wrote:
> >>>>>
> >>>>>> On Thu, Apr 24 2008, FUJITA Tomonori wrote:
> >>>>>>> On Thu, 24 Apr 2008 13:31:21 +0900
> >>>>>>> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> >>>>>>>
> >>>>>>>> On Wed, 23 Apr 2008 17:50:42 +0300
> >>>>>>>> Boaz Harrosh <bharrosh@panasas.com> wrote:
> >>>>>>>>
> >>>>>>>>> The support for large commands was dropped from the for-2.6.26 branch
> >>>>>>>>> and will probably not get accepted into next kernel.
> >>>>>>>>>
> >>>>>>>>> I have tried to take all comments from Jens and Bart. and incorporate
> >>>>>>>>> it into a new patchset. This is basically Tomo's patchset but with
> >>>>>>>>> proposed changes.
> >>>>>>>> Have you seen the patchset to remove request on the stack?
> >>>>>>>>
> >>>>>>>> http://marc.info/?l=linux-ide&m=120882410712466&w=2
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> They are based on current linux-block/master. They will probably conflict with
> >>>>>>>>> latest patch sent by Tomo for the blk_get_request(). Once those patches
> >>>>>>>>> get accepted at some git tree, (Where will that be?), I will rebase these
> >>>>>>>>> on top of them. Please CC me of any progress.
> >>>>>>>>>
> >>>>>>>>> [PATCH 1/5] block: no need to initialize rq->cmd
> >>>>>>>>>   This is 2 of Tomo's patches squashed together as they are
> >>>>>>>>>   small and do the same. Tomo is this OK?
> >>>>>>>>>
> >>>>>>>>> [PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB
> >>>>>>>>>   Tomos patch rebased to here
> >>>>>>>>>
> >>>>>>>>> [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
> >>>>>>>>> [PATCH 4/5] block: Use new blk_init_rq
> >>>>>>>>>   These patches are basically what Jens and Bart has suggested, that with
> >>>>>>>>>   a small code change to blk-core.c we can memset at rq_init() and only set
> >>>>>>>>>   none zero members. We can also export that initializer and use it all over
> >>>>>>>>>   the ide tree where ever requests don't come from a request queue. (OK also
> >>>>>>>>>   at scsi_error.c)
> >>>>>>>> +void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
> >>>>>>>>
> >>>>>>>> Hmm, would it be better to modify the block layer to let rq_init just
> >>>>>>>> memset() the request structure?
> >>>>>>> I think, if we move rq_init to blk_alloc_request from get_request,
> >>>>>>> rq_init can just memset() the structure.
> >>>>>>>
> >>>>>>> Then we can export rq_init and rq_init works for everyone.
> >>>>>> Wont work, see the io scheduler set_request() functions.
> >>>>> Sorry, can you be more specific?
> >>>>>
> >>>>> Only cfq uses set_request for now. cfq_set_request uses rq->cmd_flags
> >>>>> and stores information at rq->elevator_private and
> >>>>> rq->elevator_private2.
> >>>>>
> >>>>> The patch does memset() the request structure and sets up
> >>>>> rq->cmd_flags, and then elv_set_request. Doesn't it work?
> >>>> Sorry, with the moved rq_init() it should work, didn't look closely
> >>>> enough.
> >>> No problem.
> >>>
> >>> So are you ok with the patch? If so, I'll re-send it with a proper
> >>> description and the signed-off.
> >> Please do - I actually already merged it, but do resend with a full
> >> description and signed-off etc.
> > 
> > I just stole your description and added my signed-off.
> > 
> > Will you merge the large command support for 2.6.26? Or only this
> > clean-up patch?
> > 
> > =
> > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Subject: [PATCH] block: make rq_init() do a full memset()
> > 
> > This requires moving rq_init() from get_request() to blk_alloc_request().
> > The upside is that we can now require an rq_init() from any path that
> > wishes to hand the request to the block layer.
> > 
> > rq_init() will be exported for the code that uses struct request
> > without blk_get_request.
> > 
> > This is a preparation for large command support, which needs to
> > initialize struct request in a proper way (that is, just doing a
> > memset() will not work).
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> > ---
> <snip>
> 
> Sorry for the late response. Those hebrew holidays on they way of Linux
> coding ;-).
> 
> I don't mind as long as these things get accepted. But how is that any
> different then the patch I sent?
>   [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
> It does exactly 100% the same move of rq_init to blk_alloc_request and the memset and
> all, Have you looked at the patches at all? I feel like a mute person ;-(

No, it's not same at all. Please look at the patchset again. Your
interface is:

+void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)

I don't like that. It's hacky. You needed that hacky inferface since
you didn't fix the root problem, rq_init was not able to do a full
memset().

I fixed the root problem with this patch:

http://marc.info/?l=linux-scsi&m=120901807514386&w=2


Then, we have the same interface as before:

+void blk_rq_init(struct request_queue *q, struct request *rq)


> If you are going to export the rq_init function then I think the name is very
> wrong. And you have not exported it?

It was renamed blk_rq_init.

Re: [RFC 0/5] block large commands support continue

[FUJITA Tomonori]

On Sun, 27 Apr 2008 11:26:20 +0300
Boaz Harrosh <bharrosh@panasas.com> wrote:

> On Fri, Apr 25 2008 at 13:03 +0300, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> > On Fri, 25 Apr 2008 11:31:41 +0200
> > Jens Axboe <jens.axboe@oracle.com> wrote:
> > 
> >> On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> >>> On Fri, 25 Apr 2008 11:22:04 +0200
> >>> Jens Axboe <jens.axboe@oracle.com> wrote:
> >>>
> >>>> On Fri, Apr 25 2008, FUJITA Tomonori wrote:
> >>>>> On Thu, 24 Apr 2008 12:49:30 +0200
> >>>>> Jens Axboe <jens.axboe@oracle.com> wrote:
> >>>>>
> >>>>>> On Thu, Apr 24 2008, FUJITA Tomonori wrote:
> >>>>>>> On Thu, 24 Apr 2008 13:31:21 +0900
> >>>>>>> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> >>>>>>>
> >>>>>>>> On Wed, 23 Apr 2008 17:50:42 +0300
> >>>>>>>> Boaz Harrosh <bharrosh@panasas.com> wrote:
> >>>>>>>>
> >>>>>>>>> The support for large commands was dropped from the for-2.6.26 branch
> >>>>>>>>> and will probably not get accepted into next kernel.
> >>>>>>>>>
> >>>>>>>>> I have tried to take all comments from Jens and Bart. and incorporate
> >>>>>>>>> it into a new patchset. This is basically Tomo's patchset but with
> >>>>>>>>> proposed changes.
> >>>>>>>> Have you seen the patchset to remove request on the stack?
> >>>>>>>>
> >>>>>>>> http://marc.info/?l=linux-ide&m=120882410712466&w=2
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>> They are based on current linux-block/master. They will probably conflict with
> >>>>>>>>> latest patch sent by Tomo for the blk_get_request(). Once those patches
> >>>>>>>>> get accepted at some git tree, (Where will that be?), I will rebase these
> >>>>>>>>> on top of them. Please CC me of any progress.
> >>>>>>>>>
> >>>>>>>>> [PATCH 1/5] block: no need to initialize rq->cmd
> >>>>>>>>>   This is 2 of Tomo's patches squashed together as they are
> >>>>>>>>>   small and do the same. Tomo is this OK?
> >>>>>>>>>
> >>>>>>>>> [PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB
> >>>>>>>>>   Tomos patch rebased to here
> >>>>>>>>>
> >>>>>>>>> [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
> >>>>>>>>> [PATCH 4/5] block: Use new blk_init_rq
> >>>>>>>>>   These patches are basically what Jens and Bart has suggested, that with
> >>>>>>>>>   a small code change to blk-core.c we can memset at rq_init() and only set
> >>>>>>>>>   none zero members. We can also export that initializer and use it all over
> >>>>>>>>>   the ide tree where ever requests don't come from a request queue. (OK also
> >>>>>>>>>   at scsi_error.c)
> >>>>>>>> +void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
> >>>>>>>>
> >>>>>>>> Hmm, would it be better to modify the block layer to let rq_init just
> >>>>>>>> memset() the request structure?
> >>>>>>> I think, if we move rq_init to blk_alloc_request from get_request,
> >>>>>>> rq_init can just memset() the structure.
> >>>>>>>
> >>>>>>> Then we can export rq_init and rq_init works for everyone.
> >>>>>> Wont work, see the io scheduler set_request() functions.
> >>>>> Sorry, can you be more specific?
> >>>>>
> >>>>> Only cfq uses set_request for now. cfq_set_request uses rq->cmd_flags
> >>>>> and stores information at rq->elevator_private and
> >>>>> rq->elevator_private2.
> >>>>>
> >>>>> The patch does memset() the request structure and sets up
> >>>>> rq->cmd_flags, and then elv_set_request. Doesn't it work?
> >>>> Sorry, with the moved rq_init() it should work, didn't look closely
> >>>> enough.
> >>> No problem.
> >>>
> >>> So are you ok with the patch? If so, I'll re-send it with a proper
> >>> description and the signed-off.
> >> Please do - I actually already merged it, but do resend with a full
> >> description and signed-off etc.
> > 
> > I just stole your description and added my signed-off.
> > 
> > Will you merge the large command support for 2.6.26? Or only this
> > clean-up patch?
> > 
> > =
> > From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Subject: [PATCH] block: make rq_init() do a full memset()
> > 
> > This requires moving rq_init() from get_request() to blk_alloc_request().
> > The upside is that we can now require an rq_init() from any path that
> > wishes to hand the request to the block layer.
> > 
> > rq_init() will be exported for the code that uses struct request
> > without blk_get_request.
> > 
> > This is a preparation for large command support, which needs to
> > initialize struct request in a proper way (that is, just doing a
> > memset() will not work).
> > 
> > Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
> > Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
> > ---
> <snip>
> 
> Sorry for the late response. Those hebrew holidays on they way of Linux
> coding ;-).
> 
> I don't mind as long as these things get accepted. But how is that any
> different then the patch I sent?
>   [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
> It does exactly 100% the same move of rq_init to blk_alloc_request and the memset and
> all, Have you looked at the patches at all? I feel like a mute person ;-(

No, it's not same at all. Please look at the patchset again. Your
interface is:

+void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)

I don't like that. It's hacky. You needed that hacky inferface because
you didn't fix the root problem, rq_init was not able to do a full
memset().

I fixed the root problem with this patch:

http://marc.info/?l=linux-scsi&m=120901807514386&w=2


Then, we have the same interface as before:

+void blk_rq_init(struct request_queue *q, struct request *rq)


> If you are going to export the rq_init function then I think the name is very
> wrong. And you have not exported it?

It was renamed blk_rq_init since we have lots of blk_rq_* functions.

Re: [RFC 0/5] block large commands support continue

[Boaz Harrosh]

On Sun, Apr 27 2008 at 11:42 +0300, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
> On Sun, 27 Apr 2008 11:26:20 +0300
> Boaz Harrosh <bharrosh@panasas.com> wrote:
> 
>> On Fri, Apr 25 2008 at 13:03 +0300, FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>>> On Fri, 25 Apr 2008 11:31:41 +0200
>>> Jens Axboe <jens.axboe@oracle.com> wrote:
>>>
>>>> On Fri, Apr 25 2008, FUJITA Tomonori wrote:
>>>>> On Fri, 25 Apr 2008 11:22:04 +0200
>>>>> Jens Axboe <jens.axboe@oracle.com> wrote:
>>>>>
>>>>>> On Fri, Apr 25 2008, FUJITA Tomonori wrote:
>>>>>>> On Thu, 24 Apr 2008 12:49:30 +0200
>>>>>>> Jens Axboe <jens.axboe@oracle.com> wrote:
>>>>>>>
>>>>>>>> On Thu, Apr 24 2008, FUJITA Tomonori wrote:
>>>>>>>>> On Thu, 24 Apr 2008 13:31:21 +0900
>>>>>>>>> FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp> wrote:
>>>>>>>>>
>>>>>>>>>> On Wed, 23 Apr 2008 17:50:42 +0300
>>>>>>>>>> Boaz Harrosh <bharrosh@panasas.com> wrote:
>>>>>>>>>>
>>>>>>>>>>> The support for large commands was dropped from the for-2.6.26 branch
>>>>>>>>>>> and will probably not get accepted into next kernel.
>>>>>>>>>>>
>>>>>>>>>>> I have tried to take all comments from Jens and Bart. and incorporate
>>>>>>>>>>> it into a new patchset. This is basically Tomo's patchset but with
>>>>>>>>>>> proposed changes.
>>>>>>>>>> Have you seen the patchset to remove request on the stack?
>>>>>>>>>>
>>>>>>>>>> http://marc.info/?l=linux-ide&m=120882410712466&w=2
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> They are based on current linux-block/master. They will probably conflict with
>>>>>>>>>>> latest patch sent by Tomo for the blk_get_request(). Once those patches
>>>>>>>>>>> get accepted at some git tree, (Where will that be?), I will rebase these
>>>>>>>>>>> on top of them. Please CC me of any progress.
>>>>>>>>>>>
>>>>>>>>>>> [PATCH 1/5] block: no need to initialize rq->cmd
>>>>>>>>>>>   This is 2 of Tomo's patches squashed together as they are
>>>>>>>>>>>   small and do the same. Tomo is this OK?
>>>>>>>>>>>
>>>>>>>>>>> [PATCH 2/5] block: replace sizeof(rq->cmd) with BLK_MAX_CDB
>>>>>>>>>>>   Tomos patch rebased to here
>>>>>>>>>>>
>>>>>>>>>>> [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
>>>>>>>>>>> [PATCH 4/5] block: Use new blk_init_rq
>>>>>>>>>>>   These patches are basically what Jens and Bart has suggested, that with
>>>>>>>>>>>   a small code change to blk-core.c we can memset at rq_init() and only set
>>>>>>>>>>>   none zero members. We can also export that initializer and use it all over
>>>>>>>>>>>   the ide tree where ever requests don't come from a request queue. (OK also
>>>>>>>>>>>   at scsi_error.c)
>>>>>>>>>> +void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
>>>>>>>>>>
>>>>>>>>>> Hmm, would it be better to modify the block layer to let rq_init just
>>>>>>>>>> memset() the request structure?
>>>>>>>>> I think, if we move rq_init to blk_alloc_request from get_request,
>>>>>>>>> rq_init can just memset() the structure.
>>>>>>>>>
>>>>>>>>> Then we can export rq_init and rq_init works for everyone.
>>>>>>>> Wont work, see the io scheduler set_request() functions.
>>>>>>> Sorry, can you be more specific?
>>>>>>>
>>>>>>> Only cfq uses set_request for now. cfq_set_request uses rq->cmd_flags
>>>>>>> and stores information at rq->elevator_private and
>>>>>>> rq->elevator_private2.
>>>>>>>
>>>>>>> The patch does memset() the request structure and sets up
>>>>>>> rq->cmd_flags, and then elv_set_request. Doesn't it work?
>>>>>> Sorry, with the moved rq_init() it should work, didn't look closely
>>>>>> enough.
>>>>> No problem.
>>>>>
>>>>> So are you ok with the patch? If so, I'll re-send it with a proper
>>>>> description and the signed-off.
>>>> Please do - I actually already merged it, but do resend with a full
>>>> description and signed-off etc.
>>> I just stole your description and added my signed-off.
>>>
>>> Will you merge the large command support for 2.6.26? Or only this
>>> clean-up patch?
>>>
>>> =
>>> From: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>>> Subject: [PATCH] block: make rq_init() do a full memset()
>>>
>>> This requires moving rq_init() from get_request() to blk_alloc_request().
>>> The upside is that we can now require an rq_init() from any path that
>>> wishes to hand the request to the block layer.
>>>
>>> rq_init() will be exported for the code that uses struct request
>>> without blk_get_request.
>>>
>>> This is a preparation for large command support, which needs to
>>> initialize struct request in a proper way (that is, just doing a
>>> memset() will not work).
>>>
>>> Signed-off-by: FUJITA Tomonori <fujita.tomonori@lab.ntt.co.jp>
>>> Signed-off-by: Jens Axboe <jens.axboe@oracle.com>
>>> ---
>> <snip>
>>
>> Sorry for the late response. Those hebrew holidays on they way of Linux
>> coding ;-).
>>
>> I don't mind as long as these things get accepted. But how is that any
>> different then the patch I sent?
>>   [PATCH 3/5] block: Export rq_init, rename to blk_init_rq
>> It does exactly 100% the same move of rq_init to blk_alloc_request and the memset and
>> all, Have you looked at the patches at all? I feel like a mute person ;-(
> 
> No, it's not same at all. Please look at the patchset again. Your
> interface is:
> 
> +void blk_init_rq(struct request_queue *q, struct request *rq, int cmd_flags)
> 
> I don't like that. It's hacky. You needed that hacky inferface because
> you didn't fix the root problem, rq_init was not able to do a full
> memset().
> 

What are you talking about? I did the full memset and moved the call to
blk_alloc_request() exactly like you did it. I added the flags param because
I think it was nice, since all places need set the flags afterwards so it is 
more elegant with the flags in the function call. Also it tells user that we 
must set what kind of request it is just like blk_alloc_request() takes rw flags 
parameter. So I did not do any hacking I did it because it is more stylish.
But if you don't like it, that's fine, but at least comment on the exact problems 
so I know what you don't like.

> I fixed the root problem with this patch:
> 
> http://marc.info/?l=linux-scsi&m=120901807514386&w=2
> 
> 
> Then, we have the same interface as before:
> 
> +void blk_rq_init(struct request_queue *q, struct request *rq)
> 

As I said, I did not want same interface as before I wanted the flags
added so it is easier for users.

> 
>> If you are going to export the rq_init function then I think the name is very
>> wrong. And you have not exported it?
> 
> It was renamed blk_rq_init since we have lots of blk_rq_* functions.

Sorry. yes you did that in the next patch. I should read all mail before
I reply.

Any way it all looks very good to me. I am just now compiling it together 
with the scsi bits for testing with the OSD stack, but I'm sure it is all
good.

Thanks Tomo, this looks promising. I hope it will get upstream soon, I
wish you would have CCed me on all these patches so I get notify of their
progress into the trees. At the end, I'm the one that needs them the most.

Boaz