linux-ext4 - Re: [PATCH 2/3] ext4: hoist ext4_block_write_begin and replace the __block_write

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20240829092631.oa2fxpfs3ok6uzbw@quack3>
Date: Thu, 29 Aug 2024 11:26:31 +0200
From: Jan Kara <jack@...e.cz>
To: zhangshida <starzhangzsd@...il.com>
Cc: tytso@....edu, adilger.kernel@...ger.ca, jack@...e.com,
	ebiggers@...nel.org, linux-ext4@...r.kernel.org,
	linux-kernel@...r.kernel.org, zhangshida@...inos.cn,
	Jan Kara <jack@...e.cz>
Subject: Re: [PATCH 2/3] ext4: hoist ext4_block_write_begin and replace the
 __block_write_begin

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@...inos.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@...e.cz>
> > Suggested-by: Eric Biggers <ebiggers@...nel.org>
> > Signed-off-by: Shida Zhang <zhangshida@...inos.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@...e.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@...e.com>
SUSE Labs, CR