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@xxxxxxxxxx>
>> ---
>> 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
>
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
