Skip to content

Commit

Permalink
Simplify iprop update locking and avoid deadlock
Browse files Browse the repository at this point in the history 
Since we are no longer treating the update log like a journal (#7552),
we don't need two-stage update logging.  In kdb5.c, add an update log
entry after each DB change in one step, without getting an explicit
lock.  In kdb_log.c, combine ulog_add_update with ulog_finish_update,
and make ulog_add_update lock the ulog internally.

This change avoids deadlock by removing the only cases where the ulog
is locked before the DB.

ticket: 7861
  • Loading branch information
@greghudson
greghudson committed Feb 20, 2014
1 parent dba768e commit 444ef5f
Show file tree
Hide file tree
Showing 3 changed files with 37 additions and 140 deletions.
2 changes: 0 additions & 2 deletions src/include/kdb_log.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,6 @@ krb5_error_code ulog_map(krb5_context context, const char *logname,
uint32_t entries, int caller, char **db_args);
void ulog_init_header(krb5_context context);
krb5_error_code ulog_add_update(krb5_context context, kdb_incr_update_t *upd);
krb5_error_code ulog_finish_update(krb5_context context,
kdb_incr_update_t *upd);
krb5_error_code ulog_get_entries(krb5_context context, const kdb_last_t *last,
kdb_incr_result_t *ulog_handle);
krb5_error_code ulog_replay(krb5_context context, kdb_incr_result_t *incr_ret,
Expand Down
116 changes: 19 additions & 97 deletions src/lib/kdb/kdb5.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -884,23 +884,8 @@ krb5_error_code
krb5_db_put_principal(krb5_context kcontext, krb5_db_entry *entry)
{
krb5_error_code status = 0;
kdb_vftabl *v;
char **db_args = NULL;
kdb_incr_update_t *upd = NULL;
char *princ_name = NULL;
int ulog_locked = 0;

status = get_vftabl(kcontext, &v);
if (status)
return status;
if (v->put_principal == NULL)
return KRB5_PLUGIN_OP_NOTSUPP;

status = extract_db_args_from_tl_data(kcontext, &entry->tl_data,
&entry->n_tl_data,
&db_args);
if (status)
goto cleanup;

if (logging(kcontext)) {
upd = k5alloc(sizeof(*upd), &status);
Expand All @@ -909,30 +894,22 @@ krb5_db_put_principal(krb5_context kcontext, krb5_db_entry *entry)
if ((status = ulog_conv_2logentry(kcontext, entry, upd)))
goto cleanup;

status = ulog_lock(kcontext, KRB5_LOCKMODE_EXCLUSIVE);
if (status != 0)
goto cleanup;
ulog_locked = 1;

status = krb5_unparse_name(kcontext, entry->princ, &princ_name);
if (status != 0)
goto cleanup;

upd->kdb_princ_name.utf8str_t_val = princ_name;
upd->kdb_princ_name.utf8str_t_len = strlen(princ_name);

if ((status = ulog_add_update(kcontext, upd)) != 0)
goto cleanup;
}

status = v->put_principal(kcontext, entry, db_args);
if (status == 0 && ulog_locked)
(void) ulog_finish_update(kcontext, upd);
status = krb5int_put_principal_no_log(kcontext, entry);
if (status)
goto cleanup;

if (logging(kcontext))
status = ulog_add_update(kcontext, upd);

cleanup:
if (ulog_locked)
ulog_lock(kcontext, KRB5_LOCKMODE_UNLOCK);
free_db_args(kcontext, db_args);
ulog_free_entries(upd, 1);
return status;
}
Expand All @@ -956,45 +933,23 @@ krb5_error_code
krb5_db_delete_principal(krb5_context kcontext, krb5_principal search_for)
{
krb5_error_code status = 0;
kdb_vftabl *v;
kdb_incr_update_t upd;
char *princ_name = NULL;
int ulog_locked = 0;

status = get_vftabl(kcontext, &v);
if (status)
status = krb5int_delete_principal_no_log(kcontext, search_for);
if (status || !logging(kcontext))
return status;
if (v->delete_principal == NULL)
return KRB5_PLUGIN_OP_NOTSUPP;

if (logging(kcontext)) {
status = ulog_lock(kcontext, KRB5_LOCKMODE_EXCLUSIVE);
if (status)
return status;
ulog_locked = 1;

status = krb5_unparse_name(kcontext, search_for, &princ_name);
if (status)
goto cleanup;

(void) memset(&upd, 0, sizeof (kdb_incr_update_t));

upd.kdb_princ_name.utf8str_t_val = princ_name;
upd.kdb_princ_name.utf8str_t_len = strlen(princ_name);
upd.kdb_deleted = TRUE;

status = ulog_add_update(kcontext, &upd);
if (status)
goto cleanup;
}
status = krb5_unparse_name(kcontext, search_for, &princ_name);
if (status)
return status;

status = v->delete_principal(kcontext, search_for);
if (status == 0 && ulog_locked)
(void) ulog_finish_update(kcontext, &upd);
memset(&upd, 0, sizeof(kdb_incr_update_t));
upd.kdb_princ_name.utf8str_t_val = princ_name;
upd.kdb_princ_name.utf8str_t_len = strlen(princ_name);
upd.kdb_deleted = TRUE;

cleanup:
if (ulog_locked)
ulog_lock(kcontext, KRB5_LOCKMODE_UNLOCK);
status = ulog_add_update(kcontext, &upd);
free(princ_name);
return status;
}
Expand Down Expand Up @@ -2301,28 +2256,17 @@ krb5_db_create_policy(krb5_context kcontext, osa_policy_ent_t policy)
{
krb5_error_code status = 0;
kdb_vftabl *v;
int ulog_locked = 0;

status = get_vftabl(kcontext, &v);
if (status)
return status;
if (v->create_policy == NULL)
return KRB5_PLUGIN_OP_NOTSUPP;

if (logging(kcontext)) {
status = ulog_lock(kcontext, KRB5_LOCKMODE_EXCLUSIVE);
if (status != 0)
return status;
ulog_locked = 1;
}

status = v->create_policy(kcontext, policy);
/* iprop does not support policy mods; force full resync. */
if (!status && ulog_locked)
if (!status && logging(kcontext))
ulog_init_header(kcontext);

if (ulog_locked)
ulog_lock(kcontext, KRB5_LOCKMODE_UNLOCK);
return status;
}

Expand All @@ -2345,28 +2289,17 @@ krb5_db_put_policy(krb5_context kcontext, osa_policy_ent_t policy)
{
krb5_error_code status = 0;
kdb_vftabl *v;
int ulog_locked = 0;

status = get_vftabl(kcontext, &v);
if (status)
return status;
if (v->put_policy == NULL)
return KRB5_PLUGIN_OP_NOTSUPP;

if (logging(kcontext)) {
status = ulog_lock(kcontext, KRB5_LOCKMODE_EXCLUSIVE);
if (status)
return status;
ulog_locked = 1;
}

status = v->put_policy(kcontext, policy);
/* iprop does not support policy mods; force full resync. */
if (!status && ulog_locked)
if (!status && logging(kcontext))
ulog_init_header(kcontext);

if (ulog_locked)
ulog_lock(kcontext, KRB5_LOCKMODE_UNLOCK);
return status;
}

Expand All @@ -2390,28 +2323,17 @@ krb5_db_delete_policy(krb5_context kcontext, char *policy)
{
krb5_error_code status = 0;
kdb_vftabl *v;
int ulog_locked = 0;

status = get_vftabl(kcontext, &v);
if (status)
return status;
if (v->delete_policy == NULL)
return KRB5_PLUGIN_OP_NOTSUPP;

if (logging(kcontext)) {
status = ulog_lock(kcontext, KRB5_LOCKMODE_EXCLUSIVE);
if (status)
return status;
ulog_locked = 1;
}

status = v->delete_policy(kcontext, policy);
/* iprop does not support policy mods; force full resync. */
if (!status && ulog_locked)
if (!status && logging(kcontext))
ulog_init_header(kcontext);

if (ulog_locked)
ulog_lock(kcontext, KRB5_LOCKMODE_UNLOCK);
return status;
}

Expand Down
59 changes: 18 additions & 41 deletions src/lib/kdb/kdb_log.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -203,12 +203,13 @@ ulog_add_update(krb5_context context, kdb_incr_update_t *upd)
int ulogfd;

INIT_ULOG(context);
retval = ulog_lock(context, KRB5_LOCKMODE_EXCLUSIVE);
if (retval)
return retval;

ulogentries = log_ctx->ulogentries;
ulogfd = log_ctx->ulogfd;

if (upd == NULL)
return KRB5_LOG_ERROR;

time_current(&ktime);

upd_size = xdr_sizeof((xdrproc_t)xdr_kdb_incr_update_t, upd);
Expand All @@ -218,38 +219,39 @@ ulog_add_update(krb5_context context, kdb_incr_update_t *upd)
if (recsize > ulog->kdb_block) {
retval = resize(ulog, ulogentries, ulogfd, recsize);
if (retval)
return retval;
goto cleanup;
}

/* If we have reached the last possible serial number, reinitialize the
* ulog and start over. Slaves will do a full resync. */
if (ulog->kdb_last_sno == (kdb_sno_t)-1)
reset_header(ulog);

/* Get the next serial number and save it for finish_update() to index. */
cur_sno = ulog->kdb_last_sno + 1;
upd->kdb_entry_sno = cur_sno;
ulog->kdb_state = KDB_UNSTABLE;

/* Get the next serial number and find its update entry. */
cur_sno = ulog->kdb_last_sno + 1;
i = (cur_sno - 1) % ulogentries;
indx_log = INDEX(ulog, i);

memset(indx_log, 0, ulog->kdb_block);
indx_log->kdb_umagic = KDB_ULOG_MAGIC;
indx_log->kdb_entry_size = upd_size;
indx_log->kdb_entry_sno = cur_sno;
indx_log->kdb_entry_sno = upd->kdb_entry_sno = cur_sno;
indx_log->kdb_time = upd->kdb_time = ktime;
indx_log->kdb_commit = upd->kdb_commit = FALSE;

ulog->kdb_state = KDB_UNSTABLE;

xdrmem_create(&xdrs, (char *)indx_log->entry_data,
indx_log->kdb_entry_size, XDR_ENCODE);
if (!xdr_kdb_incr_update_t(&xdrs, upd))
return KRB5_LOG_CONV;
if (!xdr_kdb_incr_update_t(&xdrs, upd)) {
retval = KRB5_LOG_CONV;
goto cleanup;
}

indx_log->kdb_commit = TRUE;
retval = sync_update(ulog, indx_log);
if (retval)
return retval;
goto cleanup;

if (ulog->kdb_num < ulogentries)
ulog->kdb_num++;
Expand All @@ -269,37 +271,12 @@ ulog_add_update(krb5_context context, kdb_incr_update_t *upd)
ulog->kdb_first_time = indx_log->kdb_time;
}

ulog_sync_header(ulog);
return 0;
}

/* Mark the log entry as committed and sync the memory mapped log to file. */
krb5_error_code
ulog_finish_update(krb5_context context, kdb_incr_update_t *upd)
{
krb5_error_code retval;
kdb_ent_header_t *indx_log;
unsigned int i;
kdb_log_context *log_ctx;
kdb_hlog_t *ulog = NULL;
uint32_t ulogentries;

INIT_ULOG(context);
ulogentries = log_ctx->ulogentries;

i = (upd->kdb_entry_sno - 1) % ulogentries;

indx_log = INDEX(ulog, i);
indx_log->kdb_commit = TRUE;

ulog->kdb_state = KDB_STABLE;

retval = sync_update(ulog, indx_log);
if (retval)
return retval;

cleanup:
ulog_sync_header(ulog);
return 0;
ulog_lock(context, KRB5_LOCKMODE_UNLOCK);
return retval;
}

/* Used by the slave to update its hash db from* the incr update log. Must be
Expand Down

0 comments on commit 444ef5f

Please sign in to comment.