Re: [PATCH 2/3] ext4: hoist ext4_block_write_begin and replace the __block_write_begin

[Jan Kara <jack@suse.cz>]

On Thu 29-08-24 11:12:50, Jan Kara wrote:
> On Thu 29-08-24 16:54:06, zhangshida wrote:
> > From: Shida Zhang <zhangshida@kylinos.cn>
> > 
> > Using __block_write_begin() make it inconvenient to journal the
> > user data dirty process. We can't tell the block layer maintainer,
> > ‘Hey, we want to trace the dirty user data in ext4, can we add some
> > special code for ext4 in __block_write_begin?’:P
> > 
> > So use ext4_block_write_begin() instead.
> > 
> > The two functions are basically doing the same thing except for the
> > fscrypt related code. Remove the unnecessary #ifdef since
> > fscrypt_inode_uses_fs_layer_crypto() returns false (and it's known at
> > compile time) when !CONFIG_FS_ENCRYPTION.
> > 
> > And hoist the ext4_block_write_begin so that it can be used in other
> > files.
> > 
> > Suggested-by: Jan Kara <jack@suse.cz>
> > Suggested-by: Eric Biggers <ebiggers@kernel.org>
> > Signed-off-by: Shida Zhang <zhangshida@kylinos.cn>
> 
> I think I've given my Reviewed-by on this already in previous version - you
> can keep those tags unless the patch significantly changes. Anyway: feel
> free to add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>

I've realized the patch slightly changed so that's likely why you've
dropped the Reviewed-by so I retract my comment :)

I've also realized one thing:

> > --- a/fs/ext4/inode.c
> > +++ b/fs/ext4/inode.c
> > @@ -1024,10 +1024,10 @@ int do_journal_get_write_access(handle_t *handle, struct inode *inode,
> >  	if (!buffer_mapped(bh) || buffer_freed(bh))
> >  		return 0;
> >  	/*
> > -	 * __block_write_begin() could have dirtied some buffers. Clean
> > +	 * ext4_block_write_begin() could have dirtied some buffers. Clean
> >  	 * the dirty bit as jbd2_journal_get_write_access() could complain
> >  	 * otherwise about fs integrity issues. Setting of the dirty bit
> > -	 * by __block_write_begin() isn't a real problem here as we clear
> > +	 * by ext4_block_write_begin() isn't a real problem here as we clear
> >  	 * the bit before releasing a page lock and thus writeback cannot
> >  	 * ever write the buffer.
> >  	 */

This comment and the special buffer_dirty() handling in this function can
be removed after patch 3 of this series. Nice additional cleanup :).

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