On 2019/11/12 0:45, Jeff Layton wrote:
On Mon, 2019-11-11 at 06:51 -0500, xiubli@xxxxxxxxxx wrote:
From: Xiubo Li <xiubli@xxxxxxxxxx>
For example, if we have 5 mds in the mdsmap and the states are:
m_info[5] --> [-1, 1, -1, 1, 1]
If we get a ramdon number 1, then we should get the mds index 3 as
expected, but actually we will get index 2, which the state is -1.
Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
---
fs/ceph/mdsmap.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/fs/ceph/mdsmap.c b/fs/ceph/mdsmap.c
index ce2d00da5096..2011147f76bf 100644
--- a/fs/ceph/mdsmap.c
+++ b/fs/ceph/mdsmap.c
@@ -20,7 +20,7 @@
int ceph_mdsmap_get_random_mds(struct ceph_mdsmap *m)
{
int n = 0;
- int i;
+ int i, j;
/* special case for one mds */
if (1 == m->m_num_mds && m->m_info[0].state > 0)
@@ -35,9 +35,12 @@ int ceph_mdsmap_get_random_mds(struct ceph_mdsmap *m)
/* pick */
n = prandom_u32() % n;
- for (i = 0; n > 0; i++, n--)
- while (m->m_info[i].state <= 0)
- i++;
+ for (j = 0, i = 0; i < m->m_num_mds; i++) {
+ if (m->m_info[0].state > 0)
+ j++;
+ if (j > n)
+ break;
+ }
return i;
}
Looks good. I'll merge after some testing.
Took me a minute but the crux of the issue is that the for loop
increment gets done regardless of the outcome of the while loop test.
Yeah, in another case when there has only one mds is up and if
'm_num_mds > 1', such as [-1, 1, -1], so after `n = prandom_u32() % n`,
the 'n' would always be 0. Then the 'for' and 'while' loops wouldn't
have any chance to run and the 'i' would always be 0 and be returned.
This looks nicer too. I don't think it's actually possible, but the
while loop _looks_ like it could walk off the end of the array.
From my following test cases the 'while' loop seemed won't walk off the
end of the array because the 'n > 0' in the 'for' loop would help guard it.
[1, 1, 1, -1, -1, -1]
[-1, 1, -1, -1]
...
The fix here will just skip the MDSs whose states are down and until we
have count enough MDSs in up state.
Thanks
BRs
Thanks,
