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

[liubaolin]

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, sashiko-bot@xxxxxxxxxx 写道:
> 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 <liubaolin@xxxxxxxxxx>
> 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?