On 2019/6/10 上午9:23, Su Yue wrote:
>
>
> On 2019/6/6 7:06 PM, Qu Wenruo wrote:
>> [BUG]
>> When there are over 32 (in my example, 35) online CPUs, btrfs-image -c9
>> will just hang.
>>
>> [CAUSE]
>> Btrfs-image has a hard coded limit (32) on how many threads we can use.
>> For the "-t" option we do the up limit check.
>>
>> But when we don't specify "-t" option and speicified "-c" option, then
>> btrfs-image will try to auto detect the number of online CPUs, and use
>> it without checking if it's over the up limit.
>>
>> And for num_threads larger than the up limit, we will over write the
>> adjust members of metadump_struct/mdrestore_struct, corrupting
>> pthread_mutex_t and pthread_cond_t, causing synchronising problem.
>>
>> Nowadays, with SMT/HT and higher cpu core counts, it's not hard to go
>> beyond 32 threads, and hit the bug.
>>
>> [FIX]
>> Just do extra num_threads check before using the number from sysconf().
>>
>> Signed-off-by: Qu Wenruo <wqu@xxxxxxxx>
>
> This does fix an issue.
> And as the commit says, why limit the max threads to 32?
That's completely due to the hard coded metadump_struct.
We can switch to dynamically allocated pthread_t. Shouldn't be that hard
to convert.
> Does it still make sense in nowadays multiple cores CPU?
Well, thanks to the slow improvement caused by monopoly (cough, cough,
Intel), after one decade we're finally getting mainstream
16core/32threads CPUs in this year.
Personally speaking 32 threads should be already good enough for such a
less-common used tools.
So I'd prefer to keep the hard-coded limit for a while.
Thanks,
Qu
> Can we increase the limit?
> However, this is another story.
>
> For this patch:
> Reviewed-by: Su Yue <Damenly_Su@xxxxxxx>
>
>> ---
>> image/main.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/image/main.c b/image/main.c
>> index fb9fc48c..80f09c21 100644
>> --- a/image/main.c
>> +++ b/image/main.c
>> @@ -2758,6 +2758,7 @@ int main(int argc, char *argv[])
>>
>> if (tmp <= 0)
>> tmp = 1;
>> + tmp = min_t(long, tmp, MAX_WORKER_THREADS);
>> num_threads = tmp;
>> }
>> } else {
>>
>
>
Attachment:
signature.asc
Description: OpenPGP digital signature
