[PATCH] notify block layer when using temporary change to cache_type

[PATCH] notify block layer when using temporary change to cache_type

[Vaughan Cao]

This is a fix for commit:
  39c60a0948cc06139e2fbfe084f83cb7e7deae3b sd: fix array cache flushing bug causing performance problems
We must notify the block layer via q->flush_flags after temporary change the cache_type to write through.
If not, SYNCHRONIZE CACHE command will still be generated.

Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
---
 drivers/scsi/sd.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
index 6146b9d..366e48b 100644
--- a/drivers/scsi/sd.c
+++ b/drivers/scsi/sd.c
@@ -144,6 +144,7 @@ sd_store_cache_type(struct device *dev, struct device_attribute *attr,
 	struct scsi_sense_hdr sshdr;
 	static const char temp[] = "temporary ";
 	int len;
+	unsigned flush;
 
 	if (sdp->type != TYPE_DISK)
 		/* no cache control on RBC devices; theoretically they
@@ -174,6 +175,17 @@ sd_store_cache_type(struct device *dev, struct device_attribute *attr,
 	if (sdkp->cache_override) {
 		sdkp->WCE = wce;
 		sdkp->RCD = rcd;
+
+		/* set flush_flags to notify the block layer */
+		flush = 0;
+		if (sdkp->WCE) {
+			flush |= REQ_FLUSH;
+			if (sdkp->DPOFUA)
+				flush |= REQ_FUA;
+		}
+
+		blk_queue_flush(sdkp->disk->queue, flush);
+
 		return count;
 	}
 
-- 
1.9.0

Re: [PATCH] notify block layer when using temporary change to cache_type

[James Bottomley]

On Tue, 2014-05-27 at 19:39 +0800, Vaughan Cao wrote:
> This is a fix for commit:
>   39c60a0948cc06139e2fbfe084f83cb7e7deae3b sd: fix array cache flushing bug causing performance problems
> We must notify the block layer via q->flush_flags after temporary change the cache_type to write through.
> If not, SYNCHRONIZE CACHE command will still be generated.
> 
> Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
> ---
>  drivers/scsi/sd.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> index 6146b9d..366e48b 100644
> --- a/drivers/scsi/sd.c
> +++ b/drivers/scsi/sd.c
> @@ -144,6 +144,7 @@ sd_store_cache_type(struct device *dev, struct device_attribute *attr,

This is a weird function name.  The name in the vanilla kernel is
cache_type_store().

>  	struct scsi_sense_hdr sshdr;
>  	static const char temp[] = "temporary ";
>  	int len;
> +	unsigned flush;
>  
>  	if (sdp->type != TYPE_DISK)
>  		/* no cache control on RBC devices; theoretically they
> @@ -174,6 +175,17 @@ sd_store_cache_type(struct device *dev, struct device_attribute *attr,
>  	if (sdkp->cache_override) {
>  		sdkp->WCE = wce;
>  		sdkp->RCD = rcd;
> +
> +		/* set flush_flags to notify the block layer */
> +		flush = 0;
> +		if (sdkp->WCE) {
> +			flush |= REQ_FLUSH;
> +			if (sdkp->DPOFUA)
> +				flush |= REQ_FUA;
> +		}
> +
> +		blk_queue_flush(sdkp->disk->queue, flush);
> +

Is there a reason you cut and paste from sd_revalidate_disk() instead of
just calling it directly?

James

Re: [PATCH] notify block layer when using temporary change to cache_type

[Vaughan Cao]

On 05/28/2014 12:18 AM, James Bottomley wrote:
> On Tue, 2014-05-27 at 19:39 +0800, Vaughan Cao wrote:
>> This is a fix for commit:
>>   39c60a0948cc06139e2fbfe084f83cb7e7deae3b sd: fix array cache flushing bug causing performance problems
>> We must notify the block layer via q->flush_flags after temporary change the cache_type to write through.
>> If not, SYNCHRONIZE CACHE command will still be generated.
>>
>> Signed-off-by: Vaughan Cao <vaughan.cao@oracle.com>
>> ---
>>  drivers/scsi/sd.c | 12 ++++++++++++
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
>> index 6146b9d..366e48b 100644
>> --- a/drivers/scsi/sd.c
>> +++ b/drivers/scsi/sd.c
>> @@ -144,6 +144,7 @@ sd_store_cache_type(struct device *dev, struct device_attribute *attr,
> This is a weird function name.  The name in the vanilla kernel is
> cache_type_store().
Sorry, this patch is created on oracle uek kernel, I just checked it can
apply to vanilla kernel cleanly and didn't notice they are using
different device attribute function name.

>>  	struct scsi_sense_hdr sshdr;
>>  	static const char temp[] = "temporary ";
>>  	int len;
>> +	unsigned flush;
>>  
>>  	if (sdp->type != TYPE_DISK)
>>  		/* no cache control on RBC devices; theoretically they
>> @@ -174,6 +175,17 @@ sd_store_cache_type(struct device *dev, struct device_attribute *attr,
>>  	if (sdkp->cache_override) {
>>  		sdkp->WCE = wce;
>>  		sdkp->RCD = rcd;
>> +
>> +		/* set flush_flags to notify the block layer */
>> +		flush = 0;
>> +		if (sdkp->WCE) {
>> +			flush |= REQ_FLUSH;
>> +			if (sdkp->DPOFUA)
>> +				flush |= REQ_FUA;
>> +		}
>> +
>> +		blk_queue_flush(sdkp->disk->queue, flush);
>> +
> Is there a reason you cut and paste from sd_revalidate_disk() instead of
> just calling it directly?
No, I just want to keep the modification as small as possible. Checked
the actions of sd_revalidate_disk() again, it seems no harm to call it
from here. Also actual actions are skipped in sd_read_cache_type() if
cache_override!=0, so I suppose your original plan is to jump to
sd_revalidate_disk() from here.

However, that way may not be acceptable after further code review.
Changing the sdkp->WCE,RCD will cause these real parameters of the
underlying device *lost*. In sd_shutdown() and sd_suspend_common(), we
need them to call sd_sync_cache() if necessary. I don't think this
action is avoidable, just for the performance issue to solve. And we
can't change the mode just by setting q->flush_flags while leave
sdkp->WCE,RCD untouched either, because cache_type_show() needs
sdkp->WCE,RCD to present the temporary config to userspace.

static void sd_shutdown(struct device *dev)
{
...
	if (sdkp->WCE && sdkp->media_present) {
		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
		sd_sync_cache(sdkp);
	}

static int sd_suspend_common(struct device *dev, bool ignore_stop_errors)
{
...
	if (sdkp->WCE && sdkp->media_present) {
		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
		ret = sd_sync_cache(sdkp);

So, It seems new fields like realWCE and readRCD in scsi_disk are needed
to save those configuration. What's your opinion?

Vaughan
>
> James
>
>
>

Re: [PATCH] notify block layer when using temporary change to cache_type

[Christoph Hellwig]

On Wed, May 28, 2014 at 12:58:27PM +0800, Vaughan Cao wrote:
> No, I just want to keep the modification as small as possible. Checked
> the actions of sd_revalidate_disk() again, it seems no harm to call it
> from here. Also actual actions are skipped in sd_read_cache_type() if
> cache_override!=0, so I suppose your original plan is to jump to
> sd_revalidate_disk() from here.

I think the right fix is to factor out a helper like:

static void sd_set_flush_flag(struct scsi_disk *sdkp)
{
	unsigned flush = 0;

	if (sdkp->WCE) {
		flush |= REQ_FLUSH;
		if (sdkp->DPOFUA)
			flush |= REQ_FUA;
	}

	blk_queue_flush(sdkp->disk->queue, flush);
}

which can be called from sd_revalidate_disk and cache_type_store.

> 
> However, that way may not be acceptable after further code review.
> Changing the sdkp->WCE,RCD will cause these real parameters of the
> underlying device *lost*. In sd_shutdown() and sd_suspend_common(), we
> need them to call sd_sync_cache() if necessary. I don't think this
> action is avoidable, just for the performance issue to solve. And we
> can't change the mode just by setting q->flush_flags while leave
> sdkp->WCE,RCD untouched either, because cache_type_show() needs
> sdkp->WCE,RCD to present the temporary config to userspace.

If the user overrides the cache type he should expect data not to be
flushed, so I don't really think this is na issue.  Even if it is it
should be a separate issue as it already exists before your patch.