Re: [PATCH] scsi/sata write barrier support

[Jens Axboe <axboe@suse.de>]

On Fri, Jan 28 2005, James Bottomley wrote:
> On Fri, 2005-01-28 at 10:38 +0100, Jens Axboe wrote:
> > +/*
> > + * snoop succesfull completion of mode select commands that update the
> > + * write back cache state
> > + */
> > +#define MS_CACHE_PAGE	0x08
> > +static void sd_snoop_cmd(struct scsi_cmnd *cmd)
> > +{
> > +	struct scsi_disk *sdpk;
> > +	char *page;
> > +
> > +	if (cmd->result)
> > +		return;
> > +
> > +	switch (cmd->cmnd[0]) {
> > +		case MODE_SELECT:
> > +		case MODE_SELECT_10:
> > +			page = cmd->request_buffer;
> > +			if (!page)
> > +				break;
> > +			if ((page[0] & 0x3f) != MS_CACHE_PAGE)
> > +				break;
> > +			sdpk = dev_get_drvdata(&cmd->device->sdev_gendev);
> > +			sdpk->WCE = (page[2] & 0x04) != 0;
> > +			break;
> > +	}
> > +}
> > +
> >  /**
> >   *	sd_rw_intr - bottom half handler: called when the lower level
> >   *	driver has completed (successfully or otherwise) a scsi command.
> > @@ -773,6 +831,9 @@ static void sd_rw_intr(struct scsi_cmnd 
> >  			SCpnt->sense_buffer[13]));
> >  	}
> >  #endif
> > +
> > +	sd_snoop_cmd(SCpnt);
> > +
> 
> Good grief no!
> 
> If you're going to try something like this, it needs to be a separate
> patch over the scsi-list for one thing.  And to save time:
> 
> 1) The patch is actually wrong.  There's more than one caching mode page
> and not all of them affect current behaviour.

It also gets the offset wrong :)

> 2) We have a current interface to update the WCE bit:  You twiddle all
> the disc parameters and then trigger a device rescan via sysfs (I'll
> check that this updates the cache bits, I think it does, but if it
> doesn't I'll make it).
> 3) If we think this is a quantity the users would like to see and alter,
> then reading and setting it should be exported via sysfs.
> 4) Snooping SCSI commands is really bad ... it can get you into all
> sorts of trouble which is why we prefer asking the device what state
> it's in to trying to guess ourselves.

I would _much_ prefer some sort of easily tweakable way to change the
write back mode, if this is something we want to support. As I wrote in
the original reply, I hate the concept of command snooping.

The barrier stuff works fine as-is, I'll just rely on scsi getting the
WCE updating correct.

-- 
Jens Axboe