At 10:34 AM 12/10/2007, Tom Tucker wrote:
>
>On Fri, 2007-12-07 at 12:02 -0500, Talpey, Thomas wrote:
>> At 06:11 PM 12/6/2007, James Lentini wrote:
>> >On Thu, 6 Dec 2007, Trond Myklebust wrote:
>> >> Eeewwww.... What an unholy mess!
>> >>
>> >> Why are you trying to shoehorn reading and writing into
>> >> rpcrdma_create_chunks()? The way every action is split into 'if
>> >> (read) {} else {}' should be an obvious hint that none of this
>> >> belongs together. Please split this up for 2.6.25.
>> >
>> >ok.
>>
>> Agreed - well except for the "unholy mess" part. This code has been
>> in the nfs/rdma client basically since it was written (for 2.4.18). The
>> split version introduces quite a bit of redundancy, but clarity is good
>> too. I'll split it willingly.
>>
>
>Hmm. I guess I disagree. I don't see how consolidating similar logic is
>an unholy mess. The purpose of the function is singular, i.e. preparing
>the chunk list contained in the request sent to the server. Although it
I think you and I (the two Tom's) are in agreement. I'll still commit to
splitting the routine, as an exercise for comparison. When it was first
written, the code would be largely duplicated, therefore harder to maintain
in the long run. But it might be different today.
Another thing to think about is the possibility that in the future, multiple
chunk lists might need to be marshaled, for instance an NFSv4 COMPOUND
that needed both a chunked argument and chunked result. The code today
doesn't need to support that, but someday it might. That was deferred
intentionally, of course (there's a comment to that effect).
Tom.
>is true that the chunklist encodes memory in the response (for read
>requests), the chunk list itself is a single entity. Splitting it up may
>be more confusing than combining and will certainly introduce copied
>logic.
>
>
>> >> So yes, for read buffers, you should be looking at the storage
>> >> buffer length (xdr_buf->buflen) since the message length is
>> >> obviously going to be zero.
>> >
>> >Agreed. This is why we removed the incorrect references to len in the
>> >patch.
>> >
>> >What is your opinion on including the fix in 2.6.24? It addresses an
>> >observed problem for certain I/Os over NFS/RDMA. Would you like me to
>> >resend the patch with a revised description?
>>
>> The patch basically removes a debug-only assertion (the "failed to marshal"
>> message), but the single line that tested for buflen > pos has the side
>> effect of failing to marshal the tail buffer. So that one line is
>the key which
>> introduces the issue, and it should be removed for 2.6.24.
>>
>> The other stuff is harmless, if wrong, and can be removed later.
>>
>> Tom.
>>
>> -------------------------------------------------------------------------
>> SF.Net email is sponsored by:
>> Check out the new SourceForge.net Marketplace.
>> It's the best place to buy or sell services for
>> just about anything Open Source.
>> http://sourceforge.net/services/buy/index.php
>> _______________________________________________
>> NFS maillist - [email protected]
>> https://lists.sourceforge.net/lists/listinfo/nfs
>> _______________________________________________
>> Please note that [email protected] is being discontinued.
>> Please subscribe to [email protected] instead.
>> http://vger.kernel.org/vger-lists.html#linux-nfs