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: <20240829091250.gq2cgkfubbvlysxy@quack3>
Date: Thu, 29 Aug 2024 11:12:50 +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 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>

								Honza

> ---
>  fs/ext4/ext4.h   |  2 ++
>  fs/ext4/inline.c | 10 +++++-----
>  fs/ext4/inode.c  | 24 +++++-------------------
>  3 files changed, 12 insertions(+), 24 deletions(-)
> 
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 08acd152261e..5f8257b68190 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -3851,6 +3851,8 @@ static inline int ext4_buffer_uptodate(struct buffer_head *bh)
>  	return buffer_uptodate(bh);
>  }
>  
> +extern int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
> +				  get_block_t *get_block);
>  #endif	/* __KERNEL__ */
>  
>  #define EFSBADCRC	EBADMSG		/* Bad CRC detected */
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index e7a09a99837b..0a1a8431e281 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -601,10 +601,10 @@ static int ext4_convert_inline_data_to_extent(struct address_space *mapping,
>  		goto out;
>  
>  	if (ext4_should_dioread_nolock(inode)) {
> -		ret = __block_write_begin(&folio->page, from, to,
> -					  ext4_get_block_unwritten);
> +		ret = ext4_block_write_begin(folio, from, to,
> +					     ext4_get_block_unwritten);
>  	} else
> -		ret = __block_write_begin(&folio->page, from, to, ext4_get_block);
> +		ret = ext4_block_write_begin(folio, from, to, ext4_get_block);
>  
>  	if (!ret && ext4_should_journal_data(inode)) {
>  		ret = ext4_walk_page_buffers(handle, inode,
> @@ -856,8 +856,8 @@ static int ext4_da_convert_inline_data_to_extent(struct address_space *mapping,
>  			goto out;
>  	}
>  
> -	ret = __block_write_begin(&folio->page, 0, inline_size,
> -				  ext4_da_get_block_prep);
> +	ret = ext4_block_write_begin(folio, 0, inline_size,
> +				     ext4_da_get_block_prep);
>  	if (ret) {
>  		up_read(&EXT4_I(inode)->xattr_sem);
>  		folio_unlock(folio);
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index a0a55cb8db53..4964c67e029e 100644
> --- 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.
>  	 */
> @@ -1041,9 +1041,8 @@ int do_journal_get_write_access(handle_t *handle, struct inode *inode,
>  	return ret;
>  }
>  
> -#ifdef CONFIG_FS_ENCRYPTION
> -static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
> -				  get_block_t *get_block)
> +int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
> +			   get_block_t *get_block)
>  {
>  	unsigned from = pos & (PAGE_SIZE - 1);
>  	unsigned to = from + len;
> @@ -1134,7 +1133,6 @@ static int ext4_block_write_begin(struct folio *folio, loff_t pos, unsigned len,
>  
>  	return err;
>  }
> -#endif
>  
>  /*
>   * To preserve ordering, it is essential that the hole instantiation and
> @@ -1216,19 +1214,11 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
>  	/* In case writeback began while the folio was unlocked */
>  	folio_wait_stable(folio);
>  
> -#ifdef CONFIG_FS_ENCRYPTION
>  	if (ext4_should_dioread_nolock(inode))
>  		ret = ext4_block_write_begin(folio, pos, len,
>  					     ext4_get_block_unwritten);
>  	else
>  		ret = ext4_block_write_begin(folio, pos, len, ext4_get_block);
> -#else
> -	if (ext4_should_dioread_nolock(inode))
> -		ret = __block_write_begin(&folio->page, pos, len,
> -					  ext4_get_block_unwritten);
> -	else
> -		ret = __block_write_begin(&folio->page, pos, len, ext4_get_block);
> -#endif
>  	if (!ret && ext4_should_journal_data(inode)) {
>  		ret = ext4_walk_page_buffers(handle, inode,
>  					     folio_buffers(folio), from, to,
> @@ -1241,7 +1231,7 @@ static int ext4_write_begin(struct file *file, struct address_space *mapping,
>  
>  		folio_unlock(folio);
>  		/*
> -		 * __block_write_begin may have instantiated a few blocks
> +		 * ext4_block_write_begin may have instantiated a few blocks
>  		 * outside i_size.  Trim these off again. Don't need
>  		 * i_size_read because we hold i_rwsem.
>  		 *
> @@ -2961,11 +2951,7 @@ static int ext4_da_write_begin(struct file *file, struct address_space *mapping,
>  	if (IS_ERR(folio))
>  		return PTR_ERR(folio);
>  
> -#ifdef CONFIG_FS_ENCRYPTION
>  	ret = ext4_block_write_begin(folio, pos, len, ext4_da_get_block_prep);
> -#else
> -	ret = __block_write_begin(&folio->page, pos, len, ext4_da_get_block_prep);
> -#endif
>  	if (ret < 0) {
>  		folio_unlock(folio);
>  		folio_put(folio);
> -- 
> 2.33.0
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ