Thanks for your advice, We did found this problem during code review.
I will revise it according to your opinion.
-----邮件原件-----
发件人: Andreas Dilger <adilger@xxxxxxxxx>
发送时间: 2021年4月2日 15:58
收件人: changfengnan@xxxxxxxx
抄送: Theodore Y. Ts'o <tytso@xxxxxxx>; Ext4 Developers List
<linux-ext4@xxxxxxxxxxxxxxx>
主题: Re: [PATCH] ext4: fix error code in ext4_commit_super
On Apr 1, 2021, at 9:32 PM, <changfengnan@xxxxxxxx> <changfengnan@xxxxxxxx>
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@xxxxxxxx>
> 发送时间: 2021年3月29日 11:58
> 收件人: tytso@xxxxxxx; adilger.kernel@xxxxxxxxx;
> linux-ext4@xxxxxxxxxxxxxxx
>
> 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@xxxxxxxx>
> ---
> 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
