Commit 48a163db authored by Ilya Dryomov's avatar Ilya Dryomov Committed by Sage Weil
Browse files

crush: fix off-by-one errors in total_tries refactor 



Back in 27f4d1f6bc32c2ed7b2c5080cbd58b14df622607 we refactored the CRUSH
code to allow adjustment of the retry counts on a per-pool basis.  That
commit had an off-by-one bug: the previous "tries" counter was a *retry*
count, not a *try* count, but the new code was passing in 1 meaning
there should be no retries.

Fix the ftotal vs tries comparison to use < instead of <= to fix the
problem.  Note that the original code used <= here, which means the
global "choose_total_tries" tunable is actually counting retries.
Compensate for that by adding 1 in crush_do_rule when we pull the tunable
into the local variable.

This was noticed looking at output from a user provided osdmap.
Unfortunately the map doesn't illustrate the change in mapping behavior
and I haven't managed to construct one yet that does.  Inspection of the
crush debug output now aligns with prior versions, though.

Reflects ceph.git commit 795704fd615f0b008dcc81aa088a859b2d075138.

Signed-off-by: default avatarIlya Dryomov <ilya.dryomov@inktank.com>
Reviewed-by: default avatarJosh Durgin <josh.durgin@inktank.com>
parent cc48c3e8

net/ceph/crush/mapper.c

+27 −19
Original line number Diff line number Diff line
@@ -292,8 +292,8 @@ static int is_out(const struct crush_map *map, 
 * @outpos: our position in that vector

 * @tries: number of attempts to make

 * @recurse_tries: number of attempts to have recursive chooseleaf make

 * @local_tries: localized retries

 * @local_fallback_tries: localized fallback retries

 * @local_retries: localized retries

 * @local_fallback_retries: localized fallback retries

 * @recurse_to_leaf: true if we want one device under each item of given type (chooseleaf instead of choose)

 * @out2: second output vector for leaf items (if @recurse_to_leaf)

 */
@@ -304,8 +304,8 @@ static int crush_choose_firstn(const struct crush_map *map, 
			       int *out, int outpos,

			       unsigned int tries,

			       unsigned int recurse_tries,

			       unsigned int local_tries,

			       unsigned int local_fallback_tries,

			       unsigned int local_retries,

			       unsigned int local_fallback_retries,

			       int recurse_to_leaf,

			       int *out2)

{
@@ -344,9 +344,9 @@ static int crush_choose_firstn(const struct crush_map *map, 
					reject = 1;

					goto reject;

				}

				if (local_fallback_tries > 0 &&

				if (local_fallback_retries > 0 &&

				    flocal >= (in->size>>1) &&

				    flocal > local_fallback_tries)

				    flocal > local_fallback_retries)

					item = bucket_perm_choose(in, x, r);

				else

					item = crush_bucket_choose(in, x, r);
@@ -393,8 +393,8 @@ static int crush_choose_firstn(const struct crush_map *map, 
							 x, outpos+1, 0,

							 out2, outpos,

							 recurse_tries, 0,

							 local_tries,

							 local_fallback_tries,

							 local_retries,

							 local_fallback_retries,

							 0,

							 NULL) <= outpos)

							/* didn't get leaf */
@@ -420,14 +420,14 @@ reject: 
					ftotal++;

					flocal++;



					if (collide && flocal <= local_tries)

					if (collide && flocal <= local_retries)

						/* retry locally a few times */

						retry_bucket = 1;

					else if (local_fallback_tries > 0 &&

						 flocal <= in->size + local_fallback_tries)

					else if (local_fallback_retries > 0 &&

						 flocal <= in->size + local_fallback_retries)

						/* exhaustive bucket search */

						retry_bucket = 1;

					else if (ftotal <= tries)

					else if (ftotal < tries)

						/* then retry descent */

						retry_descent = 1;

					else
@@ -640,10 +640,18 @@ int crush_do_rule(const struct crush_map *map, 
	__u32 step;

	int i, j;

	int numrep;

	int choose_tries = map->choose_total_tries;

	int choose_local_tries = map->choose_local_tries;

	int choose_local_fallback_tries = map->choose_local_fallback_tries;

	/*

	 * the original choose_total_tries value was off by one (it

	 * counted "retries" and not "tries").  add one.

	 */

	int choose_tries = map->choose_total_tries + 1;

	int choose_leaf_tries = 0;

	/*

	 * the local tries values were counted as "retries", though,

	 * and need no adjustment

	 */

	int choose_local_retries = map->choose_local_tries;

	int choose_local_fallback_retries = map->choose_local_fallback_tries;



	if ((__u32)ruleno >= map->max_rules) {

		dprintk(" bad ruleno %d\n", ruleno);
@@ -677,12 +685,12 @@ int crush_do_rule(const struct crush_map *map, 


		case CRUSH_RULE_SET_CHOOSE_LOCAL_TRIES:

			if (curstep->arg1 > 0)

				choose_local_tries = curstep->arg1;

				choose_local_retries = curstep->arg1;

			break;



		case CRUSH_RULE_SET_CHOOSE_LOCAL_FALLBACK_TRIES:

			if (curstep->arg1 > 0)

				choose_local_fallback_tries = curstep->arg1;

				choose_local_fallback_retries = curstep->arg1;

			break;



		case CRUSH_RULE_CHOOSELEAF_FIRSTN:
@@ -734,8 +742,8 @@ int crush_do_rule(const struct crush_map *map, 
						o+osize, j,

						choose_tries,

						recurse_tries,

						choose_local_tries,

						choose_local_fallback_tries,

						choose_local_retries,

						choose_local_fallback_retries,

						recurse_to_leaf,

						c+osize);

				} else {

CRA Git | Maintained and supported by SUSTech CRA and CCSE