Re: [PATCH 2/3] ext4: hoist ext4_block_write_begin and replace the __block_write_begin
From: Jan Kara
Date: Thu Aug 29 2024 - 05:27:55 EST
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@xxxxxxxxxx>
> >
> > 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@xxxxxxx>
> > Suggested-by: Eric Biggers <ebiggers@xxxxxxxxxx>
> > Signed-off-by: Shida Zhang <zhangshida@xxxxxxxxxx>
>
> 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@xxxxxxx>
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@xxxxxxxx>
SUSE Labs, CR