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) <[email protected]> > # 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) <[email protected]> > # 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/ _______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
