Re: [PATCH] Updated Comm::Read I/O API

On 5/06/2014 5:16 a.m., Alex Rousskov wrote:
> On 06/04/2014 09:10 AM, Amos Jeffries wrote:
>
>> * Comm::Read() which accepts the Comm::Connection pointer for the
>> socket to read on and an AsyncCall callback to be run when read is
>> ready. The Job is responsible for separately initiating read(2) or
>> alternative action when that callback is run.
>>
>> * Comm::ReadNow() which accepts an SBuf buffer and a CommIoCbParams
>> initialized to contain the Comm::Connection pointer for the socket to
>> read on which will be filled out with result flag, xerrno, and size.
>> This synchronously performs read(2) operations to append bytes to the
>> provided buffer. It returns a comm_err_t flag for use in determining how
>> to handle the results and signalling one of OK, INPROGRESS, ERROR,
>> ERR_CLOSING as having happened.
>
>
>> +inline void comm_read(const Comm::ConnectionPointer &conn, char *buf, int len, AsyncCall::Pointer &callback)
>> +{
>> + assert(buf != NULL);
>
> but:
>
>> commHalfClosedCheck(void *)
>> {
> ...
>> comm_read(c, NULL, 0, call);
>
> Please fix and double check that no other comm_read calls are using a
> NULL buffer. If you cannot be sure, let's not enforce this to avoid
> breaking working code.
>

Oops. Fixed and all of src/ audited for this just to be sure.

The handler commHalfClosedReader() is now uselessly asserting size==0.
But I have left that alone for now.

>
>> + if (retval > 0) { // data read most common case
> ...
>> + fd_bytes(params.conn->fd, retval, FD_READ);
>
>> + /* Note - read 0 == socket EOF, which is a valid read */
>> + if (retval >= 0) {
>> + fd_bytes(fd, retval, FD_READ);
>
> Please be consistent. Since bytes accounting is not your patch focus, I
> suggest calling fd_bytes() for zero returned value to avoid a subtle
> irrelevant change.
>

Updating the API to improve performance is the overall goal of this (and
earlier patches). Removing a useless convenience-library jump and some
if-statements in the new SBuf pathway comes under that even if it is
small. Given the new flag returns optimizing makes sense at this point
rather than delaying. I did audit the fd_bytes() code path to ensure the
change was a no-op removal.

>
>> + // if the caller did not supply a buffer, just schedule callback
>
> Please detail along these lines:
>
> // without a buffer, just call back and the caller will ReadNow()
>

We place no criteria on what the caller will do next. The connection
monitor callers will in fact close() instead of ReadNow().

I am using the first half of your statement, but not the "and the caller
will ..."

>> + /* For legacy callers : Attempt a read */
>
> Please add something like "Keep in sync with Comm::ReadNow()!"
>

Done although this is not a strict requirement. ReadNow() is for updated
callers and may perform some things differently as per our new model
(ie. flag return values, SBuf usage, readv(2)?, etc.) the old read(2)
acts must be kept more in sync with old callers expectations.

>
>> + debugs(5, 3, "SBuf " << params.conn << ", size " << sz << ", retval " << retval << ", errno " << params.xerrno);
>
>> + debugs(5, 3, "char FD " << fd << ", size " << ccb->size << ", retval " << retval << ", errno " << errno);
>
> Please remove the "SBuf " and "char " prefixes. These lines are about
> Comm I/O, these specific prefixes do not make much sense here, and
> debugs() will supply the right context.

Removed.

>
>> + SBuf::size_type sz = buf.spaceSize();
>
> Please make const if possible.
>

Done.

>
>> + char *b = buf.rawSpace(sz);
>
> Please rename "b" to "space", "rawSpace", or something like that to ease
> reading/searching.
>
Done. "theBuf"

>
>> + int retval = FD_READ_METHOD(params.conn->fd, b, sz);
>
> Please make constant.
>
Done.

>
>> + } else if (retval == 0) { // remote closure (somewhat less) common
>> + // Note - read 0 == socket EOF, which is a valid read.
>> + params.flag = COMM_ERR_CLOSING;
>
> It is a bad idea to overload COMM_ERR_CLOSING like this! The reaction to
> COMM_ERR_CLOSING in the callback is very different to the reaction to
> EOF! And EOF is not really an error. If you think this case deserves a
> dedicated flag, add COMM_EOF instead.
>

Okay. Added and changed to COMM_EOF.

>
>> + debugs(5, 3, params.conn << " COMM_ERROR: " << xstrerror());
>
> Supply params.xerrno. The errno global may have changed.
>

Fixed.

>
>> /* Bail out quickly on COMM_ERR_CLOSING - close handlers will tidy up */
>> -
>> if (io.flag == COMM_ERR_CLOSING) {
>> - debugs(33,5, HERE << io.conn << " closing Bailout.");
>> + debugs(33,5, io.conn << " closing Bailout.");
>> return;
>> }
>
> Avoid non-changes.
>

Needs doing eventually and we put off a big patch so we could do these
incrementally like this. I am re-witing the whole of the callers read
handler function here.

>
>> - /*
>> - * Don't reset the timeout value here. The timeout value will be
>> - * set to Config.Timeout.request by httpAccept() and
>> - * clientWriteComplete(), and should apply to the request as a
>> - * whole, not individual read() calls. Plus, it breaks our
>> - * lame half-close detection
>> - */
>
> Are these comments no longer correct? Restore if unsure.
>

The content was very stale. Restored and updated to describe only how
the scope of timeout prescribes leaving it unchanged here.

>
> I believe the above adjustments can be made without another round of
> reviews.
>

Applied then as trunk rev.13442

>
> FWIW, I was not able to verify that the moved Comm code and the reshaped
> ConnStateData::clientReadRequest() code contained only the intentional
> changes.

Noted. Our trust we place in cut-n-paste :-)

Amos
Received on Thu Jun 05 2014 - 08:33:51 MDT