Re: [PATCH v5 0/4] hide ->i_state behind accessors

On Sat 20-09-25 07:47:46, Mateusz Guzik wrote:

On Sat, Sep 20, 2025 at 6:31 AM Russell Haley [off-list ref] wrote:

quoted

On 9/19/25 10:49 AM, Mateusz Guzik wrote:

quoted

This is generated against:
https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git/commit/?h=vfs-6.18.inode.refcount.preliminaries

First commit message quoted verbatim with rationable + API:

[quote]
Open-coded accesses prevent asserting they are done correctly. One
obvious aspect is locking, but significantly more can checked. For
example it can be detected when the code is clearing flags which are
already missing, or is setting flags when it is illegal (e.g., I_FREEING
when ->i_count > 0).

Given the late stage of the release cycle this patchset only aims to
hide access, it does not provide any of the checks.

Consumers can be trivially converted. Suppose flags I_A and I_B are to
be handled, then:

state = inode->i_state        => state = inode_state_read(inode)
inode->i_state |= (I_A | I_B)         => inode_state_add(inode, I_A | I_B)
inode->i_state &= ~(I_A | I_B)        => inode_state_del(inode, I_A | I_B)
inode->i_state = I_A | I_B    => inode_state_set(inode, I_A | I_B)
[/quote]

Drive-by bikeshedding: s/set/replace/g

"replace" removes ambiguity with the concept of setting a bit ( |= ). An
alternative would be "set_only".

I agree _set may be ambiguous here. I was considering something like
_assign or _set_value instead.

I agree _assign might be a better option. In fact my favorite variant would
be:

inode_state_set() - setting bit in state
inode_state_clear() - clearing bit in state
inode_state_assign() - assigning value to state

But if you just rename inode_state_set() to inode_state_assign() that would
be already good.

								Honza
-- 
Jan Kara [off-list ref]
SUSE Labs, CR