From: Timo Sirainen Date: Sun, 5 Apr 2020 15:36:44 +0000 (+0300) Subject: lib-index: Cache purging shouldn't always change YES decisions to TEMP X-Git-Tag: 2.3.11.2~344 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=bf34319b7151b7fc57d0b17c5c317d0835b9a365;p=thirdparty%2Fdovecot%2Fcore.git lib-index: Cache purging shouldn't always change YES decisions to TEMP Previously every purging changed YES decision to be changed to TEMP. This behaved rather badly if a cache file was purged twice within short time period, because the clients might not have had time to access the mailbox and change the decision back to YES. That in turn could have dropped old mails' cached fields even though the client might still want to use them. (A workaround for this has been to list all useful fields in mail_always_cache_fields setting.) The new behavior is to update the last_used field for a YES decision only when the YES decision has been confirmed. If it's not confirmed for 30 days (mail_cache_unaccessed_field_drop) then its decision is changed to TEMP on the next purge. The new behavior also doubles the time when unaccessed field is dropped from 30 days to 60 days (2*mail_cache_unaccessed_field_drop). This is needed so that the field isn't dropped too early after YES -> TEMP decision is changed. --- diff --git a/src/lib-index/mail-cache-decisions.c b/src/lib-index/mail-cache-decisions.c index 5987060525..5e8d3103dc 100644 --- a/src/lib-index/mail-cache-decisions.c +++ b/src/lib-index/mail-cache-decisions.c @@ -110,23 +110,38 @@ void mail_cache_decision_state_update(struct mail_cache_view *view, /* update last_used about once a day */ bool last_used_need_update = ioloop_time - cache->fields[field].field.last_used > 3600*24; - if (last_used_need_update) - mail_cache_update_last_used(cache, field); - if (dec != MAIL_CACHE_DECISION_TEMP) { + if (dec == MAIL_CACHE_DECISION_NO || + (dec & MAIL_CACHE_DECISION_FORCED) != 0) { /* a) forced decision - b) not cached, mail_cache_decision_add() will handle this - c) permanently cached already, okay. */ + b) not cached, mail_cache_decision_add() will handle this */ + if (last_used_need_update) + mail_cache_update_last_used(cache, field); return; } + if (dec == MAIL_CACHE_DECISION_YES) { + if (!last_used_need_update) + return; + /* update last_used only when we can confirm that the YES + decision is still correct. */ + } else { + /* see if we want to change decision from TEMP to YES */ + i_assert(dec == MAIL_CACHE_DECISION_TEMP); + if (last_used_need_update) + mail_cache_update_last_used(cache, field); + } mail_index_lookup_uid(view->view, seq, &uid); hdr = mail_index_get_header(view->view); - /* see if we want to change decision from TEMP to YES */ if (uid >= cache->fields[field].uid_highwater && uid >= hdr->day_first_uid[7]) { cache->fields[field].uid_highwater = uid; + } else if (dec == MAIL_CACHE_DECISION_YES) { + /* Confirmed that we still want to preserve YES as cache + decision. We can update last_used now. */ + i_assert(last_used_need_update); + mail_cache_update_last_used(cache, field); } else { /* a) nonordered access within this session. if client doesn't request messages in growing order, we assume it doesn't @@ -135,6 +150,7 @@ void mail_cache_decision_state_update(struct mail_cache_view *view, client with no local cache. if it was just a new client generating the local cache for the first time, we'll drop back to TEMP within few months. */ + i_assert(dec == MAIL_CACHE_DECISION_TEMP); cache->fields[field].field.decision = MAIL_CACHE_DECISION_YES; cache->fields[field].decision_dirty = TRUE; cache->field_header_write_pending = TRUE; diff --git a/src/lib-index/mail-cache-purge.c b/src/lib-index/mail-cache-purge.c index cc9c74e0fa..32f1f85c42 100644 --- a/src/lib-index/mail-cache-purge.c +++ b/src/lib-index/mail-cache-purge.c @@ -16,7 +16,7 @@ struct mail_cache_copy_context { struct mail_cache *cache; struct event *event; - time_t max_drop_time; + time_t max_temp_drop_time, max_yes_downgrade_time; buffer_t *buffer, *field_seen; ARRAY(unsigned int) bitmask_pos; @@ -135,7 +135,6 @@ mail_cache_purge_get_fields(struct mail_cache_copy_context *ctx, unsigned int used_fields_count) { struct mail_cache *cache = ctx->cache; - struct mail_cache_field *field; unsigned int i, j, idx; /* Make mail_cache_header_fields_get() return the fields in @@ -157,23 +156,6 @@ mail_cache_purge_get_fields(struct mail_cache_copy_context *ctx, cache->file_field_map[idx] = i; j++; } - - /* change permanent decisions to temporary decisions. - if they're still permanent they'll get updated later. */ - field = &cache->fields[i].field; - if (field->decision == MAIL_CACHE_DECISION_YES) { - field->decision = MAIL_CACHE_DECISION_TEMP; - - struct event_passthrough *e = - mail_cache_decision_changed_event( - cache, ctx->event, i)-> - add_str("old_decision", "yes")-> - add_str("new_decision", "temp"); - e_debug(e->event(), "Purge changes field %s " - "cache decision yes -> temp " - "(last_used=%"PRIdTIME_T")", - field->name, field->last_used); - } } i_assert(j == used_fields_count); @@ -188,11 +170,12 @@ mail_cache_purge_check_field(struct mail_cache_copy_context *ctx, struct mail_cache_field_private *priv = &ctx->cache->fields[field]; enum mail_cache_decision_type dec = priv->field.decision; - /* if the decision isn't forced and this field hasn't - been accessed for a while, drop it */ - if ((dec & MAIL_CACHE_DECISION_FORCED) == 0 && - priv->field.last_used < ctx->max_drop_time && - dec != MAIL_CACHE_DECISION_NO) { + if ((dec & MAIL_CACHE_DECISION_FORCED) != 0) + ; + else if (dec != MAIL_CACHE_DECISION_NO && + priv->field.last_used < ctx->max_temp_drop_time) { + /* YES or TEMP decision field hasn't been accessed for a long + time now. Drop it. */ const char *dec_str = cache_decision_str(dec); struct event_passthrough *e = event_create_passthrough(ctx->event)-> @@ -204,8 +187,22 @@ mail_cache_purge_check_field(struct mail_cache_copy_context *ctx, "(decision=%s, last_used=%"PRIdTIME_T")", priv->field.name, dec_str, priv->field.last_used); dec = MAIL_CACHE_DECISION_NO; - priv->field.decision = dec; + } else if (dec == MAIL_CACHE_DECISION_YES && + priv->field.last_used < ctx->max_yes_downgrade_time) { + /* YES decision field hasn't been accessed for a while + now. Change its decision to TEMP. */ + struct event_passthrough *e = + mail_cache_decision_changed_event( + ctx->cache, ctx->event, field)-> + add_str("old_decision", "yes")-> + add_str("new_decision", "temp"); + e_debug(e->event(), "Purge changes field %s " + "cache decision yes -> temp " + "(last_used=%"PRIdTIME_T")", + priv->field.name, priv->field.last_used); + dec = MAIL_CACHE_DECISION_TEMP; } + priv->field.decision = dec; /* drop all fields we don't want */ if ((dec & ~MAIL_CACHE_DECISION_FORCED) == MAIL_CACHE_DECISION_NO) { @@ -270,9 +267,12 @@ mail_cache_copy(struct mail_cache *cache, struct mail_index_transaction *trans, /* @UNSAFE: drop unused fields and create a field mapping for used fields */ idx_hdr = mail_index_get_header(view); - ctx.max_drop_time = idx_hdr->day_stamp == 0 ? 0 : - idx_hdr->day_stamp - - cache->index->optimization_set.cache.unaccessed_field_drop_secs; + if (idx_hdr->day_stamp != 0) { + ctx.max_yes_downgrade_time = idx_hdr->day_stamp - + cache->index->optimization_set.cache.unaccessed_field_drop_secs; + ctx.max_temp_drop_time = idx_hdr->day_stamp - + 2 * cache->index->optimization_set.cache.unaccessed_field_drop_secs; + } orig_fields_count = cache->fields_count; if (cache->file_fields_count == 0) { diff --git a/src/lib-index/test-mail-cache-purge.c b/src/lib-index/test-mail-cache-purge.c index 463a343dbc..170f57d169 100644 --- a/src/lib-index/test-mail-cache-purge.c +++ b/src/lib-index/test-mail-cache-purge.c @@ -609,7 +609,14 @@ static void test_mail_cache_resetid_mismatch2(void) test_end(); } -static void test_mail_cache_purge_field_changes_int(bool drop_fields) +enum test_drop { + TEST_DROP_NOTHING, + TEST_DROP_YES_TO_TEMP_FIRST, + TEST_DROP_YES_TO_TEMP_LAST, + TEST_DROP_TEMP_TO_NO, +}; + +static void test_mail_cache_purge_field_changes_int(enum test_drop drop) { enum { TEST_FIELD_NO, @@ -708,9 +715,22 @@ static void test_mail_cache_purge_field_changes_int(bool drop_fields) /* set the last_used time just at the boundary of being dropped or being kept */ for (i = 0; i < ctx.cache->fields_count; i++) { - ctx.cache->fields[i].field.last_used = day_stamp - - (drop_fields ? 1 : 0) - - optimization_set.cache.unaccessed_field_drop_secs; + unsigned int secs = optimization_set.cache.unaccessed_field_drop_secs; + switch (drop) { + case TEST_DROP_NOTHING: + break; + case TEST_DROP_YES_TO_TEMP_FIRST: + secs++; + break; + case TEST_DROP_YES_TO_TEMP_LAST: + secs *= 2; + break; + case TEST_DROP_TEMP_TO_NO: + secs *= 2; + secs++; + break; + } + ctx.cache->fields[i].field.last_used = day_stamp - secs; } test_assert(mail_cache_purge(ctx.cache, (uint32_t)-1, "test") == 0); test_mail_cache_view_sync(&ctx); @@ -725,16 +745,26 @@ static void test_mail_cache_purge_field_changes_int(bool drop_fields) test_assert(ctx.cache->fields[cache_fields[TEST_FIELD_YES_FORCED].idx].field.decision == (MAIL_CACHE_DECISION_YES | MAIL_CACHE_DECISION_FORCED)); - if (drop_fields) { + switch (drop) { + case TEST_DROP_NOTHING: test_assert(ctx.cache->fields[cache_fields[TEST_FIELD_TEMP].idx].field.decision == - MAIL_CACHE_DECISION_NO); + MAIL_CACHE_DECISION_TEMP); test_assert(ctx.cache->fields[cache_fields[TEST_FIELD_YES].idx].field.decision == - MAIL_CACHE_DECISION_NO); - } else { + MAIL_CACHE_DECISION_YES); + break; + case TEST_DROP_YES_TO_TEMP_FIRST: + case TEST_DROP_YES_TO_TEMP_LAST: test_assert(ctx.cache->fields[cache_fields[TEST_FIELD_TEMP].idx].field.decision == MAIL_CACHE_DECISION_TEMP); test_assert(ctx.cache->fields[cache_fields[TEST_FIELD_YES].idx].field.decision == MAIL_CACHE_DECISION_TEMP); + break; + case TEST_DROP_TEMP_TO_NO: + test_assert(ctx.cache->fields[cache_fields[TEST_FIELD_TEMP].idx].field.decision == + MAIL_CACHE_DECISION_NO); + test_assert(ctx.cache->fields[cache_fields[TEST_FIELD_YES].idx].field.decision == + MAIL_CACHE_DECISION_NO); + break; } /* verify that cache fields exist as expected after purging */ @@ -743,13 +773,13 @@ static void test_mail_cache_purge_field_changes_int(bool drop_fields) test_assert(cache_equals(cache_view, 1, cache_fields[TEST_FIELD_NO_FORCED].idx, NULL)); test_assert(cache_equals(cache_view, 2, cache_fields[TEST_FIELD_NO_FORCED].idx, NULL)); test_assert(cache_equals(cache_view, 1, cache_fields[TEST_FIELD_TEMP].idx, NULL)); - if (drop_fields) + if (drop == TEST_DROP_TEMP_TO_NO) test_assert(cache_equals(cache_view, 2, cache_fields[TEST_FIELD_TEMP].idx, NULL)); else test_assert(cache_equals(cache_view, 2, cache_fields[TEST_FIELD_TEMP].idx, "temp-value")); test_assert(cache_equals(cache_view, 1, cache_fields[TEST_FIELD_TEMP_FORCED].idx, NULL)); test_assert(cache_equals(cache_view, 2, cache_fields[TEST_FIELD_TEMP_FORCED].idx, "temp-forced-value")); - if (drop_fields) + if (drop != TEST_DROP_NOTHING) test_assert(cache_equals(cache_view, 1, cache_fields[TEST_FIELD_YES].idx, NULL)); else test_assert(cache_equals(cache_view, 1, cache_fields[TEST_FIELD_YES].idx, "yes-value")); @@ -763,15 +793,29 @@ static void test_mail_cache_purge_field_changes_int(bool drop_fields) static void test_mail_cache_purge_field_changes(void) { - test_begin("mail cache purge field changes"); - test_mail_cache_purge_field_changes_int(FALSE); + test_begin("mail cache purge field changes (nothing)"); + test_mail_cache_purge_field_changes_int(TEST_DROP_NOTHING); test_end(); } static void test_mail_cache_purge_field_changes2(void) { - test_begin("mail cache purge field changes"); - test_mail_cache_purge_field_changes_int(TRUE); + test_begin("mail cache purge field changes (yes -> temp, first)"); + test_mail_cache_purge_field_changes_int(TEST_DROP_YES_TO_TEMP_FIRST); + test_end(); +} + +static void test_mail_cache_purge_field_changes3(void) +{ + test_begin("mail cache purge field changes (yes -> temp, last)"); + test_mail_cache_purge_field_changes_int(TEST_DROP_YES_TO_TEMP_LAST); + test_end(); +} + +static void test_mail_cache_purge_field_changes4(void) +{ + test_begin("mail cache purge field changes (temp -> no)"); + test_mail_cache_purge_field_changes_int(TEST_DROP_TEMP_TO_NO); test_end(); } @@ -990,6 +1034,8 @@ int main(void) test_mail_cache_resetid_mismatch2, test_mail_cache_purge_field_changes, test_mail_cache_purge_field_changes2, + test_mail_cache_purge_field_changes3, + test_mail_cache_purge_field_changes4, test_mail_cache_purge_already_done, test_mail_cache_purge_bitmask, test_mail_cache_update_need_purge_continued_records, diff --git a/src/lib-index/test-mail-cache.c b/src/lib-index/test-mail-cache.c index c554a06b85..315b816ed9 100644 --- a/src/lib-index/test-mail-cache.c +++ b/src/lib-index/test-mail-cache.c @@ -408,15 +408,9 @@ static void test_mail_cache_add_decisions(void) test_mail_cache_add_mail(&ctx, UINT_MAX, NULL); test_assert(mail_cache_purge(ctx.cache, (uint32_t)-1, "test") == 0); - /* purging changes YES -> TEMP */ - expected_decisions[TEST_FIELD_YES] = MAIL_CACHE_DECISION_TEMP; + /* check that decisions haven't changed */ for (i = 0; i < TEST_FIELD_COUNT; i++) test_assert_idx(ctx.cache->fields[cache_fields[i].idx].field.decision == expected_decisions[i], i); - /* but change it back */ - ctx.cache->fields[cache_fields[TEST_FIELD_YES].idx].field.decision = - MAIL_CACHE_DECISION_YES; - ctx.cache->fields[cache_fields[TEST_FIELD_YES].idx].decision_dirty = TRUE; - expected_decisions[TEST_FIELD_YES] = MAIL_CACHE_DECISION_YES; cache_view = mail_cache_view_open(ctx.cache, ctx.view); trans = mail_index_transaction_begin(ctx.view, 0); @@ -525,6 +519,7 @@ static void test_mail_cache_lookup_decisions_int(bool header_lookups) const struct mail_cache_field_private *priv = &ctx.cache->fields[cache_fields[i].idx]; + time_t prev_last_used = priv->field.last_used; ioloop_time++; if (!header_lookups) { test_assert_idx(mail_cache_lookup_field(cache_view, @@ -550,6 +545,12 @@ static void test_mail_cache_lookup_decisions_int(bool header_lookups) session. */ expected_uid_highwater[i] = 2; break; + case TEST_FIELD_YES: + /* YES decision doesn't change last_used until the + cache decision has been confirmed again. */ + expected_last_used[i] = prev_last_used; + expected_uid_highwater[i] = 2; + break; } test_assert_idx(priv->field.decision == expected_decisions[i], i); test_assert_idx(priv->uid_highwater == expected_uid_highwater[i], i); @@ -597,6 +598,12 @@ static void test_mail_cache_lookup_decisions_int(bool header_lookups) cache_view, str, seq, &cache_fields[i].idx, 1) == 0, i); } + if (i == TEST_FIELD_YES && seq == 2) { + /* YES decision is confirmed now. The last_used + timestamp was updated for the first old + mail. */ + expected_last_used[i] = ioloop_time; + } test_assert_idx(priv->field.decision == expected_decisions[i], i); test_assert_idx(priv->uid_highwater == expected_uid_highwater[i], i); test_assert_idx(priv->field.last_used == expected_last_used[i], i);