On Thu 08-08-24 16:50:42, Christian Brauner wrote: > On Thu, Aug 08, 2024 at 07:35:05AM GMT, Darrick J. Wong wrote: > > On Thu, Aug 08, 2024 at 01:47:20PM +0200, Amir Goldstein wrote: > > > On Wed, Aug 7, 2024 at 8:31 PM Jan Kara <jack@xxxxxxx> wrote: > > > > > > > > Now that old constants are gone, remove the unnecessary underscore from > > > > the new _SB_I_ constants. Pure mechanical replacement, no functional > > > > changes. > > > > > > > > > > This is a potential backporting bomb. > > > It is true that code using the old constant names with new macros > > > will not build on stable kernels, but I think this is still asking for trouble. > > > > > > Also, it is a bit strange that SB_* flags are bit masks and SB_I_* > > > flags are bit numbers. > > > How about leaving the underscore and using sb_*_iflag() macros to add > > > the underscore? > > > > Or append _BIT to the new names, as is sometimes done elsewhere in the > > kernel? > > > > #define SB_I_VERSION_BIT 23 > > Yeah, that's better (Fwiw, SB_I_VERSION is confusingly not an > sb->i_flags. I complained about this when it was added.). > > I don't want to end up with the same confusion that we have for > __I_NEW/I_NEW and __I_SYNC/I_SYNC which trips me up every so often when > I read that code. OK, _BIT suffix sounds nice. > So t probably wouldn't be the worst if we had: > > #define SB_I_NODEV_BIT 3 > #define SB_I_NODEV BIT(SB_I_NODEV_BIT) > > so filesystems that raise that flag when they're initialized can do: > > sb->i_flags |= SB_I_NODEV; > > and not pointlessly make them do: > > sb->i_flags |= 1 << SB_I_NODEV_BIT; Well, all sb->i_flags modifications should be using sb_set_iflag() / sb_clear_iflag(). I know it is unnecessarily more expensive in some cases but none of those paths is really that performance sensitive. The only (three) places where we have expression like 1 << SB_I_<foo>_BIT are there because the flags are also used for fc->s_iflags. I think that keeping SB_I_NODEV around together with SB_I_NODEV_BIT makes it easier to write code like sb->i_flags |= val without thinking twice and the three callsites that would be simplified are not really worth it. But if someone feels strongly about this, I can live with it. Honza -- Jan Kara <jack@xxxxxxxx> SUSE Labs, CR
