@matheusbsbs / matheus-bagliano / commits / 46b5f67019

refs/reftable: handle reloading stacks in the reftable backend

When accessing a stack we almost always have to reload the stack before reading data from it. This is mostly because Git does not have a notification mechanism for when underlying data has been changed, and thus we are forced to opportunistically reload the stack every single time to account for any changes that may have happened concurrently. Handle the reload internally in `backend_for()`. For one this forces callsites to think about whether or not they need to reload the stack. But second this makes the logic to access stacks more self-contained by letting the `struct reftable_backend` manage themselves. Update callsites where we don't reload the stack to document why we don't. In some cases it's unclear whether it is the right thing to do in the first place, but fixing that is outside of the scope of this patch series. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Patrick Steinhardt committed Nov 26, 2024 at 07:42 UTC 46b5f67019bbf9864416d13693f6292bca62d7af

1 file changed +126 -58

refs/reftable-backend.c

+126 -58

@@ -114,21 +114,25 @@ static struct reftable_ref_store *reftable_be_downcast(struct ref_store *ref_sto

114 * like `worktrees/$worktree/refs/heads/foo` as worktree stacks will store

115 * those references in their normalized form.

116 */

117 -static struct reftable_backend *backend_for(struct reftable_ref_store *store,

118 - const char *refname,

119 - const char **rewritten_ref)

117 +static int backend_for(struct reftable_backend **out,

118 + struct reftable_ref_store *store,

119 + const char *refname,

120 + const char **rewritten_ref,

121 + int reload)

122 {

123 + struct reftable_backend *be;

124 const char *wtname;

125 int wtname_len;

126

124 - if (!refname)

125 - return &store->main_backend;

127 + if (!refname) {

128 + be = &store->main_backend;

129 + goto out;

130 + }

131

132 switch (parse_worktree_ref(refname, &wtname, &wtname_len, rewritten_ref)) {

133 case REF_WORKTREE_OTHER: {

134 static struct strbuf wtname_buf = STRBUF_INIT;

135 struct strbuf wt_dir = STRBUF_INIT;

131 - struct reftable_backend *be;

136

137 /*

138 * We're using a static buffer here so that we don't need to

@@ -162,7 +166,7 @@ static struct reftable_backend *backend_for(struct reftable_ref_store *store,

166 }

167

168 strbuf_release(&wt_dir);

165 - return be;

169 + goto out;

170 }

171 case REF_WORKTREE_CURRENT:

172 /*

@@ -170,14 +174,27 @@ static struct reftable_backend *backend_for(struct reftable_ref_store *store,

174 * main worktree. We thus return the main stack in that case.

175 */

176 if (!store->worktree_backend.stack)

173 - return &store->main_backend;

174 - return &store->worktree_backend;

177 + be = &store->main_backend;

178 + else

179 + be = &store->worktree_backend;

180 + goto out;

181 case REF_WORKTREE_MAIN:

182 case REF_WORKTREE_SHARED:

177 - return &store->main_backend;

183 + be = &store->main_backend;

184 + goto out;

185 default:

186 BUG("unhandled worktree reference type");

187 }

188 +

189 +out:

190 + if (reload) {

191 + int ret = reftable_stack_reload(be->stack);

192 + if (ret)

193 + return ret;

194 + }

195 + *out = be;

196 +

197 + return 0;

198 }

199

200 static int should_write_log(struct reftable_ref_store *refs, const char *refname)

@@ -828,17 +845,17 @@ static int reftable_be_read_raw_ref(struct ref_store *ref_store,

845 {

846 struct reftable_ref_store *refs =

847 reftable_be_downcast(ref_store, REF_STORE_READ, "read_raw_ref");

831 - struct reftable_stack *stack = backend_for(refs, refname, &refname)->stack;

848 + struct reftable_backend *be;

849 int ret;

850

851 if (refs->err < 0)

852 return refs->err;

853

837 - ret = reftable_stack_reload(stack);

854 + ret = backend_for(&be, refs, refname, &refname, 1);

855 if (ret)

856 return ret;

857

841 - ret = read_ref_without_reload(refs, stack, refname, oid, referent, type);

858 + ret = read_ref_without_reload(refs, be->stack, refname, oid, referent, type);

859 if (ret < 0)

860 return ret;

861 if (ret > 0) {

@@ -855,15 +872,15 @@ static int reftable_be_read_symbolic_ref(struct ref_store *ref_store,

872 {

873 struct reftable_ref_store *refs =

874 reftable_be_downcast(ref_store, REF_STORE_READ, "read_symbolic_ref");

858 - struct reftable_stack *stack = backend_for(refs, refname, &refname)->stack;

875 struct reftable_ref_record ref = {0};

876 + struct reftable_backend *be;

877 int ret;

878

862 - ret = reftable_stack_reload(stack);

879 + ret = backend_for(&be, refs, refname, &refname, 1);

880 if (ret)

881 return ret;

882

866 - ret = reftable_stack_read_ref(stack, refname, &ref);

883 + ret = reftable_stack_read_ref(be->stack, refname, &ref);

884 if (ret == 0 && ref.value_type == REFTABLE_REF_SYMREF)

885 strbuf_addstr(referent, ref.value.symref);

886 else

@@ -880,7 +897,7 @@ struct reftable_transaction_update {

897

898 struct write_transaction_table_arg {

899 struct reftable_ref_store *refs;

883 - struct reftable_stack *stack;

900 + struct reftable_backend *be;

901 struct reftable_addition *addition;

902 struct reftable_transaction_update *updates;

903 size_t updates_nr;

@@ -915,27 +932,37 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,

932 struct ref_update *update,

933 struct strbuf *err)

934 {

918 - struct reftable_stack *stack = backend_for(refs, update->refname, NULL)->stack;

935 struct write_transaction_table_arg *arg = NULL;

936 + struct reftable_backend *be;

937 size_t i;

938 int ret;

939

940 + /*

941 + * This function gets called in a loop, and we don't want to repeatedly

942 + * reload the stack for every single ref update. Instead, we manually

943 + * reload further down in the case where we haven't yet prepared the

944 + * specific `reftable_backend`.

945 + */

946 + ret = backend_for(&be, refs, update->refname, NULL, 0);

947 + if (ret)

948 + return ret;

949 +

950 /*

951 * Search for a preexisting stack update. If there is one then we add

952 * the update to it, otherwise we set up a new stack update.

953 */

954 for (i = 0; !arg && i < tx_data->args_nr; i++)

928 - if (tx_data->args[i].stack == stack)

955 + if (tx_data->args[i].be == be)

956 arg = &tx_data->args[i];

957

958 if (!arg) {

959 struct reftable_addition *addition;

960

934 - ret = reftable_stack_reload(stack);

961 + ret = reftable_stack_reload(be->stack);

962 if (ret)

963 return ret;

964

938 - ret = reftable_stack_new_addition(&addition, stack,

965 + ret = reftable_stack_new_addition(&addition, be->stack,

966 REFTABLE_STACK_NEW_ADDITION_RELOAD);

967 if (ret) {

968 if (ret == REFTABLE_LOCK_ERROR)

@@ -947,7 +974,7 @@ static int prepare_transaction_update(struct write_transaction_table_arg **out,

974 tx_data->args_alloc);

975 arg = &tx_data->args[tx_data->args_nr++];

976 arg->refs = refs;

950 - arg->stack = stack;

977 + arg->be = be;

978 arg->addition = addition;

979 arg->updates = NULL;

980 arg->updates_nr = 0;

@@ -1002,6 +1029,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,

1029 struct strbuf referent = STRBUF_INIT, head_referent = STRBUF_INIT;

1030 struct string_list affected_refnames = STRING_LIST_INIT_NODUP;

1031 struct reftable_transaction_data *tx_data = NULL;

1032 + struct reftable_backend *be;

1033 struct object_id head_oid;

1034 unsigned int head_type = 0;

1035 size_t i;

@@ -1048,7 +1076,22 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,

1076 goto done;

1077 }

1078

1051 - ret = read_ref_without_reload(refs, backend_for(refs, "HEAD", NULL)->stack, "HEAD",

1079 + /*

1080 + * TODO: it's dubious whether we should reload the stack that "HEAD"

1081 + * belongs to or not. In theory, it may happen that we only modify

1082 + * stacks which are _not_ part of the "HEAD" stack. In that case we

1083 + * wouldn't have prepared any transaction for its stack and would not

1084 + * have reloaded it, which may mean that it is stale.

1085 + *

1086 + * On the other hand, reloading that stack without locking it feels

1087 + * wrong, too, as the value of "HEAD" could be modified concurrently at

1088 + * any point in time.

1089 + */

1090 + ret = backend_for(&be, refs, "HEAD", NULL, 0);

1091 + if (ret)

1092 + goto done;

1093 +

1094 + ret = read_ref_without_reload(refs, be->stack, "HEAD",

1095 &head_oid, &head_referent, &head_type);

1096 if (ret < 0)

1097 goto done;

@@ -1057,10 +1100,18 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,

1100 for (i = 0; i < transaction->nr; i++) {

1101 struct ref_update *u = transaction->updates[i];

1102 struct object_id current_oid = {0};

1060 - struct reftable_stack *stack;

1103 const char *rewritten_ref;

1104

1063 - stack = backend_for(refs, u->refname, &rewritten_ref)->stack;

1105 + /*

1106 + * There is no need to reload the respective backends here as

1107 + * we have already reloaded them when preparing the transaction

1108 + * update. And given that the stacks have been locked there

1109 + * shouldn't have been any concurrent modifications of the

1110 + * stack.

1111 + */

1112 + ret = backend_for(&be, refs, u->refname, &rewritten_ref, 0);

1113 + if (ret)

1114 + goto done;

1115

1116 /* Verify that the new object ID is valid. */

1117 if ((u->flags & REF_HAVE_NEW) && !is_null_oid(&u->new_oid) &&

@@ -1116,7 +1167,7 @@ static int reftable_be_transaction_prepare(struct ref_store *ref_store,

1167 string_list_insert(&affected_refnames, new_update->refname);

1168 }

1169

1119 - ret = read_ref_without_reload(refs, stack, rewritten_ref,

1170 + ret = read_ref_without_reload(refs, be->stack, rewritten_ref,

1171 &current_oid, &referent, &u->type);

1172 if (ret < 0)

1173 goto done;

@@ -1318,7 +1369,7 @@ static int transaction_update_cmp(const void *a, const void *b)

1369 static int write_transaction_table(struct reftable_writer *writer, void *cb_data)

1370 {

1371 struct write_transaction_table_arg *arg = cb_data;

1321 - uint64_t ts = reftable_stack_next_update_index(arg->stack);

1372 + uint64_t ts = reftable_stack_next_update_index(arg->be->stack);

1373 struct reftable_log_record *logs = NULL;

1374 struct ident_split committer_ident = {0};

1375 size_t logs_nr = 0, logs_alloc = 0, i;

@@ -1354,7 +1405,7 @@ static int write_transaction_table(struct reftable_writer *writer, void *cb_data

1405 struct reftable_log_record log = {0};

1406 struct reftable_iterator it = {0};

1407

1357 - ret = reftable_stack_init_log_iterator(arg->stack, &it);

1408 + ret = reftable_stack_init_log_iterator(arg->be->stack, &it);

1409 if (ret < 0)

1410 goto done;

1411

@@ -1799,10 +1850,9 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,

1850 {

1851 struct reftable_ref_store *refs =

1852 reftable_be_downcast(ref_store, REF_STORE_WRITE, "rename_ref");

1802 - struct reftable_stack *stack = backend_for(refs, newrefname, &newrefname)->stack;

1853 + struct reftable_backend *be;

1854 struct write_copy_arg arg = {

1855 .refs = refs,

1805 - .stack = stack,

1856 .oldname = oldrefname,

1857 .newname = newrefname,

1858 .logmsg = logmsg,

@@ -1814,10 +1864,11 @@ static int reftable_be_rename_ref(struct ref_store *ref_store,

1864 if (ret < 0)

1865 goto done;

1866

1817 - ret = reftable_stack_reload(stack);

1867 + ret = backend_for(&be, refs, newrefname, &newrefname, 1);

1868 if (ret)

1869 goto done;

1820 - ret = reftable_stack_add(stack, &write_copy_table, &arg);

1870 + arg.stack = be->stack;

1871 + ret = reftable_stack_add(be->stack, &write_copy_table, &arg);

1872

1873 done:

1874 assert(ret != REFTABLE_API_ERROR);

@@ -1831,10 +1882,9 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,

1882 {

1883 struct reftable_ref_store *refs =

1884 reftable_be_downcast(ref_store, REF_STORE_WRITE, "copy_ref");

1834 - struct reftable_stack *stack = backend_for(refs, newrefname, &newrefname)->stack;

1885 + struct reftable_backend *be;

1886 struct write_copy_arg arg = {

1887 .refs = refs,

1837 - .stack = stack,

1888 .oldname = oldrefname,

1889 .newname = newrefname,

1890 .logmsg = logmsg,

@@ -1845,10 +1895,11 @@ static int reftable_be_copy_ref(struct ref_store *ref_store,

1895 if (ret < 0)

1896 goto done;

1897

1848 - ret = reftable_stack_reload(stack);

1898 + ret = backend_for(&be, refs, newrefname, &newrefname, 1);

1899 if (ret)

1900 goto done;

1851 - ret = reftable_stack_add(stack, &write_copy_table, &arg);

1901 + arg.stack = be->stack;

1902 + ret = reftable_stack_add(be->stack, &write_copy_table, &arg);

1903

1904 done:

1905 assert(ret != REFTABLE_API_ERROR);

@@ -2012,15 +2063,23 @@ static int reftable_be_for_each_reflog_ent_reverse(struct ref_store *ref_store,

2063 {

2064 struct reftable_ref_store *refs =

2065 reftable_be_downcast(ref_store, REF_STORE_READ, "for_each_reflog_ent_reverse");

2015 - struct reftable_stack *stack = backend_for(refs, refname, &refname)->stack;

2066 struct reftable_log_record log = {0};

2067 struct reftable_iterator it = {0};

2068 + struct reftable_backend *be;

2069 int ret;

2070

2071 if (refs->err < 0)

2072 return refs->err;

2073

2023 - ret = reftable_stack_init_log_iterator(stack, &it);

2074 + /*

2075 + * TODO: we should adapt this callsite to reload the stack. There is no

2076 + * obvious reason why we shouldn't.

2077 + */

2078 + ret = backend_for(&be, refs, refname, &refname, 0);

2079 + if (ret)

2080 + goto done;

2081 +

2082 + ret = reftable_stack_init_log_iterator(be->stack, &it);

2083 if (ret < 0)

2084 goto done;

2085

@@ -2052,16 +2111,24 @@ static int reftable_be_for_each_reflog_ent(struct ref_store *ref_store,

2111 {

2112 struct reftable_ref_store *refs =

2113 reftable_be_downcast(ref_store, REF_STORE_READ, "for_each_reflog_ent");

2055 - struct reftable_stack *stack = backend_for(refs, refname, &refname)->stack;

2114 struct reftable_log_record *logs = NULL;

2115 struct reftable_iterator it = {0};

2116 + struct reftable_backend *be;

2117 size_t logs_alloc = 0, logs_nr = 0, i;

2118 int ret;

2119

2120 if (refs->err < 0)

2121 return refs->err;

2122

2064 - ret = reftable_stack_init_log_iterator(stack, &it);

2123 + /*

2124 + * TODO: we should adapt this callsite to reload the stack. There is no

2125 + * obvious reason why we shouldn't.

2126 + */

2127 + ret = backend_for(&be, refs, refname, &refname, 0);

2128 + if (ret)

2129 + goto done;

2130 +

2131 + ret = reftable_stack_init_log_iterator(be->stack, &it);

2132 if (ret < 0)

2133 goto done;

2134

@@ -2101,20 +2168,20 @@ static int reftable_be_reflog_exists(struct ref_store *ref_store,

2168 {

2169 struct reftable_ref_store *refs =

2170 reftable_be_downcast(ref_store, REF_STORE_READ, "reflog_exists");

2104 - struct reftable_stack *stack = backend_for(refs, refname, &refname)->stack;

2171 struct reftable_log_record log = {0};

2172 struct reftable_iterator it = {0};

2173 + struct reftable_backend *be;

2174 int ret;

2175

2176 ret = refs->err;

2177 if (ret < 0)

2178 goto done;

2179

2113 - ret = reftable_stack_reload(stack);

2180 + ret = backend_for(&be, refs, refname, &refname, 1);

2181 if (ret < 0)

2182 goto done;

2183

2117 - ret = reftable_stack_init_log_iterator(stack, &it);

2184 + ret = reftable_stack_init_log_iterator(be->stack, &it);

2185 if (ret < 0)

2186 goto done;

2187

@@ -2186,10 +2253,9 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,

2253 {

2254 struct reftable_ref_store *refs =

2255 reftable_be_downcast(ref_store, REF_STORE_WRITE, "create_reflog");

2189 - struct reftable_stack *stack = backend_for(refs, refname, &refname)->stack;

2256 + struct reftable_backend *be;

2257 struct write_reflog_existence_arg arg = {

2258 .refs = refs,

2192 - .stack = stack,

2259 .refname = refname,

2260 };

2261 int ret;

@@ -2198,11 +2264,12 @@ static int reftable_be_create_reflog(struct ref_store *ref_store,

2264 if (ret < 0)

2265 goto done;

2266

2201 - ret = reftable_stack_reload(stack);

2267 + ret = backend_for(&be, refs, refname, &refname, 1);

2268 if (ret)

2269 goto done;

2270 + arg.stack = be->stack;

2271

2205 - ret = reftable_stack_add(stack, &write_reflog_existence_table, &arg);

2272 + ret = reftable_stack_add(be->stack, &write_reflog_existence_table, &arg);

2273

2274 done:

2275 return ret;

@@ -2260,17 +2327,18 @@ static int reftable_be_delete_reflog(struct ref_store *ref_store,

2327 {

2328 struct reftable_ref_store *refs =

2329 reftable_be_downcast(ref_store, REF_STORE_WRITE, "delete_reflog");

2263 - struct reftable_stack *stack = backend_for(refs, refname, &refname)->stack;

2330 + struct reftable_backend *be;

2331 struct write_reflog_delete_arg arg = {

2265 - .stack = stack,

2332 .refname = refname,

2333 };

2334 int ret;

2335

2270 - ret = reftable_stack_reload(stack);

2336 + ret = backend_for(&be, refs, refname, &refname, 1);

2337 if (ret)

2338 return ret;

2273 - ret = reftable_stack_add(stack, &write_reflog_delete_table, &arg);

2339 + arg.stack = be->stack;

2340 +

2341 + ret = reftable_stack_add(be->stack, &write_reflog_delete_table, &arg);

2342

2343 assert(ret != REFTABLE_API_ERROR);

2344 return ret;

@@ -2369,13 +2437,13 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,

2437 */

2438 struct reftable_ref_store *refs =

2439 reftable_be_downcast(ref_store, REF_STORE_WRITE, "reflog_expire");

2372 - struct reftable_stack *stack = backend_for(refs, refname, &refname)->stack;

2440 struct reftable_log_record *logs = NULL;

2441 struct reftable_log_record *rewritten = NULL;

2442 struct reftable_ref_record ref_record = {0};

2443 struct reftable_iterator it = {0};

2444 struct reftable_addition *add = NULL;

2445 struct reflog_expiry_arg arg = {0};

2446 + struct reftable_backend *be;

2447 struct object_id oid = {0};

2448 uint8_t *last_hash = NULL;

2449 size_t logs_nr = 0, logs_alloc = 0, i;

@@ -2384,11 +2452,11 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,

2452 if (refs->err < 0)

2453 return refs->err;

2454

2387 - ret = reftable_stack_reload(stack);

2455 + ret = backend_for(&be, refs, refname, &refname, 1);

2456 if (ret < 0)

2457 goto done;

2458

2391 - ret = reftable_stack_init_log_iterator(stack, &it);

2459 + ret = reftable_stack_init_log_iterator(be->stack, &it);

2460 if (ret < 0)

2461 goto done;

2462

@@ -2396,11 +2464,11 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,

2464 if (ret < 0)

2465 goto done;

2466

2399 - ret = reftable_stack_new_addition(&add, stack, 0);

2467 + ret = reftable_stack_new_addition(&add, be->stack, 0);

2468 if (ret < 0)

2469 goto done;

2470

2403 - ret = reftable_stack_read_ref(stack, refname, &ref_record);

2471 + ret = reftable_stack_read_ref(be->stack, refname, &ref_record);

2472 if (ret < 0)

2473 goto done;

2474 if (reftable_ref_record_val1(&ref_record))

@@ -2479,8 +2547,8 @@ static int reftable_be_reflog_expire(struct ref_store *ref_store,

2547 arg.refs = refs;

2548 arg.records = rewritten;

2549 arg.len = logs_nr;

2482 - arg.stack = stack,

2483 - arg.refname = refname,

2550 + arg.stack = be->stack;

2551 + arg.refname = refname;

2552

2553 ret = reftable_addition_add(add, &write_reflog_expiry_table, &arg);

2554 if (ret < 0)