SCSI Userspace Target: scsi-ml changes

RFC PATCH In review Mike Christie 3 participants 10 messages Jan 25, 2006 – Feb 6, 2006 SCSI

lore

Share & tools

lore

Mike Christie

author

2006-01-25 02:58 UTC #0 lore raw

RFC PATCH 1/3 SCSI Userspace Target: scsi-ml changes

Export functions needed by tgt_scsi_lib and its LLDs.

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

Signed-off-by FUJITA Tomonori <[email protected]>

drivers/scsi/hosts.c +3 -0

index 5881079..0147af7 100644
@@ -264,6 +264,9 @@ static void scsi_host_dev_release(struct
 	if (shost->work_q)
 		destroy_workqueue(shost->work_q);
 
+	if (shost->uspace_req_q)
+		scsi_free_queue(shost->uspace_req_q);
+
 	scsi_destroy_command_freelist(shost);
 	kfree(shost->shost_data);

drivers/scsi/scsi.c +92 -0

104 lines not on this page · raw message

drivers/scsi/scsi_lib.c +18 -7

index 3574ba9..79f1bdd 100644
@@ -1685,29 +1685,40 @@ u64 scsi_calculate_bounce_limit(struct S
 }
 EXPORT_SYMBOL(scsi_calculate_bounce_limit);
 
-struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
+struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
+					 request_fn_proc *request_fn)
 {
-	struct Scsi_Host *shost = sdev->host;
 	struct request_queue *q;
 
-	q = blk_init_queue(scsi_request_fn, NULL);
+	q = blk_init_queue(request_fn, NULL);
 	if (!q)
 		return NULL;
 
-	blk_queue_prep_rq(q, scsi_prep_fn);
-
 	blk_queue_max_hw_segments(q, shost->sg_tablesize);
 	blk_queue_max_phys_segments(q, SCSI_MAX_PHYS_SEGMENTS);
 	blk_queue_max_sectors(q, shost->max_sectors);
 	blk_queue_bounce_limit(q, scsi_calculate_bounce_limit(shost));
 	blk_queue_segment_boundary(q, shost->dma_boundary);
-	blk_queue_issue_flush_fn(q, scsi_issue_flush_fn);
-	blk_queue_softirq_done(q, scsi_softirq_done);
 
 	if (!shost->use_clustering)
 		clear_bit(QUEUE_FLAG_CLUSTER, &q->queue_flags);
 	return q;
 }
+EXPORT_SYMBOL(__scsi_alloc_queue);
+
+struct request_queue *scsi_alloc_queue(struct scsi_device *sdev)
+{
+	struct request_queue *q;
+
+	q = __scsi_alloc_queue(sdev->host, scsi_request_fn);
+	if (!q)
+		return NULL;
+
+	blk_queue_prep_rq(q, scsi_prep_fn);
+	blk_queue_issue_flush_fn(q, scsi_issue_flush_fn);
+	blk_queue_softirq_done(q, scsi_softirq_done);
+	return q;
+}
 
 void scsi_free_queue(struct request_queue *q)
 {

drivers/scsi/scsi_scan.c +2 -1

index 752fb5d..0fdd11a 100644
@@ -1013,7 +1013,7 @@ static void scsi_sequential_lun_scan(str
  *     Given a struct scsi_lun of: 0a 04 0b 03 00 00 00 00, this function returns
  *     the integer: 0x0b030a04
  **/
-static int scsilun_to_int(struct scsi_lun *scsilun)
+int scsilun_to_int(struct scsi_lun *scsilun)
 {
 	int i;
 	unsigned int lun;

@@ -1024,6 +1024,7 @@ static int scsilun_to_int(struct scsi_lu
 			      scsilun->scsi_lun[i + 1]) << (i * 8));
 	return lun;
 }
+EXPORT_SYMBOL(scsilun_to_int);
 
 /**
  * int_to_scsilun: reverts an int into a scsi_lun

include/scsi/scsi_cmnd.h +10 -0

index 7529f43..0bcc352 100644
@@ -8,6 +8,7 @@
 
 struct request;
 struct scatterlist;
+struct Scsi_Host;
 struct scsi_device;
 struct scsi_request;

@@ -31,6 +32,7 @@ struct scsi_pointer {
 struct scsi_cmnd {
 	int     sc_magic;
 
+	struct Scsi_Host *shost;
 	struct scsi_device *device;
 	struct scsi_request *sc_request;

@@ -131,6 +133,11 @@ struct scsi_cmnd {
 
 	unsigned char tag;	/* SCSI-II queued command tag */
 	unsigned long pid;	/* Process ID, starts at 0. Unique per host. */
+	/*
+	 * Work struct to process completion of scsi commands in process
+	 * context. (this should be ifdefd, we could try to do the SG_IO option)
+	 */
+	struct work_struct work;
 };
 
 /*

@@ -147,7 +154,10 @@ struct scsi_cmnd {
 #define SCSI_STATE_MLQUEUE         0x100b
 
 
+extern struct scsi_cmnd *scsi_host_get_command(struct Scsi_Host *,
+					       enum dma_data_direction, gfp_t);
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
+extern void scsi_host_put_command(struct scsi_cmnd *);
 extern void scsi_put_command(struct scsi_cmnd *);
 extern void scsi_io_completion(struct scsi_cmnd *, unsigned int, unsigned int);
 extern void scsi_finish_command(struct scsi_cmnd *cmd);

include/scsi/scsi_device.h +1 -0

index e94ca4d..1b09f4f 100644
@@ -264,6 +264,7 @@ extern void scsi_target_block(struct dev
 extern void scsi_target_unblock(struct device *);
 extern void scsi_remove_target(struct device *);
 extern void int_to_scsilun(unsigned int, struct scsi_lun *);
+extern int scsilun_to_int(struct scsi_lun *);
 extern const char *scsi_device_state_name(enum scsi_device_state);
 extern int scsi_is_sdev_device(const struct device *);
 extern int scsi_is_target_device(const struct device *);

include/scsi/scsi_host.h +25 -0

index 467274a..fbab68c 100644
@@ -7,6 +7,7 @@
 #include <linux/workqueue.h>
 #include <linux/mutex.h>
 
+struct request_queue;
 struct block_device;
 struct completion;
 struct module;

@@ -123,6 +124,21 @@ struct scsi_host_template {
 			     void (*done)(struct scsi_cmnd *));
 
 	/*
+	 * The transfer functions are used to queue a scsi command to
+	 * the LLD. When the driver is finished processing the command
+	 * the done callback is invoked.
+	 *
+	 * return values: see queuecommand
+	 *
+	 * STATUS: REQUIRED FOR TARGET DRIVERS
+	 */
+	/* TODO: rename */
+	int (* transfer_response)(struct scsi_cmnd *,
+				  void (*done)(struct scsi_cmnd *));
+	int (* transfer_data)(struct scsi_cmnd *,
+			      void (*done)(struct scsi_cmnd *));
+
+	/*
 	 * This is an error handling strategy routine.  You don't need to
 	 * define one of these if you don't want to - there is a default
 	 * routine that is present that should work in most cases.  For those

@@ -573,6 +589,12 @@ struct Scsi_Host {
 	 */
 	unsigned int max_host_blocked;
 
+	/*
+	 * q used for scsi_tgt msgs, async events or any other requests that
+	 * need to be processed in userspace
+ 	 */
+	struct request_queue *uspace_req_q;
+
 	/* legacy crap */
 	unsigned long base;
 	unsigned long io_port;

@@ -675,6 +697,9 @@ extern void scsi_unblock_requests(struct
 extern void scsi_block_requests(struct Scsi_Host *);
 
 struct class_container;
+
+extern struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost,
+					     void (*) (struct request_queue *));
 /*
  * These two functions are used to allocate and free a pseudo device
  * which will connect to the host adapter itself rather than any

James Bottomley

2006-02-05 18:30 UTC #1 lore raw

OK ... I think this looks good enough to fix up in tree. I just have one minor quibble, so could you fix that and I'll put it in.

include/scsi/scsi_cmnd.h struct scsi_pointer { · line 36

Quoted 6 lines · Mike Christie

include/scsi/scsi_cmnd.h +1 -0

@@ -31,6 +32,7 @@ struct scsi_pointer {
 struct scsi_cmnd {
        int     sc_magic;
 
+       struct Scsi_Host *shost;
        struct scsi_device *device;

This is an unnecessary addition; you can get this from device->host.

Do you have a pointer for the userspace side of this yet?

James

FUJITA Tomonori

2006-02-06 02:34 UTC #2 lore raw

From: James Bottomley <[email protected]>
Subject: Re: [PATCH RFC 1/3] SCSI Userspace Target: scsi-ml changes
Date: Sun, 05 Feb 2006 12:30:28 -0600

Quoted 4 lines

OK ... I think this looks good enough to fix up in tree. I just have one minor quibble, so could you fix that and I'll put it in.
On Tue, 2006-01-24 at 20:58 -0600, Mike Christie wrote:

Quoted 8 lines

include/scsi/scsi_cmnd.h struct scsi_pointer { · line 36
Quoted 6 lines
include/scsi/scsi_cmnd.h +1 -0
@@ -31,6 +32,7 @@ struct scsi_pointer {
 struct scsi_cmnd {
        int     sc_magic;
 
+       struct Scsi_Host *shost;
        struct scsi_device *device;
This is an unnecessary addition; you can get this from device->host.

Thanks a lot. I've just fixed this.

As Mike said, we replaced our mapping code with the standard helper functions (such as bio_map_user) and some netlink code with the packet socket interface. We've been preparing for the second submission. We will do that in a couple of days.

Quoted 1 line

Do you have a pointer for the userspace side of this yet?

Yes, we have though it's really messy.

http://svn.berlios.de/svnroot/repos/stgt/branches/use-scsi-ml/usr

The current code doesn't catch up with the changes that I've discribed. We'll update it until the seccond submission.

Mike Christie

author

2006-02-06 17:54 UTC #3 lore raw

Quoted 14 lines · FUJITA Tomonori

From: James Bottomley <[email protected]>
Subject: Re: [PATCH RFC 1/3] SCSI Userspace Target: scsi-ml changes
Date: Sun, 05 Feb 2006 12:30:28 -0600
Quoted 9 lines
OK ... I think this looks good enough to fix up in tree. I just have one minor quibble, so could you fix that and I'll put it in.
Quoted 4 lines · Mike Christie
include/scsi/scsi_cmnd.h +0 -0
@@ -31,6 +32,7 @@ struct scsi_pointer {
 struct scsi_cmnd {
       int     sc_magic;
 

Quoted 8 lines

Quoted 4 lines
include/scsi/scsi_cmnd.h struct scsi_pointer { · line 36
Quoted 2 lines
include/scsi/scsi_cmnd.h +1 -0
+       struct Scsi_Host *shost;
       struct scsi_device *device;
This is an unnecessary addition; you can get this from device->host.
Thanks a lot. I've just fixed this.

We actually need ths shost pointer becuase we do not represent the LU in the kernel. So we go from:

scsi_host -> host's uspace request_queue (used to pass commands and messages from the target LLD interrup handler to our netlink code/userspace -> in userspace we represent the target, device/LU, and host.

Tomo did you add a virtual scsi_device somewhere?

James Bottomley

2006-02-06 18:03 UTC #4 lore raw

Quoted 8 lines · Mike Christie

We actually need ths shost pointer becuase we do not represent the LU in the kernel. So we go from:
scsi_host -> host's uspace request_queue (used to pass commands and messages from the target LLD interrup handler to our netlink code/userspace -> in userspace we represent the target, device/LU, and host.
Tomo did you add a virtual scsi_device somewhere?

OK, well, the problem is that struct scsi_cmnd is allocated in the critical I/O path. The more elements it has, the more time it takes to set up and tear down. Kenneth Chen of intel has already produced figures showing that you can get measurable I/O improvement simply by dropping fields from it, so I'm very, very reluctant to increase its size.

James

Mike Christie

author

2006-02-06 18:22 UTC #5 lore raw

Quoted 9 lines · James Bottomley

Quoted 5 lines · Mike Christie
We actually need ths shost pointer becuase we do not represent the LU in the kernel. So we go from:
scsi_host -> host's uspace request_queue (used to pass commands and messages from the target LLD interrup handler to our netlink code/userspace -> in userspace we represent the target, device/LU, and host.
Tomo did you add a virtual scsi_device somewhere?

Quoted 7 lines

OK, well, the problem is that struct scsi_cmnd is allocated in the critical I/O path. The more elements it has, the more time it takes to set up and tear down. Kenneth Chen of intel has already produced figures showing that you can get measurable I/O improvement simply by dropping fields from it, so I'm very, very reluctant to increase its size.

I understand. To make this worse, for the target code since we are mapping data like SG_IO block layer code (we used their functions) we need process context for the unmap. To handle this we added a work struct on the scsi_cmnd and the work struct is pretty large and only used for targets. We can move all the target specific to a:

scsi_tgt_cmnd {
	work;
	host;
};

And store out scsi_tgt_cmnd in the scsi_cmnd->host_scribble or somewhere on the SCp. Is that too hacky?

Mike Christie

author

2006-02-06 18:30 UTC #6 lore raw

Quoted 20 lines · Mike Christie

Quoted 10 lines · James Bottomley
Quoted 5 lines · Mike Christie
We actually need ths shost pointer becuase we do not represent the LU in the kernel. So we go from:
scsi_host -> host's uspace request_queue (used to pass commands and messages from the target LLD interrup handler to our netlink code/userspace -> in userspace we represent the target, device/LU, and host.
Tomo did you add a virtual scsi_device somewhere?
Quoted 7 lines
OK, well, the problem is that struct scsi_cmnd is allocated in the critical I/O path. The more elements it has, the more time it takes to set up and tear down. Kenneth Chen of intel has already produced figures showing that you can get measurable I/O improvement simply by dropping fields from it, so I'm very, very reluctant to increase its size.

Quoted 9 lines

I understand. To make this worse, for the target code since we are mapping data like SG_IO block layer code (we used their functions) we need process context for the unmap. To handle this we added a work struct on the scsi_cmnd and the work struct is pretty large and only used for targets. We can move all the target specific to a:
scsi_tgt_cmnd {
    work;
    host;
};
And store out scsi_tgt_cmnd in the scsi_cmnd->host_scribble or somewhere on the SCp. Is that too hacky?

Actually scratch that. We can use the struct requests end_io_data or special pointer to store the scsi_tgt_cmnd so we do not need to reuse any scsi_cmnd fields.

James Bottomley

2006-02-06 18:34 UTC #7 lore raw

In reply to Mike Christie

Quoted 6 lines · Mike Christie

I understand. To make this worse, for the target code since we are mapping data like SG_IO block layer code (we used their functions) we need process context for the unmap. To handle this we added a work struct on the scsi_cmnd and the work struct is pretty large and only used for targets. We can move all the target specific to a:

Quoted 7 lines

scsi_tgt_cmnd {
	work;
	host;
};
And store out scsi_tgt_cmnd in the scsi_cmnd->host_scribble or somewhere on the SCp. Is that too hacky?

Could you take a look at the execute_in_user_context() function I'm hatching to add to the workqueue here:

http://marc.theaimsgroup.com/?l=linux-scsi&m=113916262109180&w=2

And see if that would suit your purpose without any additions to scsi_command (probably the data carried would have to be the actual command).

James

Mike Christie

author

2006-02-06 18:45 UTC #8 lore raw

Quoted 12 lines · James Bottomley

Quoted 8 lines · Mike Christie
I understand. To make this worse, for the target code since we are mapping data like SG_IO block layer code (we used their functions) we need process context for the unmap. To handle this we added a work struct on the scsi_cmnd and the work struct is pretty large and only used for targets. We can move all the target specific to a:
scsi_tgt_cmnd {
	work;
	host;
};
And store out scsi_tgt_cmnd in the scsi_cmnd->host_scribble or somewhere on the SCp. Is that too hacky?

Quoted 9 lines

Could you take a look at the execute_in_user_context() function I'm hatching to add to the workqueue here:
http://marc.theaimsgroup.com/?l=linux-scsi&m=113916262109180&w=2
And see if that would suit your purpose without any additions to scsi_command (probably the data carried would have to be the actual command).

Yeah it looks that will work for the work struct and scsi_host pointer we were adding. Thanks. Will convert our code.

James Bottomley

2006-02-06 22:15 UTC #9 lore raw

Quoted 2 lines · Mike Christie

Yeah it looks that will work for the work struct and scsi_host pointer we were adding. Thanks. Will convert our code.

OK ... don't rely on it just yet ... workqueue.c isn't exactly my code to modify ...

James