Re: [PATCH v4 v4 2/3] ext4: use atomic operations for s_mb_stats accesses

Dear All,
    Sashiko AI reviewed the patch and provided two points. I will take 
these into consideration and submit a revised version for v5 as soon as 
possible.

Thanks,
Baolin

在 2026/5/9 04:02, [email protected] 写道:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] Architectural Anti-pattern: Using `atomic_t` for a simple configuration flag
> --
> 
> commit 4ef770887d49b5db285ae4848dba912e51abd333
> Author: Baolin Liu <[email protected]>
> ext4: use atomic operations for s_mb_stats accesses
> 
> This commit converts the s_mb_stats configuration flag in ext4 from an
> unsigned int to an atomic_t to fix data races between sysfs updates and
> mballoc reads.
> 
>> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
>> index 94283a991e5c4..4fb19fcb1f5f3 100644
>> --- a/fs/ext4/ext4.h
>> +++ b/fs/ext4/ext4.h
> [ ... ]
>> @@ -1622,7 +1622,7 @@ struct ext4_sb_info {
>>   	unsigned int s_mb_stream_request;
>>   	unsigned int s_mb_max_to_scan;
>>   	unsigned int s_mb_min_to_scan;
>> -	unsigned int s_mb_stats;
>> +	atomic_t s_mb_stats;
> 
> Since s_mb_stats is only loaded and stored concurrently without any
> read-modify-write operations (like atomic_inc), would it be better to retain
> the unsigned int type and use READ_ONCE() and WRITE_ONCE() instead? Using
> atomic_t for simple configuration flags can add unnecessary conceptual
> overhead.
> 
>> diff --git a/fs/ext4/sysfs.c b/fs/ext4/sysfs.c
>> index 923b375e017fa..fad2819f36319 100644
>> --- a/fs/ext4/sysfs.c
>> +++ b/fs/ext4/sysfs.c
> [ ... ]
>> @@ -540,6 +541,12 @@ static ssize_t ext4_generic_attr_store(struct ext4_attr *a,
>>   			return ret;
>>   		*((unsigned long *) ptr) = lt;
>>   		return len;
>> +	case attr_pointer_atomic:
>> +		ret = kstrtoint(skip_spaces(buf), 0, &i);
>> +		if (ret)
>> +			return ret;
>> +		atomic_set((atomic_t *)ptr, i);
>> +		return len;
>>   	}
>>   	return 0;
> 
> Does migrating from attr_pointer_ui (which uses kstrtouint) to
> attr_pointer_atomic unintentionally allow logically invalid negative
> values to be written to s_mb_stats, since kstrtoint accepts negative numbers?
>