[PATCH] SG: cap reserved_size values at max_sectors

[PATCH] SG: cap reserved_size values at max_sectors

[Alan Stern]

This patch (as857) modifies the SG_GET_RESERVED_SIZE and
SG_SET_RESERVED_SIZE ioctls in the sg driver, capping the values at
the device's request_queue's max_sectors value.  This will permit
cdrecord to obtain a legal value for the maximum transfer length,
fixing Bugzilla #7026.

The patch also caps the initial reserved_size value.  There's no
reason to have a reserved buffer larger than max_sectors, since it
would be impossible to use the extra space.

The corresponding ioctls in the block layer are modified similarly,
and the initial value for the reserved_size is set as large as
possible.  This will effectively make it default to max_sectors.
Note that the actual value is meaningless anyway, since block devices
don't have a reserved buffer.

Finally, the BLKSECTGET ioctl is added to sg, so that there will be a
uniform way for users to determine the actual max_sectors value for
any raw SCSI transport.

Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

---

Index: usb-2.6/drivers/scsi/sg.c
===================================================================
--- usb-2.6.orig/drivers/scsi/sg.c
+++ usb-2.6/drivers/scsi/sg.c
@@ -917,6 +917,8 @@ sg_ioctl(struct inode *inode, struct fil
 			return result;
                 if (val < 0)
                         return -EINVAL;
+		val = min_t(int, val,
+				sdp->device->request_queue->max_sectors * 512);
 		if (val != sfp->reserve.bufflen) {
 			if (sg_res_in_use(sfp) || sfp->mmap_called)
 				return -EBUSY;
@@ -925,7 +927,8 @@ sg_ioctl(struct inode *inode, struct fil
 		}
 		return 0;
 	case SG_GET_RESERVED_SIZE:
-		val = (int) sfp->reserve.bufflen;
+		val = min_t(int, sfp->reserve.bufflen,
+				sdp->device->request_queue->max_sectors * 512);
 		return put_user(val, ip);
 	case SG_SET_COMMAND_Q:
 		result = get_user(val, ip);
@@ -1061,6 +1064,9 @@ sg_ioctl(struct inode *inode, struct fil
 		if (sdp->detached)
 			return -ENODEV;
 		return scsi_ioctl(sdp->device, cmd_in, p);
+	case BLKSECTGET:
+		return put_user(sdp->device->request_queue->max_sectors * 512,
+				ip);
 	default:
 		if (read_only)
 			return -EPERM;	/* don't know so take safe approach */
@@ -2339,6 +2345,7 @@ sg_add_sfp(Sg_device * sdp, int dev)
 {
 	Sg_fd *sfp;
 	unsigned long iflags;
+	int bufflen;
 
 	sfp = kzalloc(sizeof(*sfp), GFP_ATOMIC | __GFP_NOWARN);
 	if (!sfp)
@@ -2369,7 +2376,9 @@ sg_add_sfp(Sg_device * sdp, int dev)
 	if (unlikely(sg_big_buff != def_reserved_size))
 		sg_big_buff = def_reserved_size;
 
-	sg_build_reserve(sfp, sg_big_buff);
+	bufflen = min_t(int, sg_big_buff,
+			sdp->device->request_queue->max_sectors * 512);
+	sg_build_reserve(sfp, bufflen);
 	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp:   bufflen=%d, k_use_sg=%d\n",
 			   sfp->reserve.bufflen, sfp->reserve.k_use_sg));
 	return sfp;
Index: usb-2.6/block/ll_rw_blk.c
===================================================================
--- usb-2.6.orig/block/ll_rw_blk.c
+++ usb-2.6/block/ll_rw_blk.c
@@ -1925,6 +1925,8 @@ blk_init_queue_node(request_fn_proc *rfn
 	blk_queue_max_hw_segments(q, MAX_HW_SEGMENTS);
 	blk_queue_max_phys_segments(q, MAX_PHYS_SEGMENTS);
 
+	q->sg_reserved_size = INT_MAX;
+
 	/*
 	 * all done
 	 */
Index: usb-2.6/block/scsi_ioctl.c
===================================================================
--- usb-2.6.orig/block/scsi_ioctl.c
+++ usb-2.6/block/scsi_ioctl.c
@@ -78,7 +78,9 @@ static int sg_set_timeout(request_queue_
 
 static int sg_get_reserved_size(request_queue_t *q, int __user *p)
 {
-	return put_user(q->sg_reserved_size, p);
+	unsigned val = min(q->sg_reserved_size, q->max_sectors << 9);
+
+	return put_user(val, p);
 }
 
 static int sg_set_reserved_size(request_queue_t *q, int __user *p)

Re: [PATCH] SG: cap reserved_size values at max_sectors

[Mike Christie]

Alan Stern wrote:
> This patch (as857) modifies the SG_GET_RESERVED_SIZE and
> SG_SET_RESERVED_SIZE ioctls in the sg driver, capping the values at
> the device's request_queue's max_sectors value.  This will permit
> cdrecord to obtain a legal value for the maximum transfer length,
> fixing Bugzilla #7026.
> 
> The patch also caps the initial reserved_size value.  There's no
> reason to have a reserved buffer larger than max_sectors, since it
> would be impossible to use the extra space.
> 
> The corresponding ioctls in the block layer are modified similarly,
> and the initial value for the reserved_size is set as large as
> possible.  This will effectively make it default to max_sectors.
> Note that the actual value is meaningless anyway, since block devices
> don't have a reserved buffer.
> 
> Finally, the BLKSECTGET ioctl is added to sg, so that there will be a
> uniform way for users to determine the actual max_sectors value for
> any raw SCSI transport.

I think you actually want max_hw_sectors. Well, you might and you might
not :) I think a estimate of the max transfer length would be more like:

unsigned int max_segment_size, max_xfer,
int max_segments;

/*
 * does not account for any weird arch segments boundary limits
 * or vmerge limits
 */
if (!(q->queue_flags & (1 << QUEUE_FLAG_CLUSTER)))
	max_segment_size = PAGE_SIZE;
else
	max_segment_size = q->max_segment_size;

max_segments = min(q->max_hw_segments, q->max_phys_segments);

max_xfer = min(q->max_hw_sectors * 512,
		max_segments * max_segment_size);

return max_xfer;


The problem is that we assume we will get nice large segments. When
using sg it will try to allocate multiple pages and make large segments.
We could hit a bad case where we cannot allocate enough large segments,
so a worst case would result in a max_segment_size of PAGE_SIZE:

max_segments = min(q->max_hw_segments, q->max_phys_segments);

max_xfer = min(q->max_hw_sectors * 512,
		max_segments * PAGE_SIZE);

return max_xfer;

I think you have to take into account the scatterlist limits because
some drivers like lpfc have a small sht->sg_tablesize/q->max_hw_segments
(64), but have a large q->max_hw_sectors (0xFFFF) and uses the default
q->max_sectors (1024). So in the worst case we could end up with a max
transfer size of only 64 * PAGE_SIZE and this a lot smaller than
q->max_sectors.

Re: [PATCH] SG: cap reserved_size values at max_sectors

[Mike Christie]

Mike Christie wrote:
> 
> The problem is that we assume we will get nice large segments. When
> using sg it will try to allocate multiple pages and make large segments.
> We could hit a bad case where we cannot allocate enough large segments,
> so a worst case would result in a max_segment_size of PAGE_SIZE:
> 
> max_segments = min(q->max_hw_segments, q->max_phys_segments);
> 
> max_xfer = min(q->max_hw_sectors * 512,
> 		max_segments * PAGE_SIZE);
> 
> return max_xfer;
> 
Oh yeah, you could also check how large the reserved buffer is already.
The estimate above does not take into account if we had already
allocated a nice large reserved buffer with large segments.

Re: [PATCH] SG: cap reserved_size values at max_sectors

[Alan Stern]

On Tue, 20 Feb 2007, Mike Christie wrote:

> I think you actually want max_hw_sectors. Well, you might and you might
> not :)

I think we do not.  We don't care about the maximum transfer length the
driver can theoretically support; we care about the maximum transfer
length the system will allow.  For example, if max_sectors is 240 and
max_hw_sectors is 512 then the system will reject a attempted transfer of
300 sectors, even though the hardware is capable of carrying it out.

> I think a estimate of the max transfer length would be more like:
> 
> unsigned int max_segment_size, max_xfer,
> int max_segments;
> 
> /*
>  * does not account for any weird arch segments boundary limits
>  * or vmerge limits
>  */
> if (!(q->queue_flags & (1 << QUEUE_FLAG_CLUSTER)))
> 	max_segment_size = PAGE_SIZE;
> else
> 	max_segment_size = q->max_segment_size;
> 
> max_segments = min(q->max_hw_segments, q->max_phys_segments);
> 
> max_xfer = min(q->max_hw_sectors * 512,
> 		max_segments * max_segment_size);

Isn't it possible that this multiplication might overflow?

> 
> return max_xfer;
> 
> 
> The problem is that we assume we will get nice large segments. When
> using sg it will try to allocate multiple pages and make large segments.
> We could hit a bad case where we cannot allocate enough large segments,
> so a worst case would result in a max_segment_size of PAGE_SIZE:
> 
> max_segments = min(q->max_hw_segments, q->max_phys_segments);
> 
> max_xfer = min(q->max_hw_sectors * 512,
> 		max_segments * PAGE_SIZE);
> 
> return max_xfer;

That is a very pessimistic estimate.  I don't see any reason for using it, 
especially since sg sets aside a reserved buffer which guarantees that a 
certain minimum amount of memory will always be available.

> I think you have to take into account the scatterlist limits because
> some drivers like lpfc have a small sht->sg_tablesize/q->max_hw_segments
> (64), but have a large q->max_hw_sectors (0xFFFF) and uses the default
> q->max_sectors (1024). So in the worst case we could end up with a max
> transfer size of only 64 * PAGE_SIZE and this a lot smaller than
> q->max_sectors.

Would you care to rewrite the patch?

Keep in mind particularly that the SG_GET_RESERVED_SIZE ioctl is supposed 
to return the size of the reserved area, not the maximum transfer length.

Alan Stern

Re: [PATCH] SG: cap reserved_size values at max_sectors

[Mike Christie]

Alan Stern wrote:
> On Tue, 20 Feb 2007, Mike Christie wrote:
> 
>> I think you actually want max_hw_sectors. Well, you might and you might
>> not :)
> 
> I think we do not.  We don't care about the maximum transfer length the
> driver can theoretically support; we care about the maximum transfer
> length the system will allow.  For example, if max_sectors is 240 and
> max_hw_sectors is 512 then the system will reject a attempted transfer of
> 300 sectors, even though the hardware is capable of carrying it out.

The sg/REQ_TYPE_BLOCK_PC code (really the scatterlist and request
building code) checks max_hw_sectors so it will allow what the
driver/hardware can support for that value. You are correct for normal
fs io though.

> 
>> I think a estimate of the max transfer length would be more like:
>>
>> unsigned int max_segment_size, max_xfer,
>> int max_segments;
>>
>> /*
>>  * does not account for any weird arch segments boundary limits
>>  * or vmerge limits
>>  */
>> if (!(q->queue_flags & (1 << QUEUE_FLAG_CLUSTER)))
>> 	max_segment_size = PAGE_SIZE;
>> else
>> 	max_segment_size = q->max_segment_size;
>>
>> max_segments = min(q->max_hw_segments, q->max_phys_segments);
>>
>> max_xfer = min(q->max_hw_sectors * 512,
>> 		max_segments * max_segment_size);
> 
> Isn't it possible that this multiplication might overflow?

I am not posting this as actual patch. I was just posting so you could
see what other values come into play. It is not just max_hw_sectors
which limits the size.

> 
>> return max_xfer;
>>
>>
>> The problem is that we assume we will get nice large segments. When
>> using sg it will try to allocate multiple pages and make large segments.
>> We could hit a bad case where we cannot allocate enough large segments,
>> so a worst case would result in a max_segment_size of PAGE_SIZE:
>>
>> max_segments = min(q->max_hw_segments, q->max_phys_segments);
>>
>> max_xfer = min(q->max_hw_sectors * 512,
>> 		max_segments * PAGE_SIZE);
>>
>> return max_xfer;
> 
> That is a very pessimistic    I don't see any reason for using it, 
> especially since sg sets aside a reserved buffer which guarantees that a 
> certain minimum amount of memory will always be available.

Yeah you are right getting memory is not a problem I replied about that
in the other mail. You do not have to use it, but the min of the
reserved buffer and max_sectors or max_hw_sectors could still be off for
drivers that do not support clustering or if there is a weird arch
segment boundary or limit (maybe the arch segment limits and boundary is
not used much though).

I am working on fix to both the hack and make sure the reserved buffer
is actually a size that the driver can handle. It is those patches that
converts sg to use the scatterlist and request helpers and modify the
sg.c code to allocate the reserved buffer within all the system, driver
and hw limits.

If you are pushing your fixes for the current release, I would say do
not wait for me, but I am also saying max_sectors and the reserved
buffer size is not always a correct estimate for a couple reasons.

Re: [PATCH] SG: cap reserved_size values at max_sectors

[Mike Christie]

Mike Christie wrote:
> Yeah you are right getting memory is not a problem I replied about that
> in the other mail. You do not have to use it, but the min of the
> reserved buffer and max_sectors or max_hw_sectors could still be off for
> drivers that do not support clustering or if there is a weird arch
> segment boundary or limit (maybe the arch segment limits and boundary is
> not used much though).
> 
Oh yeah, to handle the clustering I think you could just stick a check
in the sg reserved buffer allocation code to handle that. You could also
stick the max sectors check in there too if you wanted the reserved
buffer to then reflect what was reserved and what a device could handle
at the same time. I am not sure about those other arch segment limits
though. I though I saw something from parsic or something before, but I
am still working on the reserved buffer fixes and just stared digging
into that.

Re: [PATCH] SG: cap reserved_size values at max_sectors

[Alan Stern]

On Tue, 20 Feb 2007, Mike Christie wrote:

> Yeah you are right getting memory is not a problem I replied about that
> in the other mail. You do not have to use it, but the min of the
> reserved buffer and max_sectors or max_hw_sectors could still be off for
> drivers that do not support clustering or if there is a weird arch
> segment boundary or limit (maybe the arch segment limits and boundary is
> not used much though).
> 
> I am working on fix to both the hack and make sure the reserved buffer
> is actually a size that the driver can handle. It is those patches that
> converts sg to use the scatterlist and request helpers and modify the
> sg.c code to allocate the reserved buffer within all the system, driver
> and hw limits.
> 
> If you are pushing your fixes for the current release, I would say do
> not wait for me, but I am also saying max_sectors and the reserved
> buffer size is not always a correct estimate for a couple reasons.

Okay, I'll stand by the original patch for now.  At least it will allow 
cdrecord to work correctly with USB devices.

It is true that many other factors come into play.  In the end, we need a 
way to tell userspace how large a transfer length will work reliably.  It 
should be available to all devices and drivers that export any sort of raw 
I/O interface -- including sg and the block layer's SG_IO interface.

Alan Stern

Re: [PATCH] SG: cap reserved_size values at max_sectors

[Alan Stern]

Doug:

I haven't gotten any feedback on this patch:

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

Pending Mike's efforts to get a completely realistic estimate for the
maximum allowable size of a transfer request, is there any reason not to
accept the patch in its original form?  Will Mike's other recent changes
affect it at all?

Alan Stern

Re: [PATCH] SG: cap reserved_size values at max_sectors

[Douglas Gilbert]

Alan Stern wrote:
> This patch (as857) modifies the SG_GET_RESERVED_SIZE and
> SG_SET_RESERVED_SIZE ioctls in the sg driver, capping the values at
> the device's request_queue's max_sectors value.  This will permit
> cdrecord to obtain a legal value for the maximum transfer length,
> fixing Bugzilla #7026.
> 
> The patch also caps the initial reserved_size value.  There's no
> reason to have a reserved buffer larger than max_sectors, since it
> would be impossible to use the extra space.
> 
> The corresponding ioctls in the block layer are modified similarly,
> and the initial value for the reserved_size is set as large as
> possible.  This will effectively make it default to max_sectors.
> Note that the actual value is meaningless anyway, since block devices
> don't have a reserved buffer.
> 
> Finally, the BLKSECTGET ioctl is added to sg, so that there will be a
> uniform way for users to determine the actual max_sectors value for
> any raw SCSI transport.
> 
> Signed-off-by: Alan Stern <stern@rowland.harvard.edu>

Alan,
I have voiced my concerns about this earlier but I will
now sign off to unblock the process (and deal with the
consequences to sg users, if any).

Signed-off-by: Douglas Gilbert <dougg@torque.net>

> 
> ---
> 
> Index: usb-2.6/drivers/scsi/sg.c
> ===================================================================
> --- usb-2.6.orig/drivers/scsi/sg.c
> +++ usb-2.6/drivers/scsi/sg.c
> @@ -917,6 +917,8 @@ sg_ioctl(struct inode *inode, struct fil
>  			return result;
>                  if (val < 0)
>                          return -EINVAL;
> +		val = min_t(int, val,
> +				sdp->device->request_queue->max_sectors * 512);
>  		if (val != sfp->reserve.bufflen) {
>  			if (sg_res_in_use(sfp) || sfp->mmap_called)
>  				return -EBUSY;
> @@ -925,7 +927,8 @@ sg_ioctl(struct inode *inode, struct fil
>  		}
>  		return 0;
>  	case SG_GET_RESERVED_SIZE:
> -		val = (int) sfp->reserve.bufflen;
> +		val = min_t(int, sfp->reserve.bufflen,
> +				sdp->device->request_queue->max_sectors * 512);
>  		return put_user(val, ip);
>  	case SG_SET_COMMAND_Q:
>  		result = get_user(val, ip);
> @@ -1061,6 +1064,9 @@ sg_ioctl(struct inode *inode, struct fil
>  		if (sdp->detached)
>  			return -ENODEV;
>  		return scsi_ioctl(sdp->device, cmd_in, p);
> +	case BLKSECTGET:
> +		return put_user(sdp->device->request_queue->max_sectors * 512,
> +				ip);
>  	default:
>  		if (read_only)
>  			return -EPERM;	/* don't know so take safe approach */
> @@ -2339,6 +2345,7 @@ sg_add_sfp(Sg_device * sdp, int dev)
>  {
>  	Sg_fd *sfp;
>  	unsigned long iflags;
> +	int bufflen;
>  
>  	sfp = kzalloc(sizeof(*sfp), GFP_ATOMIC | __GFP_NOWARN);
>  	if (!sfp)
> @@ -2369,7 +2376,9 @@ sg_add_sfp(Sg_device * sdp, int dev)
>  	if (unlikely(sg_big_buff != def_reserved_size))
>  		sg_big_buff = def_reserved_size;
>  
> -	sg_build_reserve(sfp, sg_big_buff);
> +	bufflen = min_t(int, sg_big_buff,
> +			sdp->device->request_queue->max_sectors * 512);
> +	sg_build_reserve(sfp, bufflen);
>  	SCSI_LOG_TIMEOUT(3, printk("sg_add_sfp:   bufflen=%d, k_use_sg=%d\n",
>  			   sfp->reserve.bufflen, sfp->reserve.k_use_sg));
>  	return sfp;
> Index: usb-2.6/block/ll_rw_blk.c
> ===================================================================
> --- usb-2.6.orig/block/ll_rw_blk.c
> +++ usb-2.6/block/ll_rw_blk.c
> @@ -1925,6 +1925,8 @@ blk_init_queue_node(request_fn_proc *rfn
>  	blk_queue_max_hw_segments(q, MAX_HW_SEGMENTS);
>  	blk_queue_max_phys_segments(q, MAX_PHYS_SEGMENTS);
>  
> +	q->sg_reserved_size = INT_MAX;
> +
>  	/*
>  	 * all done
>  	 */
> Index: usb-2.6/block/scsi_ioctl.c
> ===================================================================
> --- usb-2.6.orig/block/scsi_ioctl.c
> +++ usb-2.6/block/scsi_ioctl.c
> @@ -78,7 +78,9 @@ static int sg_set_timeout(request_queue_
>  
>  static int sg_get_reserved_size(request_queue_t *q, int __user *p)
>  {
> -	return put_user(q->sg_reserved_size, p);
> +	unsigned val = min(q->sg_reserved_size, q->max_sectors << 9);
> +
> +	return put_user(val, p);
>  }
>  
>  static int sg_set_reserved_size(request_queue_t *q, int __user *p)
> 
> -
> 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
>