Merge pull request #1840 from pi-hole/fix/cache_recycling

Implement DNS cache recycling

src/database/query-table.c

@@ -1065,19 +1065,20 @@ void DB_read_queries(void)
 			query->qtype = type - 100;
 		}
 		counters->querytype[query->type]++;
-		log_debug(DEBUG_GC, "query type %d set (database), ID = %d, new count = %d", query->type, counters->queries, counters->querytype[query->type]);
+		log_debug(DEBUG_STATUS, "query type %d set (database), ID = %d, new count = %d", query->type, counters->queries, counters->querytype[query->type]);
 
 		// Status is set below
 		query->domainID = domainID;
 		query->clientID = clientID;
 		query->upstreamID = upstreamID;
+		query->cacheID = findCacheID(domainID, clientID, query->type, true);
 		query->id = counters->queries;
 		query->response = 0;
 		query->flags.response_calculated = reply_time_avail;
 		query->dnssec = dnssec;
 		query->reply = reply;
 		counters->reply[query->reply]++;
-		log_debug(DEBUG_GC, "reply type %d set (database), ID = %d, new count = %d", query->reply, counters->queries, counters->reply[query->reply]);
+		log_debug(DEBUG_STATUS, "reply type %d set (database), ID = %d, new count = %d", query->reply, counters->queries, counters->reply[query->reply]);
 		query->response = reply_time;
 		query->CNAME_domainID = -1;
 		// Initialize flags
@@ -1125,8 +1126,7 @@ void DB_read_queries(void)
 			// Set ID of the domainlist entry that was the reason for permitting/blocking this query
 			// We assume the value in this field is said ID when it is not a CNAME-related domain
 			// (checked above) and the value of additional_info is not NULL (0 bytes storage size)
-			const int cacheID = findCacheID(query->domainID, query->clientID, query->type, true);
-			DNSCacheData *cache = getDNSCache(cacheID, true);
+			DNSCacheData *cache = getDNSCache(query->cacheID, true);
 			// Only load if
 			//  a) we have a cache entry
 			//  b) the value of additional_info is not NULL (0 bytes storage size)
@@ -1459,8 +1459,8 @@ bool queries_to_database(void)
 		}
 
 		// Get cache entry for this query
-		const int cacheID = findCacheID(query->domainID, query->clientID, query->type, false);
-		DNSCacheData *cache = cacheID < 0 ? NULL : getDNSCache(cacheID, true);
+		const int cacheID = query->cacheID >= 0 ? query->cacheID : findCacheID(query->domainID, query->clientID, query->type, false);
+		DNSCacheData *cache = getDNSCache(cacheID, true);
 
 		// ADDITIONAL_INFO
 		if(query->status == QUERY_GRAVITY_CNAME ||

src/datastructure.c

@@ -137,10 +137,10 @@ int _findUpstreamID(const char *upstreamString, const in_port_t port, int line,
 	return upstreamID;
 }
 
-static int get_next_domainID(void)
+static int get_next_free_domainID(void)
 {
 	// Compare content of domain against known domain IP addresses
-	for(int domainID=0; domainID < counters->domains; domainID++)
+	for(int domainID = 0; domainID < counters->domains; domainID++)
 	{
 		// Get domain pointer
 		domainsData* domain = getDomain(domainID, false);
@@ -188,7 +188,7 @@ int _findDomainID(const char *domainString, const bool count, int line, const ch
 
 	// If we did not return until here, then this domain is not known
 	// Store ID
-	const int domainID = get_next_domainID();
+	const int domainID = get_next_free_domainID();
 
 	// Get domain pointer
 	domainsData* domain = _getDomain(domainID, false, line, func, file);
@@ -218,10 +218,10 @@ int _findDomainID(const char *domainString, const bool count, int line, const ch
 	return domainID;
 }
 
-static int get_next_clientID(void)
+static int get_next_free_clientID(void)
 {
 	// Compare content of client against known client IP addresses
-	for(int clientID=0; clientID < counters->clients; clientID++)
+	for(int clientID = 0; clientID < counters->clients; clientID++)
 	{
 		// Get client pointer
 		clientsData* client = getClient(clientID, false);
@@ -271,7 +271,7 @@ int _findClientID(const char *clientIP, const bool count, const bool aliasclient
 
 	// If we did not return until here, then this client is definitely new
 	// Store ID
-	const int clientID = get_next_clientID();
+	const int clientID = get_next_free_clientID();
 
 	// Get client pointer
 	clientsData* client = _getClient(clientID, false, line, func, file);
@@ -369,10 +369,10 @@ void change_clientcount(clientsData *client, int total, int blocked, int overTim
 		}
 }
 
-static int get_next_cacheID(void)
+static int get_next_free_cacheID(void)
 {
 	// Compare content of cache against known cache IP addresses
-	for(int cacheID=0; cacheID < counters->dns_cache_size; cacheID++)
+	for(int cacheID = 0; cacheID < counters->dns_cache_size; cacheID++)
 	{
 		// Get cache pointer
 		DNSCacheData* cache = getDNSCache(cacheID, false);
@@ -415,7 +415,7 @@ int _findCacheID(const int domainID, const int clientID, const enum query_type q
 		return -1;
 
 	// Get ID of new cache entry
-	const int cacheID = get_next_cacheID();
+	const int cacheID = get_next_free_cacheID();
 
 	// Get client pointer
 	DNSCacheData* dns_cache = _getDNSCache(cacheID, false, line, func, file);
@@ -426,6 +426,9 @@ int _findCacheID(const int domainID, const int clientID, const enum query_type q
 		return -1;
 	}
 
+	log_debug(DEBUG_GC, "New cache entry: domainID %d, clientID %d, query_type %d (ID %d)",
+	          domainID, clientID, query_type, cacheID);
+
 	// Initialize cache entry
 	dns_cache->magic = MAGICBYTE;
 	dns_cache->blocking_status = UNKNOWN_BLOCKED;
@@ -555,12 +558,17 @@ void FTL_reset_per_client_domain_data(void)
 
 	for(int cacheID = 0; cacheID < counters->dns_cache_size; cacheID++)
 	{
-		// Reset all blocking yes/no fields for all domains and clients
-		// This forces a reprocessing of all available filters for any
-		// given domain and client the next time they are seen
-		DNSCacheData *dns_cache = getDNSCache(cacheID, true);
-		if(dns_cache != NULL)
-			dns_cache->blocking_status = UNKNOWN_BLOCKED;
+		// Get cache pointer
+		DNSCacheData* dns_cache = getDNSCache(cacheID, true);
+
+		// Check if the returned pointer is valid before trying to access it
+		if(dns_cache == NULL)
+			continue;
+
+		// Reset blocking status
+		dns_cache->blocking_status = UNKNOWN_BLOCKED;
+		// Reset domainlist ID
+		dns_cache->domainlist_id = -1;
 	}
 }
 
@@ -1039,10 +1047,10 @@ void _query_set_status(queriesData *query, const enum query_status new_status, c
 	if(!init)
 	{
 		counters->status[old_status]--;
-		log_debug(DEBUG_GC, "status %d removed (!init), ID = %d, new count = %d", QUERY_UNKNOWN, query->id, counters->status[QUERY_UNKNOWN]);
+		log_debug(DEBUG_STATUS, "status %d removed (!init), ID = %d, new count = %d", QUERY_UNKNOWN, query->id, counters->status[QUERY_UNKNOWN]);
 	}
 	counters->status[new_status]++;
-	log_debug(DEBUG_GC, "status %d set, ID = %d, new count = %d", new_status, query->id, counters->status[new_status]);
+	log_debug(DEBUG_STATUS, "status %d set, ID = %d, new count = %d", new_status, query->id, counters->status[new_status]);
 
 	// ... update overTime counters, ...
 	const int timeidx = getOverTimeID(query->timestamp);

src/datastructure.h

@@ -30,6 +30,7 @@ typedef struct {
 	int domainID;
 	int clientID;
 	int upstreamID;
+	int cacheID;
 	int id; // the ID is a (signed) int in dnsmasq, so no need for a long int here
 	int CNAME_domainID; // only valid if query has a CNAME blocking status
 	int ede;

src/dnsmasq_interface.c

@@ -747,7 +747,7 @@ bool _FTL_new_query(const unsigned int flags, const char *name,
 	query->timestamp = querytimestamp;
 	query->type = querytype;
 	counters->querytype[querytype]++;
-	log_debug(DEBUG_GC, "query type %d set (new query), ID = %d, new count = %d", query->type, id, counters->querytype[query->type]);
+	log_debug(DEBUG_STATUS, "query type %d set (new query), ID = %d, new count = %d", query->type, id, counters->querytype[query->type]);
 	query->qtype = qtype;
 	query->id = id; // Has to be set before calling query_set_status()
 
@@ -764,7 +764,7 @@ bool _FTL_new_query(const unsigned int flags, const char *name,
 	// Initialize reply type
 	query->reply = REPLY_UNKNOWN;
 	counters->reply[REPLY_UNKNOWN]++;
-	log_debug(DEBUG_GC, "reply type %d set (new query), ID = %d, new count = %d", query->reply, query->id, counters->reply[query->reply]);
+	log_debug(DEBUG_STATUS, "reply type %d set (new query), ID = %d, new count = %d", query->reply, query->id, counters->reply[query->reply]);
 	// Store DNSSEC result for this domain
 	query->dnssec = DNSSEC_UNKNOWN;
 	query->CNAME_domainID = -1;
@@ -783,6 +783,10 @@ bool _FTL_new_query(const unsigned int flags, const char *name,
 	// Query extended DNS error
 	query->ede = EDE_UNSET;
 
+	// Initialize cache ID, may be reusing an existing one if this
+	// (domain,client,type) tuple was already seen before
+	query->cacheID = findCacheID(domainID, clientID, querytype, true);
+
 	// This query is new and not yet known to the database
 	query->db = -1;
 
@@ -1295,7 +1299,12 @@ static bool _FTL_check_blocking(int queryID, int domainID, int clientID, const c
 	}
 
 	// Get cache pointer
-	unsigned int cacheID = findCacheID(domainID, clientID, query->type, true);
+	// When this function is called with a different domain than the one
+	// already stored in the query, we have to re-lookup the cache ID.
+	// This can happen when a CNAME chain is followed and analyzed
+	const int cacheID = query->domainID == domainID && query->clientID == clientID ?
+	                    query->cacheID :
+	                    findCacheID(domainID, clientID, query->type, true);
 	DNSCacheData *dns_cache = getDNSCache(cacheID, true);
 	if(dns_cache == NULL)
 	{
@@ -1588,7 +1597,7 @@ bool _FTL_CNAME(const char *dst, const char *src, const int id, const char* file
 		else if(query->status == QUERY_REGEX)
 		{
 			// Get parent and child DNS cache entries
-			const int parent_cacheID = findCacheID(parent_domainID, clientID, query->type, false);
+			const int parent_cacheID = query->cacheID;
 			const int child_cacheID = findCacheID(child_domainID, clientID, query->type, false);
 
 			// Get cache pointers
@@ -2733,12 +2742,12 @@ static void _query_set_reply(const unsigned int flags, const enum reply_type rep
 
 	// Subtract from old reply counter
 	counters->reply[query->reply]--;
-	log_debug(DEBUG_GC, "reply type %d removed (set_reply), ID = %d, new count = %d", query->reply, query->id, counters->reply[query->reply]);
+	log_debug(DEBUG_STATUS, "reply type %d removed (set_reply), ID = %d, new count = %d", query->reply, query->id, counters->reply[query->reply]);
 	// Add to new reply counter
 	counters->reply[new_reply]++;
 	// Store reply type
 	query->reply = new_reply;
-	log_debug(DEBUG_GC, "reply type %d added (set_reply), ID = %d, new count = %d", query->reply, query->id, counters->reply[query->reply]);
+	log_debug(DEBUG_STATUS, "reply type %d added (set_reply), ID = %d, new count = %d", query->reply, query->id, counters->reply[query->reply]);
 
 	// Save response time
 	// Skipped internally if already computed
@@ -3355,10 +3364,10 @@ void FTL_multiple_replies(const int id, int *firstID)
 
 	// Copy relevant information over
 	counters->reply[duplicated_query->reply]--;
-	log_debug(DEBUG_GC, "duplicated_query reply type %d removed, ID = %d, new count = %d", duplicated_query->reply, duplicated_query->id, counters->reply[duplicated_query->reply]);
+	log_debug(DEBUG_STATUS, "duplicated_query reply type %d removed, ID = %d, new count = %d", duplicated_query->reply, duplicated_query->id, counters->reply[duplicated_query->reply]);
 	duplicated_query->reply = source_query->reply;
 	counters->reply[duplicated_query->reply]++;
-	log_debug(DEBUG_GC, "duplicated_query reply type %d set, ID = %d, new count = %d", duplicated_query->reply, duplicated_query->id, counters->reply[duplicated_query->reply]);
+	log_debug(DEBUG_STATUS, "duplicated_query reply type %d set, ID = %d, new count = %d", duplicated_query->reply, duplicated_query->id, counters->reply[duplicated_query->reply]);
 
 	duplicated_query->dnssec = source_query->dnssec;
 	duplicated_query->flags.complete = true;

src/gc.c

@@ -51,8 +51,8 @@ static void recycle(void)
 {
 	bool *client_used = calloc(counters->clients, sizeof(bool));
 	bool *domain_used = calloc(counters->domains, sizeof(bool));
-	bool *upstreams_used = calloc(counters->upstreams, sizeof(bool));
-	if(client_used == NULL || domain_used == NULL || upstreams_used == NULL)
+	bool *cache_used = calloc(counters->dns_cache_size, sizeof(bool));
+	if(client_used == NULL || domain_used == NULL || cache_used == NULL)
 	{
 		log_err("Cannot allocate memory for recycling");
 		return;
@@ -70,13 +70,13 @@ static void recycle(void)
 		client_used[query->clientID] = true;
 		domain_used[query->domainID] = true;
 
-		// Mark upstream as used (if any)
-		if(query->upstreamID > -1)
-			upstreams_used[query->upstreamID] = true;
-
 		// Mark CNAME domain as used (if any)
-		if(query->CNAME_domainID >= 0)
+		if(query->CNAME_domainID > -1)
 			domain_used[query->CNAME_domainID] = true;
+
+		// Mark cache entry as used (if any)
+		if(query->cacheID > -1)
+			cache_used[query->cacheID] = true;
 	}
 
 	// Recycle clients
@@ -128,12 +128,11 @@ static void recycle(void)
 	unsigned int cache_recycled = 0;
 	for(int cacheID = 0; cacheID < counters->dns_cache_size; cacheID++)
 	{
-		DNSCacheData *cache = getDNSCache(cacheID, true);
-		if(cache == NULL)
+		if(cache_used[cacheID])
 			continue;
 
-		// Skip cache entries that are still in use
-		if(cache->magic != 0x00)
+		DNSCacheData *cache = getDNSCache(cacheID, true);
+		if(cache == NULL)
 			continue;
 
 		log_debug(DEBUG_GC, "Recycling cache entry with ID %d", cacheID);
@@ -147,7 +146,7 @@ static void recycle(void)
 	// Free memory
 	free(client_used);
 	free(domain_used);
-	free(upstreams_used);
+	free(cache_used);
 
 	// Scan number of recycled clients and domains if in debug mode
 	if(config.debug.gc.v.b)
@@ -181,10 +180,10 @@ static void recycle(void)
 				free_cache++;
 		}
 
-		log_debug(DEBUG_GC, "Recycler summary: %u/%d (max %d) clients, %u/%d (max %d) domains and %u/%d (max %d) cache records are free",
-		          free_clients, counters->clients, counters->clients_MAX,
-		          free_domains, counters->domains, counters->domains_MAX,
-			  free_cache, counters->dns_cache_size, counters->dns_cache_MAX);
+		log_debug(DEBUG_GC, "%d/%d clients, %d/%d domains and %d/%d cache records are free",
+		          counters->clients_MAX + (int)free_clients - counters->clients, counters->clients_MAX,
+		          counters->domains_MAX + (int)free_domains - counters->domains_MAX, counters->domains_MAX,
+		          counters->dns_cache_MAX + (int)free_cache - counters->dns_cache_MAX, counters->dns_cache_MAX);
 
 		log_debug(DEBUG_GC, "Recycled additional %u clients, %u domains, and %u cache records (scanned %d queries)",
 		          clients_recycled, domains_recycled, cache_recycled, counters->queries);
@@ -378,17 +377,17 @@ void runGC(const time_t now, time_t *lastGCrun, const bool flush)
 
 		// Update reply counters
 		counters->reply[query->reply]--;
-		log_debug(DEBUG_GC, "reply type %d removed (GC), ID = %d, new count = %d", query->reply, query->id, counters->reply[query->reply]);
+		log_debug(DEBUG_STATUS, "reply type %d removed (GC), ID = %d, new count = %d", query->reply, query->id, counters->reply[query->reply]);
 
 		// Update type counters
 		counters->querytype[query->type]--;
-		log_debug(DEBUG_GC, "query type %d removed (GC), ID = %d, new count = %d", query->type, query->id, counters->querytype[query->type]);
+		log_debug(DEBUG_STATUS, "query type %d removed (GC), ID = %d, new count = %d", query->type, query->id, counters->querytype[query->type]);
 
 		// Subtract UNKNOWN from the counters before
 		// setting the status if different.
 		// Minus one here and plus one below = net zero
 		counters->status[QUERY_UNKNOWN]--;
-		log_debug(DEBUG_GC, "status %d removed (GC), ID = %d, new count = %d", QUERY_UNKNOWN, query->id, counters->status[QUERY_UNKNOWN]);
+		log_debug(DEBUG_STATUS, "status %d removed (GC), ID = %d, new count = %d", QUERY_UNKNOWN, query->id, counters->status[QUERY_UNKNOWN]);
 
 		// Set query again to UNKNOWN to reset the counters
 		query_set_status(query, QUERY_UNKNOWN);

src/log.c

@@ -482,6 +482,7 @@ void log_counter_info(void)
 	log_info(" -> Unknown DNS queries: %i", counters->status[QUERY_UNKNOWN]);
 	log_info(" -> Unique domains: %i", counters->domains);
 	log_info(" -> Unique clients: %i", counters->clients);
+	log_info(" -> DNS cache records: %i", counters->dns_cache_size);
 	log_info(" -> Known forward destinations: %i", counters->upstreams);
 }