cache: move open to thread pool
洪志道
hongzhidao at gmail.com
Fri Aug 16 14:50:53 UTC 2019
Great job, the patch works well.
On Fri, Aug 16, 2019 at 10:24 PM Roman Arutyunyan <arut at nginx.com> wrote:
> 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 at 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 at 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 at nginx.org <mailto:nginx-devel at 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 at nginx.com
> > > > > > <mailto:maxim at 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 at nginx.com <mailto:maxim at 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 at nginx.org <mailto:nginx-devel at nginx.org>
> > > > > > http://mailman.nginx.org/mailman/listinfo/nginx-devel
> > > > > >
> > > > > >
> > > > > > _______________________________________________
> > > > > > nginx-devel mailing list
> > > > > > nginx-devel at nginx.org
> > > > > > http://mailman.nginx.org/mailman/listinfo/nginx-devel
> > > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Maxim Konovalov
> > > > >
> > >
> > > > _______________________________________________
> > > > nginx-devel mailing list
> > > > nginx-devel at nginx.org
> > > > http://mailman.nginx.org/mailman/listinfo/nginx-devel
> > >
> > >
> > > --
> > > Roman Arutyunyan
> > > _______________________________________________
> > > nginx-devel mailing list
> > > nginx-devel at nginx.org
> > > http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
> > _______________________________________________
> > nginx-devel mailing list
> > nginx-devel at nginx.org
> > http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
>
> --
> Roman Arutyunyan
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20190816/607160dd/attachment-0001.html>
More information about the nginx-devel
mailing list