cache: move open to thread pool

[Maxim Dounin]

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 at 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/