James Smart wrote:
>
> Boaz Harrosh wrote:
> <snip..>
>>> + if (rsp) {
>>> + rsp_len = blk_rq_bytes(rsp);
>>> + BUG_ON(job->reply->reply_payload_rcv_len > rsp_len);
>>> + /* set reply (bidi) residual */
>>> + rsp->data_len = (rsp_len - job->reply->reply_payload_rcv_len);
>>> + }
>>> +
>>> + /* we assume all request payload was transferred */
>> - /* we assume all request payload was transferred */
>> + /* we assume all request payload was transferred, residual == 0 */
>> + req->data_len = 0;
>>
>> Note: this is the proper procedure. Otherwise bsg user will receive transfer
>> residual of all payload. On the other hand you might have more precise residual
>> information here, as received from the scsi transport? (Specially in the error
>> case, perhaps in the future).
>>
>>> + blk_end_request(req, err, blk_rq_bytes(req));
>> - blk_end_request(req, err, blk_rq_bytes(req));
>> + blk_end_bidi_request(req, err, req_len, rsp_len);
>>
>>> +
>>> + fc_destroy_bsgjob(job);
>>> +}
>> This should take care of all possible cases, (bidi or otherwise), and will
>> return the proper transfer residual count of 0 to user-mode.
>
> We should be calling blk_end_bidi_request() even in cases where there wasn't a
> req->next_rq ?
>
> Here's what I would have done. Has your changes for setting req_len and
> req->data_len, and Seokman's mods for calling either blk_end_bidi_request() or
> blk_end_request() depending on whether there is a rsp request.
>
> -- james s
>
>
> /**
> * fc_bsg_jobdone - completion routine for bsg requests that the LLD has
> * completed
> * @job: fc_bsg_job that is complete
> */
> static void
> fc_bsg_jobdone(struct fc_bsg_job *job)
> {
> struct request *req = job->req;
> struct request *rsp = req->next_rq;
> unsigned long flags;
> unsigned rsp_len = 0, req_len = blk_rq_bytes(req);
>
> int err;
>
> spin_lock_irqsave(&job->job_lock, flags);
> job->state_flags |= FC_RQST_STATE_DONE;
> job->ref_cnt--;
> spin_unlock_irqrestore(&job->job_lock, flags);
>
> err = job->req->errors = job->reply->result;
> if (err < 0)
> /* we're only returning the result field in the reply */
> job->req->sense_len = sizeof(uint32_t);
> else
> job->req->sense_len = job->reply_len;
>
> /* we assume all request payload was transferred, residual == 0 */
> req->data_len = 0;
>
> if (rsp) {
> rsp_len = blk_rq_bytes(rsp);
> BUG_ON(job->reply->reply_payload_rcv_len > rsp_len);
> /* set reply (bidi) residual */
> rsp->data_len = (rsp_len - job->reply->reply_payload_rcv_len);
> blk_end_bidi_request(req, err, req_len, rsp_len);
> } else
> blk_end_request(req, err, req_len);
>
> fc_destroy_bsgjob(job);
> }
No! Please most certainly don't. this is the all issue here. blk_end_request
is just a wrapper for an internal blk_end_bidi_request(). They are essentially
the same exact call when the first just puts a zero at bidi_bytes.
My code is the preferred and easiest and most correct way. Again calling
blk_end_bidi_request() is absolutely always safe, only thing missing is a proper
comment for blk_end_bidi_request() that says so.
Cheers
Boaz
--
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
