Nginx ignores proxy_no_cache

[Clima Gabriel]

> my personal preference is to keep disks mirrored

Certainly, this is ideal, however Nginx is used by many businesses (cdn,
content providers, etc) whose margins are often too low to justify hardware
redundancy. And using cheap SSDs can have incredibly bad impact on
performance, as we've encountered certain ssds (870 QVO f.e.) that, at the
extremes, took (literally) seconds to return from open() / read() / write()

can be reconsidered if there is an understanding that this is quite
> important in some setups and there is a simple enough way to handle such
> failures

We'll need to implement this regardless so I'll, fingers crossed, be back
with updates.

Following are notes on my initial idea which proved too problematic to
pursue further, so feel free to skip.
Create the temp file ahead of time, before the proxy_no_cache check, and
take the proxy_no_cache code path if we fail to create the file.
Clearly not very elegant either as:
The temp file will need to be removed in the else of "if (p->cacheable) ..."
The temp file may be created successfully yet you may still encounter an IO
error a few nanoseconds later when you try to write to it.
There were several variants of this (diff file attached) which either
leaked tempfiles or broke proxy_no_cache.


On Fri, Jan 31, 2025 at 11:36 AM Maxim Dounin <mdounin at mdounin.ru> wrote:

> Hello!
>
> On Thu, Jan 30, 2025 at 11:01:32AM +0200, Clima Gabriel wrote:
>
> > Hello Maxim,
> > Hope this helps.
> > We encounter disk failures fairly often and what the kernel will do most
> of
> > the time is re-mount the disk as read-only.
> > What I did was add this check which checks if the disk is healthy before
> > hand and executes the proxy_no_cache path if it is.
> > It doesn't cover all the possible disk failures, like sometimes you'll
> just
> > get IO errors and still return 5XX to clients. But to cover all cases a
> > much cleverer reshuffling of the code is needed.
> >
> >
> > diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
> > index d7f427d50..839ed6c0d 100644
> > --- a/src/http/ngx_http_upstream.c
> > +++ b/src/http/ngx_http_upstream.c
> > @@ -8,6 +8,7 @@
> >  #include <ngx_config.h>
> >  #include <ngx_core.h>
> >  #include <ngx_http.h>
> > +#include <sys/statvfs.h>
> >
> >  #include <ngx_http_proxy_module.h>
> >
> > @@ -3424,6 +3425,16 @@ ngx_http_upstream_send_response(ngx_http_request_t
> > *r, ngx_http_upstream_t *u)
> >          break;
> >
> >      default: /* NGX_OK */
> > +        if (r->cache) {
> > +            struct statvfs  fs;
> > +            if (statvfs((char *)r->cache->file_cache->path->name.data,
> > &fs) == -1) {
> > +                return NGX_ERROR;
> > +            }
> > +            if ((fs.f_flag & ST_RDONLY) != 0) {
> > +                u->cacheable = 0;
> > +                break;
> > +            }
> > +        }
> >
> >          if (u->cache_status == NGX_HTTP_CACHE_BYPASS) {
>
> I cannot say I like this approach.
>
> Rather, I would recommend adding an explicit test via the
> proxy_no_cache configuration directive if that's an expected state
> in your setup.  Right now this certainly can be done with the
> embedded Perl module, but we can consider extending "if" with "-r"
> and "-w" tests to make things easier.
>
> Alternatively, we can consider handling (at least some) cache
> access failures gracefully and ensure that we'll fall back to
> normal proxying if this happens.  This wasn't done previously to
> keep the code simple, but can be reconsidered if there is an
> understanding that this is quite important in some setups and
> there is a simple enough way to handle such failures.
>
> Please also note that right now even proxying won't work if
> a temporary file is needed and cannot be created, as file access
> failures are considered fatal.
>
> OTOH, my personal preference is to keep disks mirrored, this
> ensures that a single disk failure won't affect server operations
> and provides better performance as a bonus.
>
> (Note well that the code in question was modified in freenginx to
> address the issue reported by Kirill in the thread you are
> replying to, and your patch won't apply, see
> https://freenginx.org/hg/nginx/rev/c5623963c29e for details.)
>
> [...]
>
> --
> Maxim Dounin
> http://mdounin.ru/
> _______________________________________________
> nginx mailing list
> nginx at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://freenginx.org/pipermail/nginx/attachments/20250131/8012b338/attachment.htm>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ideas_MISS_if_write_tempfile_failed.diff
Type: text/x-patch
Size: 3848 bytes
Desc: not available
URL: <http://freenginx.org/pipermail/nginx/attachments/20250131/8012b338/attachment.bin>