Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 21 Nov 2022 22:34:59 -0500
From:   Eric Whitney <enwlinux@gmail.com>
To:     Ye Bin <yebin@huaweicloud.com>
Cc:     tytso@mit.edu, adilger.kernel@dilger.ca,
        linux-ext4@vger.kernel.org, linux-kernel@vger.kernel.org,
        jack@suse.cz, Ye Bin <yebin10@huawei.com>
Subject: Re: [PATCH v2 2/3] ext4: record error when detect abnormal
 'i_reserved_data_blocks'
Message-ID: <Y3xDY4GVAF+eKAqY@debian-BULLSEYE-live-builder-AMD64>
References: <20221121121434.1061725-1-yebin@huaweicloud.com>
 <20221121121434.1061725-3-yebin@huaweicloud.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221121121434.1061725-3-yebin@huaweicloud.com>
Precedence: bulk

* Ye Bin <yebin@huaweicloud.com>:
> From: Ye Bin <yebin10@huawei.com>
> 
> If 'i_reserved_data_blocks' is not cleared which mean something wrong with
> code, free space accounting is likely wrong, according to Jan Kara's advice
> use ext4_error() to record this abnormal let fsck to repair and also we can
> capture this issue.
> 
> Signed-off-by: Ye Bin <yebin10@huawei.com>
> ---
>  fs/ext4/super.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 0690e2e0b74d..3d30007502a4 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1387,10 +1387,9 @@ static void ext4_destroy_inode(struct inode *inode)
>  	}
>  
>  	if (EXT4_I(inode)->i_reserved_data_blocks)
> -		ext4_msg(inode->i_sb, KERN_ERR,

Per the coding standard, IIRC, the string should not be split across lines
for "greppability", so it should remain as is.  It's always good to run
checkpatch to catch stuff like this.

> -			 "Inode %lu (%p): i_reserved_data_blocks (%u) not cleared!",
> -			 inode->i_ino, EXT4_I(inode),
> -			 EXT4_I(inode)->i_reserved_data_blocks);
> +		ext4_error(inode->i_sb, "Inode %lu (%p) i_reserved_data_blocks"
> +			   " (%u) not cleared!", inode->i_ino, EXT4_I(inode),
> +			   EXT4_I(inode)->i_reserved_data_blocks);
>  }
>  
>  static void init_once(void *foo)

This is an improvement over the first version.  If i_reserved_data_blocks is
non-zero, something is definitely broken, but it's perhaps less likely to
indicate file system damage than if it hits zero while there are still
outstanding delayed blocks (handled well elsewhere).  So, it's not clear
we need to escalate handling this case but it doesn't hurt, either.

Eric

> -- 
> 2.31.1
>