You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by oknet <gi...@git.apache.org> on 2016/05/27 10:27:21 UTC
[GitHub] trafficserver pull request: TS-4487: fix write_to_net_io issues
GitHub user oknet opened a pull request:
https://github.com/apache/trafficserver/pull/673
TS-4487: fix write_to_net_io issues
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/oknet/trafficserver patch-13
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/673.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #673
----
commit 1f7b4e670bc70e5583958e6c25e473fbd683228d
Author: Oknet <xu...@gmail.com>
Date: 2016-05-27T10:26:19Z
TS-4487: fix write_to_net_io issues
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4487: fix write_to_net_io issues
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the pull request:
https://github.com/apache/trafficserver/pull/673#issuecomment-222300709
vote on PR #629, but the 2nd issue: did not check the change of lock after return from wbe callback, not included.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4487: fix write_to_net_io issues
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/673#issuecomment-222324666
Good point on the lock check. Could have finished the current vio in the signal and then end of mysteriously disabling the next vio.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4487: fix write_to_net_io issues
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/673#discussion_r64913617
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -545,57 +545,61 @@ write_to_net_io(NetHandler *nh, UnixNetVConnection *vc, EThread *thread)
vc->write.triggered = 0;
write_signal_error(nh, vc, (int)-r);
return;
- } else {
- int wbe_event = vc->write_buffer_empty_event; // save so we can clear if needed.
+ }
- NET_SUM_DYN_STAT(net_write_bytes_stat, r);
+ int wbe_event = vc->write_buffer_empty_event; // save so we can clear if needed.
- // Remove data from the buffer and signal continuation.
- ink_assert(buf.reader()->read_avail() >= r);
- buf.reader()->consume(r);
- ink_assert(buf.reader()->read_avail() >= 0);
- s->vio.ndone += r;
+ NET_SUM_DYN_STAT(net_write_bytes_stat, r);
- // If the empty write buffer trap is set, clear it.
- if (!(buf.reader()->is_read_avail_more_than(0)))
- vc->write_buffer_empty_event = 0;
+ // Remove data from the buffer and signal continuation.
+ ink_assert(buf.reader()->read_avail() >= r);
+ buf.reader()->consume(r);
+ ink_assert(buf.reader()->read_avail() >= 0);
+ s->vio.ndone += r;
- net_activity(vc, thread);
- // If there are no more bytes to write, signal write complete,
- ink_assert(ntodo >= 0);
- if (s->vio.ntodo() <= 0) {
- write_signal_done(VC_EVENT_WRITE_COMPLETE, nh, vc);
- return;
- } else if (signalled && (wbe_event != vc->write_buffer_empty_event)) {
+ // If the empty write buffer trap is set, clear it.
+ if (!(buf.reader()->is_read_avail_more_than(0)))
+ vc->write_buffer_empty_event = 0;
+
+ net_activity(vc, thread);
+ // If there are no more bytes to write, signal write complete,
+ ink_assert(ntodo >= 0);
+ int e = 0;
+ if (s->vio.ntodo() <= 0) {
+ write_signal_done(VC_EVENT_WRITE_COMPLETE, nh, vc);
+ return;
+ } else {
+ if (!signalled) {
+ e = VC_EVENT_WRITE_READY;
+ } else if (wbe_event != vc->write_buffer_empty_event) {
// @a signalled means we won't send an event, and the event values differing means we
// had a write buffer trap and cleared it, so we need to send it now.
- if (write_signal_and_update(wbe_event, vc) != EVENT_CONT)
- return;
- } else if (!signalled) {
- if (write_signal_and_update(VC_EVENT_WRITE_READY, vc) != EVENT_CONT) {
- return;
- }
-
- // change of lock... don't look at shared variables!
- if (lock.get_mutex() != s->vio.mutex.get()) {
- write_reschedule(nh, vc);
- return;
- }
+ e = wbe_event;
}
-
- if (!buf.reader()->read_avail()) {
- write_disable(nh, vc);
+ }
+ if (e) {
+ if (write_signal_and_update(e, vc) != EVENT_CONT) {
return;
}
- if ((needs & EVENTIO_WRITE) == EVENTIO_WRITE) {
+ // change of lock... don't look at shared variables!
+ if (lock.get_mutex() != s->vio.mutex.get()) {
write_reschedule(nh, vc);
+ return;
}
- if ((needs & EVENTIO_READ) == EVENTIO_READ) {
- read_reschedule(nh, vc);
- }
+ }
+
+ if (needs==0 && !buf.reader()->read_avail()) {
--- End diff --
Should use `read_avail_more_than(0)` here for performance as the exact number of bytes is irrelevant.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4487: fix write_to_net_io issues
Posted by shinrich <gi...@git.apache.org>.
Github user shinrich commented on the pull request:
https://github.com/apache/trafficserver/pull/673#issuecomment-222175909
I think the right thing to do is push PR #629. That moves the read/write loops to use the IOBuffer interface instead of the IOBlock interface. I've been holding off on this one until the event loop changes landed since it solved a real problem I had seen. But if other folks are seeing problems in this area the IOBuffer interface code is much easier to reason about and debug.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4487: fix write_to_net_io issues
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the pull request:
https://github.com/apache/trafficserver/pull/673#issuecomment-222517497
Due to the #629 already merged, I will update the patch to fit it.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #673: TS-4487: fix write_to_net_io issues
Posted by oknet <gi...@git.apache.org>.
Github user oknet commented on the issue:
https://github.com/apache/trafficserver/pull/673
@zwoop @shinrich a new PR#754 created based on the newest master branch.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #673: TS-4487: fix write_to_net_io issues
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/673
@oknet This needs a rebase please. @shinrich Can you review please?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request #673: TS-4487: fix write_to_net_io issues
Posted by oknet <gi...@git.apache.org>.
Github user oknet closed the pull request at:
https://github.com/apache/trafficserver/pull/673
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: TS-4487: fix write_to_net_io issues
Posted by SolidWallOfCode <gi...@git.apache.org>.
Github user SolidWallOfCode commented on a diff in the pull request:
https://github.com/apache/trafficserver/pull/673#discussion_r64913857
--- Diff: iocore/net/UnixNetVConnection.cc ---
@@ -545,57 +545,61 @@ write_to_net_io(NetHandler *nh, UnixNetVConnection *vc, EThread *thread)
vc->write.triggered = 0;
write_signal_error(nh, vc, (int)-r);
return;
- } else {
- int wbe_event = vc->write_buffer_empty_event; // save so we can clear if needed.
+ }
- NET_SUM_DYN_STAT(net_write_bytes_stat, r);
+ int wbe_event = vc->write_buffer_empty_event; // save so we can clear if needed.
- // Remove data from the buffer and signal continuation.
- ink_assert(buf.reader()->read_avail() >= r);
- buf.reader()->consume(r);
- ink_assert(buf.reader()->read_avail() >= 0);
- s->vio.ndone += r;
+ NET_SUM_DYN_STAT(net_write_bytes_stat, r);
- // If the empty write buffer trap is set, clear it.
- if (!(buf.reader()->is_read_avail_more_than(0)))
- vc->write_buffer_empty_event = 0;
+ // Remove data from the buffer and signal continuation.
+ ink_assert(buf.reader()->read_avail() >= r);
--- End diff --
Use `is_read_avail_more_than(r)` and `is_read_avail_more_than(0)`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---