| Message ID | 20191111115105.58758-1-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: fix geting random mds from mdsmap | expand |
On Mon, 2019-11-11 at 06:51 -0500, xiubli@redhat.com wrote: > From: Xiubo Li <xiubli@redhat.com> > > 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@redhat.com> > --- > 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. 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. Thanks,
On 2019/11/12 0:45, Jeff Layton wrote: > On Mon, 2019-11-11 at 06:51 -0500, xiubli@redhat.com wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> 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@redhat.com> >> --- >> 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) There is one type mistake when resolving the conflict. if (m->m_info[0].state > 0) ---> if (m->m_info[i].state > 0) Thanks BRs >> + 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. > > 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. > > Thanks,
On 2019/11/12 0:45, Jeff Layton wrote: > On Mon, 2019-11-11 at 06:51 -0500, xiubli@redhat.com wrote: >> From: Xiubo Li <xiubli@redhat.com> >> >> 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@redhat.com> >> --- >> 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,
On Tue, 2019-11-12 at 09:29 +0800, Xiubo Li wrote: > On 2019/11/12 0:45, Jeff Layton wrote: > > On Mon, 2019-11-11 at 06:51 -0500, xiubli@redhat.com wrote: > > > From: Xiubo Li <xiubli@redhat.com> > > > > > > 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@redhat.com> > > > --- > > > 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) > > There is one type mistake when resolving the conflict. > > if (m->m_info[0].state > 0) ---> if (m->m_info[i].state > 0) > Ahh yes...fixed in tree now. Thanks,
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; }