Re: [PATCH 03/11] block: add rq->resid_len

[Boaz Harrosh]

On 05/12/2009 12:07 PM, Tejun Heo wrote:
> Hello, Boaz.
> 
> Boaz Harrosh wrote:
>>> When a request failed, the whole buffer is garbage.
>> ret is the transferred size, right? I don't see any check for  
>> success/failure in below code.
>>
>>> There's no
>>> partial transfer.  There shouldn't be.  I don't think residual count
>>> on request failure means anything. 
>> That's not true, there are many cases when transfer failed eventually
>> but some bytes are valid. Even the simple read/write case. Imagine a 
>> very large transfer with last sector encounter a "bad sector". that can
>> be critical, (trying to rescue a disk). And many other examples.
> 
> No, it doesn't work like that.  As I wrote before, those partial
> transfers are returned to the issuer as partial _SUCCESS_ not partial
> failure.  ie. it's successful request w/ positive residual count not
> the other way around.  The error is communicated to the issuer when
> the leftover is re-issued.  There's no such thing as partial failure.
> 
> Please consider write failing after successfully writing certain
> number of bytes.  A failed write command MUST NOT cause any actual
> write on the device.
> 

I know for sure that scsi has other cases. The above is a per-command
per-sense-information. Some commands might write/read some bytes and still return
CHECK-CONDITION with some sense-info+residual pertaining what was actually
executed.

> Residual count is not about how many bytes have been transferred.  The
> number of transferred bytes itself doesn't mean a thing because it
> changes depending on which transfer protocol is used regardless of
> where the actual failure is.  Residual count is about how many bytes
> have been actually produced or consumed and when a command fails none
> should have been.
> 

See above. I know for a fact that this is not always the case, and that
it is a per command. In fact I relay on such behaviour in the osd filesystems.
If the midlayer would filter such cases for me I would have had data
corruptions.

[Read passed end of osd-object will read up to end, and return a warning
 check condition. With a choice of returning zero-bytes at invalid bytes
 or returning residual]

> Please note that in the above 'command' and 'residual count' are not
> as defined in SCSI.  Those terms refer to how those concepts are used
> in the kernel and via SG_IO interface.
> 

I do have Kernel code that relays on this. I do use bsg's SG_IO successfully
that has above behaviour.

That is: total transparency of residual count and sense information
communicated between user-code and target. Then user-code can make intelligent
decisions based on what he's done. And this is what the Kernel policy
should be. No more, no less.

>>> Also, the 'whenever possible'
>>> doesn't mean much when the issuer can't determine whether the value is
>>> valid or not.  On success, we should guarantee resid count is valid,
>>> on failure, I don't see a way we can.
>>>
>> Code is as strong as it's weakest link, right? If lower
>> driver/firmware is brain-dead, what can we do? But why give up where
>> you can do better?
>>
>> The scsi standard is very clear about what every one should do with
>> the residual and what it means at every stage, everyone should do
>> his part. Here at the middle layer we need to correctly translate
>> what lower level returned and pass it up the chain.
>>
>> Must stacks are amateuristic in regard to error handling but some
>> are not, what
>>
>> should we strive for, if we can? 
> 
> No it's not about being good or bad.  It's about being plain
> undefined.  There is no value in setting an inherently undefined value
> to certain values in certain corner cases.
> 

Again. With some commands/subsystems these are undefined, with some
like SCSI, they are very well defined in all cases. (Even in the undefined
cases). So please keep them intact, some code might want to relay on this.

(On the undefined cases Kernel should set a policy on what is returned)

>>>> I know that in iscsi a negative residual is possible which means
>>>> over-flow. That is: the target had more data to give then the buffer
>>>> had space for. (which is not an error at all)
>>> Hmm... I've never seen negative residual in use.  Is it even defined?
>> It is defined, as I explained before. But yes no one uses it in Kernel.
>> The "good" low-level drivers fix it up by setting resid to zero, in that
>> case. (other wise the upper layers might crash)
> 
> If the upper layer might crash if the value is negative, I think it's
> quite understandable to call negative values to be undefined.  Or
> should we call it unsupported?
> 

Yes, unsupported.

Since current meaning means success, actually, and there is no real extra information
that users need.
Look at it this way, until today negative meant Zero with extra information which
no one cares for.

> Thanks.
> 

Cheers
Boaz
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/