| Message ID | 20150814094701.GA14179@mwanda |
|---|---|
| State | Accepted, archived |
| Headers | show |
> On Aug 14, 2015, at 3:47 AM, Dan Carpenter <dan.carpenter@oracle.com> wrote: > > 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. > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Reviewed-by: Andreas Dilger <adilger@dilger.ca> > --- > v2: move the code around even more, add a warning message > > diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c > index 8313ca3..0880ec9 100644 > --- a/fs/ext4/mmp.c > +++ b/fs/ext4/mmp.c > @@ -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); 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. > + return ret; > } > > /* Cheers, Andreas -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Aug 14, 2015 at 11:03:46AM -0600, Andreas Dilger wrote: > > 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. > > > > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > > Reviewed-by: Andreas Dilger <adilger@dilger.ca> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Aug 15, 2015, at 9:37 AM, Theodore Ts'o <tytso@mit.edu> wrote: > > On Fri, Aug 14, 2015 at 11:03:46AM -0600, Andreas Dilger wrote: >>> 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. >>> >>> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> >> >> Reviewed-by: Andreas Dilger <adilger@dilger.ca> > > 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. > 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..... 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 -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c index 8313ca3..0880ec9 100644 --- a/fs/ext4/mmp.c +++ b/fs/ext4/mmp.c @@ -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; } /*
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. Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- v2: move the code around even more, add a warning message -- To unsubscribe from this list: send the line "unsubscribe linux-ext4" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html