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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



> On Aug 14, 2015, at 3:47 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> 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@xxxxxxxxxx>

Reviewed-by: Andreas Dilger <adilger@xxxxxxxxx>

> ---
> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Reiser Filesystem Development]     [Ceph FS]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite National Park]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Device Mapper]     [Linux Media]

  Powered by Linux