| Message ID | 20180118141928.GA28320@bogon.didichuxing.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
On Thu, 2018-01-18 at 22:19 +0800, weiping zhang wrote: > - return sprintf(buf, "%s\n", sd_cache_types[ct]); > + return sprintf(buf, "%s\n%s\nwrite:%s, read:%s\n", > sd_cache_types[ct], > + sd_wce_rcd[ct], sdkp->WCE ? "on" : "off", > + sdkp->RCD ? "off" : "on"); I don't think we can do this. The output of the cache type in sysfs is a user exported ABI. We'd potentially break it if we add extra stuff. James
Hi Weiping, > Add user friendly command strings sd_wce_rcd to enable/disable > write&read cache. User can enable both write and read cache by one of > the following commands: I am not entirely sure what the rationale is behind your patch. Is there some deficiency in the existing cache control interface that you are trying to address? Or is the problem simply that it is poorly documented?
2018-01-19 10:41 GMT+08:00 Martin K. Petersen <martin.petersen@oracle.com>: > > Hi Weiping, > >> Add user friendly command strings sd_wce_rcd to enable/disable >> write&read cache. User can enable both write and read cache by one of >> the following commands: > > I am not entirely sure what the rationale is behind your patch. Is there > some deficiency in the existing cache control interface that you are > trying to address? Or is the problem simply that it is poorly > documented? Hi Marin, currently, there are four combinations as following: "write through", "none", "write back", "write back, no read (daft)" cache_type can control both write and read cache, but for "write through" and "write back" we can not know clearly how to control the read cache. I prefer use words like"w0r1", "w0r0", "w1r1", "w1r0", that "1" means enable, "0" means disable. The user know clearly what they are doing when typing these short words. Hi James, > I don't think we can do this. The output of the cache type in sysfs is > a user exported ABI. We'd potentially break it if we add extra stuff. If so, I'll not change the output, only add new match array. > -- > Martin K. Petersen Oracle Linux Engineering
Hi Weiping, > currently, there are four combinations as following: "write through", > "none", "write back", "write back, no read (daft)" > > cache_type can control both write and read cache, but for "write > through" and "write back" we can not know clearly how to control the > read cache. That's what I meant by using the term "arcane" and alluding to the fact that this interface is not well enough documented. > I prefer use words like"w0r1", "w0r0", "w1r1", "w1r0", that "1" means > enable, "0" means disable. The user know clearly what they are doing > when typing these short words. We can't change the existing interface without breaking stuff. We can entertain adding stuff, but I do think that a better solution is to document what's there so the effect of echoing each of the following strings becomes crystal clear: static const char *sd_cache_types[] = { "write through", "none", "write back", "write back, no read (daft)" }; I would also like to see the "temporary" string documented.
2018-01-23 8:23 GMT+08:00 Martin K. Petersen <martin.petersen@oracle.com>: > > Hi Weiping, > >> currently, there are four combinations as following: "write through", >> "none", "write back", "write back, no read (daft)" >> >> cache_type can control both write and read cache, but for "write >> through" and "write back" we can not know clearly how to control the >> read cache. > > That's what I meant by using the term "arcane" and alluding to the fact > that this interface is not well enough documented. > >> I prefer use words like"w0r1", "w0r0", "w1r1", "w1r0", that "1" means >> enable, "0" means disable. The user know clearly what they are doing >> when typing these short words. > > We can't change the existing interface without breaking stuff. We can > entertain adding stuff, but I do think that a better solution is to > document what's there so the effect of echoing each of the following > strings becomes crystal clear: > OK, I'll add more detail comments for these words, but I prefer add new stuff like "w0r1", for old user script keep using "write back", for new script users can also use "w1r1". > static const char *sd_cache_types[] = { > "write through", "none", "write back", > "write back, no read (daft)" > }; > > I would also like to see the "temporary" string documented. OK, add it in V3. > > -- > Martin K. Petersen Oracle Linux Engineering
diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c index a028ab3..722e440 100644 --- a/drivers/scsi/sd.c +++ b/drivers/scsi/sd.c @@ -139,6 +139,15 @@ static const char *sd_cache_types[] = { "write back, no read (daft)" }; +/* + * wce rcd write_cache read_cache + * 0 0 off on + * 0 1 off off + * 1 0 on on + * 1 1 on off + */ +static const char * const sd_wce_rcd[] = {"00", "01", "10", "11"}; + static void sd_set_flush_flag(struct scsi_disk *sdkp) { bool wc = false, fua = false; @@ -180,8 +189,11 @@ cache_type_store(struct device *dev, struct device_attribute *attr, } ct = sysfs_match_string(sd_cache_types, buf); - if (ct < 0) - return -EINVAL; + if (ct < 0) { + ct = sysfs_match_string(sd_wce_rcd, buf); + if (ct < 0) + return -EINVAL; + } rcd = ct & 0x01 ? 1 : 0; wce = (ct & 0x02) && !sdkp->write_prot ? 1 : 0; @@ -282,7 +294,9 @@ cache_type_show(struct device *dev, struct device_attribute *attr, char *buf) struct scsi_disk *sdkp = to_scsi_disk(dev); int ct = sdkp->RCD + 2*sdkp->WCE; - return sprintf(buf, "%s\n", sd_cache_types[ct]); + return sprintf(buf, "%s\n%s\nwrite:%s, read:%s\n", sd_cache_types[ct], + sd_wce_rcd[ct], sdkp->WCE ? "on" : "off", + sdkp->RCD ? "off" : "on"); } static DEVICE_ATTR_RW(cache_type);
Add user friendly command strings sd_wce_rcd to enable/disable write&read cache. User can enable both write and read cache by one of the following commands: echo 10 > cache_type echo "write back" > cache_type wce rcd write_cache read_cache 0 0 off on 0 1 off off 1 0 on on 1 1 on off When execute "cat cache_type", it will show more detail info like following: write back 10 write:on, read:on Signed-off-by: weiping zhang <zhangweiping@didichuxing.com> --- drivers/scsi/sd.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-)