Re: [RFC PATCH 3/5] streamline REQ_OP_READ-WRITE access

[Douglas Gilbert <dgilbert@xxxxxxxxxxxx>]

On 2018-08-06 01:41 AM, Damien Le Moal wrote:
> On 2018/08/06 13:51, Douglas Gilbert wrote:
> > Make the two block layer operations most frequently used (REQ_OP_READ
> > and REQ_OP_WRITE) bypass the switch statements in the submission and
> > response paths. Assume these two enums are 0 and 1 which allows a
> > single comparison to select both of them. Check that assumption
> > at driver start-up.
>
> What about simply moving the F cases at the top of
> the switch case list ? Since that gets (usually) compiled as a jump, the effect
> should be the same, no ?

Yes, that could be done. The proposed code takes it out of the hands
of the compiler's switch implementation and makes the favouring of
REQ_OP_READ and REQ_OP_WRITE more explicit at the cost of code
simplicity.

Also the leading "if (likely(...))" gives the runtime the option of
not caching the corresponding "else" code (which could be placed in
a separate function), leaving code cache space free for other fast
path functions.


A previous patch proposal by me had sd_init_command() callback going
straight into sd_setup_read_write_cmnd() and then going to a selection
function when the invocation was not for either REQ_OP_READ and
REQ_OP_WRITE. That saves one level of calls on the fast path. A reviewer
didn't like that.

Doug Gilbert

> > Signed-off-by: Douglas Gilbert <dgilbert@xxxxxxxxxxxx>
> > ---
> >   drivers/scsi/sd.c | 82 +++++++++++++++++++++++++++++++----------------
> >   1 file changed, 55 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/scsi/sd.c b/drivers/scsi/sd.c
> > index 4b1402633c36..9f047fd3c92d 100644
> > --- a/drivers/scsi/sd.c
> > +++ b/drivers/scsi/sd.c
> > @@ -1227,6 +1227,14 @@ static int sd_init_command(struct scsi_cmnd *cmd)
> >   {
> >   	struct request *rq = cmd->request;
> >   
> > +	/*
> > +	 * Assumption that REQ_OP_READ and REQ_OP_WRITE are 0 and 1 is
> > +	 * checked in init_sd(). The following "if" is done to dispatch
> > +	 * REQ_OP_READ and REQ_OP_WRITE quickly.
> > +	 */
> > +	if (likely(req_op(rq) < 2))
> > +		return sd_setup_read_write_cmnd(cmd);
> > +
> >   	switch (req_op(rq)) {
> >   	case REQ_OP_DISCARD:
> >   		switch (scsi_disk(rq->rq_disk)->provisioning_mode) {
> > @@ -1247,9 +1255,6 @@ static int sd_init_command(struct scsi_cmnd *cmd)
> >   		return sd_setup_write_same_cmnd(cmd);
> >   	case REQ_OP_FLUSH:
> >   		return sd_setup_flush_cmnd(cmd);
> > -	case REQ_OP_READ:
> > -	case REQ_OP_WRITE:
> > -		return sd_setup_read_write_cmnd(cmd);
> >   	case REQ_OP_ZONE_REPORT:
> >   		return sd_zbc_setup_report_cmnd(cmd);
> >   	case REQ_OP_ZONE_RESET:
> > @@ -1973,30 +1978,12 @@ static int sd_done(struct scsi_cmnd *SCpnt)
> >   	int sense_valid = 0;
> >   	int sense_deferred = 0;
> >   
> > -	switch (req_op(req)) {
> > -	case REQ_OP_DISCARD:
> > -	case REQ_OP_WRITE_ZEROES:
> > -	case REQ_OP_WRITE_SAME:
> > -	case REQ_OP_ZONE_RESET:
> > -		if (!result) {
> > -			good_bytes = blk_rq_bytes(req);
> > -			scsi_set_resid(SCpnt, 0);
> > -		} else {
> > -			good_bytes = 0;
> > -			scsi_set_resid(SCpnt, blk_rq_bytes(req));
> > -		}
> > -		break;
> > -	case REQ_OP_ZONE_REPORT:
> > -		if (!result) {
> > -			good_bytes = scsi_bufflen(SCpnt)
> > -				- scsi_get_resid(SCpnt);
> > -			scsi_set_resid(SCpnt, 0);
> > -		} else {
> > -			good_bytes = 0;
> > -			scsi_set_resid(SCpnt, blk_rq_bytes(req));
> > -		}
> > -		break;
> > -	default:
> > +	/*
> > +	 * Assumption that REQ_OP_READ and REQ_OP_WRITE are 0 and 1 is
> > +	 * checked in init_sd(). The following "if" is done to expedite
> > +	 * REQ_OP_READ and REQ_OP_WRITE.
> > +	 */
> > +	if (likely(req_op(req) < 2)) {
> >   		/*
> >   		 * In case of bogus fw or device, we could end up having
> >   		 * an unaligned partial completion. Check this here and force
> > @@ -2011,6 +1998,36 @@ static int sd_done(struct scsi_cmnd *SCpnt)
> >   				    round_up(resid, sector_size));
> >   			scsi_set_resid(SCpnt, resid);
> >   		}
> > +	} else {
> > +		switch (req_op(req)) {
> > +		case REQ_OP_DISCARD:
> > +		case REQ_OP_WRITE_ZEROES:
> > +		case REQ_OP_WRITE_SAME:
> > +		case REQ_OP_ZONE_RESET:
> > +			if (!result) {
> > +				good_bytes = blk_rq_bytes(req);
> > +				scsi_set_resid(SCpnt, 0);
> > +			} else {
> > +				good_bytes = 0;
> > +				scsi_set_resid(SCpnt, blk_rq_bytes(req));
> > +			}
> > +			break;
> > +		case REQ_OP_ZONE_REPORT:
> > +			if (!result) {
> > +				good_bytes = scsi_bufflen(SCpnt)
> > +					- scsi_get_resid(SCpnt);
> > +				scsi_set_resid(SCpnt, 0);
> > +			} else {
> > +				good_bytes = 0;
> > +				scsi_set_resid(SCpnt, blk_rq_bytes(req));
> > +			}
> > +			break;
> > +		default:
> > +			sd_printk(KERN_NOTICE, sdkp, "unexpected REQ_OP=%u\n",
> > +				  (unsigned int)req_op(req));
> > +			WARN_ON_ONCE(true);
> > +			break;
> > +		}
> >   	}
> >   
> >   	if (result) {
> > @@ -3597,6 +3614,17 @@ static int __init init_sd(void)
> >   	int majors = 0, i, err;
> >   
> >   	SCSI_LOG_HLQUEUE(3, printk("init_sd: sd driver entry point\n"));
> > +	/*
> > +	 * The sd_init_command() and sd_done() assume REQ_OP_READ and
> > +	 * REQ_OP_WRITE are 0 and 1 and will fail if they are not. If they
> > +	 * are not, would prefer a compile failure but the preprocessor can't
> > +	 * use enum constants. Place check here because only need to check
> > +	 * early and once.
> > +	 */
> > +	if (REQ_OP_READ + REQ_OP_WRITE > 1)
> > +		pr_err("%s: REQ_OP_READ=%d REQ_OP_WRITE=%d %s.\n", __func__,
> > +		       REQ_OP_READ, REQ_OP_WRITE,
> > +		       "expected 0 and 1. Logic ERROR");
> >   
> >   	for (i = 0; i < SD_MAJORS; i++) {
> >   		if (register_blkdev(sd_major(i), "sd") != 0)