scsi_sgtable implementation

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Boaz Harrosh
Sent: Thursday, July 05, 2007 6:43 AM
To: James Bottomley; FUJITA Tomonori; Andrew Morton; linux-scsi
Subject: [RFC 1/8] stex driver BROKEN

I just comment out the code, but now user-mode is broken what to do ??

---
 drivers/scsi/stex.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

drivers/scsi/stex.c +2 -2

index adda296..935e2a6 100644
@@ -719,8 +719,8 @@ static void stex_ys_commands(struct st_hba *hba,
 
 	if (ccb->cmd->cmnd[0] == MGT_CMD &&
 		resp->scsi_status != SAM_STAT_CHECK_CONDITION) {
-		scsi_bufflen(ccb->cmd) =
-			le32_to_cpu(*(__le32 *)&resp->variable[0]);
+/*		scsi_bufflen(ccb->cmd) =
+			le32_to_cpu(*(__le32 *)&resp->variable[0]);*/
 		return;
 	}
 

Signature · 4 lines

1.5.2.2.249.g45fd


-

- The old scsi_{alloc,free}_sgtable() is no longer exported. This will break scsi_sgt which will need to be converted to new implementation.

drivers/scsi/scsi_lib.c +6 -0

+/*
+ * Should fit within a single page.
+ */
+enum { SCSI_MAX_SG_SEGMENTS =
+	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
+	sizeof(struct scatterlist)) };

Quoted 13 lines · Boaz Harrosh

drivers/scsi/scsi_lib.c +13 -0

+/*
+ * Should fit within a single page.
+ */
+enum { SCSI_MAX_SG_SEGMENTS =
+	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
+	sizeof(struct scatterlist)) };
+
+enum { SG_MEMPOOL_NR =
+	(SCSI_MAX_SG_SEGMENTS >= 7) +
+	(SCSI_MAX_SG_SEGMENTS >= 15) +
+	(SCSI_MAX_SG_SEGMENTS >= 31) +
+	(SCSI_MAX_SG_SEGMENTS >= 63) +
+	(SCSI_MAX_SG_SEGMENTS >= 127) +

drivers/scsi/scsi_lib.c line 55

Quoted 4 lines

drivers/scsi/scsi_lib.c +3 -0

+	(SCSI_MAX_SG_SEGMENTS >= 255) +
+	(SCSI_MAX_SG_SEGMENTS >= 511)
+};

What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or some other arch, we were going over a page when doing SCSI_MAX_PHYS_SEGMENTS of 256 right?

What happened to Jens's scatter list chaining and how does this relate to it then?

From: Mike Christie <[email protected]>
Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
Date: Thu, 12 Jul 2007 14:09:44 -0500

Quoted 13 lines

Quoted 12 lines · Boaz Harrosh

drivers/scsi/scsi_lib.c +12 -0

+/*
+ * Should fit within a single page.
+ */
+enum { SCSI_MAX_SG_SEGMENTS =
+	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
+	sizeof(struct scatterlist)) };
+
+enum { SG_MEMPOOL_NR =
+	(SCSI_MAX_SG_SEGMENTS >= 7) +
+	(SCSI_MAX_SG_SEGMENTS >= 15) +
+	(SCSI_MAX_SG_SEGMENTS >= 31) +
+	(SCSI_MAX_SG_SEGMENTS >= 63) +

Quoted 8 lines

drivers/scsi/scsi_lib.c line 55

Quoted 5 lines

drivers/scsi/scsi_lib.c +4 -0

+	(SCSI_MAX_SG_SEGMENTS >= 127) +
+	(SCSI_MAX_SG_SEGMENTS >= 255) +
+	(SCSI_MAX_SG_SEGMENTS >= 511)
+};

What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or some other arch, we were going over a page when doing SCSI_MAX_PHYS_SEGMENTS of 256 right?

Seems that 170 with x86 and 127 with x86_64.

Quoted 3 lines

What happened to Jens's scatter list chaining and how does this relate to it then?

With Jens' sglist, we can set SCSI_MAX_SG_SEGMENTS to whatever we want. We can remove the above code.

We need to push this and Jens' sglist together in one merge window, I think.

Quoted 26 lines · FUJITA Tomonori

From: Mike Christie <[email protected]>
Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
Date: Thu, 12 Jul 2007 14:09:44 -0500

Quoted 13 lines

Quoted 12 lines · Boaz Harrosh

drivers/scsi/scsi_lib.c +12 -0

+/*
+ * Should fit within a single page.
+ */
+enum { SCSI_MAX_SG_SEGMENTS =
+	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
+	sizeof(struct scatterlist)) };
+
+enum { SG_MEMPOOL_NR =
+	(SCSI_MAX_SG_SEGMENTS >= 7) +
+	(SCSI_MAX_SG_SEGMENTS >= 15) +
+	(SCSI_MAX_SG_SEGMENTS >= 31) +
+	(SCSI_MAX_SG_SEGMENTS >= 63) +

Quoted 8 lines

drivers/scsi/scsi_lib.c line 55

Quoted 5 lines

drivers/scsi/scsi_lib.c +4 -0

+	(SCSI_MAX_SG_SEGMENTS >= 127) +
+	(SCSI_MAX_SG_SEGMENTS >= 255) +
+	(SCSI_MAX_SG_SEGMENTS >= 511)
+};

What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or some other arch, we were going over a page when doing SCSI_MAX_PHYS_SEGMENTS of 256 right?

Quoted 2 lines

Seems that 170 with x86 and 127 with x86_64.

with scsi_sgtable we get one less than now

Arch                      | SCSI_MAX_SG_SEGMENTS =  | sizeof(struct scatterlist)
--------------------------|-------------------------|---------------------------
x86_64                    | 127                     |32
i386 CONFIG_HIGHMEM64G=y  | 204                     |20
i386 other                | 255                     |16

What's nice about this code is that now finally it is automatically calculated in compile time. Arch people don't have the headache "did I break SCSI-ml?". For example observe the current bug with i386 CONFIG_HIGHMEM64G=y.

The same should be done with BIO's. Than ARCHs with big pages can gain even more.

Quoted 4 lines

Quoted 3 lines

What happened to Jens's scatter list chaining and how does this relate to it then?

Quoted 5 lines

With Jens' sglist, we can set SCSI_MAX_SG_SEGMENTS to whatever we want. We can remove the above code.

We need to push this and Jens' sglist together in one merge window, I think.

No Tomo the above does not go away. What goes away is maybe:

drivers/scsi/scsi_lib.c +0 -1

 	blk_queue_max_hw_segments(q, shost->sg_tablesize);
-	blk_queue_max_phys_segments(q, SCSI_MAX_SG_SEGMENTS);
 	blk_queue_max_sectors(q, shost->max_sectors);

I'm working on a convergence patches that will do scsi_sg_pools cleanup which is common to both our patches, than scsi_sgtable, and than sg-chaining on top of that. I hope it gets accepted.
The sg-chaining is much much simpler over scsi_sgtables.

Quoted 50 lines · Boaz Harrosh

Quoted 25 lines · FUJITA Tomonori

From: Mike Christie <[email protected]>
Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
Date: Thu, 12 Jul 2007 14:09:44 -0500

Quoted 13 lines

Quoted 12 lines · Boaz Harrosh

drivers/scsi/scsi_lib.c +12 -0

+/*
+ * Should fit within a single page.
+ */
+enum { SCSI_MAX_SG_SEGMENTS =
+	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
+	sizeof(struct scatterlist)) };
+
+enum { SG_MEMPOOL_NR =
+	(SCSI_MAX_SG_SEGMENTS >= 7) +
+	(SCSI_MAX_SG_SEGMENTS >= 15) +
+	(SCSI_MAX_SG_SEGMENTS >= 31) +
+	(SCSI_MAX_SG_SEGMENTS >= 63) +

Quoted 8 lines

drivers/scsi/scsi_lib.c line 55

Quoted 5 lines

drivers/scsi/scsi_lib.c +4 -0

+	(SCSI_MAX_SG_SEGMENTS >= 127) +
+	(SCSI_MAX_SG_SEGMENTS >= 255) +
+	(SCSI_MAX_SG_SEGMENTS >= 511)
+};

What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or some other arch, we were going over a page when doing SCSI_MAX_PHYS_SEGMENTS of 256 right?

Quoted 2 lines

Seems that 170 with x86 and 127 with x86_64.

with scsi_sgtable we get one less than now

Arch                      | SCSI_MAX_SG_SEGMENTS =  | sizeof(struct scatterlist)
--------------------------|-------------------------|---------------------------
x86_64                    | 127                     |32
i386 CONFIG_HIGHMEM64G=y  | 204                     |20
i386 other                | 255                     |16

What's nice about this code is that now finally it is automatically calculated in compile time. Arch people don't have the headache "did I break SCSI-ml?". For example observe the current bug with i386 CONFIG_HIGHMEM64G=y.

The same should be done with BIO's. Than ARCHs with big pages can gain even more.

Quoted 5 lines

Quoted 2 lines

What happened to Jens's scatter list chaining and how does this relate to it then?

With Jens' sglist, we can set SCSI_MAX_SG_SEGMENTS to whatever we want. We can remove the above code.

Quoted 3 lines

Quoted 2 lines

We need to push this and Jens' sglist together in one merge window, I think.

No Tomo the above does not go away. What goes away is maybe:

It does go away, since we can just set it to some safe value and use chaining to get us where we want.

drivers/scsi/scsi_lib.c struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost, · line 1609

Quoted 5 lines

drivers/scsi/scsi_lib.c +0 -1

 	blk_queue_max_hw_segments(q, shost->sg_tablesize);
-	blk_queue_max_phys_segments(q, SCSI_MAX_SG_SEGMENTS);
 	blk_queue_max_sectors(q, shost->max_sectors);

Quoted 4 lines

I'm working on a convergence patches that will do scsi_sg_pools cleanup which is common to both our patches, than scsi_sgtable, and than sg-chaining on top of that. I hope it gets accepted.
The sg-chaining is much much simpler over scsi_sgtables.

Sorry, I don't follow this paragraph at all. What is the scsi_sgtables change you are referring to? And how does it make sg chaining so much simpler?

I guess my problem is that I don't know what problem this scsi_sgtables you refer to is fixing?

Quoted 53 lines · Jens Axboe

Quoted 52 lines · Boaz Harrosh

Quoted 25 lines · FUJITA Tomonori

From: Mike Christie <[email protected]>
Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
Date: Thu, 12 Jul 2007 14:09:44 -0500

Quoted 13 lines

Quoted 12 lines · Boaz Harrosh

drivers/scsi/scsi_lib.c +12 -0

+/*
+ * Should fit within a single page.
+ */
+enum { SCSI_MAX_SG_SEGMENTS =
+	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
+	sizeof(struct scatterlist)) };
+
+enum { SG_MEMPOOL_NR =
+	(SCSI_MAX_SG_SEGMENTS >= 7) +
+	(SCSI_MAX_SG_SEGMENTS >= 15) +
+	(SCSI_MAX_SG_SEGMENTS >= 31) +
+	(SCSI_MAX_SG_SEGMENTS >= 63) +

Quoted 8 lines

drivers/scsi/scsi_lib.c line 55

Quoted 5 lines

drivers/scsi/scsi_lib.c +4 -0

+	(SCSI_MAX_SG_SEGMENTS >= 127) +
+	(SCSI_MAX_SG_SEGMENTS >= 255) +
+	(SCSI_MAX_SG_SEGMENTS >= 511)
+};

What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or some other arch, we were going over a page when doing SCSI_MAX_PHYS_SEGMENTS of 256 right?

Quoted 2 lines

Seems that 170 with x86 and 127 with x86_64.

with scsi_sgtable we get one less than now

Arch                      | SCSI_MAX_SG_SEGMENTS =  | sizeof(struct scatterlist)
--------------------------|-------------------------|---------------------------
x86_64                    | 127                     |32
i386 CONFIG_HIGHMEM64G=y  | 204                     |20
i386 other                | 255                     |16

What's nice about this code is that now finally it is automatically calculated in compile time. Arch people don't have the headache "did I break SCSI-ml?". For example observe the current bug with i386 CONFIG_HIGHMEM64G=y.

The same should be done with BIO's. Than ARCHs with big pages can gain even more.

Quoted 7 lines

Quoted 2 lines

What happened to Jens's scatter list chaining and how does this relate to it then?

With Jens' sglist, we can set SCSI_MAX_SG_SEGMENTS to whatever we want. We can remove the above code.

We need to push this and Jens' sglist together in one merge window, I think.

Quoted 4 lines

Quoted 1 line

No Tomo the above does not go away. What goes away is maybe:

It does go away, since we can just set it to some safe value and use chaining to get us where we want.

In my patches SCSI_MAX_PHYS_SEGMENTS has went away it does not exist anymore.

The new SCSI_MAX_SG_SEGMENTS (your name by the way) is right there and is calculated to maximize allocation in one page.

(I guess the right name is SCSI_MAX_PHYS_SEGMENTS_IN_A_PAGE) which will be needed in both your patches and mine.

Quoted 4 lines

drivers/scsi/scsi_lib.c struct request_queue *__scsi_alloc_queue(struct Scsi_Host *shost, · line 1609

Quoted 4 lines

drivers/scsi/scsi_lib.c +0 -1

 	blk_queue_max_hw_segments(q, shost->sg_tablesize);
-	blk_queue_max_phys_segments(q, SCSI_MAX_SG_SEGMENTS);
 	blk_queue_max_sectors(q, shost->max_sectors);

The reporting above is not needed and can be what ever block layer considers safe/optimized.

Quoted 5 lines

Quoted 4 lines

I'm working on a convergence patches that will do scsi_sg_pools cleanup which is common to both our patches, than scsi_sgtable, and than sg-chaining on top of that. I hope it gets accepted.
The sg-chaining is much much simpler over scsi_sgtables.

Quoted 7 lines

Sorry, I don't follow this paragraph at all. What is the scsi_sgtables change you are referring to? And how does it make sg chaining so much simpler?

I guess my problem is that I don't know what problem this scsi_sgtables you refer to is fixing?

scsi_sgtable is a solution proposed by James Bottomley where all I/O members of struct scsi_cmnd and the resid member, which need to be duplicated for bidirectional transfers, can be allocated together with the sg-list they are pointing to. This way when bidi comes, the all structure can be duplicated with minimal change to code, and with no extra baggage when bidi is not used. This was the all motivation for the data accessors and cleanup, swiping through the entire scsi tree. So when implementation changes drivers do not change with them. Now meanwhile moving over drivers code we (well Tomo mostly) removed the old !use_sg code path, and also abstracted the 2 major hot spots of above usages with scsi_dma_{un,}map, and the scsi_for_each_sg. Actually that one was changed from the original definition to match you macro.

Since scsi_sgtable is an encapsulation of the actual scatterlist array together with the sg_count bufflen and pool_index, it gives code a nice clean OO touch, and makes handling very easy, thats all. It only simplifies things at the scsi-ml level.

Quoted 57 lines · Boaz Harrosh

Quoted 56 lines · Jens Axboe

Quoted 45 lines · Boaz Harrosh

Quoted 25 lines · FUJITA Tomonori

From: Mike Christie <[email protected]>
Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
Date: Thu, 12 Jul 2007 14:09:44 -0500

Quoted 13 lines

Quoted 12 lines · Boaz Harrosh

(quoted diff) +12 -0

+/*
+ * Should fit within a single page.
+ */
+enum { SCSI_MAX_SG_SEGMENTS =
+	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
+	sizeof(struct scatterlist)) };
+
+enum { SG_MEMPOOL_NR =
+	(SCSI_MAX_SG_SEGMENTS >= 7) +
+	(SCSI_MAX_SG_SEGMENTS >= 15) +
+	(SCSI_MAX_SG_SEGMENTS >= 31) +
+	(SCSI_MAX_SG_SEGMENTS >= 63) +

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +4 -0

+	(SCSI_MAX_SG_SEGMENTS >= 127) +
+	(SCSI_MAX_SG_SEGMENTS >= 255) +
+	(SCSI_MAX_SG_SEGMENTS >= 511)
+};

What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or some other arch, we were going over a page when doing SCSI_MAX_PHYS_SEGMENTS of 256 right?

Quoted 2 lines

Seems that 170 with x86 and 127 with x86_64.

with scsi_sgtable we get one less than now

Arch                      | SCSI_MAX_SG_SEGMENTS =  | sizeof(struct scatterlist)
--------------------------|-------------------------|---------------------------
x86_64                    | 127                     |32
i386 CONFIG_HIGHMEM64G=y  | 204                     |20
i386 other                | 255                     |16

What's nice about this code is that now finally it is automatically calculated in compile time. Arch people don't have the headache "did I break SCSI-ml?". For example observe the current bug with i386 CONFIG_HIGHMEM64G=y.

The same should be done with BIO's. Than ARCHs with big pages can gain even more.

Quoted 8 lines

Quoted 7 lines

Quoted 2 lines

What happened to Jens's scatter list chaining and how does this relate to it then?

With Jens' sglist, we can set SCSI_MAX_SG_SEGMENTS to whatever we want. We can remove the above code.

We need to push this and Jens' sglist together in one merge window, I think.

No Tomo the above does not go away. What goes away is maybe:

It does go away, since we can just set it to some safe value and use chaining to get us where we want.

Quoted 2 lines

In my patches SCSI_MAX_PHYS_SEGMENTS has went away it does not exist anymore.

Sure, I could just kill it as well. The point is that it's a parallel development, there's nothing in your patch that helps the sg chaining whatsoever. The only "complex" thing in the SCSI layer for sg chaining, is chaining when allocating and walking that chain on freeing. That's it!

Quoted 3 lines

The new SCSI_MAX_SG_SEGMENTS (your name by the way) is right there and is calculated to maximize allocation in one page.

Yes, it's still a good idea to make sure you get good packing in the page, it's of course cheaper to have less chaining. But it's not critical in the same way as before, when it could impact IO layout and performance.

Quoted 2 lines

(I guess the right name is SCSI_MAX_PHYS_SEGMENTS_IN_A_PAGE) which will be needed in both your patches and mine.

That would be better name.

Quoted 6 lines

Quoted 4 lines

Review

Quoted 4 lines

(quoted diff) +0 -1

 	blk_queue_max_hw_segments(q, shost->sg_tablesize);
-	blk_queue_max_phys_segments(q, SCSI_MAX_SG_SEGMENTS);
 	blk_queue_max_sectors(q, shost->max_sectors);

The reporting above is not needed and can be what ever block layer considers safe/optimized.

You still need to set it, you can't just ignore it. Whether you redefine SCSI_MAX_SG_SEGMENTS or use (unsigned short) -1 doesn't really matter a whole lot.

Quoted 25 lines

Quoted 11 lines

Quoted 4 lines

I'm working on a convergence patches that will do scsi_sg_pools cleanup which is common to both our patches, than scsi_sgtable, and than sg-chaining on top of that. I hope it gets accepted.
The sg-chaining is much much simpler over scsi_sgtables.

Sorry, I don't follow this paragraph at all. What is the scsi_sgtables change you are referring to? And how does it make sg chaining so much simpler?

I guess my problem is that I don't know what problem this scsi_sgtables you refer to is fixing?

scsi_sgtable is a solution proposed by James Bottomley where all I/O members of struct scsi_cmnd and the resid member, which need to be duplicated for bidirectional transfers, can be allocated together with the sg-list they are pointing to. This way when bidi comes, the all structure can be duplicated with minimal change to code, and with no extra baggage when bidi is not used. This was the all motivation for the data accessors and cleanup, swiping through the entire scsi tree. So when implementation changes drivers do not change with them. Now meanwhile moving over drivers code we (well Tomo mostly) removed the old !use_sg code path, and also abstracted the 2 major hot spots of above usages with scsi_dma_{un,}map, and the scsi_for_each_sg. Actually that one was changed from the original definition to match you macro.

Quoted 4 lines

Since scsi_sgtable is an encapsulation of the actual scatterlist array together with the sg_count bufflen and pool_index, it gives code a nice clean OO touch, and makes handling very easy, thats all. It only simplifies things at the scsi-ml level.

So it's a pre-requisite for bidi support, it has no bearing on sg chaining. The only thing they have in common is that the touch some of the same code, that does not make them dependent or related beyond that necessarily.

Quoted 58 lines · Jens Axboe

Quoted 57 lines · Boaz Harrosh

Quoted 56 lines · Jens Axboe

Quoted 45 lines · Boaz Harrosh

Quoted 25 lines · FUJITA Tomonori

From: Mike Christie <[email protected]>
Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
Date: Thu, 12 Jul 2007 14:09:44 -0500

Quoted 13 lines

Quoted 12 lines · Boaz Harrosh

(quoted diff) +12 -0

+/*
+ * Should fit within a single page.
+ */
+enum { SCSI_MAX_SG_SEGMENTS =
+	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
+	sizeof(struct scatterlist)) };
+
+enum { SG_MEMPOOL_NR =
+	(SCSI_MAX_SG_SEGMENTS >= 7) +
+	(SCSI_MAX_SG_SEGMENTS >= 15) +
+	(SCSI_MAX_SG_SEGMENTS >= 31) +
+	(SCSI_MAX_SG_SEGMENTS >= 63) +

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +4 -0

+	(SCSI_MAX_SG_SEGMENTS >= 127) +
+	(SCSI_MAX_SG_SEGMENTS >= 255) +
+	(SCSI_MAX_SG_SEGMENTS >= 511)
+};

What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or some other arch, we were going over a page when doing SCSI_MAX_PHYS_SEGMENTS of 256 right?

Quoted 2 lines

Seems that 170 with x86 and 127 with x86_64.

with scsi_sgtable we get one less than now

Arch                      | SCSI_MAX_SG_SEGMENTS =  | sizeof(struct scatterlist)
--------------------------|-------------------------|---------------------------
x86_64                    | 127                     |32
i386 CONFIG_HIGHMEM64G=y  | 204                     |20
i386 other                | 255                     |16

What's nice about this code is that now finally it is automatically calculated in compile time. Arch people don't have the headache "did I break SCSI-ml?". For example observe the current bug with i386 CONFIG_HIGHMEM64G=y.

The same should be done with BIO's. Than ARCHs with big pages can gain even more.

Quoted 8 lines

Quoted 7 lines

Quoted 2 lines

What happened to Jens's scatter list chaining and how does this relate to it then?

With Jens' sglist, we can set SCSI_MAX_SG_SEGMENTS to whatever we want. We can remove the above code.

We need to push this and Jens' sglist together in one merge window, I think.

No Tomo the above does not go away. What goes away is maybe:

It does go away, since we can just set it to some safe value and use chaining to get us where we want.

Quoted 8 lines

Quoted 2 lines

In my patches SCSI_MAX_PHYS_SEGMENTS has went away it does not exist anymore.

Sure, I could just kill it as well. The point is that it's a parallel development, there's nothing in your patch that helps the sg chaining whatsoever. The only "complex" thing in the SCSI layer for sg chaining, is chaining when allocating and walking that chain on freeing. That's it!

It seems like having the pool index in the sgtable structure simplifies the implementation a bit for allocation and freeing of linked sgtables. Boaz will send an example tomorrow (hopefully) showing how the merged code looks like.

Quoted 68 lines

Quoted 66 lines · Jens Axboe

Quoted 52 lines · Boaz Harrosh

Quoted 51 lines · Jens Axboe

Quoted 50 lines · Boaz Harrosh

Quoted 25 lines · FUJITA Tomonori

From: Mike Christie <[email protected]>
Subject: Re: [RFC 4/8] scsi-ml: scsi_sgtable implementation
Date: Thu, 12 Jul 2007 14:09:44 -0500

Quoted 13 lines

Quoted 12 lines · Boaz Harrosh

(quoted diff) +12 -0

+/*
+ * Should fit within a single page.
+ */
+enum { SCSI_MAX_SG_SEGMENTS =
+	((PAGE_SIZE - sizeof(struct scsi_sgtable)) /
+	sizeof(struct scatterlist)) };
+
+enum { SG_MEMPOOL_NR =
+	(SCSI_MAX_SG_SEGMENTS >= 7) +
+	(SCSI_MAX_SG_SEGMENTS >= 15) +
+	(SCSI_MAX_SG_SEGMENTS >= 31) +
+	(SCSI_MAX_SG_SEGMENTS >= 63) +

Quoted 8 lines

Review

Quoted 5 lines

(quoted diff) +4 -0

+	(SCSI_MAX_SG_SEGMENTS >= 127) +
+	(SCSI_MAX_SG_SEGMENTS >= 255) +
+	(SCSI_MAX_SG_SEGMENTS >= 511)
+};

What does SCSI_MAX_SG_SEGMENTS end up being on x86 now? On x86_64 or some other arch, we were going over a page when doing SCSI_MAX_PHYS_SEGMENTS of 256 right?

Quoted 2 lines

Seems that 170 with x86 and 127 with x86_64.

with scsi_sgtable we get one less than now

Arch                      | SCSI_MAX_SG_SEGMENTS =  | sizeof(struct scatterlist)
--------------------------|-------------------------|---------------------------
x86_64                    | 127                     |32
i386 CONFIG_HIGHMEM64G=y  | 204                     |20
i386 other                | 255                     |16

What's nice about this code is that now finally it is automatically calculated in compile time. Arch people don't have the headache "did I break SCSI-ml?". For example observe the current bug with i386 CONFIG_HIGHMEM64G=y.

The same should be done with BIO's. Than ARCHs with big pages can gain even more.

Quoted 5 lines

Quoted 2 lines

What happened to Jens's scatter list chaining and how does this relate to it then?

With Jens' sglist, we can set SCSI_MAX_SG_SEGMENTS to whatever we want. We can remove the above code.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

Quoted 2 lines

We need to push this and Jens' sglist together in one merge window, I think.

No Tomo the above does not go away. What goes away is maybe:

It does go away, since we can just set it to some safe value and use chaining to get us where we want.

In my patches SCSI_MAX_PHYS_SEGMENTS has went away it does not exist anymore.

Sure, I could just kill it as well. The point is that it's a parallel development, there's nothing in your patch that helps the sg chaining whatsoever. The only "complex" thing in the SCSI layer for sg chaining, is chaining when allocating and walking that chain on freeing. That's it!

Quoted 4 lines

It seems like having the pool index in the sgtable structure simplifies the implementation a bit for allocation and freeing of linked sgtables. Boaz will send an example tomorrow (hopefully) showing how the merged code looks like.

The index stuff isn't complex, so I don't think you can call that a real simplification. It's not for free either, there's a size cost to pay.

I've attached the sgtable patch for review. It's against the sglist-arch branch in Jens' tree.

---
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] move all the I/O descriptors to a new scsi_sgtable structure

based on Boaz Harrosh <[email protected]> scsi_sgtable patch.

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

---
 drivers/scsi/scsi_lib.c  |   92 +++++++++++++++++++++++++++------------------
 include/scsi/scsi_cmnd.h |   39 +++++++++++++------
 2 files changed, 82 insertions(+), 49 deletions(-)

drivers/scsi/scsi_lib.c +0 -0

index 5fb1048..2fa1852 100644

drivers/scsi/scsi_lib.c +2 -1

 static void scsi_run_queue(struct request_queue *q);
 
 /*

@@ -731,16 +733,27 @@ static inline unsigned int scsi_sgtable_index(unsigned short nents)
 	return index;
 }
 
-struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
+struct scsi_sgtable *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask,
+					int sg_count)
 {

drivers/scsi/scsi_lib.c +2 -8

-	left = cmd->use_sg;
+	left = sg_count;
 	ret = prev = NULL;
 	do {
 		this = left;

@@ -764,7 +777,7 @@ struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 		 * first loop through, set initial index and return value
 		 */
 		if (!ret) {
-			cmd->sglist_len = index;
+			sgt->sglist_len = index;
 			ret = sgl;
 		}
 

@@ -776,21 +789,18 @@ struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 		if (prev)
 			sg_chain(prev, SCSI_MAX_SG_SEGMENTS, sgl);
 
-		/*
-		 * don't allow subsequent mempool allocs to sleep, it would
-		 * violate the mempool principle.
-		 */
-		gfp_mask &= ~__GFP_WAIT;
-		gfp_mask |= __GFP_HIGH;
 		prev = sgl;
 	} while (left);
 
 	/*

drivers/scsi/scsi_lib.c +18 -12

+	return sgt;
 enomem:
 	if (ret) {
 		/*

@@ -809,6 +819,8 @@ enomem:
 
 		mempool_free(prev, sgp->pool);
 	}
+	kmem_cache_free(scsi_sgtable_cache, sgt);
+
 	return NULL;
 }
 

@@ -816,21 +828,22 @@ EXPORT_SYMBOL(scsi_alloc_sgtable);
 
 void scsi_free_sgtable(struct scsi_cmnd *cmd)
 {
-	struct scatterlist *sgl = cmd->request_buffer;
+	struct scsi_sgtable *sgt = cmd->sgtable;
+	struct scatterlist *sgl = sgt->sglist;
 	struct scsi_host_sg_pool *sgp;
 
-	BUG_ON(cmd->sglist_len >= SG_MEMPOOL_NR);
+	BUG_ON(sgt->sglist_len >= SG_MEMPOOL_NR);
 
 	/*
 	 * if this is the biggest size sglist, check if we have
 	 * chained parts we need to free
 	 */
-	if (cmd->__use_sg > SCSI_MAX_SG_SEGMENTS) {
+	if (sgt->__sg_count > SCSI_MAX_SG_SEGMENTS) {
 		unsigned short this, left;
 		struct scatterlist *next;
 		unsigned int index;
 
-		left = cmd->__use_sg - (SCSI_MAX_SG_SEGMENTS - 1);
+		left = sgt->__sg_count - (SCSI_MAX_SG_SEGMENTS - 1);
 		next = sg_chain_ptr(&sgl[SCSI_MAX_SG_SEGMENTS - 1]);
 		while (left && next) {
 			sgl = next;

@@ -854,11 +867,12 @@ void scsi_free_sgtable(struct scsi_cmnd *cmd)
 		/*
 		 * Restore original, will be freed below
 		 */
-		sgl = cmd->request_buffer;
+		sgl = sgt->sglist;
 	}
 
-	sgp = scsi_sg_pools + cmd->sglist_len;
+	sgp = scsi_sg_pools + sgt->sglist_len;
 	mempool_free(sgl, sgp->pool);
+	kmem_cache_free(scsi_sgtable_cache, sgt);
 }
 
 EXPORT_SYMBOL(scsi_free_sgtable);

@@ -882,15 +896,14 @@ EXPORT_SYMBOL(scsi_free_sgtable);
  */
 static void scsi_release_buffers(struct scsi_cmnd *cmd)
 {
-	if (cmd->use_sg)
+	if (cmd->sgtable)
 		scsi_free_sgtable(cmd);
 
 	/*
 	 * Zero these out.  They now point to freed memory, and it is
 	 * dangerous to hang onto the pointers.
 	 */
-	cmd->request_buffer = NULL;
-	cmd->request_bufflen = 0;
+	cmd->sgtable = NULL;
 }
 
 /*

@@ -924,13 +937,14 @@ static void scsi_release_buffers(struct scsi_cmnd *cmd)
 void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 {
 	int result = cmd->result;
-	int this_count = cmd->request_bufflen;
+	int this_count = scsi_bufflen(cmd);
 	request_queue_t *q = cmd->device->request_queue;
 	struct request *req = cmd->request;
 	int clear_errors = 1;
 	struct scsi_sense_hdr sshdr;
 	int sense_valid = 0;
 	int sense_deferred = 0;
+	int resid = scsi_get_resid(cmd);
 
 	scsi_release_buffers(cmd);
 

@@ -956,7 +970,7 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
 				req->sense_len = len;
 			}
 		}
-		req->data_len = cmd->resid;
+		req->data_len = resid;
 	}
 
 	/*

@@ -1098,6 +1112,7 @@ EXPORT_SYMBOL(scsi_io_completion);
 static int scsi_init_io(struct scsi_cmnd *cmd)
 {
 	struct request     *req = cmd->request;
+	struct scsi_sgtable *sgt;
 	int		   count;
 
 	/*

@@ -1105,35 +1120,36 @@ static int scsi_init_io(struct scsi_cmnd *cmd)
 	 * but now we do (it makes highmem I/O easier to support without
 	 * kmapping pages)
 	 */
-	cmd->use_sg = req->nr_phys_segments;
+	sgt = scsi_alloc_sgtable(cmd, GFP_ATOMIC, req->nr_phys_segments);
 
 	/*
 	 * If sg table allocation fails, requeue request later.

drivers/scsi/scsi_lib.c +8 -10

 	req->buffer = NULL;
 	if (blk_pc_request(req))
-		cmd->request_bufflen = req->data_len;
+		sgt->length = req->data_len;
 	else
-		cmd->request_bufflen = req->nr_sectors << 9;
+		sgt->length = req->nr_sectors << 9;
 
 	/* 
 	 * Next, walk the list, and fill in the addresses and sizes of
 	 * each segment.
 	 */
-	count = blk_rq_map_sg(req->q, req, cmd->request_buffer);
-	if (likely(count <= cmd->use_sg)) {
-		cmd->use_sg = count;
+	count = blk_rq_map_sg(req->q, req, cmd->sgtable->sglist);
+	if (likely(count <= sgt->sg_count)) {
+		sgt->sg_count = count;
 		return BLKPREP_OK;
 	}
 
 	printk(KERN_ERR "Incorrect number of segments after building list\n");
-	printk(KERN_ERR "counted %d, received %d\n", count, cmd->use_sg);
+	printk(KERN_ERR "counted %d, received %d\n", count, sgt->sg_count);
 	printk(KERN_ERR "req nr_sec %lu, cur_nr_sec %u\n", req->nr_sectors,
 			req->current_nr_sectors);
 

@@ -1189,7 +1205,7 @@ static void scsi_blk_pc_done(struct scsi_cmnd *cmd)
 	 * successfully. Since this is a REQ_BLOCK_PC command the
 	 * caller should check the request's errors value
 	 */
-	scsi_io_completion(cmd, cmd->request_bufflen);
+	scsi_io_completion(cmd, scsi_bufflen(cmd));
 }
 
 static int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)

@@ -1218,9 +1234,7 @@ static int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)
 		BUG_ON(req->data_len);
 		BUG_ON(req->data);
 
-		cmd->request_bufflen = 0;
-		cmd->request_buffer = NULL;
-		cmd->use_sg = 0;
+		cmd->sgtable = NULL;
 		req->buffer = NULL;
 	}
 

drivers/scsi/scsi_lib.c +0 -0

 	for (i = 0; i < SG_MEMPOOL_NR; i++) {
 		struct scsi_host_sg_pool *sgp = scsi_sg_pools + i;
 		int size = sgp->size * sizeof(struct scatterlist);

include/scsi/scsi_cmnd.h +0 -0

index 937b3c4..9ead940 100644
@@ -11,6 +11,14 @@ struct scatterlist;
 struct Scsi_Host;

include/scsi/scsi_cmnd.h +1 -2

 /* embedded in scsi_cmnd */
 struct scsi_pointer {

@@ -64,10 +72,9 @@ struct scsi_cmnd {
 	/* These elements define the operation we are about to perform */
 #define MAX_COMMAND_SIZE	16
 	unsigned char cmnd[MAX_COMMAND_SIZE];
-	unsigned request_bufflen;	/* Actual request size */
 
 	struct timer_list eh_timeout;	/* Used to time out the command. */
-	void *request_buffer;		/* Actual requested buffer */
+	struct scsi_sgtable *sgtable;
 
 	/* These elements define the operation we ultimately want to perform */
 	unsigned short use_sg;	/* Number of pieces of scatter-gather */

include/scsi/scsi_cmnd.h +16 -5

@@ -133,24 +136,36 @@ extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
 				 size_t *offset, size_t *len);
 extern void scsi_kunmap_atomic_sg(void *virt);
 
-extern struct scatterlist *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t);
+extern struct scsi_sgtable *scsi_alloc_sgtable(struct scsi_cmnd *, gfp_t, int);
 extern void scsi_free_sgtable(struct scsi_cmnd *);
 
 extern int scsi_dma_map(struct scsi_cmnd *cmd);
 extern void scsi_dma_unmap(struct scsi_cmnd *cmd);
 
-#define scsi_sg_count(cmd) ((cmd)->use_sg)
-#define scsi_sglist(cmd) ((struct scatterlist *)(cmd)->request_buffer)
-#define scsi_bufflen(cmd) ((cmd)->request_bufflen)
+static inline unsigned scsi_sg_count(struct scsi_cmnd *cmd)
+{
+	return cmd->sgtable ? cmd->sgtable->sg_count : 0;
+}
+
+static inline struct scatterlist *scsi_sglist(struct scsi_cmnd *cmd)
+{
+	return cmd->sgtable ? cmd->sgtable->sglist : 0;
+}
+
+static inline unsigned scsi_bufflen(struct scsi_cmnd *cmd)
+{
+	return cmd->sgtable ? cmd->sgtable->length : 0;
+}
 
 static inline void scsi_set_resid(struct scsi_cmnd *cmd, int resid)
 {
-	cmd->resid = resid;
+	if (cmd->sgtable)

As proposed by James Bottomley all I/O members of struc scsi_cmnd and the resid member, which need to be duplicated for bidirectional transfers. Can be allocated together with the sg-list they are pointing to. This way when bidi comes the all structure can be duplicated with minimal change to code, and with no extra baggage when bidi is not used. The resulting code is the use of a new mechanism called scsi_sgtable. (Old code will be removed in next patch for easier reviewing)

- sd and sr would adjust IO size to align on device's block size. So code needs to change once we move to scsi_sgtable implementation. (Not compatible with un-converted drivers)
- Convert code to use scsi_for_each_sg
- Use Data accessors wherever is appropriate.

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/scsi/sd.c |   10 ++++------
 drivers/scsi/sr.c |   27 ++++++++++++---------------
 2 files changed, 16 insertions(+), 21 deletions(-)

drivers/scsi/sd.c +1 -1

index 448d316..459fd23 100644
@@ -338,7 +338,7 @@ static int sd_init_command(struct scsi_cmnd * SCpnt)
 	struct request *rq = SCpnt->request;
 	struct gendisk *disk = rq->rq_disk;
 	sector_t block = rq->sector;
-	unsigned int this_count = SCpnt->request_bufflen >> 9;
+	unsigned int this_count = scsi_bufflen(SCpnt) >> 9;
 	unsigned int timeout = sdp->timeout;
 
 	SCSI_LOG_HLQUEUE(1, scmd_printk(KERN_INFO, SCpnt,

drivers/scsi/sd.c +3 -2

 	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,

@@ -480,7 +477,8 @@ static int sd_init_command(struct scsi_cmnd * SCpnt)
 		SCpnt->cmnd[4] = (unsigned char) this_count;
 		SCpnt->cmnd[5] = 0;
 	}
-	SCpnt->request_bufflen = this_count * sdp->sector_size;
+	BUG_ON(!SCpnt->sgtable);
+	SCpnt->sgtable->length = this_count * sdp->sector_size;
 
 	/*
 	 * We shouldn't disconnect in the middle of a sector, so with a dumb

@@ -892,7 +890,7 @@ static struct block_device_operations sd_fops = {
 static void sd_rw_intr(struct scsi_cmnd * SCpnt)
 {
 	int result = SCpnt->result;
- 	unsigned int xfer_size = SCpnt->request_bufflen;
+ 	unsigned int xfer_size = scsi_bufflen(SCpnt);
  	unsigned int good_bytes = result ? 0 : xfer_size;
  	u64 start_lba = SCpnt->request->sector;
  	u64 bad_lba;

drivers/scsi/sr.c +1 -1

index f9a52af..ed61ca9 100644
@@ -218,7 +218,7 @@ int sr_media_change(struct cdrom_device_info *cdi, int slot)
 static void rw_intr(struct scsi_cmnd * SCpnt)
 {
 	int result = SCpnt->result;
-	int this_count = SCpnt->request_bufflen;
+	int this_count = scsi_bufflen(SCpnt);
 	int good_bytes = (result == 0 ? this_count : 0);
 	int block_sectors = 0;
 	long error_sector;

From: Boaz Harrosh <[email protected]>
Subject: [RFC 7/8] sd.c and sr.c move to scsi_sgtable implementation
Date: Thu, 05 Jul 2007 16:44:04 +0300

Quoted 24 lines

- sd and sr would adjust IO size to align on device's block size. So code needs to change once we move to scsi_sgtable implementation. (Not compatible with un-converted drivers)
- Convert code to use scsi_for_each_sg
- Use Data accessors wherever is appropriate.

Signed-off-by Boaz Harrosh <[email protected]>

---
 drivers/scsi/sd.c |   10 ++++------
 drivers/scsi/sr.c |   27 ++++++++++++---------------
 2 files changed, 16 insertions(+), 21 deletions(-)

drivers/scsi/sd.c +1 -1

index 448d316..459fd23 100644
@@ -338,7 +338,7 @@ static int sd_init_command(struct scsi_cmnd * SCpnt)
 	struct request *rq = SCpnt->request;
 	struct gendisk *disk = rq->rq_disk;
 	sector_t block = rq->sector;
-	unsigned int this_count = SCpnt->request_bufflen >> 9;
+	unsigned int this_count = scsi_bufflen(SCpnt) >> 9;
 	unsigned int timeout = sdp->timeout;
 

drivers/scsi/sd.c static int sd_init_command(struct scsi_cmnd * SCpnt) · line 421

Quoted 9 lines

drivers/scsi/sd.c +0 -3

 	SCSI_LOG_HLQUEUE(1, scmd_printk(KERN_INFO, SCpnt,

@@ -418,9 +418,6 @@ static int sd_init_command(struct scsi_cmnd * SCpnt)
 	} else if (rq_data_dir(rq) == READ) {
 		SCpnt->cmnd[0] = READ_6;
 		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-	} else {
-		scmd_printk(KERN_ERR, SCpnt, "Unknown command %x\n", rq->cmd_flags);
-		return 0;
 	}

Why?

Quoted 37 lines

drivers/scsi/sd.c +3 -2

 	SCSI_LOG_HLQUEUE(2, scmd_printk(KERN_INFO, SCpnt,

@@ -480,7 +477,8 @@ static int sd_init_command(struct scsi_cmnd * SCpnt)
 		SCpnt->cmnd[4] = (unsigned char) this_count;
 		SCpnt->cmnd[5] = 0;
 	}
-	SCpnt->request_bufflen = this_count * sdp->sector_size;
+	BUG_ON(!SCpnt->sgtable);
+	SCpnt->sgtable->length = this_count * sdp->sector_size;
 
 	/*
 	 * We shouldn't disconnect in the middle of a sector, so with a dumb

@@ -892,7 +890,7 @@ static struct block_device_operations sd_fops = {
 static void sd_rw_intr(struct scsi_cmnd * SCpnt)
 {
 	int result = SCpnt->result;
- 	unsigned int xfer_size = SCpnt->request_bufflen;
+ 	unsigned int xfer_size = scsi_bufflen(SCpnt);
  	unsigned int good_bytes = result ? 0 : xfer_size;
  	u64 start_lba = SCpnt->request->sector;
  	u64 bad_lba;

drivers/scsi/sr.c +1 -1

index f9a52af..ed61ca9 100644
@@ -218,7 +218,7 @@ int sr_media_change(struct cdrom_device_info *cdi, int slot)
 static void rw_intr(struct scsi_cmnd * SCpnt)
 {
 	int result = SCpnt->result;
-	int this_count = SCpnt->request_bufflen;
+	int this_count = scsi_bufflen(SCpnt);
 	int good_bytes = (result == 0 ? this_count : 0);
 	int block_sectors = 0;
 	long error_sector;

@@ -345,23 +345,20 @@ static int sr_init_command(struct scsi_cmnd * SCpnt)
 	} else if (rq_data_dir(SCpnt->request) == READ) {

drivers/scsi/sr.c static int sr_init_command(struct scsi_cmnd * SCpnt) · line 348

Quoted 6 lines

drivers/scsi/sr.c +0 -3

 		SCpnt->cmnd[0] = READ_10;
 		SCpnt->sc_data_direction = DMA_FROM_DEVICE;
-	} else {
-		blk_dump_rq_flags(SCpnt->request, "Unknown sr command");
-		return 0;
 	}

ditto.

-
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html