Re: [PATCH] Use SBuf for tunnel.cc I/O

On 13/05/2014 10:46 a.m., Alex Rousskov wrote:
> On 05/11/2014 11:16 AM, Amos Jeffries wrote:
>
>> This patch adds a Comm::Write API for accepting SBuf output buffers.
>
>
>> Unlike the char* API the buffer can be appended to before write(2) is
>> completed and the appended bytes will be handled as part of the original
>> write request.
>
> That approach scares me because the caller has to be extra careful not
> to leave the being-updated-area of the buffer in inconsistent state
> across async calls. However, I doubt we have such complex callers today,
> and the approach also reduces the number of usually pointless write
> callback calls, so I am willing to experiment.

If you have a better approach I would like to hear it.

The only one I can think of is possibly Ref-Counting the SBuf instance
itself. Adding that capability though seems to be implying that
SBuf::Pointer in general is better than just passing SBuf references
around (its not).

The raw-pointer usage which appears to be scaring you is the existing
status quo anyway...

 * char* API passes a raw pointer to the store/buffer. Any alterations
of the content are racing the actual write(2) call for those bytes,
appends are lost on callback, and truncate are not seen by the write(2).
Corrupted output one way or another.
  => requirement that the caller do nothing other than allocate a new
buffer while write is scheduled. Buffer may be re-used after the callback.

 * MemBuf API passes a raw pointer to its _internal_ data store/buffer.
It goes further by passing the store deallocator. Any alterations of the
content are racing the actual write(2) call for those bytes, appends are
either lost on callback or reallocate the backing store for the caller,
and truncate are not seen by the write(2). All operations are
potentially done on a buffer pointer already deallocated by comm.
  => requirement that the caller do nothing other than MemBuf::init()
while write is scheduled (ie allocate a new buffer). Same condition
applies even after callback (ouch).

 * SBuf API passes a reference/raw-pointer to the smart pointer SBuf
instance. All operations on the MemBlob backing store remain valid.
  => requirement that the caller do nothing to the SBuf instance other
than allocate a new one while write is scheduled.
   ==> Note this is the same requirement as made on both other API
already. But no longer restricted on what can be done with the backing
store by the caller. ie it can continue to use the SBuf it has for
regular SBuf operations.

Since these SBuf are members of a caller Job (or equivalent state
object) instead of a dynamically allocated/deallocated SBuf object that
seems a reasonable requirement.

>
>> All other behaviour is identical between the three Comm::Write APIs.
>
> You forgot to mention the fact that the new Write API, unlike the
> existing two, relies on the caller to keep the buffer area alive. That
> requirement, IMO, is a show-stopper. I strongly object to that change.

If by "buffer area" you mean MemBlob that is not a requirement. It can
have anything at all done to it during Comm::Write. If it is cleared the
write will probably return 0 bytes written to the callback. Most
callbacks assume that means the connection has terminated unexpectedly.
So its not advisable, but possible without any internal problems to comm.

If by "buffer area" you means SBuf. See above comparison with the other
two APIs.

I think that is reasonable to retain that requirement. Especially since
it only applies on the small SBuf instance.
 One either sends an SBuf which is a member of the Job/*StateData object
itself or an SBuf member of the Params object which is delivered to the
callback. Both methods ensure its alive until at least the callback is
scheduled.

If/when we start delivering lists of small SBuf objects that is a whole
other API needed for SBufList. OR perhapse a Job loop writing each SBuf
in the list individually ... with the Job doing this keeping the
relevant SBuf instance alive.

>
> The buf2 hack has already been added during unaudited "Convert the
> ConnStateData input buffer to SBuf" commit (r13324), so there is little
> I can do about it until it causes a bug, but I do object to the use of
> such raw/unprotected links between Comm and its callers in any code I am
> lucky to review before the commit.
>
> If others overrule this objection, please at least change the
> Comm::Write() sb argument type from reference to a pointer. Passing an
> argument by reference (implying temporary use) and then taking an
> address of that argument for long term object storage is wrong.

Fine.

>
>> /// Buffer to store read(2) into when set.
>> // This is a pointer to the Jobs buffer rather than an SBuf using
>> // the same store since we cannot know when or how the Job will
>> // alter its SBuf while we are reading.
>> SBuf *buf2;
>>
>> // Legacy c-string buffers used when buf2 is unset.
>> char *buf;
>
>
> Another side-effect is that "I wrote this, what's next?" notifications
> may now be delayed for a very long time (potentially forever) if the
> caller keeps adding content to the buffer. I suspect that most writers
> do not really need such notifications. Thank you for documenting this
> aspect (although it deserves to be mentioned in the commit message as
> well IMO).

There is no code doing this behaviour at present. I trust that we have
enough quality on new code now to ensure that its only taken advantage
of when appropriate.

For now the (offset > size) check for re-scheduling is still in effect.
It is just that up to TCP buffer amount more bytes can be written if
they are appended early. The callback will be delivered the accurate
written amount and teh buffer consumed() correctly either way.

I've changed the "will" to "may" to avoid implying that the indefinite
wait/hang situation.

In this round I have also added a check to ensure that caller using
consume() does not cause hanging. But there are worse logic problems in
callers doing that inappropriately.

>
>> - static void ReadClient(const Comm::ConnectionPointer &, char *buf, size_t len, comm_err_t errcode, int xerrno, void *data);
>> + static void ReadClient(const Comm::ConnectionPointer &, char *, size_t len, comm_err_t errcode, int xerrno, void *data);
>
> Please avoid this and similar non-changes to reduce merge conflicts. It
> is OK to remove the parameter name in the method _definition_ where it
> was used and is no longer needed(**), but there is no need to update
> methods that did not use the parameter or the declarations. These
> methods should be refactored/polished when TunnelStateData becomes a
> job, but that is outside your patch scope.
>
> (**) although the existing code like TunnelStateData::readServer()
> implies that such changes are not required either.
>

Fine.

>
>> - void bytesIn(int const &);
>> -#if USE_DELAY_POOLS
>> -
>> - void setDelayId(DelayId const &);
>> -#endif
>> -
>> void error(int const xerrno);
>> int debugLevelForError(int const xerrno) const;
>> /// handles a non-I/O error associated with this Connection
>> void logicError(const char *errMsg);
>> void closeIfOpen();
>> void dataSent (size_t amount);
> ...
>> int64_t *size_ptr; /* pointer to size in an ConnStateData for logging */
>>
>> Comm::ConnectionPointer conn; ///< The currently connected connection.
>>
>> - private:
>> #if USE_DELAY_POOLS
>> -
>> DelayId delayId;
>> #endif
>> -
>
>> - server.bytesIn(len);
>> +#if USE_DELAY_POOLS
>> + server.delayId.bytesIn(len);
>> +#endif
>
>> - tunnelState->server.setDelayId(DelayId());
>> + tunnelState->server.delayId = DelayId();
>
> Please avoid these and similar changes if they are unrelated to your
> patch scope.
>

With the removal of client.len/server.len byte accounting the
delayId.bytesIn() is now used directly for delay pool byte accounting.
Which makes the delayId wrappers of all types needless.

>
>> if (rep.hdr_sz < connectRespBuf->contentSize()) {
>> // preserve bytes that the server already sent after the CONNECT response
>> - server.len = connectRespBuf->contentSize() - rep.hdr_sz;
>> - memcpy(server.buf, connectRespBuf->content()+rep.hdr_sz, server.len);
>> - } else {
>> - // reset; delay pools were using this field to throttle CONNECT response
>> - server.len = 0;
>> + server.buf.append(connectRespBuf->content()+rep.hdr_sz, connectRespBuf->contentSize() - rep.hdr_sz);
>> }
>
> Can you explain why the "else" clause is no longer needed? Or was it
> bogus to start with?
>

It was accumulating value due to server.bytesIn(len) being used as
access to delayId_ from readConnectResponseDone().

We now use server.delayId.bytesIn(len) in both the read callbacks and
the else case became bogus.

>
>> - if (!tunnelState->server.len) {
>> + if (tunnelState->server.buf.isEmpty()) {
>
> and
>
>> - if (!tunnelState->client.len) {
>> + if (tunnelState->client.buf.isEmpty()) {
>
> but
>
>> - if (!tunnelState->server.len)
>> + if (!tunnelState->server.buf.length())
>
> Please be consistent: isEmpty() is probably slightly better than
> !length(), but it is your call which method to use.
>

Fixed.

>
>
>> - debugs(26, DBG_DATA, "Tunnel server PUSH Payload: \n" << Raw("", tunnelState->server.buf, tunnelState->server.len) << "\n----------");
>> + debugs(26, DBG_DATA, "Tunnel server PUSH Payload: \n" << tunnelState->server.buf << "\n----------");
>
> Please do not remove Raw() protection for dumping raw data.
>

SBuf provides generic operator<<(std::ostream&) for debugs.

If Raw() protection is required on the buffer it needs to be provided by
that. There are a number of places dumping SBuf contents to debugs()
already. Will do as a followup if necessary.

Amos