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 <mdounin@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