[BF] slab init + http file cache fixes
Maxim Dounin
mdounin at mdounin.ru
Wed Jun 15 15:54:52 UTC 2016
Hello!
On Wed, Jun 15, 2016 at 01:53:31PM +0200, Sergey Brester wrote:
> The story with shared zone (slab) resp. http file cache has a continuation.
>
> I've additionally found and fixed 2 bugs, the fixes for that you'll find the
> changesets enclosed as attachments:
>
> - _sb-slab-init-fix.patch - slab (zone pool) initialization bug: many static
> stubs not initialized in workers (because only master calls ngx_slab_init).
> So stubs initialization moved to new "ngx_slab_core_init" called from "main"
> direct after "ngx_os_init" (requires "ngx_pagesize").
>
> In consequence of this bug, the pool always allocated a full page (4K - 8K)
> instead of small slots inside page, so for example 1MB zone can store not
> more as 250 keys.
> BTW. According to the documentation: One megabyte zone can store about 8
> thousand keys.
Thanks, looks like another win32-related problem.
> - _sb-scarce-cache-fix.patch - fixes http file cache:
> * prevent failure of requests, for that cache cannot be allocated, just
> because of the cache scarce
> (NGX_HTTP_CACHE_SCARCE) - alert and send the request data nevertheless;
> * wrong counting size of cache (too many decrease of "cache->sh->size",
> because unlocked delete
> and value of "fcn->exists" was not reset);
See comments below.
[...]
> # HG changeset patch
> # User Serg G. Brester (sebres) <serg.brester at sebres.de>
> # Date 1465990539 -7200
> # Wed Jun 15 13:35:39 2016 +0200
> # Node ID ed82931de91e4eb335cc2a094896e6f4f10ac536
> # Parent 0bfc68ad1b7af8c3a7dea24d479ed18bfd024028
> * fixes http file cache:
> - prevent failure of requests, for that cache cannot be allocated, just because of the cache scarce (NGX_HTTP_CACHE_SCARCE) - alert and send the request data nevertheless;
> - wrong counting size of cache (too many decrease "cache->sh->size", because unlocked delete and "fcn->exists" was not reset);
This should be two separate patches.
Please also see http://nginx.org/en/docs/contributing_changes.html
for more hints (summary line, no more than 80 symbols, and so on).
>
> diff -r 0bfc68ad1b7a -r ed82931de91e src/http/ngx_http_file_cache.c
> --- a/src/http/ngx_http_file_cache.c Wed Jun 15 11:53:55 2016 +0200
> +++ b/src/http/ngx_http_file_cache.c Wed Jun 15 13:35:39 2016 +0200
> @@ -879,7 +879,8 @@ ngx_http_file_cache_exists(ngx_http_file
> if (fcn == NULL) {
> ngx_log_error(NGX_LOG_ALERT, ngx_cycle->log, 0,
> "could not allocate node%s", cache->shpool->log_ctx);
> - rc = NGX_ERROR;
> + /* cannot be cached (NGX_HTTP_CACHE_SCARCE), just use it without cache */
> + rc = NGX_AGAIN;
> goto failed;
> }
> }
Not sure it's a good idea to reuse min_uses return code here. I
would rather not.
> @@ -1870,24 +1871,27 @@ ngx_http_file_cache_delete(ngx_http_file
> p = ngx_hex_dump(p, fcn->key, len);
> *p = '\0';
>
> - fcn->count++;
> - fcn->deleting = 1;
> - ngx_shmtx_unlock(&cache->shpool->mutex);
> -
> - len = path->name.len + 1 + path->len + 2 * NGX_HTTP_CACHE_KEY_LEN;
> - ngx_create_hashed_filename(path, name, len);
> -
> - ngx_log_debug1(NGX_LOG_DEBUG_HTTP, ngx_cycle->log, 0,
> - "http file cache expire: \"%s\"", name);
> -
> - if (ngx_delete_file(name) == NGX_FILE_ERROR) {
> - ngx_log_error(NGX_LOG_CRIT, ngx_cycle->log, ngx_errno,
> - ngx_delete_file_n " \"%s\" failed", name);
> + if (!fcn->deleting) {
> + fcn->count++;
> + fcn->deleting = 1;
The condition added looks wrong: fcn->deleting cannot be set
here, as it's only set by this function under lock, and
fcn->count++ here will prevent the function from being called
again.
As far as I understand, you are trying to change processing of the
race which happens if the node in question is grabbed by another
request while the file is being deleted (fcn->count != 0 at
the function end). Please explain what you are trying to chanage
and how it's expected to affect various cases possible.
> + fcn->exists = 0;
> + ngx_shmtx_unlock(&cache->shpool->mutex);
> +
> + len = path->name.len + 1 + path->len + 2 * NGX_HTTP_CACHE_KEY_LEN;
> + ngx_create_hashed_filename(path, name, len);
> +
> + ngx_log_debug1(NGX_LOG_DEBUG_HTTP, ngx_cycle->log, 0,
> + "http file cache expire: \"%s\"", name);
> +
> + if (ngx_delete_file(name) == NGX_FILE_ERROR) {
> + ngx_log_error(NGX_LOG_CRIT, ngx_cycle->log, ngx_errno,
> + ngx_delete_file_n " \"%s\" failed", name);
> + }
> +
> + ngx_shmtx_lock(&cache->shpool->mutex);
> + fcn->count--;
> + fcn->deleting = 0;
> }
> -
> - ngx_shmtx_lock(&cache->shpool->mutex);
> - fcn->count--;
> - fcn->deleting = 0;
> }
>
> if (fcn->count == 0) {
> @@ -2006,6 +2010,7 @@ ngx_http_file_cache_manage_file(ngx_tree
>
> if (ngx_http_file_cache_add_file(ctx, path) != NGX_OK) {
> (void) ngx_http_file_cache_delete_file(ctx, path);
> + goto done;
> }
>
> if (++cache->files >= cache->loader_files) {
> @@ -2024,6 +2029,8 @@ ngx_http_file_cache_manage_file(ngx_tree
> }
> }
>
> +done:
> +
> return (ngx_quit || ngx_terminate) ? NGX_ABORT : NGX_OK;
> }
This change looks unrelated. If you think it's needed, please
explain the reasons.
> # HG changeset patch
> # User Serg G. Brester (sebres) <serg.brester at sebres.de>
> # Date 1465984435 -7200
> # Wed Jun 15 11:53:55 2016 +0200
> # Node ID 0bfc68ad1b7af8c3a7dea24d479ed18bfd024028
> # Parent b430e4546172af42bcecf0fc289ec45ef5f9e865
> Grave slab (zone pool) initialization bug fixed: many stubs not initialized in workers (because only master calls ngx_slab_init) - so stubs initializing moved to new ngx_slab_core_init called from main direct after ngx_os_init (requires ngx_pagesize).
> In consequence of this bug, the pool always allocated a full page (4K - 8K) instead of small slots inside page, so for example 1MB zone can store not more as 250 keys.
> BTW. According to the documentation: One megabyte zone can store about 8 thousand keys.
See above, the description doesn't match style and doesn't
describe that this is a win32 problem.
>
> diff -r b430e4546172 -r 0bfc68ad1b7a src/core/nginx.c
> --- a/src/core/nginx.c Tue Jun 14 16:16:17 2016 +0200
> +++ b/src/core/nginx.c Wed Jun 15 11:53:55 2016 +0200
> @@ -256,6 +256,11 @@ main(int argc, char *const *argv)
> if (ngx_os_init(log) != NGX_OK) {
> return 1;
> }
> +
> + /*
> + * ngx_slab_core_init() requires ngx_pagesize set in ngx_os_init()
> + */
> + ngx_slab_core_init();
I can't say I like the name, but I have no better suggestions.
>
> /*
> * ngx_crc32_table_init() requires ngx_cacheline_size set in ngx_os_init()
> diff -r b430e4546172 -r 0bfc68ad1b7a src/core/ngx_slab.c
> --- a/src/core/ngx_slab.c Tue Jun 14 16:16:17 2016 +0200
> +++ b/src/core/ngx_slab.c Wed Jun 15 11:53:55 2016 +0200
> @@ -70,13 +70,9 @@ static ngx_uint_t ngx_slab_exact_shift;
>
>
> void
> -ngx_slab_init(ngx_slab_pool_t *pool)
> +ngx_slab_core_init()
> {
> - u_char *p;
> - size_t size;
> - ngx_int_t m;
> - ngx_uint_t i, n, pages;
> - ngx_slab_page_t *slots;
> + ngx_uint_t n;
>
> /* STUB */
> if (ngx_slab_max_size == 0) {
This test is useless with the new logic you've introduced.
> @@ -87,6 +83,19 @@ ngx_slab_init(ngx_slab_pool_t *pool)
> }
> }
> /**/
> +}
> +
> +
> +void
> +ngx_slab_init(ngx_slab_pool_t *pool)
> +{
> + u_char *p;
> + size_t size;
> + ngx_int_t m;
> + ngx_uint_t i, n, pages;
> + ngx_slab_page_t *slots;
> +
> + ngx_slab_core_init();
This call is useless with the new logic you've introduced.
>
> pool->min_size = 1 << pool->min_shift;
>
> diff -r b430e4546172 -r 0bfc68ad1b7a src/core/ngx_slab.h
> --- a/src/core/ngx_slab.h Tue Jun 14 16:16:17 2016 +0200
> +++ b/src/core/ngx_slab.h Wed Jun 15 11:53:55 2016 +0200
> @@ -47,6 +47,7 @@ typedef struct {
> } ngx_slab_pool_t;
>
>
> +void ngx_slab_core_init();
> void ngx_slab_init(ngx_slab_pool_t *pool);
> void *ngx_slab_alloc(ngx_slab_pool_t *pool, size_t size);
> void *ngx_slab_alloc_locked(ngx_slab_pool_t *pool, size_t size);
--
Maxim Dounin
http://nginx.org/
More information about the nginx-devel
mailing list