Re: [PATCH 1/4] sd: fix cache flushing message

[Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>]

On Wednesday 29 April 2009 23:14:02 James Bottomley wrote:
> On Wed, 2009-04-29 at 22:53 +0200, Bartlomiej Zolnierkiewicz wrote:
> > On Wednesday 29 April 2009 22:22:51 Jeff Garzik wrote:
> > > James Bottomley wrote:
> > > > On Wed, 2009-04-29 at 22:06 +0200, Bartlomiej Zolnierkiewicz wrote:
> > > >> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
> > > >> ---
> > > >> some minor sd.c fixups against linux-next
> > > >>
> > > >>  drivers/scsi/sd.c |    4 ++--
> > > >>  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >>
> > > >> Index: b/drivers/scsi/sd.c
> > > >> ===================================================================
> > > >> --- a/drivers/scsi/sd.c
> > > >> +++ b/drivers/scsi/sd.c
> > > >> @@ -2140,7 +2140,7 @@ static void sd_shutdown(struct device *d
> > > >>  		return;         /* this can happen */
> > > >>  
> > > >>  	if (sdkp->WCE) {
> > > >> -		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
> > > >> +		sd_printk(KERN_NOTICE, sdkp, "Synchronizing disk cache\n");
> > > >>  		sd_sync_cache(sdkp);
> > > >>  	}
> > > >>  
> > > >> @@ -2161,7 +2161,7 @@ static int sd_suspend(struct device *dev
> > > >>  		return 0;	/* this can happen */
> > > >>  
> > > >>  	if (sdkp->WCE) {
> > > >> -		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
> > > >> +		sd_printk(KERN_NOTICE, sdkp, "Synchronizing disk cache\n");
> > > >>  		ret = sd_sync_cache(sdkp);
> > > >>  		if (ret)
> > > >>  			goto done;
> > > > 
> > > > So you want it to say 
> > > > 
> > > > sda: synchronizing disk cache
> > > > 
> > > > instead of
> > > > 
> > > > sda: synchronizing SCSI cache
> > > > 
> > > > I don't really see that buying us anything in terms of clarity.  What's
> > > > the actual problem it solves?
> > 
> > Inconsistent kernel messages:
> > 
> > 	if (sdkp->WCE) {
> > 		sd_printk(KERN_NOTICE, sdkp, "Synchronizing SCSI cache\n");
> > 		sd_sync_cache(sdkp);
> > 	}
> > 
> > 	if (system_state != SYSTEM_RESTART && sdkp->device->manage_start_stop) {
> > 		sd_printk(KERN_NOTICE, sdkp, "Stopping disk\n");
> > 		sd_start_stop_device(sdkp, 0);
> > 	}
> > 
> > > Indeed.
> > > 
> > > And we can argue for weeks about English text, too.  Non-disk devices 
> > 
> > Or we can just apply the patch and go back with the program.
> > 
> > > like SSDs have caches too, so I'd say "SCSI cache" is more accurate than 
> > > "disk cache".  But maybe "SCSI writeback cache" would be even better.
> > 
> > Could you please explain me what *SCSI* cache is?
> 
> It's the cache described by SBC (SCSI Block Commandset).

Sorry, I don't buy it.

There is no "SCSI cache" in the SBC spec (at least not in sbc2r16).

> > Does ATA disks have *SCSI* cache (whatever it is)?
> 
> Only if you use it via SAT (SCSI to ATA translation).

Hmm, and without SAT there is ATA cache, right? :)

> > Please also note that sd.c uses "disk" all over the place.
> 
> Yes, but I think the four other suggestions for text demonstrate the
> need to get something worm proof back on this can ASAP.

Well, "SCSI device writeback cache" should be fine with everybody, no? ;)

Anyway, please just scrap that patch.

Thanks,
Bart