On Thu, 27 Nov 2008 13:51:51 +0200
Boaz Harrosh <bharrosh@xxxxxxxxxxx> wrote:
> >>>> + 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'll cheat: tell blk layer all of the xmt data was sent.
> >>>> + * but try to be honest about the amount of rcv data received
> >>>> + */
> >>>> + if (rsp)
> >>>> + blk_end_bidi_request(job->req, err, blk_rq_bytes(job->req),
> >>>> + job->reply->reply_payload_rcv_len);
> >>>> + else
> >>>> + blk_end_request(job->req, err, blk_rq_bytes(job->req));
> >>> I think that you can use blk_end_bidi_request() for non-bidi requests:
> >>>
> >>> blk_end_bidi_request(job->req, err, blk_rq_bytes(job->req),
> >>> rsp ?
> >>> job->reply->reply_payload_rcv_len : 0);
> >>>
> >>>
> >>> I guess that it would be better to have one function to complete a
> >>> request, instead of blk_end_bidi_request and blk_end_request.
> >>>
> >>>
> >> + /*
> >> + * tell blk layer all of the xmt data was sent.
> >> + * but set residual count to: requested - received
> >> + */
> >> +
> >> + if (rsp) {
> >> + bytes_requested = blk_rq_bytes(rsp);
> >> + rsp->data_len = bytes_requested - job->reply->reply_payload_rcv_len;
> >> + }
> >> +
> >> + blk_end_bidi_request(job->req, err, blk_rq_bytes(job->req), bytes_requested);
> >>
> >> The residual count is left in req->data_len. Does bsg have a way to return the
> >> residual to user-mode? It must, since Pete was using that for sure. Note that
> >> you are looking for the bidi_read residual count.
> >
> > Yeah, bsg has. struct sg_io_v4 has:
> >
> > __s32 din_resid; /* [o] din_xfer_len - actual_din_xfer_len */
> > __s32 dout_resid; /* [o] dout_xfer_len - actual_dout_xfer_len */
> >
> >
> >> As was said by people. You must complete ALL bytes on both sides. Residual information
> >> is passed through req->data_len. Other wise the request is still active.
> >>
> >> (And yes blk_end_request uses blk_end_bidi_request internally)
> >
> > We always complete all bytes on both sides. So why we do something
> > like:
> >
> > int blk_end_request(struct request *rq, int error, unsigned int nr_bytes)
> > {
> > unsigned int bidi_bytes = 0;
> >
> > if (blk_bidi_rq(rq))
> > bidi_bytes = req->next_rq->data_len;
> >
> > return blk_end_io(rq, error, nr_bytes, bidi_bytes, NULL);
> > }
> >
> > The callers can do something like:
> >
> > blk_end_request(rq, err, rq->data_len);
> > rq-->next_rq->data_len = resid;
>
> Sorry TOMO, I do not understand what you mean. Do you say that we should
> change blk_end_request() in blk-core.c ?
Having two kinds of functions (blk_end_request and
blk_end_bidi_request) to complete requests confuse people. As we saw,
developers tend to do something like this:
+ if (rsp)
+ blk_end_bidi_request(job->req, err, blk_rq_bytes(job->req),
+ job->reply->reply_payload_rcv_len);
+ else
+ blk_end_request(job->req, err, blk_rq_bytes(job->req));
The callers don't care about whether a request is bidi or not. It's be
simpler to have a single function to complete a request (whether a
request is bidi or not) rather than having two different functions.
We must complete all bytes on both sides with a bidi request. So why
can't we modify blk_end_request to handle both bidi and non-bidi
requests:
int blk_end_request(struct request *rq, int error, unsigned int nr_bytes)
{
unsigned int bidi_bytes = 0;
if (blk_bidi_rq(rq))
bidi_bytes = blk_rq_bytes(rq->next_rq);
return blk_end_io(rq, error, nr_bytes, bidi_bytes, NULL);
}
> In anyway, the code you suggest has a bug you can not use rq-> after call to blk_end_io()
> because it might not exist at this point. You must set residual before. And also
What is 'rq->' exactly?
We must set residual before calling blk_end_request? Really?
Note that scsi-ml and bsg (blk_execute_rq) work differently. For
scsi-ml, blk_end_io frees request structure (end_that_request_last)
but for blk_execute_rq, it doesn't.
Anyway, it's fine to set bidi_resid before blk_end_request, I
guess. FC pass thru code could do something like this if we modify
blk_end_request in the above way:
/* we calculate bidi_resid here */
if (blk_bidi_rq(req))
req->next_rq->data_len = bidi_resid;
blk_end_request(req, 0, blk_rq_bytes(req));
--
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
