Welcome! Log In Create A New Profile

Advanced

Re: cache: move open to thread pool

Roman Arutyunyan
August 16, 2019 10:26AM
Hi,

On Wed, Jul 24, 2019 at 11:47:19AM +0800, 洪志道 wrote:
> Hi, I found an issue with the patch.
>
> 1. Reproduce
> (/usr/local/nginx/html/5M.txt, Just making `ngx_http_set_write_handler` be
> called.)
>
> telnet localhost 80
> GET /5M.txt HTTP/1.1
> HOST: 1.1.1.1
>
> -- response--
>
> GET /5M.txt HTTP/1.1
> HOST: 1.1.1.1
>
> --50 error-- /* happened in keepalive */

Thanks for reporting this.

> 2. Reason
> It happens in the case of keepalive.
> Since `ngx_http_set_write_handler` is called, c->write is active and ready,
> after receiving http request:
> ngx_http_static_handler is called
> {
> ngx_http_static_send(); // task has been added thread pool, but its
> event is not complete.
> r->write_event_handler = ngx_http_static_write_event_handler;
> }
> And the epoll write event is triggered. So
> ngx_http_static_write_event_handler is called again.
> But task->event.active is true. ngx_thread_task_post() will fail.
> Throws "task #%ui already active".

The problem only manifests itself with epoll. After both EPOLLIN and EPOLLOUT
for the same socket are registered in epoll, EPOLLOUT is reported along with
EPOLLIN despite the fact that only the read state of the socket changed but the
write state didn't (socket was and is writable). This is not a problem by
itself, but it helped to find the problem with this patch.

While thread task is in progress, calling ngx_http_static_write_event_handler()
resulted in posting the task again, which produced the error.

> 3. Early patch.
> ```
> diff -r bd11e3399021 src/http/modules/ngx_http_static_module.c
> --- a/src/http/modules/ngx_http_static_module.c Tue Oct 30 15:00:49 2018
> +0300
> +++ b/src/http/modules/ngx_http_static_module.c Tue Jul 23 23:46:22 2019
> -0400
> @@ -318,6 +318,12 @@ ngx_http_static_write_event_handler(ngx_
> {
> ngx_int_t rc;
>
> + if (r->open_file_info.thread_task != NULL
> + && r->open_file_info.thread_task->event.complete == 0)
> + {
> + return;
> + }
> +
> rc = ngx_http_static_send(r);
>
> if (rc != NGX_DONE) {

Similar problem in thread read is prevented by checking the flag r->aio.
I did the same for thread open (see attachment).

> ```
>
> Thanks.
>
>
> On Mon, Jul 22, 2019 at 10:05 PM Roman Arutyunyan <arut@nginx.com> wrote:
>
> > Hi,
> >
> > On Sat, Jul 20, 2019 at 09:44:25AM +0800, 洪志道 wrote:
> > > > The patch wasn't updated since that but I suspect it could be still
> > > applied to nginx, maybe with some minor tweaks.
> > >
> > > We still appreciate your work.
> > >
> > > BTW, are the patches in the attachment up to date?
> > > If not, can you share the latest patch?
> >
> > We had no patches since november 1, 2018. The last patch is in this
> > message:
> >
> > http://mailman.nginx.org/pipermail/nginx-devel/2018-November/011538.html
> >
> > > We are happy to apply it and feedback if possible.
> > > When there are many static files and accesses, do you think it will works
> > > very well?
> >
> > We didn't have much feedback so far. So we would definitely appreciate
> > your
> > contribution to testing it.
> >
> > > On Sat, Jul 20, 2019 at 2:42 AM Maxim Konovalov <maxim@nginx.com> wrote:
> > >
> > > > Hi hongzhidao,
> > > >
> > > > The patch wasn't merged as we didn't see much interest and real
> > > > technical feedback from potential testers.
> > > >
> > > > The code adds additional complexity to the nginx core with all
> > > > associated costs of maintaining the code virtually forever. In the
> > > > same time at this point it brings no measurable value to the
> > > > community. At least, we haven't seen any proofs that it does.
> > > >
> > > > The patch wasn't updated since that but I suspect it could be still
> > > > applied to nginx, maybe with some minor tweaks.
> > > >
> > > > Maxim
> > > >
> > > > On 19/07/2019 18:26, 洪志道 wrote:
> > > > > Hi.
> > > > > Will this patch be merged into the main branch?
> > > > > What is the latest patch? We can help with the test.
> > > > > Thanks.
> > > > >
> > > > > On Sat, Feb 9, 2019 at 6:40 AM Ka-Hing Cheung via nginx-devel
> > > > > <nginx-devel@nginx.org <mailto:nginx-devel@nginx.org>> wrote:
> > > > >
> > > > > Unfortunately our test colo is not setup to do performance
> > testing
> > > > > (the traffic it receives varies too much). We do intend to merge
> > > > > this
> > > > > to our production colos but there's no timeline yet.
> > > > >
> > > > > Yuchen (CC'ed) will be the main contact from now on as today is
> > my
> > > > > last day at Cloudflare.
> > > > >
> > > > > - Ka-Hing
> > > > >
> > > > > On Thu, Feb 7, 2019 at 5:39 AM Maxim Konovalov <maxim@nginx.com
> > > > > <mailto:maxim@nginx.com>> wrote:
> > > > > >
> > > > > > Great. Thanks for the testing!
> > > > > >
> > > > > > Did you see any measurable perf. metrics changes comparing to
> > your
> > > > > > aio open implementation or comparing to nginx without aio open
> > > > > support?
> > > > > >
> > > > > > We are still waiting for additional input from another tester,
> > who
> > > > > > expressed interest before.
> > > > > >
> > > > > > Thanks,
> > > > > >
> > > > > > Maxim
> > > > > >
> > > > > > On 07/02/2019 00:19, Ka-Hing Cheung wrote:
> > > > > > > This has been running in our test colo for the past week
> > > > > with no ill effects.
> > > > > > >
> > > > > > > On Wed, Jan 23, 2019 at 4:39 AM Maxim Konovalov
> > > > > <maxim@nginx.com <mailto:maxim@nginx.com>> wrote:
> > > > > > >>
> > > > > > >> Hi Ka-Hing,
> > > > > > >>
> > > > > > >> Roman told me that the delta is because of your changes.
> > > > > > >>
> > > > > > >> Thanks for your time on that. Waiting for your testing
> > > > > results.
> > > > > > >>
> > > > > > >> Maxim
> > > > > > >>
> > > > > > >> On 22/01/2019 22:34, Ka-Hing Cheung via nginx-devel wrote:
> > > > > > >>> I spoke too soon, just realized our test colo is running
> > > > > the patches
> > > > > > >>> without aio_open on. Flipping that switch now.
> > > > > > >>>
> > > > > > >>> Also, including the patch that we applied on top in
> > > > > addition to the
> > > > > > >>> massaging we did to resolve conflicts. I haven't dug too
> > > > > deep to see
> > > > > > >>> if stock nginx also requires similar changes or they are
> > only
> > > > > > >>> necessary because of our other nginx changes:
> > > > > > >>>
> > > > > > >> [...]
> > > > > > >>
> > > > > > >> --
> > > > > > >> Maxim Konovalov
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Maxim Konovalov
> > > > > _______________________________________________
> > > > > nginx-devel mailing list
> > > > > nginx-devel@nginx.org <mailto:nginx-devel@nginx.org>
> > > > > http://mailman.nginx.org/mailman/listinfo/nginx-devel
> > > > >
> > > > >
> > > > > _______________________________________________
> > > > > nginx-devel mailing list
> > > > > nginx-devel@nginx.org
> > > > > http://mailman.nginx.org/mailman/listinfo/nginx-devel
> > > > >
> > > >
> > > >
> > > > --
> > > > Maxim Konovalov
> > > >
> >
> > > _______________________________________________
> > > nginx-devel mailing list
> > > nginx-devel@nginx.org
> > > http://mailman.nginx.org/mailman/listinfo/nginx-devel
> >
> >
> > --
> > Roman Arutyunyan
> > _______________________________________________
> > nginx-devel mailing list
> > nginx-devel@nginx.org
> > http://mailman.nginx.org/mailman/listinfo/nginx-devel

> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel


--
Roman Arutyunyan
diff --git a/src/http/modules/ngx_http_static_module.c b/src/http/modules/ngx_http_static_module.c
--- a/src/http/modules/ngx_http_static_module.c
+++ b/src/http/modules/ngx_http_static_module.c
@@ -318,6 +318,10 @@ ngx_http_static_write_event_handler(ngx_
{
ngx_int_t rc;

+ if (r->aio) {
+ return;
+ }
+
rc = ngx_http_static_send(r);

if (rc != NGX_DONE) {
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Subject Author Views Posted

cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 986 August 07, 2018 05:28PM

Re: cache: move open to thread pool

Maxim Dounin 201 August 08, 2018 02:18PM

RE: cache: move open to thread pool

erankor 178 August 08, 2018 03:46PM

Re: cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 145 August 27, 2018 06:00PM

RE: cache: move open to thread pool

erankor 160 August 28, 2018 05:18AM

Re: cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 159 August 09, 2018 05:44PM

Re: cache: move open to thread pool

Maxim Dounin 163 August 10, 2018 07:42AM

Re: cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 138 August 27, 2018 05:40PM

Re: cache: move open to thread pool

Maxim Dounin 217 September 03, 2018 12:10PM

Re: cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 145 September 04, 2018 08:00PM

Re: cache: move open to thread pool

Maxim Dounin 152 September 13, 2018 02:12PM

Re: cache: move open to thread pool

Roman Arutyunyan 214 October 04, 2018 05:34AM

Re: cache: move open to thread pool

Roman Arutyunyan 283 November 01, 2018 08:30AM

Re: cache: move open to thread pool

Maxim Konovalov 145 November 01, 2018 12:08PM

Re: cache: move open to thread pool

Maxim Konovalov 156 November 08, 2018 09:20AM

Re: cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 137 November 15, 2018 05:54PM

Re: cache: move open to thread pool

Maxim Konovalov 149 November 16, 2018 04:12AM

Re: cache: move open to thread pool

Maxim Konovalov 153 December 05, 2018 04:44AM

Re: cache: move open to thread pool

Maxim Konovalov 112 January 10, 2019 03:32AM

Re: cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 121 January 14, 2019 08:54PM

Re: cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 93 January 22, 2019 01:22PM

Re: cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 118 January 22, 2019 02:36PM

Re: cache: move open to thread pool

Maxim Konovalov 108 January 23, 2019 07:40AM

Re: cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 92 February 06, 2019 04:20PM

Re: cache: move open to thread pool

Maxim Konovalov 98 February 07, 2019 08:40AM

Re: cache: move open to thread pool

Ka-Hing Cheung via nginx-devel 140 February 08, 2019 05:42PM

Re: cache: move open to thread pool

karton 31 July 19, 2019 11:28AM

Re: cache: move open to thread pool

Maxim Konovalov 38 July 19, 2019 02:44PM

Re: cache: move open to thread pool

karton 32 July 19, 2019 09:46PM

Re: cache: move open to thread pool

Roman Arutyunyan 30 July 22, 2019 10:06AM

Re: cache: move open to thread pool

karton 32 July 22, 2019 11:40AM

Re: cache: move open to thread pool

karton 34 July 23, 2019 11:48PM

Re: cache: move open to thread pool

Roman Arutyunyan 15 August 16, 2019 10:26AM

Re: cache: move open to thread pool

karton 24 August 16, 2019 10:52AM



Sorry, you do not have permission to post/reply in this forum.

Online Users

Guests: 74
Record Number of Users: 6 on February 13, 2018
Record Number of Guests: 421 on December 02, 2018
Powered by nginx      Powered by FreeBSD      PHP Powered      Powered by MariaDB      ipv6 ready