linux-ext4 - Re: [PATCH v6 10/10] ext4: make fast commit ineligible on ext4_reserve_inode

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: <20240628144711.pdvrpztjdenadg6o@quack3>
Date: Fri, 28 Jun 2024 16:47:11 +0200
From: Jan Kara <jack@...e.cz>
To: Harshad Shirwadkar <harshadshirwadkar@...il.com>
Cc: linux-ext4@...r.kernel.org, tytso@....edu, saukad@...gle.com,
	harshads@...gle.com
Subject: Re: [PATCH v6 10/10] ext4: make fast commit ineligible on
 ext4_reserve_inode_write failure

On Wed 29-05-24 01:20:03, Harshad Shirwadkar wrote:
> Fast commit by default makes every inode on which
> ext4_reserve_inode_write() is called. Thus, if that function
> fails for some reason, make the next fast commit ineligible.
> 
> Signed-off-by: Harshad Shirwadkar <harshadshirwadkar@...il.com>

Yeah, makes sense. The hunk in ext4_reserve_inode_write() will need redoing
once you fix the problem I've pointed out in patch 2 but otherwise the
patch looks good.

								Honza

> ---
>  fs/ext4/fast_commit.c       |  1 +
>  fs/ext4/fast_commit.h       |  1 +
>  fs/ext4/inode.c             | 29 ++++++++++++++++++-----------
>  include/trace/events/ext4.h |  7 +++++--
>  4 files changed, 25 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
> index 55a13d3ff681..e7cac190527c 100644
> --- a/fs/ext4/fast_commit.c
> +++ b/fs/ext4/fast_commit.c
> @@ -2291,6 +2291,7 @@ static const char * const fc_ineligible_reasons[] = {
>  	[EXT4_FC_REASON_FALLOC_RANGE] = "Falloc range op",
>  	[EXT4_FC_REASON_INODE_JOURNAL_DATA] = "Data journalling",
>  	[EXT4_FC_REASON_ENCRYPTED_FILENAME] = "Encrypted filename",
> +	[EXT4_FC_REASON_INODE_RSV_WRITE_FAIL] = "Inode reserve write failure"
>  };
>  
>  int ext4_fc_info_show(struct seq_file *seq, void *v)
> diff --git a/fs/ext4/fast_commit.h b/fs/ext4/fast_commit.h
> index 2fadb2c4780c..f7f85c3dd3af 100644
> --- a/fs/ext4/fast_commit.h
> +++ b/fs/ext4/fast_commit.h
> @@ -97,6 +97,7 @@ enum {
>  	EXT4_FC_REASON_FALLOC_RANGE,
>  	EXT4_FC_REASON_INODE_JOURNAL_DATA,
>  	EXT4_FC_REASON_ENCRYPTED_FILENAME,
> +	EXT4_FC_REASON_INODE_RSV_WRITE_FAIL,
>  	EXT4_FC_REASON_MAX
>  };
>  
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index f00408017c7a..8fd6e5637542 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -5729,20 +5729,27 @@ ext4_reserve_inode_write(handle_t *handle, struct inode *inode,
>  {
>  	int err;
>  
> -	if (unlikely(ext4_forced_shutdown(inode->i_sb)))
> -		return -EIO;
> +	if (unlikely(ext4_forced_shutdown(inode->i_sb))) {
> +		err = -EIO;
> +		goto out;
> +	}
>  
>  	err = ext4_get_inode_loc(inode, iloc);
> -	if (!err) {
> -		BUFFER_TRACE(iloc->bh, "get_write_access");
> -		err = ext4_journal_get_write_access(handle, inode->i_sb,
> -						    iloc->bh, EXT4_JTR_NONE);
> -		if (err) {
> -			brelse(iloc->bh);
> -			iloc->bh = NULL;
> -		}
> -		ext4_fc_track_inode(handle, inode);
> +	if (err)
> +		goto out;
> +
> +	BUFFER_TRACE(iloc->bh, "get_write_access");
> +	err = ext4_journal_get_write_access(handle, inode->i_sb,
> +						iloc->bh, EXT4_JTR_NONE);
> +	if (err) {
> +		brelse(iloc->bh);
> +		iloc->bh = NULL;
>  	}
> +	ext4_fc_track_inode(handle, inode);
> +out:
> +	if (err)
> +		ext4_fc_mark_ineligible(inode->i_sb,
> +			EXT4_FC_REASON_INODE_RSV_WRITE_FAIL, handle);
>  	ext4_std_error(inode->i_sb, err);
>  	return err;
>  }
> diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
> index a697f4b77162..597845d5c1e8 100644
> --- a/include/trace/events/ext4.h
> +++ b/include/trace/events/ext4.h
> @@ -105,6 +105,7 @@ TRACE_DEFINE_ENUM(EXT4_FC_REASON_RENAME_DIR);
>  TRACE_DEFINE_ENUM(EXT4_FC_REASON_FALLOC_RANGE);
>  TRACE_DEFINE_ENUM(EXT4_FC_REASON_INODE_JOURNAL_DATA);
>  TRACE_DEFINE_ENUM(EXT4_FC_REASON_ENCRYPTED_FILENAME);
> +TRACE_DEFINE_ENUM(EXT4_FC_REASON_INODE_RSV_WRITE_FAIL);
>  TRACE_DEFINE_ENUM(EXT4_FC_REASON_MAX);
>  
>  #define show_fc_reason(reason)						\
> @@ -118,7 +119,8 @@ TRACE_DEFINE_ENUM(EXT4_FC_REASON_MAX);
>  		{ EXT4_FC_REASON_RENAME_DIR,	"RENAME_DIR"},		\
>  		{ EXT4_FC_REASON_FALLOC_RANGE,	"FALLOC_RANGE"},	\
>  		{ EXT4_FC_REASON_INODE_JOURNAL_DATA,	"INODE_JOURNAL_DATA"}, \
> -		{ EXT4_FC_REASON_ENCRYPTED_FILENAME,	"ENCRYPTED_FILENAME"})
> +		{ EXT4_FC_REASON_ENCRYPTED_FILENAME,	"ENCRYPTED_FILENAME"}, \
> +		{ EXT4_FC_REASON_INODE_RSV_WRITE_FAIL,	"INODE_RSV_WRITE_FAIL"})
>  
>  TRACE_DEFINE_ENUM(CR_POWER2_ALIGNED);
>  TRACE_DEFINE_ENUM(CR_GOAL_LEN_FAST);
> @@ -2805,7 +2807,7 @@ TRACE_EVENT(ext4_fc_stats,
>  	),
>  
>  	TP_printk("dev %d,%d fc ineligible reasons:\n"
> -		  "%s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u"
> +		  "%s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u, %s:%u"
>  		  "num_commits:%lu, ineligible: %lu, numblks: %lu",
>  		  MAJOR(__entry->dev), MINOR(__entry->dev),
>  		  FC_REASON_NAME_STAT(EXT4_FC_REASON_XATTR),
> @@ -2818,6 +2820,7 @@ TRACE_EVENT(ext4_fc_stats,
>  		  FC_REASON_NAME_STAT(EXT4_FC_REASON_FALLOC_RANGE),
>  		  FC_REASON_NAME_STAT(EXT4_FC_REASON_INODE_JOURNAL_DATA),
>  		  FC_REASON_NAME_STAT(EXT4_FC_REASON_ENCRYPTED_FILENAME),
> +		  FC_REASON_NAME_STAT(EXT4_FC_REASON_INODE_RSV_WRITE_FAIL),
>  		  __entry->fc_commits, __entry->fc_ineligible_commits,
>  		  __entry->fc_numblks)
>  );
> -- 
> 2.45.1.288.g0e0cd299f1-goog
> 
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR