ceph: fix geting random mds from mdsmap
diff mbox series

Message ID 20191111115105.58758-1-xiubli@redhat.com
State New
Headers show
Series
  • ceph: fix geting random mds from mdsmap
Related show

Commit Message

Xiubo Li Nov. 11, 2019, 11:51 a.m. UTC
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(-)

Comments

Jeff Layton Nov. 11, 2019, 4:45 p.m. UTC | #1
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,
Xiubo Li Nov. 12, 2019, 1:29 a.m. UTC | #2
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,
Xiubo Li Nov. 12, 2019, 2:13 a.m. UTC | #3
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,
Jeff Layton Nov. 12, 2019, 11:07 a.m. UTC | #4
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,

Patch
diff mbox series

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;
 }