Received-SPF: pass (google.com: domain of linux-kernel+bounces-209777-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Date: Tue, 11 Jun 2024 13:40:06 +0200
From: Jan Kara <jack@suse.cz>
To: Mateusz Guzik <mjguzik@gmail.com>
Cc: Dave Chinner <david@fromorbit.com>, Jan Kara <jack@suse.cz>,
	brauner@kernel.org, viro@zeniv.linux.org.uk,
	linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org
Subject: Re: [PATCH] vfs: partially sanitize i_state zeroing on inode creation
Message-ID: <20240611114006.swwo2o7cldvp2wyy@quack3>
References: <20240611041540.495840-1-mjguzik@gmail.com>
 <20240611100222.htl43626sklgso5p@quack3>
 <kge4tzrxi2nxz7zg3j2qxgvnf4fcaywtlckgsc7d52eubvzmj4@zwmwknndha5y>
 <ZmgtaGglOL33Wkzr@dread.disaster.area>
 <q5xcdmugfoccgu2cs5n7ku6asyaslunm2tty6r757cc2jkqjnm@g6cl4rayvxcq>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <q5xcdmugfoccgu2cs5n7ku6asyaslunm2tty6r757cc2jkqjnm@g6cl4rayvxcq>

On Tue 11-06-24 13:26:45, Mateusz Guzik wrote:
> On Tue, Jun 11, 2024 at 08:56:40PM +1000, Dave Chinner wrote:
> > On Tue, Jun 11, 2024 at 12:23:59PM +0200, Mateusz Guzik wrote:
> > > I did not patch inode_init_always because it is exported and xfs uses it
> > > in 2 spots, only one of which zeroing the thing immediately after.
> > > Another one is a little more involved, it probably would not be a
> > > problem as the value is set altered later anyway, but I don't want to
> > > mess with semantics of the func if it can be easily avoided.
> > 
> > Better to move the zeroing to inode_init_always(), do the basic
> > save/restore mod to xfs_reinit_inode(), and let us XFS people worry
> > about whether inode_init_always() is the right thing to be calling
> > in their code...
> > 
> > All you'd need to do in xfs_reinit_inode() is this
> > 
> > +	unsigned long	state = inode->i_state;
> > 
> > 	.....
> > 	error = inode_init_always(mp->m_super, inode);
> > 	.....
> > +	inode->i_state = state;
> > 	.....
> > 
> > And it should behave as expected.
> > 
> 
> Ok, so what would be the logistics of submitting this?
> 
> Can I submit one patch which includes the above + i_state moved to
> inode_init_always?
> 
> Do I instead ship a two-part patchset, starting with the xfs change and
> stating it was your idea?
> 
> Something else?

Well, I'd do it as 4 patches actually:

1) xfs i_state workaround in xfs_reinit_inode()
2) add i_state zeroing to inode_init_always(), drop pointless zeroing from
VFS.
3) drop now pointless zeroing from xfs
4) drop now pointless zeroing from bcachefs

This way also respective maintainers can easily ack the bits they care about.
I can live with two as you suggest since the changes are tiny but four is
IMHO a "proper" way to do things ;).

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR