ngx_ext_rename_file: remove the target file if ngx_copy_file() fails

[Maxim Dounin]

Hello!

On Thu, Jul 09, 2015 at 02:10:48PM +0100, Mindaugas Rasiukevicius wrote:

> Hi,
> 
> Some background: nginx 1.9.2, used as a cache, can get into the state
> when it stops evicting the objects and eventually stops caching without
> being able to recover.  This happens when the disk is full.  Consider the
> following nginx.conf fragment:
> 
> proxy_cache_path /cache/nginx levels=1:2
>         keys_zone=c3:4096m max_size=8500g
>         inactive=30d use_temp_path=on;
> proxy_temp_path /cache/nginx-tmp 1 2;
> 
> The disk is filled because the workers have been fetching the data from
> the backend faster than the cache manager is able to evict:
> 
> $ df -h | grep cache
> /dev/sdb1             8.7T  8.7T   16M 100% /cache
> tmpfs                 2.0G     0  2.0G   0% /cache/nginx-tmp
> 
> Since /cache and /cache/nginx-tmp are separate mount points, nginx has to
> perform copy instead of rename.  The copy functions fails due to ENOSPC,
> but the ngx_ext_rename_file() does not clean up the failed target.  At this
> point, based on ngx_http_file_cache_sh_t::size, the cache manager believes
> that the 8.5 TB threshold has not been crossed and nginx fails to recover.
> 
> Please find the patch attached.
> 
> -- 
> Mindaugas

> # HG changeset patch
> # User Mindaugas Rasiukevicius <rmind at netbsd.org>
> # Date 1436387157 -3600
> #      Wed Jul 08 21:25:57 2015 +0100
> # Node ID 9c978e51bf0e9bd6fc0fc04f4016a3c5701e9912
> # Parent  dcae651b2a0cbd3de2f1fd5cf5b8c72627db94fd
> ngx_ext_rename_file: remove the target file if ngx_copy_file() fails.
> 
> diff -r dcae651b2a0c -r 9c978e51bf0e src/core/ngx_file.c
> --- a/src/core/ngx_file.c	Tue Jul 07 16:38:49 2015 +0300
> +++ b/src/core/ngx_file.c	Wed Jul 08 21:25:57 2015 +0100
> @@ -727,12 +727,11 @@ ngx_ext_rename_file(ngx_str_t *src, ngx_
>              ngx_log_error(NGX_LOG_CRIT, ext->log, ngx_errno,
>                            ngx_rename_file_n " \"%s\" to \"%s\" failed",
>                            name, to->data);
> +        }
>  
> -            if (ngx_delete_file(name) == NGX_FILE_ERROR) {
> -                ngx_log_error(NGX_LOG_CRIT, ext->log, ngx_errno,
> -                              ngx_delete_file_n " \"%s\" failed", name);
> -
> -            }
> +        if (ngx_delete_file(name) == NGX_FILE_ERROR) {
> +            ngx_log_error(NGX_LOG_CRIT, ext->log, ngx_errno,
> +                          ngx_delete_file_n " \"%s\" failed", name);
>          }
>  
>          ngx_free(name);

By calling ngx_delete_file() at this point, you do this for all 
errors returned by ngx_copy_file(), including cases when it wasn't 
able to open a destination file for some reason.  This will result 
in additional confusing errors, and looks like a wrong approach.

If you want nginx to remove a destination file, this should be 
done in ngx_copy_file().

-- 
Maxim Dounin
http://nginx.org/