Hello! On Tue, Sep 04, 2018 at 04:58:05PM -0700, Ka-Hing Cheung via nginx-devel wrote:
> On Mon, Sep 3, 2018 at 9:09 AM, Maxim Dounin <mdou...@mdounin.ru> wrote: > > The biggest problem with in-place solution is that it requires > > in-place coding for all the places where we want to use aio_open. > > Currently we use ngx_open_cached_file() to open files everywhere, > > and it would be a good idea to preserve it as a starting point for > > all file open operations. > > > > This should be beneficial even if aio_open is only used when open > > file cache is actually disabled. And I'm actually fine with > > implementing this only when open file cache is disabled - but > > implementing this in ngx_open_cached_file(), to make it easier to > > add more places which use aio_open, and also makeing it possible > > to further improve things by making it compatible with > > open_file_cache. > > The most difficult thing to switch to threaded open isn't which method > to call, but to expect NGX_AGAIN and have the necessary bookkeeping in > place to resume, so imo this is a pretty weak argument. Sure. But adding an alternative interface and dozens of lines of the otherwise non-needed code everywhere we want to support aio_open is something we can easily avoid, and we should do it. > > Adding a code path somewhere in ngx_open_file_wrapper() should be > > simple enough. Note that probably there is no need to cover > > stat() as well, as well as to address various symlinks cases - I > > think it would be fine if aio_open will only work when there is no > > need to disable symlinks, and will only work for open(), as stat() > > after an open is expected to be fast. > > A problem is that someone need to own the thread task (which for other > aio is owned by ngx_file, and someone needs to own the ngx_file). > ngx_open_cached_file doesn't care about anything beyond the file name. > That can of course change, but changing API is subjective and > difficult for me as an external contributor to decide on. I can try to > find time to do the change if you want to comment on how it should > look like. The ngx_open_file_cache() function takes a ngx_open_file_info_t structure with various arguments, and it would be logical to extend this structure to keep thread task as well. > >> > - The code calls ngx_open_and_stat_file() whithin a thread, which > >> > is relatively complex function never designed to be thread safe. > >> > While it looks like currently it is, a better solution would be to > >> > introduce a separate simple function to execute within a thread, > >> > similar to ngx_thread_read_handler(). > >> > >> I kept using ngx_open_and_stat_file() as we are looking into moving > >> other types of open into thread pool, so we will probably need to have > >> similar logic anyway. > > > > The problem is that ngx_open_and_stat_file() is never coded to be > > thread safe, and this is expected to cause problems sooner or > > switched to use ngx_open_file_wrapper You probably didn't get the point. You shouldn't use existing functions - all them were not designed to be thread safe. Instead, write a simple function using only low-level calls. Please take a look at ngx_thread_read_handler() for an example. > > While this looks slightly better than in the previous iteration, > > this still leaves a lot to be desired. In particular, this still > > uses duplicate "rv == NGX_DECLINED" checks. > > I poked around at it and thinks this one line duplication is the > cleanest. ngx_http_file_cache_read can also return DECLINED and > cleaning up nginx return code seems to be beyond the scope of this > work. We can live with some duplication, but the whole thing needs to be improved somehow to be more readable. The thing which bugs me most is an opaque "c->open_rv" field, which is also passed as a separate argument into the ngx_http_file_cache_aio_open() function. Using some more descriptive name and saving it always to avoid separate argument might be a way to go. > >> + ngx_open_file_info_t of; > >> + ngx_int_t rv; > >> +} ngx_thread_file_open_ctx_t; > >> + > >> +typedef struct { > >> + union { > >> + ngx_thread_file_open_ctx_t open; > >> + ngx_thread_file_read_ctx_t read; > >> + }; > >> } ngx_thread_file_ctx_t; > > > > Please keep things in a single structure instead. This will allow > > to avoid many coresponding changes in the file. > > Is 72 bytes per request not worth saving? To save bytes, consider re-thinking the data used for open (and you anyway has to do it when re-implementing the in-thread code as a simple function using only low-level calls). Just a file name, fd, and a return code should be enough here - and most of these fields are already available in the existing ngx_thread_file_ctx_t structure. > >> typedef int ngx_fd_t; > >> typedef struct stat ngx_file_info_t; > >> typedef ino_t ngx_file_uniq_t; > >> > >> +#include <ngx_open_file_cache.h> > >> > > > > This introduces a dependency of low-level code from high-level > > OS-independant structures in ngx_open_file_cache.h. Instead, some > > low-level interface should be used. > > Do you have a more concrete suggestion? See above. [...] > diff --git src/core/ngx_open_file_cache.h src/core/ngx_open_file_cache.h > index d119c129..9c1c6bdc 100644 > --- src/core/ngx_open_file_cache.h > +++ src/core/ngx_open_file_cache.h > @@ -7,6 +7,7 @@ > > #include <ngx_config.h> > #include <ngx_core.h> > +#include <ngx_queue.h> > > > #ifndef _NGX_OPEN_FILE_CACHE_H_INCLUDED_ This looks like an unrelated change. [...] > @@ -398,7 +356,6 @@ done: > return rv; > } > > - > static ngx_int_t > ngx_http_file_cache_lock(ngx_http_request_t *r, ngx_http_cache_t *c) > { As previously pointed out, this is an unrelated change and a style issue. [...] -- Maxim Dounin http://mdounin.ru/ _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel