Re: [PATCH] 2.6.25-rc4-git3 - inquiry cmd issued via /dev/sg? device causes infinite loop in 2.6.24

[Michael Reed <mdr@sgi.com>]

Boaz Harrosh wrote:

<snip>

> 
> All these patches do not solve the root cause of the problem and until
> solved, (easily) they will keep popping up.

I will ask that the correctness of the sg driver's construction of
a scatter list also be considered here.  Is it ever correct for bi_size
to wind up larger than the request size?  Why is rounding needed in one
case (sg_build_indirect) but not the other (sg_link_reserve)?

> The root cause is that BLOCK_PC commands *can not* be retried in chunks. 
> The midlayer has no way to know how to retry them. and any attempt to do so
> will result in an illegal scsi command.
> 
> I'm really sorry for not seeing this before. I have seen all these emails
> and patches back and forth but they addressed such remote locations, that
> I have not paid attention to them.
> 
> So lets try to touch base. BLOCK_PC command can only be completed in full
> always. In case of error or residual this should be properly reported and
> that is all the midlayer can do. The Initiator That knows what command was
> sent will then decide on a retry or not. In above case that will be user
> mode.

Those who truly understand this should comment.  It makes sense to this
novice.

> 
> Just to demonstrate what I mean a patch is attached. Just as an RFC, totally
> untested.

I can try this out and see what happens.


Mike

> ---
> git-diff --stat -p
>  drivers/scsi/scsi_lib.c |   29 ++++++++++++++++-------------
>  1 files changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index ba21d97..e5d05c6 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -784,17 +784,24 @@ EXPORT_SYMBOL(scsi_release_buffers);
>   * in req->data_len and req->next_rq->data_len. The upper-layer driver can
>   * decide what to do with this information.
>   */
> -void scsi_end_bidi_request(struct scsi_cmnd *cmd)
> +static void scsi_end_blk_request(struct scsi_cmnd *cmd)
>  {
>  	struct request *req = cmd->request;
>  	unsigned int dlen = req->data_len;
> -	unsigned int next_dlen = req->next_rq->data_len;
> +	unsigned int next_dlen;
>  
> -	req->data_len = scsi_out(cmd)->resid;
> -	req->next_rq->data_len = scsi_in(cmd)->resid;
> +	if (blk_bidi_rq(req)) {
> +		next_dlen = req->next_rq->data_len;
> +		req->data_len = scsi_out(cmd)->resid;
> +		req->next_rq->data_len = scsi_in(cmd)->resid;
> +	} else {
> +		next_dlen = 0;
> +		req->data_len = scsi_get_resid(cmd);
> +	}
>  
> -	/* The req and req->next_rq have not been completed */
> -	BUG_ON(blk_end_bidi_request(req, 0, dlen, next_dlen));
> +	if (blk_end_bidi_request(req, 0, dlen, next_dlen))
> +		/* The req and req->next_rq have not been completed */
> +		BUG_ON();
>  
>  	scsi_release_buffers(cmd);
>  
> @@ -866,15 +873,11 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned int good_bytes)
>  				req->sense_len = len;
>  			}
>  		}
> -		if (scsi_bidi_cmnd(cmd)) {
> -			/* will also release_buffers */
> -			scsi_end_bidi_request(cmd);
> -			return;
> -		}
> -		req->data_len = scsi_get_resid(cmd);
> +		/* will also release_buffers */
> +		scsi_end_blk_request(cmd);
> +		return;
>  	}
>  
> -	BUG_ON(blk_bidi_rq(req)); /* bidi not support for !blk_pc_request yet */
>  	scsi_release_buffers(cmd);
>  
>  	/*
> 
> 
> 
> 
> 
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html