[PATCH -next 1/2] xfs: simplify if-else condition in xfs_validate_new_dalign

Zeng Heng posted 2 patches 3 years, 7 months ago
[PATCH -next 1/2] xfs: simplify if-else condition in xfs_validate_new_dalign
Posted by Zeng Heng 3 years, 7 months ago
"else" is not generally useful after a return,
so remove them which makes if condition a bit
more clear.

There is no logical changes.

Signed-off-by: Zeng Heng <zengheng4@huawei.com>
---
 fs/xfs/xfs_mount.c | 38 ++++++++++++++++++++------------------
 1 file changed, 20 insertions(+), 18 deletions(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index f10c88cee116..e8bb3c2e847e 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -300,26 +300,28 @@ xfs_validate_new_dalign(
 	"alignment check failed: sunit/swidth vs. blocksize(%d)",
 			mp->m_sb.sb_blocksize);
 		return -EINVAL;
-	} else {
-		/*
-		 * Convert the stripe unit and width to FSBs.
-		 */
-		mp->m_dalign = XFS_BB_TO_FSBT(mp, mp->m_dalign);
-		if (mp->m_dalign && (mp->m_sb.sb_agblocks % mp->m_dalign)) {
-			xfs_warn(mp,
-		"alignment check failed: sunit/swidth vs. agsize(%d)",
-				 mp->m_sb.sb_agblocks);
-			return -EINVAL;
-		} else if (mp->m_dalign) {
-			mp->m_swidth = XFS_BB_TO_FSBT(mp, mp->m_swidth);
-		} else {
-			xfs_warn(mp,
-		"alignment check failed: sunit(%d) less than bsize(%d)",
-				 mp->m_dalign, mp->m_sb.sb_blocksize);
-			return -EINVAL;
-		}
 	}
 
+	/*
+	 * Convert the stripe unit and width to FSBs.
+	 */
+	mp->m_dalign = XFS_BB_TO_FSBT(mp, mp->m_dalign);
+	if (mp->m_dalign && (mp->m_sb.sb_agblocks % mp->m_dalign)) {
+		xfs_warn(mp,
+	"alignment check failed: sunit/swidth vs. agsize(%d)",
+			mp->m_sb.sb_agblocks);
+		return -EINVAL;
+	}
+
+	if (!mp->m_dalign) {
+		xfs_warn(mp,
+	"alignment check failed: sunit(%d) less than bsize(%d)",
+			mp->m_dalign, mp->m_sb.sb_blocksize);
+		return -EINVAL;
+	}
+
+	mp->m_swidth = XFS_BB_TO_FSBT(mp, mp->m_swidth);
+
 	if (!xfs_has_dalign(mp)) {
 		xfs_warn(mp,
 "cannot change alignment: superblock does not support data alignment");
-- 
2.25.1
Re: [PATCH -next 1/2] xfs: simplify if-else condition in xfs_validate_new_dalign
Posted by Darrick J. Wong 3 years, 7 months ago
On Tue, Aug 30, 2022 at 09:39:38PM +0800, Zeng Heng wrote:
> "else" is not generally useful after a return,
> so remove them which makes if condition a bit
> more clear.
> 
> There is no logical changes.
> 
> Signed-off-by: Zeng Heng <zengheng4@huawei.com>

Yep.
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  fs/xfs/xfs_mount.c | 38 ++++++++++++++++++++------------------
>  1 file changed, 20 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index f10c88cee116..e8bb3c2e847e 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -300,26 +300,28 @@ xfs_validate_new_dalign(
>  	"alignment check failed: sunit/swidth vs. blocksize(%d)",
>  			mp->m_sb.sb_blocksize);
>  		return -EINVAL;
> -	} else {
> -		/*
> -		 * Convert the stripe unit and width to FSBs.
> -		 */
> -		mp->m_dalign = XFS_BB_TO_FSBT(mp, mp->m_dalign);
> -		if (mp->m_dalign && (mp->m_sb.sb_agblocks % mp->m_dalign)) {
> -			xfs_warn(mp,
> -		"alignment check failed: sunit/swidth vs. agsize(%d)",
> -				 mp->m_sb.sb_agblocks);
> -			return -EINVAL;
> -		} else if (mp->m_dalign) {
> -			mp->m_swidth = XFS_BB_TO_FSBT(mp, mp->m_swidth);
> -		} else {
> -			xfs_warn(mp,
> -		"alignment check failed: sunit(%d) less than bsize(%d)",
> -				 mp->m_dalign, mp->m_sb.sb_blocksize);
> -			return -EINVAL;
> -		}
>  	}
>  
> +	/*
> +	 * Convert the stripe unit and width to FSBs.
> +	 */
> +	mp->m_dalign = XFS_BB_TO_FSBT(mp, mp->m_dalign);
> +	if (mp->m_dalign && (mp->m_sb.sb_agblocks % mp->m_dalign)) {
> +		xfs_warn(mp,
> +	"alignment check failed: sunit/swidth vs. agsize(%d)",
> +			mp->m_sb.sb_agblocks);
> +		return -EINVAL;
> +	}
> +
> +	if (!mp->m_dalign) {
> +		xfs_warn(mp,
> +	"alignment check failed: sunit(%d) less than bsize(%d)",
> +			mp->m_dalign, mp->m_sb.sb_blocksize);
> +		return -EINVAL;
> +	}
> +
> +	mp->m_swidth = XFS_BB_TO_FSBT(mp, mp->m_swidth);

I think this reorganization of the if test logic is correct, but I
really wish this unit abuse ("m_swidth is a BB for short periods of time
and fsblock everywhere else") would get fixed to simplify analysis and
prevent us from stumbling over things like that some time later.

So for this change,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

But I would be very happy to see some tripping hazard removal. ;)

--D

> +
>  	if (!xfs_has_dalign(mp)) {
>  		xfs_warn(mp,
>  "cannot change alignment: superblock does not support data alignment");
> -- 
> 2.25.1
>