On Apr 1, 2021, at 9:32 PM, <changfengnan@vivo.com> <changfengnan@vivo.com> wrote:
> 
> Is there any problem with this patch? I did not see a reply, please let me
> know if there is a problem. Thanks

It's only been a few days since the patch was first posted, so nobody has
had a chance to review it yet.  The patch looks "obviously correct" at
first glance, but often changing code is not as obvious as first expected.
Also, is "-EINVAL" the best code here?  For "block_device_ejected()" it
might be more clear to return "-ENODEV" so that it prints "No such device"
instead of "Invalid argument" in userspace.

> 发件人: Fengnan Chang <changfengnan@vivo.com>
> 发送时间: 2021年3月29日 11:58
> 收件人: tytso@mit.edu; adilger.kernel@dilger.ca; linux-ext4@vger.kernel.org
> 
> We should set the error code when ext4_commit_super check argument failed.

It would be useful if this also described how this problem was hit, or
what caused this issue to be seen/fixed.  "Found while reviewing code"
is OK, if that is the case, or "crashed during mount when ejecting a
floppy disk", or whatever.  That makes it more clear how important the
bug fix is to be landed and backported.

It would also be useful to include a "Fixes:" label to show which patch
caused the problem, and help decide which stable kernels need this patch.
From running "git blame fs/ext4/super.c" appears that commit 2d01ddc86606
("ext4: save error info to sb through journal if available") introduced
the problem, but it seems like that patch only copied it from older code.
However, further digging shows commit c4be0c1dc4cdc ("filesystem freeze:
add error handling of write_super_lockfs/unlockfs") added this particular
code (error = 0; return error).  Before that time, no error was returned
from this function at all, so the commit message should include:

Fixes: c4be0c1dc4cdc ("filesystem freeze: add error handling of write_super_lockfs/unlockfs")

Cheers, Andreas

> Signed-off-by: Fengnan Chang <changfengnan@vivo.com>
> ---
> fs/ext4/super.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c index
> 03373471131c..5440b8ff86a8 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -5501,7 +5501,7 @@ static int ext4_commit_super(struct super_block *sb,
> int sync)
> 	int error = 0;
> 
> 	if (!sbh || block_device_ejected(sb))
> -		return error;
> +		return -EINVAL;
> 
> 	/*
> 	 * If the file system is mounted read-only, don't update the
> --
> 2.29.0
> 
> 
> 


Cheers, Andreas