All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xfs: consolidate & clarify mount sanity checks
@ 2011-05-20 21:52 Eric Sandeen
  2011-05-31 22:28 ` Alex Elder
  0 siblings, 1 reply; 3+ messages in thread
From: Eric Sandeen @ 2011-05-20 21:52 UTC (permalink / raw)
  To: xfs-oss, Pavol Gono

Pavol pointed out that there is one silent error case in the mount
path, and that others are rather uninformative.

I've taken Pavol's suggested patch and extended it a bit to also:

* fix a message which says "turned off" but actually errors out
* consolidate the vaguely differentiated "SB sanity check [12]"
  messages, and hexdump the superblock for analysis

Original-patch-by: Pavol Gono <Pavol.Gono@siemens.com>
Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
index 8f6fc1a..c13fed8 100644
--- a/fs/xfs/xfs_fs.h
+++ b/fs/xfs/xfs_fs.h
@@ -249,6 +249,11 @@ typedef struct xfs_fsop_resblks {
 #define XFS_MAX_LOG_BYTES \
 	((2 * 1024 * 1024 * 1024ULL) - XFS_MIN_LOG_BYTES)
 
+/* Used for sanity checks on superblock */
+#define XFS_MAX_DBLOCKS(s) ((xfs_drfsbno_t)(s)->sb_agcount * (s)->sb_agblocks)
+#define XFS_MIN_DBLOCKS(s) ((xfs_drfsbno_t)((s)->sb_agcount - 1) *	\
+			 (s)->sb_agblocks + XFS_MIN_AG_BLOCKS)
+
 /*
  * Structures for XFS_IOC_FSGROWFSDATA, XFS_IOC_FSGROWFSLOG & XFS_IOC_FSGROWFSRT
  */
diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index bb3f9a7..a27dda6 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -348,7 +348,7 @@ xfs_mount_validate_sb(
 	}
 
 	/*
-	 * More sanity checking. These were stolen directly from
+	 * More sanity checking.  Most of these were stolen directly from
 	 * xfs_repair.
 	 */
 	if (unlikely(
@@ -371,23 +371,13 @@ xfs_mount_validate_sb(
 	    (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)	||
 	    (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE)	||
 	    (sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)	||
-	    (sbp->sb_imax_pct > 100 /* zero sb_imax_pct is valid */))) {
+	    (sbp->sb_imax_pct > 100 /* zero sb_imax_pct is valid */)	||
+	    sbp->sb_dblocks == 0					||
+	    sbp->sb_dblocks > XFS_MAX_DBLOCKS(sbp)			||
+	    sbp->sb_dblocks < XFS_MIN_DBLOCKS(sbp))) {
 		if (loud)
-			xfs_warn(mp, "SB sanity check 1 failed");
-		return XFS_ERROR(EFSCORRUPTED);
-	}
-
-	/*
-	 * Sanity check AG count, size fields against data size field
-	 */
-	if (unlikely(
-	    sbp->sb_dblocks == 0 ||
-	    sbp->sb_dblocks >
-	     (xfs_drfsbno_t)sbp->sb_agcount * sbp->sb_agblocks ||
-	    sbp->sb_dblocks < (xfs_drfsbno_t)(sbp->sb_agcount - 1) *
-			      sbp->sb_agblocks + XFS_MIN_AG_BLOCKS)) {
-		if (loud)
-			xfs_warn(mp, "SB sanity check 2 failed");
+			XFS_CORRUPTION_ERROR("SB sanity check failed",
+				XFS_ERRLEVEL_LOW, mp, sbp);
 		return XFS_ERROR(EFSCORRUPTED);
 	}
 
@@ -864,7 +854,8 @@ xfs_update_alignment(xfs_mount_t *mp)
 		if ((BBTOB(mp->m_dalign) & mp->m_blockmask) ||
 		    (BBTOB(mp->m_swidth) & mp->m_blockmask)) {
 			if (mp->m_flags & XFS_MOUNT_RETERR) {
-				xfs_warn(mp, "alignment check 1 failed");
+				xfs_warn(mp, "alignment check failed: "
+					 "(sunit/swidth vs. blocksize)");
 				return XFS_ERROR(EINVAL);
 			}
 			mp->m_dalign = mp->m_swidth = 0;
@@ -875,6 +866,8 @@ xfs_update_alignment(xfs_mount_t *mp)
 			mp->m_dalign = XFS_BB_TO_FSBT(mp, mp->m_dalign);
 			if (mp->m_dalign && (sbp->sb_agblocks % mp->m_dalign)) {
 				if (mp->m_flags & XFS_MOUNT_RETERR) {
+					xfs_warn(mp, "alignment check failed: "
+					 	 "(sunit/swidth vs. ag size)");
 					return XFS_ERROR(EINVAL);
 				}
 				xfs_warn(mp,
@@ -889,8 +882,8 @@ xfs_update_alignment(xfs_mount_t *mp)
 				mp->m_swidth = XFS_BB_TO_FSBT(mp, mp->m_swidth);
 			} else {
 				if (mp->m_flags & XFS_MOUNT_RETERR) {
-					xfs_warn(mp,
-		"stripe alignment turned off: sunit(%d) less than bsize(%d)",
+					xfs_warn(mp, "alignment check failed: "
+						"sunit(%d) less than bsize(%d)",
 						mp->m_dalign,
 						mp->m_blockmask +1);
 					return XFS_ERROR(EINVAL);

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] xfs: consolidate & clarify mount sanity checks
  2011-05-20 21:52 [PATCH] xfs: consolidate & clarify mount sanity checks Eric Sandeen
@ 2011-05-31 22:28 ` Alex Elder
  2011-06-01  1:43   ` Eric Sandeen
  0 siblings, 1 reply; 3+ messages in thread
From: Alex Elder @ 2011-05-31 22:28 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Pavol Gono, xfs-oss

On Fri, 2011-05-20 at 16:52 -0500, Eric Sandeen wrote:
> Pavol pointed out that there is one silent error case in the mount
> path, and that others are rather uninformative.
> 
> I've taken Pavol's suggested patch and extended it a bit to also:
> 
> * fix a message which says "turned off" but actually errors out
> * consolidate the vaguely differentiated "SB sanity check [12]"
>   messages, and hexdump the superblock for analysis
> 
> Original-patch-by: Pavol Gono <Pavol.Gono@siemens.com>
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>

Looks good to me.  Based on Linus' message about the 3.0 release,
I'll hold this for 3.1 unless you really feel strongly it belongs
in 3.0.

Reviewed-by: Alex Elder <aelder@sgi.com>

> ---
> 
> diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
> index 8f6fc1a..c13fed8 100644
> --- a/fs/xfs/xfs_fs.h
> +++ b/fs/xfs/xfs_fs.h
> @@ -249,6 +249,11 @@ typedef struct xfs_fsop_resblks {
>  #define XFS_MAX_LOG_BYTES \
>  	((2 * 1024 * 1024 * 1024ULL) - XFS_MIN_LOG_BYTES)
>  
> +/* Used for sanity checks on superblock */
> +#define XFS_MAX_DBLOCKS(s) ((xfs_drfsbno_t)(s)->sb_agcount * (s)->sb_agblocks)
> +#define XFS_MIN_DBLOCKS(s) ((xfs_drfsbno_t)((s)->sb_agcount - 1) *	\
> +			 (s)->sb_agblocks + XFS_MIN_AG_BLOCKS)
> +
>  /*
>   * Structures for XFS_IOC_FSGROWFSDATA, XFS_IOC_FSGROWFSLOG & XFS_IOC_FSGROWFSRT
>   */
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index bb3f9a7..a27dda6 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -348,7 +348,7 @@ xfs_mount_validate_sb(
>  	}
>  
>  	/*
> -	 * More sanity checking. These were stolen directly from
> +	 * More sanity checking.  Most of these were stolen directly from
>  	 * xfs_repair.
>  	 */
>  	if (unlikely(
> @@ -371,23 +371,13 @@ xfs_mount_validate_sb(
>  	    (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)	||
>  	    (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE)	||
>  	    (sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)	||
> -	    (sbp->sb_imax_pct > 100 /* zero sb_imax_pct is valid */))) {
> +	    (sbp->sb_imax_pct > 100 /* zero sb_imax_pct is valid */)	||
> +	    sbp->sb_dblocks == 0					||
> +	    sbp->sb_dblocks > XFS_MAX_DBLOCKS(sbp)			||
> +	    sbp->sb_dblocks < XFS_MIN_DBLOCKS(sbp))) {
>  		if (loud)
> -			xfs_warn(mp, "SB sanity check 1 failed");
> -		return XFS_ERROR(EFSCORRUPTED);
> -	}
> -
> -	/*
> -	 * Sanity check AG count, size fields against data size field
> -	 */
> -	if (unlikely(
> -	    sbp->sb_dblocks == 0 ||
> -	    sbp->sb_dblocks >
> -	     (xfs_drfsbno_t)sbp->sb_agcount * sbp->sb_agblocks ||
> -	    sbp->sb_dblocks < (xfs_drfsbno_t)(sbp->sb_agcount - 1) *
> -			      sbp->sb_agblocks + XFS_MIN_AG_BLOCKS)) {
> -		if (loud)
> -			xfs_warn(mp, "SB sanity check 2 failed");
> +			XFS_CORRUPTION_ERROR("SB sanity check failed",
> +				XFS_ERRLEVEL_LOW, mp, sbp);
>  		return XFS_ERROR(EFSCORRUPTED);
>  	}
>  
> @@ -864,7 +854,8 @@ xfs_update_alignment(xfs_mount_t *mp)
>  		if ((BBTOB(mp->m_dalign) & mp->m_blockmask) ||
>  		    (BBTOB(mp->m_swidth) & mp->m_blockmask)) {
>  			if (mp->m_flags & XFS_MOUNT_RETERR) {
> -				xfs_warn(mp, "alignment check 1 failed");
> +				xfs_warn(mp, "alignment check failed: "
> +					 "(sunit/swidth vs. blocksize)");
>  				return XFS_ERROR(EINVAL);
>  			}
>  			mp->m_dalign = mp->m_swidth = 0;
> @@ -875,6 +866,8 @@ xfs_update_alignment(xfs_mount_t *mp)
>  			mp->m_dalign = XFS_BB_TO_FSBT(mp, mp->m_dalign);
>  			if (mp->m_dalign && (sbp->sb_agblocks % mp->m_dalign)) {
>  				if (mp->m_flags & XFS_MOUNT_RETERR) {
> +					xfs_warn(mp, "alignment check failed: "
> +					 	 "(sunit/swidth vs. ag size)");
>  					return XFS_ERROR(EINVAL);
>  				}
>  				xfs_warn(mp,
> @@ -889,8 +882,8 @@ xfs_update_alignment(xfs_mount_t *mp)
>  				mp->m_swidth = XFS_BB_TO_FSBT(mp, mp->m_swidth);
>  			} else {
>  				if (mp->m_flags & XFS_MOUNT_RETERR) {
> -					xfs_warn(mp,
> -		"stripe alignment turned off: sunit(%d) less than bsize(%d)",
> +					xfs_warn(mp, "alignment check failed: "
> +						"sunit(%d) less than bsize(%d)",
>  						mp->m_dalign,
>  						mp->m_blockmask +1);
>  					return XFS_ERROR(EINVAL);
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs



_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] xfs: consolidate & clarify mount sanity checks
  2011-05-31 22:28 ` Alex Elder
@ 2011-06-01  1:43   ` Eric Sandeen
  0 siblings, 0 replies; 3+ messages in thread
From: Eric Sandeen @ 2011-06-01  1:43 UTC (permalink / raw)
  To: aelder; +Cc: Pavol Gono, xfs-oss

On 5/31/11 5:28 PM, Alex Elder wrote:
> On Fri, 2011-05-20 at 16:52 -0500, Eric Sandeen wrote:
>> Pavol pointed out that there is one silent error case in the mount
>> path, and that others are rather uninformative.
>>
>> I've taken Pavol's suggested patch and extended it a bit to also:
>>
>> * fix a message which says "turned off" but actually errors out
>> * consolidate the vaguely differentiated "SB sanity check [12]"
>>   messages, and hexdump the superblock for analysis
>>
>> Original-patch-by: Pavol Gono <Pavol.Gono@siemens.com>
>> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> 
> Looks good to me.  Based on Linus' message about the 3.0 release,
> I'll hold this for 3.1 unless you really feel strongly it belongs
> in 3.0.

Fine by me, it's been this way ~forever.

> Reviewed-by: Alex Elder <aelder@sgi.com>

Thanks!

-Eric

>> ---
>>
>> diff --git a/fs/xfs/xfs_fs.h b/fs/xfs/xfs_fs.h
>> index 8f6fc1a..c13fed8 100644
>> --- a/fs/xfs/xfs_fs.h
>> +++ b/fs/xfs/xfs_fs.h
>> @@ -249,6 +249,11 @@ typedef struct xfs_fsop_resblks {
>>  #define XFS_MAX_LOG_BYTES \
>>  	((2 * 1024 * 1024 * 1024ULL) - XFS_MIN_LOG_BYTES)
>>  
>> +/* Used for sanity checks on superblock */
>> +#define XFS_MAX_DBLOCKS(s) ((xfs_drfsbno_t)(s)->sb_agcount * (s)->sb_agblocks)
>> +#define XFS_MIN_DBLOCKS(s) ((xfs_drfsbno_t)((s)->sb_agcount - 1) *	\
>> +			 (s)->sb_agblocks + XFS_MIN_AG_BLOCKS)
>> +
>>  /*
>>   * Structures for XFS_IOC_FSGROWFSDATA, XFS_IOC_FSGROWFSLOG & XFS_IOC_FSGROWFSRT
>>   */
>> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
>> index bb3f9a7..a27dda6 100644
>> --- a/fs/xfs/xfs_mount.c
>> +++ b/fs/xfs/xfs_mount.c
>> @@ -348,7 +348,7 @@ xfs_mount_validate_sb(
>>  	}
>>  
>>  	/*
>> -	 * More sanity checking. These were stolen directly from
>> +	 * More sanity checking.  Most of these were stolen directly from
>>  	 * xfs_repair.
>>  	 */
>>  	if (unlikely(
>> @@ -371,23 +371,13 @@ xfs_mount_validate_sb(
>>  	    (sbp->sb_blocklog - sbp->sb_inodelog != sbp->sb_inopblog)	||
>>  	    (sbp->sb_rextsize * sbp->sb_blocksize > XFS_MAX_RTEXTSIZE)	||
>>  	    (sbp->sb_rextsize * sbp->sb_blocksize < XFS_MIN_RTEXTSIZE)	||
>> -	    (sbp->sb_imax_pct > 100 /* zero sb_imax_pct is valid */))) {
>> +	    (sbp->sb_imax_pct > 100 /* zero sb_imax_pct is valid */)	||
>> +	    sbp->sb_dblocks == 0					||
>> +	    sbp->sb_dblocks > XFS_MAX_DBLOCKS(sbp)			||
>> +	    sbp->sb_dblocks < XFS_MIN_DBLOCKS(sbp))) {
>>  		if (loud)
>> -			xfs_warn(mp, "SB sanity check 1 failed");
>> -		return XFS_ERROR(EFSCORRUPTED);
>> -	}
>> -
>> -	/*
>> -	 * Sanity check AG count, size fields against data size field
>> -	 */
>> -	if (unlikely(
>> -	    sbp->sb_dblocks == 0 ||
>> -	    sbp->sb_dblocks >
>> -	     (xfs_drfsbno_t)sbp->sb_agcount * sbp->sb_agblocks ||
>> -	    sbp->sb_dblocks < (xfs_drfsbno_t)(sbp->sb_agcount - 1) *
>> -			      sbp->sb_agblocks + XFS_MIN_AG_BLOCKS)) {
>> -		if (loud)
>> -			xfs_warn(mp, "SB sanity check 2 failed");
>> +			XFS_CORRUPTION_ERROR("SB sanity check failed",
>> +				XFS_ERRLEVEL_LOW, mp, sbp);
>>  		return XFS_ERROR(EFSCORRUPTED);
>>  	}
>>  
>> @@ -864,7 +854,8 @@ xfs_update_alignment(xfs_mount_t *mp)
>>  		if ((BBTOB(mp->m_dalign) & mp->m_blockmask) ||
>>  		    (BBTOB(mp->m_swidth) & mp->m_blockmask)) {
>>  			if (mp->m_flags & XFS_MOUNT_RETERR) {
>> -				xfs_warn(mp, "alignment check 1 failed");
>> +				xfs_warn(mp, "alignment check failed: "
>> +					 "(sunit/swidth vs. blocksize)");
>>  				return XFS_ERROR(EINVAL);
>>  			}
>>  			mp->m_dalign = mp->m_swidth = 0;
>> @@ -875,6 +866,8 @@ xfs_update_alignment(xfs_mount_t *mp)
>>  			mp->m_dalign = XFS_BB_TO_FSBT(mp, mp->m_dalign);
>>  			if (mp->m_dalign && (sbp->sb_agblocks % mp->m_dalign)) {
>>  				if (mp->m_flags & XFS_MOUNT_RETERR) {
>> +					xfs_warn(mp, "alignment check failed: "
>> +					 	 "(sunit/swidth vs. ag size)");
>>  					return XFS_ERROR(EINVAL);
>>  				}
>>  				xfs_warn(mp,
>> @@ -889,8 +882,8 @@ xfs_update_alignment(xfs_mount_t *mp)
>>  				mp->m_swidth = XFS_BB_TO_FSBT(mp, mp->m_swidth);
>>  			} else {
>>  				if (mp->m_flags & XFS_MOUNT_RETERR) {
>> -					xfs_warn(mp,
>> -		"stripe alignment turned off: sunit(%d) less than bsize(%d)",
>> +					xfs_warn(mp, "alignment check failed: "
>> +						"sunit(%d) less than bsize(%d)",
>>  						mp->m_dalign,
>>  						mp->m_blockmask +1);
>>  					return XFS_ERROR(EINVAL);
>>
>> _______________________________________________
>> xfs mailing list
>> xfs@oss.sgi.com
>> http://oss.sgi.com/mailman/listinfo/xfs
> 
> 
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2011-06-01  1:43 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-05-20 21:52 [PATCH] xfs: consolidate & clarify mount sanity checks Eric Sandeen
2011-05-31 22:28 ` Alex Elder
2011-06-01  1:43   ` Eric Sandeen

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.