[patch] ext4: simplify some code in read_mmp_block()

[patch] ext4: simplify some code in read_mmp_block()

[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 <dan.carpenter@oracle.com>

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 8313ca3..736f12e 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -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;

Re: [patch] ext4: simplify some code in read_mmp_block()

[Andreas Dilger]

> On Aug 10, 2015, at 3:34 PM, Dan Carpenter <dan.carpenter@oracle.com> wrote:
> 
> 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 <dan.carpenter@oracle.com>
> 
> diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
> index 8313ca3..736f12e 100644
> --- a/fs/ext4/mmp.c
> +++ b/fs/ext4/mmp.c
> @@ -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.

> -	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;

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 1/2 v2] ext4: simplify some code in read_mmp_block()

[Dan Carpenter]

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

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;
 }
 
 /*

Re: [patch 1/2 v2] ext4: simplify some code in read_mmp_block()

[Andreas Dilger]

> 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

Re: [patch 1/2 v2] ext4: simplify some code in read_mmp_block()

[Theodore Ts'o]

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

Re: [patch 1/2 v2] ext4: simplify some code in read_mmp_block()

[Andreas Dilger]

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

[patch 2/2] ext4: silence a format string false positive

[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 <dan.carpenter@oracle.com>

diff --git a/fs/ext4/mmp.c b/fs/ext4/mmp.c
index 0880ec9..8b72634 100644
--- a/fs/ext4/mmp.c
+++ b/fs/ext4/mmp.c
@@ -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

[Theodore Ts'o]

On Fri, Aug 14, 2015 at 12:47:32PM +0300, Dan Carpenter wrote:
> 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 <dan.carpenter@oracle.com>

Thanks, applied.

						- Ted