ext4: simplify some code in read_mmp_block()
Subsystems (2)
PATCH 1/1 ext4: simplify some code in read_mmp_block()
My static checker complains because it is not necessary to test "if (*bh)" when we just tested "if (!*bh)" on the lines before. We can remove the condition and pull everything in one indent level.
Then we have another "if (unlikely(!*bh))" check and the only way that can be true is if the "if (!buffer_uptodate(*bh))" condition was true so we can combine them together.
fs/ext4/mmp.c +9 -12
@@ -80,18 +80,15 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh, *bh =sb_getblk (sb, mmp_block);if (!*bh)return -ENOMEM ; -if (*bh) { -get_bh (*bh); -lock_buffer (*bh); - (*bh)->b_end_io = end_buffer_read_sync; -submit_bh (READ_SYNC |REQ_META |REQ_PRIO , *bh); -wait_on_buffer (*bh); -if (!buffer_uptodate (*bh)) { -brelse (*bh); - *bh =NULL ; - } - } -if (unlikely (!*bh)) { + +get_bh (*bh); +lock_buffer (*bh); + (*bh)->b_end_io = end_buffer_read_sync; +submit_bh (READ_SYNC |REQ_META |REQ_PRIO , *bh); +wait_on_buffer (*bh); +if (!buffer_uptodate (*bh)) { +brelse (*bh); + *bh =NULL ;ext4_warning (sb,"Error while reading MMP block %llu" , mmp_block);return -EIO ;
Quoted 18 lines · Dan Carpenter
My static checker complains because it is not necessary to test "if (*bh)" when we just tested "if (!*bh)" on the lines before. We can remove the condition and pull everything in one indent level.
Then we have another "if (
unlikely(!*bh))" check and the only way that can be true is if the "if (!buffer_uptodate(*bh))" condition was true so we can combine them together.Signed-off-by Dan Carpenter <[email protected]>fs/ext4/mmp.c +0
-0
fs/ext4/mmp.c static int read_mmp_block(struct super_block *sb, struct buffer_head **bh, · line 83
Quoted 4 lines
fs/ext4/mmp.c +0
-0@@ -80,18 +80,15 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh,if (!*bh) *bh =sb_getblk (sb, mmp_block);if (!*bh)return -ENOMEM ;
It may also be clearer if the second "if (!*bh)" check were below the first one, like:
if (!*bh) {
*bh = sb_getblk(sb, mmp_block);
if (!*bh)
return -ENOMEM;
}That moves the second check out-of-line since it can only happen if the first one is already true.
That said, it looks like this second "if (!*bh)" check was added later on in commit 860d21e2 and the original logic made sense when it was written, so that the ext4_warning() message below would get printed in this case. Otherwise the mount can fail without any message being printed on the console at all. Other ext4 mount failures print error messages, so this should be done here also.
Quoted 23 lines
fs/ext4/mmp.c +9
-12-if (*bh) { -get_bh (*bh); -lock_buffer (*bh); - (*bh)->b_end_io = end_buffer_read_sync; -submit_bh (READ_SYNC |REQ_META |REQ_PRIO , *bh); -wait_on_buffer (*bh); -if (!buffer_uptodate (*bh)) { -brelse (*bh); - *bh =NULL ; - } - } -if (unlikely (!*bh)) { + +get_bh (*bh); +lock_buffer (*bh); + (*bh)->b_end_io = end_buffer_read_sync; +submit_bh (READ_SYNC |REQ_META |REQ_PRIO , *bh); +wait_on_buffer (*bh); +if (!buffer_uptodate (*bh)) { +brelse (*bh); + *bh =NULL ;ext4_warning (sb,"Error while reading MMP block %llu" , mmp_block);
Quoted 1 line
return -EIO;
Maybe it would be better to move the ext4_warning() to the end of the function and then set rc in the failure cases with a goto err: and print the message only once?
Cheers, Andreas
PATCH v2 1/2 ext4: simplify some code in read_mmp_block()
My static check complains because we have:
if (!*bh)
return -ENOMEM;
if (*bh) {The second check is unnecessary.
I've simplified this code by moving the "if (!*bh)" checks around. Also Andreas Dilger says we should probably print a warning if sb_getblk() fails.
---
v2: move the code around even more, add a warning message
fs/ext4/mmp.c +20 -16
@@ -69,6 +69,7 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh, ext4_fsblk_t mmp_block) {struct mmp_struct *mmp; +int ret;if (*bh)clear_buffer_uptodate (*bh);
@@ -76,25 +77,24 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh,/* This would be sb_bread(sb, mmp_block), except we need to be sure * that the MD RAID device cache has been bypassed, and that the read * is not blocked in the elevator. */ -if (!*bh) +if (!*bh) { *bh =sb_getblk (sb, mmp_block); -if (!*bh) -return -ENOMEM ; -if (*bh) { -get_bh (*bh); -lock_buffer (*bh); - (*bh)->b_end_io = end_buffer_read_sync; -submit_bh (READ_SYNC |REQ_META |REQ_PRIO , *bh); -wait_on_buffer (*bh); -if (!buffer_uptodate (*bh)) { -brelse (*bh); - *bh =NULL ; +if (!*bh) { + ret = -ENOMEM ; + goto warn_exit; } } -if (unlikely (!*bh)) { -ext4_warning (sb,"Error while reading MMP block %llu" , - mmp_block); -return -EIO ; + +get_bh (*bh); +lock_buffer (*bh); + (*bh)->b_end_io = end_buffer_read_sync; +submit_bh (READ_SYNC |REQ_META |REQ_PRIO , *bh); +wait_on_buffer (*bh); +if (!buffer_uptodate (*bh)) { +brelse (*bh); + *bh =NULL ; + ret = -EIO ; + goto warn_exit; } mmp = (struct mmp_struct *)((*bh)->b_data);
@@ -103,6 +103,10 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh,return -EINVAL ;return 0 ; + +warn_exit: +ext4_warning (sb,"Error while reading MMP block %llu" , mmp_block); +return ret; }/*
Quoted 9 lines · Dan Carpenter
My static check complains because we have:
if (!*bh) return -ENOMEM; if (*bh) {
Quoted 7 lines
The second check is unnecessary.
I've simplified this code by moving the "if (!*bh)" checks around. Also Andreas Dilger says we should probably print a warning if
sb_getblk()fails.Signed-off-by Dan Carpenter <[email protected]>
Quoted 57 lines
---
v2: move the code around even more, add a warning messagefs/ext4/mmp.c +16
-16@@ -69,6 +69,7 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh, ext4_fsblk_t mmp_block) {struct mmp_struct *mmp; +int ret;if (*bh)clear_buffer_uptodate (*bh);@@ -76,25 +77,24 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh,/* This would be sb_bread(sb, mmp_block), except we need to be sure * that the MD RAID device cache has been bypassed, and that the read * is not blocked in the elevator. */ -if (!*bh) +if (!*bh) { *bh =sb_getblk (sb, mmp_block); -if (!*bh) -return -ENOMEM ; -if (*bh) { -get_bh (*bh); -lock_buffer (*bh); - (*bh)->b_end_io = end_buffer_read_sync; -submit_bh (READ_SYNC |REQ_META |REQ_PRIO , *bh); -wait_on_buffer (*bh); -if (!buffer_uptodate (*bh)) { -brelse (*bh); - *bh =NULL ; +if (!*bh) { + ret = -ENOMEM ; + goto warn_exit; } } -if (unlikely (!*bh)) { -ext4_warning (sb,"Error while reading MMP block %llu" , - mmp_block); -return -EIO ; + +get_bh (*bh); +lock_buffer (*bh); + (*bh)->b_end_io = end_buffer_read_sync; +submit_bh (READ_SYNC |REQ_META |REQ_PRIO , *bh); +wait_on_buffer (*bh); +if (!buffer_uptodate (*bh)) { +brelse (*bh); + *bh =NULL ; + ret = -EIO ; + goto warn_exit; } mmp = (struct mmp_struct *)((*bh)->b_data);
fs/ext4/mmp.c static int read_mmp_block(struct super_block *sb, struct buffer_head **bh, · line 108
Quoted 6 lines
fs/ext4/mmp.c +3
-0@@ -103,6 +103,10 @@ static int read_mmp_block(struct super_block *sb, struct buffer_head **bh,return -EINVAL ;return 0 ; + +warn_exit: +ext4_warning (sb,"Error while reading MMP block %llu" , mmp_block);
It wouldn't be terrible to include the error in this message, but since it will also be returned to userspace it isn't critical.
fs/ext4/mmp.c static int read_mmp_block(struct super_block *sb, struct buffer_head **bh, · line 112
Quoted 4 lines
fs/ext4/mmp.c +1
-0+return ret; }/*
Cheers, Andreas
Theodore Ts'o
maintainerQuoted 8 lines · Andreas Dilger
Quoted 8 lines
My static check complains because we have:
if (!*bh) return -ENOMEM; if (*bh) {The second check is unnecessary.
Quoted 7 lines
Quoted 5 lines
I've simplified this code by moving the "if (!*bh)" checks around. Also Andreas Dilger says we should probably print a warning if
sb_getblk()fails.Signed-off-by Dan Carpenter <[email protected]>Reviewed-by Andreas Dilger <[email protected]>
Applied, thanks. I've changed the patch slightly to also print a warning if the MMP magic number and/or checksum for the MMP block doesn't check out, and to print the error code to disambiguate between the various failure cases.
One thing, from looking at the function --- it looks like it might be a good idea if we were to move the call to clear_buffer_uptodate() to *after* the sb_getblk() call, no?
Otherwise we don't reread the MMP block if it is already in the cache, and it is the first time read_mmp_block() is called in a function in fs/ext4/mmp.c.....
- Ted
Quoted 13 lines · Theodore Ts'o
Quoted 11 lines · Andreas Dilger
Quoted 11 lines
My static check complains because we have:
if (!*bh) return -ENOMEM; if (*bh) {The second check is unnecessary.
I've simplified this code by moving the "if (!*bh)" checks around. Also Andreas Dilger says we should probably print a warning if
sb_getblk()fails.
Quoted 8 lines
Quoted 3 lines
Quoted 1 line
Signed-off-by Dan Carpenter <[email protected]>Reviewed-by Andreas Dilger <[email protected]>Applied, thanks. I've changed the patch slightly to also print a warning if the MMP magic number and/or checksum for the MMP block doesn't check out, and to print the error code to disambiguate between the various failure cases.
I agree that is an improvement.
Quoted 7 lines
One thing, from looking at the function --- it looks like it might be a good idea if we were to move the call to
clear_buffer_uptodate()to *after* thesb_getblk()call, no?Otherwise we don't reread the MMP block if it is already in the cache, and it is the first time
read_mmp_block()is called in a function infs/ext4/mmp.c.....
Good catch. Fortunately, this didn't cause dangerous MMP operation since the MMP block is invalidated and read a second time after a delay (to catch cases of concurrent mounts) and compared to the original.
At worst the first (stale) read would result in the second read causing a false mismatch and a cause an error during mount. While not dangerous, this could also be annoying and is good to fix.
Cheers, Andreas
PATCH 2/2 ext4: silence a format string false positive
In reply to Andreas DilgerStatic checkers complain that the format string should be "%s". It does not make a difference for the current code.
fs/ext4/mmp.c +1 -1
@@ -115,7 +115,7 @@ warn_exit:void __dump_mmp_msg (struct super_block *sb,struct mmp_struct *mmp,const char *function,unsigned int line,const char *msg) { -__ext4_warning (sb, function, line, msg); +__ext4_warning (sb, function, line,"%s" , msg); __ext4_warning(sb, function, line, "MMP failure info: last update time: %llu, last update " "node: %s, last update device: %s\n",
Re: [patch 2/2] ext4: silence a format string false positive
Quoted 4 lines · Dan Carpenter
Static checkers complain that the format string should be "%s". It does not make a difference for the current code.
Signed-off-by Dan Carpenter <[email protected]>
Thanks, applied.
- Ted