[PATCH v2] scsi: sd: add new match array for cache_type

[PATCH v2] scsi: sd: add new match array for cache_type

[weiping zhang]

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(-)

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);
 
-- 
2.9.4

Re: [PATCH v2] scsi: sd: add new match array for cache_type

[James Bottomley]

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

Re: [PATCH v2] scsi: sd: add new match array for cache_type

[Martin K. Petersen]

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?

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2] scsi: sd: add new match array for cache_type

[weiping zhang]

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

Re: [PATCH v2] scsi: sd: add new match array for cache_type

[Martin K. Petersen]

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.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2] scsi: sd: add new match array for cache_type

[weiping zhang]

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