| Message ID | 1465012678-32547-1-git-send-email-longli@microsoft.com (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Headers | show |
The main point there is not to check q->limits.max_sectors against BLK_DEF_MAX_SECTORS, but sdkp->opt_xfer_blocks against SD_DEF_XFER_BLOCKS et al.? `rw_max = BLK_DEF_MAX_SECTORS;` there is merely the fallback when sdkp->opt_xfer_blocks does not pass the conditions. With your patch `rw_max` can be indeterminate in those circumstances. On 4 June 2016 at 11:57, Long Li <longli@microsoft.com> wrote: > q->limits.max_sectors is already checked against BLK_DEF_MAX_SECTORS in __scsi_alloc_queue(), when it calls blk_queue_max_hw_sectors(). There is no need to check it again in sd. > > This change also allows a SCSI driver set an maximum sector size bigger than BLK_DEF_MAX_SECTORS, without returning values on optional VPD page 0xb0 "Block Limits". > > Signed-off-by: Long Li <longli@microsoft.com> > > --- > drivers/scsi/sd.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c > index 60bff78..d8c4047 100644 > --- a/drivers/scsi/sd.c > +++ b/drivers/scsi/sd.c > @@ -2870,11 +2870,8 @@ static int sd_revalidate_disk(struct gendisk *disk) > logical_to_bytes(sdp, sdkp->opt_xfer_blocks) >= PAGE_SIZE) { > q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks); > rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks); > - } else > - rw_max = BLK_DEF_MAX_SECTORS; > - > - /* Combine with controller limits */ > - q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q)); > + q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q)); > + } > > set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity)); > sd_config_write_same(sdkp); > -- > 2.7.4 > > -- > 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 -- 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
U29ycnksICJyZWR1bmRhbnQgY2hlY2siIGlzIG5vdCB0aGUgYmVzdCB3b3JkIHRvIGRlc2NyaWJl IHRoaXMgcGF0Y2guDQoNClRoZSByZXN1bHQgb2YgdGhpcyBwYXRjaCBpcyB0aGF0Og0KMS4gaWYg b3B0X3hmZXJfYmxvY2tzIGhhcyBhIHZhbGlkIHZhbHVlIChyZXR1cm5lZCBmb3JtIFZQRCBCTE9D SyBMSU1JVFMpLCB1c2UgaXQgdG8gc2V0IG1heF9zZWN0b3JzDQoyLiBpZiBvcHRfeGZlcl9ibG9j a3MgZG9lc24ndCBoYXZlIGEgdmFsaWQgdmFsdWUsIGxlYXZlIG1heF9zZWN0b3JzIHVuY2hhbmdl ZA0KDQpUaGUgcmVhc29uIGlzIHRoYXQsIG1heF9zZWN0b3JzIGFscmVhZHkgaGFzIHZhbHVlIGF0 IHRoaXMgcG9pbnQsIHRoZSBkZWZhdWx0IHZhbHVlIGlzIFNDU0lfREVGQVVMVF9NQVhfU0VDVE9S UyAoaW5jbHVkZS9zY3NpL3Njc2lfaG9zdC5oKS4gVGhlIGxvd2VyIGxheWVyIGhvc3QgZHJpdmVy IGNhbiBjaGFuZ2UgdGhpcyB2YWx1ZSBpbiBpdHMgdGVtcGxhdGUuIEkgdGhpbmsgdGhlIGRyaXZl cnMgY2FyZSBhYm91dCB0aGlzIHZhbHVlIGhhdmUgYWxyZWFkeSBzZXQgaXQuIFNvIGl0J3MgYmV0 dGVyIG5vdCB0byBjaGFuZ2UgaXQgYWdhaW4uIElmIHRoZXkgd2FudCBtYXhfc2VjdG9ycyB0byBi ZSBzZXQgYnkgc2QsIHRoZXkgY2FuIHVzZSBCTE9DSyBMSU1JVFMgVlBEIHRvIHRlbGwgaXQgdG8g ZG8gc28uDQoNCg0KPiAtLS0tLU9yaWdpbmFsIE1lc3NhZ2UtLS0tLQ0KPiBGcm9tOiBUb20gWWFu IFttYWlsdG86dG9tLnR5ODlAZ21haWwuY29tXQ0KPiBTZW50OiBTYXR1cmRheSwgSnVuZSA0LCAy MDE2IDE6NDEgQU0NCj4gVG86IExvbmcgTGkgPGxvbmdsaUBtaWNyb3NvZnQuY29tPg0KPiBDYzog SmFtZXMgRS5KLiBCb3R0b21sZXkgPGplamJAbGludXgudm5ldC5pYm0uY29tPjsgTWFydGluIEsu IFBldGVyc2VuDQo+IDxtYXJ0aW4ucGV0ZXJzZW5Ab3JhY2xlLmNvbT47IGxpbnV4LXNjc2lAdmdl ci5rZXJuZWwub3JnOyBsaW51eC0NCj4ga2VybmVsQHZnZXIua2VybmVsLm9yZw0KPiBTdWJqZWN0 OiBSZTogW1BBVENIXSBzZDogcmVtb3ZlIHJlZHVuZGFudCBjaGVjayBmb3INCj4gQkxLX0RFRl9N QVhfU0VDVE9SUw0KPiANCj4gVGhlIG1haW4gcG9pbnQgdGhlcmUgaXMgbm90IHRvIGNoZWNrIHEt PmxpbWl0cy5tYXhfc2VjdG9ycyBhZ2FpbnN0DQo+IEJMS19ERUZfTUFYX1NFQ1RPUlMsIGJ1dCBz ZGtwLT5vcHRfeGZlcl9ibG9ja3MgYWdhaW5zdA0KPiBTRF9ERUZfWEZFUl9CTE9DS1MgZXQgYWwu PyBgcndfbWF4ID0gQkxLX0RFRl9NQVhfU0VDVE9SUztgIHRoZXJlIGlzDQo+IG1lcmVseSB0aGUg ZmFsbGJhY2sgd2hlbiBzZGtwLT5vcHRfeGZlcl9ibG9ja3MgZG9lcyBub3QgcGFzcyB0aGUNCj4g Y29uZGl0aW9ucy4gV2l0aCB5b3VyIHBhdGNoIGByd19tYXhgIGNhbiBiZSBpbmRldGVybWluYXRl IGluIHRob3NlDQo+IGNpcmN1bXN0YW5jZXMuDQo+IA0KPiBPbiA0IEp1bmUgMjAxNiBhdCAxMTo1 NywgTG9uZyBMaSA8bG9uZ2xpQG1pY3Jvc29mdC5jb20+IHdyb3RlOg0KPiA+IHEtPmxpbWl0cy5t YXhfc2VjdG9ycyBpcyBhbHJlYWR5IGNoZWNrZWQgYWdhaW5zdCBCTEtfREVGX01BWF9TRUNUT1JT DQo+IGluIF9fc2NzaV9hbGxvY19xdWV1ZSgpLCB3aGVuIGl0IGNhbGxzIGJsa19xdWV1ZV9tYXhf aHdfc2VjdG9ycygpLiBUaGVyZQ0KPiBpcyBubyBuZWVkIHRvIGNoZWNrIGl0IGFnYWluIGluIHNk Lg0KPiA+DQo+ID4gVGhpcyBjaGFuZ2UgYWxzbyBhbGxvd3MgYSBTQ1NJIGRyaXZlciBzZXQgYW4g bWF4aW11bSBzZWN0b3Igc2l6ZSBiaWdnZXINCj4gdGhhbiBCTEtfREVGX01BWF9TRUNUT1JTLCB3 aXRob3V0IHJldHVybmluZyB2YWx1ZXMgb24gb3B0aW9uYWwgVlBEDQo+IHBhZ2UgMHhiMCAiQmxv Y2sgTGltaXRzIi4NCj4gPg0KPiA+IFNpZ25lZC1vZmYtYnk6IExvbmcgTGkgPGxvbmdsaUBtaWNy b3NvZnQuY29tPg0KPiA+DQo+ID4gLS0tDQo+ID4gIGRyaXZlcnMvc2NzaS9zZC5jIHwgNyArKy0t LS0tDQo+ID4gIDEgZmlsZSBjaGFuZ2VkLCAyIGluc2VydGlvbnMoKyksIDUgZGVsZXRpb25zKC0p DQo+ID4NCj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9zY3NpL3NkLmMgYi9kcml2ZXJzL3Njc2kv c2QuYyBpbmRleA0KPiA+IDYwYmZmNzguLmQ4YzQwNDcgMTAwNjQ0DQo+ID4gLS0tIGEvZHJpdmVy cy9zY3NpL3NkLmMNCj4gPiArKysgYi9kcml2ZXJzL3Njc2kvc2QuYw0KPiA+IEBAIC0yODcwLDEx ICsyODcwLDggQEAgc3RhdGljIGludCBzZF9yZXZhbGlkYXRlX2Rpc2soc3RydWN0IGdlbmRpc2sg KmRpc2spDQo+ID4gICAgICAgICAgICAgbG9naWNhbF90b19ieXRlcyhzZHAsIHNka3AtPm9wdF94 ZmVyX2Jsb2NrcykgPj0gUEFHRV9TSVpFKSB7DQo+ID4gICAgICAgICAgICAgICAgIHEtPmxpbWl0 cy5pb19vcHQgPSBsb2dpY2FsX3RvX2J5dGVzKHNkcCwgc2RrcC0+b3B0X3hmZXJfYmxvY2tzKTsN Cj4gPiAgICAgICAgICAgICAgICAgcndfbWF4ID0gbG9naWNhbF90b19zZWN0b3JzKHNkcCwgc2Rr cC0+b3B0X3hmZXJfYmxvY2tzKTsNCj4gPiAtICAgICAgIH0gZWxzZQ0KPiA+IC0gICAgICAgICAg ICAgICByd19tYXggPSBCTEtfREVGX01BWF9TRUNUT1JTOw0KPiA+IC0NCj4gPiAtICAgICAgIC8q IENvbWJpbmUgd2l0aCBjb250cm9sbGVyIGxpbWl0cyAqLw0KPiA+IC0gICAgICAgcS0+bGltaXRz Lm1heF9zZWN0b3JzID0gbWluKHJ3X21heCwgcXVldWVfbWF4X2h3X3NlY3RvcnMocSkpOw0KPiA+ ICsgICAgICAgICAgICAgICBxLT5saW1pdHMubWF4X3NlY3RvcnMgPSBtaW4ocndfbWF4LA0KPiBx dWV1ZV9tYXhfaHdfc2VjdG9ycyhxKSk7DQo+ID4gKyAgICAgICB9DQo+ID4NCj4gPiAgICAgICAg IHNldF9jYXBhY2l0eShkaXNrLCBsb2dpY2FsX3RvX3NlY3RvcnMoc2RwLCBzZGtwLT5jYXBhY2l0 eSkpOw0KPiA+ICAgICAgICAgc2RfY29uZmlnX3dyaXRlX3NhbWUoc2RrcCk7DQo+ID4gLS0NCj4g PiAyLjcuNA0KPiA+DQo+ID4gLS0NCj4gPiBUbyB1bnN1YnNjcmliZSBmcm9tIHRoaXMgbGlzdDog c2VuZCB0aGUgbGluZSAidW5zdWJzY3JpYmUgbGludXgtc2NzaSINCj4gPiBpbiB0aGUgYm9keSBv ZiBhIG1lc3NhZ2UgdG8gbWFqb3Jkb21vQHZnZXIua2VybmVsLm9yZyBNb3JlDQo+IG1ham9yZG9t bw0KPiA+IGluZm8gYXQNCj4gPiBodHRwczovL25hMDEuc2FmZWxpbmtzLnByb3RlY3Rpb24ub3V0 bG9vay5jb20vP3VybD1odHRwJTNhJTJmJTJmdmdlci5rDQo+ID4gZXJuZWwub3JnJTJmbWFqb3Jk b21vLQ0KPiBpbmZvLmh0bWwmZGF0YT0wMSU3YzAxJTdjbG9uZ2xpJTQwbWljcm9zb2Z0LmNvbSUN Cj4gPg0KPiA3Y2UxNDIxMjg5NThlYzQ3NjI5ZGJlMDhkMzhjNTQwMzA2JTdjNzJmOTg4YmY4NmYx NDFhZjkxYWIyZDdjZDAxMWQNCj4gYjQ3JQ0KPiA+IDdjMSZzZGF0YT1FampGODZjdkpxYXhPQU9X bk4wJTJmM1FsbjA1cWNxdXdlJTJmS0E3RGdFanRjSSUzZA0K -- 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
Never mind. I misread. I thought q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q)); can be run when rw_max is not set. On 4 June 2016 at 23:18, Long Li <longli@microsoft.com> wrote: > Sorry, "redundant check" is not the best word to describe this patch. > > The result of this patch is that: > 1. if opt_xfer_blocks has a valid value (returned form VPD BLOCK LIMITS), use it to set max_sectors > 2. if opt_xfer_blocks doesn't have a valid value, leave max_sectors unchanged > > The reason is that, max_sectors already has value at this point, the default value is SCSI_DEFAULT_MAX_SECTORS (include/scsi/scsi_host.h). The lower layer host driver can change this value in its template. I think the drivers care about this value have already set it. So it's better not to change it again. If they want max_sectors to be set by sd, they can use BLOCK LIMITS VPD to tell it to do so. > > >> -----Original Message----- >> From: Tom Yan [mailto:tom.ty89@gmail.com] >> Sent: Saturday, June 4, 2016 1:41 AM >> To: Long Li <longli@microsoft.com> >> Cc: James E.J. Bottomley <jejb@linux.vnet.ibm.com>; Martin K. Petersen >> <martin.petersen@oracle.com>; linux-scsi@vger.kernel.org; linux- >> kernel@vger.kernel.org >> Subject: Re: [PATCH] sd: remove redundant check for >> BLK_DEF_MAX_SECTORS >> >> The main point there is not to check q->limits.max_sectors against >> BLK_DEF_MAX_SECTORS, but sdkp->opt_xfer_blocks against >> SD_DEF_XFER_BLOCKS et al.? `rw_max = BLK_DEF_MAX_SECTORS;` there is >> merely the fallback when sdkp->opt_xfer_blocks does not pass the >> conditions. With your patch `rw_max` can be indeterminate in those >> circumstances. >> >> On 4 June 2016 at 11:57, Long Li <longli@microsoft.com> wrote: >> > q->limits.max_sectors is already checked against BLK_DEF_MAX_SECTORS >> in __scsi_alloc_queue(), when it calls blk_queue_max_hw_sectors(). There >> is no need to check it again in sd. >> > >> > This change also allows a SCSI driver set an maximum sector size bigger >> than BLK_DEF_MAX_SECTORS, without returning values on optional VPD >> page 0xb0 "Block Limits". >> > >> > Signed-off-by: Long Li <longli@microsoft.com> >> > >> > --- >> > drivers/scsi/sd.c | 7 ++----- >> > 1 file changed, 2 insertions(+), 5 deletions(-) >> > >> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index >> > 60bff78..d8c4047 100644 >> > --- a/drivers/scsi/sd.c >> > +++ b/drivers/scsi/sd.c >> > @@ -2870,11 +2870,8 @@ static int sd_revalidate_disk(struct gendisk *disk) >> > logical_to_bytes(sdp, sdkp->opt_xfer_blocks) >= PAGE_SIZE) { >> > q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks); >> > rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks); >> > - } else >> > - rw_max = BLK_DEF_MAX_SECTORS; >> > - >> > - /* Combine with controller limits */ >> > - q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q)); >> > + q->limits.max_sectors = min(rw_max, >> queue_max_hw_sectors(q)); >> > + } >> > >> > set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity)); >> > sd_config_write_same(sdkp); >> > -- >> > 2.7.4 >> > >> > -- >> > 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 >> > https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fvger.k >> > ernel.org%2fmajordomo- >> info.html&data=01%7c01%7clongli%40microsoft.com% >> > >> 7ce142128958ec47629dbe08d38c540306%7c72f988bf86f141af91ab2d7cd011d >> b47% >> > 7c1&sdata=EjjF86cvJqaxOAOWnN0%2f3Qln05qcquwe%2fKA7DgEjtcI%3d -- 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
>>>>> "Long" == Long Li <longli@microsoft.com> writes:
Long,
Long> The reason is that, max_sectors already has value at this point,
Long> the default value is SCSI_DEFAULT_MAX_SECTORS
Long> (include/scsi/scsi_host.h). The lower layer host driver can change
Long> this value in its template.
The LLD sets max_hw_sectors which indicates the capabilities of the
controller DMA hardware. Whereas the max_sectors limit is set by sd to
either follow advise by the device or--if not provided--use the block
layer default. max_sectors governs the size of READ/WRITE requests and
do not reflect the capabilities of the DMA hardware.
Long> I think the drivers care about this value have already set it. So
Long> it's better not to change it again. If they want max_sectors to be
Long> set by sd, they can use BLOCK LIMITS VPD to tell it to do so.
Most drivers don't have the luxury of being able to generate VPDs for
their attached target devices :)
Hi Martin, Thanks for looking into this. The problem I'm trying to solve is that, I want to have lower layer driver to setup max_sectors bigger than BLK_DEF_MAX_SECTORS. In Hyper-v, we use 2MB max transfer I/O size, in future version the max transfer I/O size will increase to 8MB. The implementation of sd.c limits the maximum value of max_sectors to BLK_DEF_MAX_SECTORS. Because sd_revalidate_disk is called late in the SCSI disk initialization process, there is no way for a lower layer driver to set this value to its "bigger" optimal size. The reason why I think it may not be necessary for sd.c to setup max_sectors, it's because this value may have already been setup twice before reaching the code in sd.c: 1. When this disk device is first scanned, or re-scanned (in scsi_scan.c), where it eventually calls __scsi_init_queue(), and use the max_sectors in the scsi_host_template. 2. in slave_configure of scsi_host_template, when the lower layer driver implements this function in its template and it can change this value there. Long > -----Original Message----- > From: Martin K. Petersen [mailto:martin.petersen@oracle.com] > Sent: Monday, June 6, 2016 8:42 PM > To: Long Li <longli@microsoft.com> > Cc: Tom Yan <tom.ty89@gmail.com>; James E.J. Bottomley > <jejb@linux.vnet.ibm.com>; Martin K. Petersen > <martin.petersen@oracle.com>; linux-scsi@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH] sd: remove redundant check for > BLK_DEF_MAX_SECTORS > > >>>>> "Long" == Long Li <longli@microsoft.com> writes: > > Long, > > Long> The reason is that, max_sectors already has value at this point, > Long> the default value is SCSI_DEFAULT_MAX_SECTORS > Long> (include/scsi/scsi_host.h). The lower layer host driver can change > Long> this value in its template. > > The LLD sets max_hw_sectors which indicates the capabilities of the > controller DMA hardware. Whereas the max_sectors limit is set by sd to > either follow advise by the device or--if not provided--use the block layer > default. max_sectors governs the size of READ/WRITE requests and do not > reflect the capabilities of the DMA hardware. > > Long> I think the drivers care about this value have already set it. So > Long> it's better not to change it again. If they want max_sectors to be > Long> set by sd, they can use BLOCK LIMITS VPD to tell it to do so. > > Most drivers don't have the luxury of being able to generate VPDs for their > attached target devices :) > > -- > Martin K. Petersen Oracle Linux Engineering -- 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
>>>>> "Long" == Long Li <longli@microsoft.com> writes:
Long,
Long> The problem I'm trying to solve is that, I want to have lower
Long> layer driver to setup max_sectors bigger than
Long> BLK_DEF_MAX_SECTORS.
Capping at BLK_DEF_MAX_SECTORS unless a device has explicitly reported
requirements is intentional. We have not had good experiences with
making I/O requests too big in general. So BLK_DEF_MAX_SECTORS has
deliberately been kept small. However, it was recently bumped to 1MB and
change by default.
Long> n Hyper-v, we use 2MB max transfer I/O size, in future version the
Long> max transfer I/O size will increase to 8MB.
But presumably you provide a BLOCK LIMITS VPD for your virtual targets?
Long> The reason why I think it may not be necessary for sd.c to setup
Long> max_sectors, it's because this value may have already been setup
Long> twice before reaching the code in sd.c: 1. When this disk device
Long> is first scanned, or re-scanned (in scsi_scan.c), where it
Long> eventually calls __scsi_init_queue(), and use the max_sectors in
Long> the scsi_host_template. 2. in slave_configure of
Long> scsi_host_template, when the lower layer driver implements this
Long> function in its template and it can change this value there.
Those cause limits to be set for the controller. We won't know the
device limits until we hit revalidate.
blk_queue_max_hw_sectors() will also clamp the R/W max at
BLK_DEF_MAX_SECTORS, though.
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index 60bff78..d8c4047 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -2870,11 +2870,8 @@ static int sd_revalidate_disk(struct gendisk *disk) logical_to_bytes(sdp, sdkp->opt_xfer_blocks) >= PAGE_SIZE) { q->limits.io_opt = logical_to_bytes(sdp, sdkp->opt_xfer_blocks); rw_max = logical_to_sectors(sdp, sdkp->opt_xfer_blocks); - } else - rw_max = BLK_DEF_MAX_SECTORS; - - /* Combine with controller limits */ - q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q)); + q->limits.max_sectors = min(rw_max, queue_max_hw_sectors(q)); + } set_capacity(disk, logical_to_sectors(sdp, sdkp->capacity)); sd_config_write_same(sdkp);
q->limits.max_sectors is already checked against BLK_DEF_MAX_SECTORS in __scsi_alloc_queue(), when it calls blk_queue_max_hw_sectors(). There is no need to check it again in sd. This change also allows a SCSI driver set an maximum sector size bigger than BLK_DEF_MAX_SECTORS, without returning values on optional VPD page 0xb0 "Block Limits". Signed-off-by: Long Li <longli@microsoft.com> --- drivers/scsi/sd.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-)