[PATCH] xfs: return -EINVAL instead of -EUCLEAN when mounting non-xfs

[PATCH] xfs: return -EINVAL instead of -EUCLEAN when mounting non-xfs

[Sergei Trofimovich]

It fixes boot panic when trying to boot from btrfs filesystem.
kernel tries to mount as xfs and gets fatal -EUCLEAN:

[    0.170000] VFS: Cannot open root device "ubda" or unknown-block(98,0): error -117
[    0.170000] Please append a correct "root=" boot option; here are the available partitions:
[    0.170000] 6200         1048576 ubda  driver: uml-blkdev
[    0.170000] Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(98,0)

init/do_mounts.c expects only -EINVAL as 'retry another' option.
Fixes regression introduced by commit 98021821a502db347bd9c7671beeee6e8ce07ea6

Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
CC: Ben Myers <bpm@sgi.com>
CC: Alex Elder <elder@kernel.org>
CC: xfs@oss.sgi.com
CC: linux-kernel@vger.kernel.org
CC: Dave Chinner <dchinner@redhat.com>
CC: Phil White <pwhite@sgi.com>
---
 fs/xfs/xfs_mount.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
index da50846..379cac1 100644
--- a/fs/xfs/xfs_mount.c
+++ b/fs/xfs/xfs_mount.c
@@ -641,41 +641,41 @@ xfs_sb_read_verify(
 /*
  * We may be probed for a filesystem match, so we may not want to emit
  * messages when the superblock buffer is not actually an XFS superblock.
  * If we find an XFS superblock, the run a normal, noisy mount because we are
  * really going to mount it and want to know about errors.
  */
 static void
 xfs_sb_quiet_read_verify(
 	struct xfs_buf	*bp)
 {
 	struct xfs_sb	sb;
 
 	xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp));
 
 	if (sb.sb_magicnum == XFS_SB_MAGIC) {
 		/* XFS filesystem, verify noisily! */
 		xfs_sb_read_verify(bp);
 		return;
 	}
 	/* quietly fail */
-	xfs_buf_ioerror(bp, EFSCORRUPTED);
+	xfs_buf_ioerror(bp, EINVAL);
 }
 
 static void
 xfs_sb_write_verify(
 	struct xfs_buf	*bp)
 {
 	xfs_sb_verify(bp);
 }
 
 const struct xfs_buf_ops xfs_sb_buf_ops = {
 	.verify_read = xfs_sb_read_verify,
 	.verify_write = xfs_sb_write_verify,
 };
 
 static const struct xfs_buf_ops xfs_sb_quiet_buf_ops = {
 	.verify_read = xfs_sb_quiet_read_verify,
 	.verify_write = xfs_sb_write_verify,
 };
 
 /*
-- 
1.8.0.2

Re: [PATCH] xfs: return -EINVAL instead of -EUCLEAN when mounting non-xfs

[Eric Sandeen]

On 12/29/12 5:16 PM, Sergei Trofimovich wrote:
> It fixes boot panic when trying to boot from btrfs filesystem.
> kernel tries to mount as xfs and gets fatal -EUCLEAN:
> 
> [    0.170000] VFS: Cannot open root device "ubda" or unknown-block(98,0): error -117
> [    0.170000] Please append a correct "root=" boot option; here are the available partitions:
> [    0.170000] 6200         1048576 ubda  driver: uml-blkdev
> [    0.170000] Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(98,0)
> 
> init/do_mounts.c expects only -EINVAL as 'retry another' option.
> Fixes regression introduced by commit 98021821a502db347bd9c7671beeee6e8ce07ea6

yeah, that should work; great minds think alike ;)  Our patches
crossed in the ether I guess.

XFS uses EWRONGFS as an alias for EINVAL internally in these cases,
so maybe we should stick with that for consistency, *shrug*

-Eric

> Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org>
> CC: Ben Myers <bpm@sgi.com>
> CC: Alex Elder <elder@kernel.org>
> CC: xfs@oss.sgi.com
> CC: linux-kernel@vger.kernel.org
> CC: Dave Chinner <dchinner@redhat.com>
> CC: Phil White <pwhite@sgi.com>
> ---
>  fs/xfs/xfs_mount.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/xfs_mount.c b/fs/xfs/xfs_mount.c
> index da50846..379cac1 100644
> --- a/fs/xfs/xfs_mount.c
> +++ b/fs/xfs/xfs_mount.c
> @@ -641,41 +641,41 @@ xfs_sb_read_verify(
>  /*
>   * We may be probed for a filesystem match, so we may not want to emit
>   * messages when the superblock buffer is not actually an XFS superblock.
>   * If we find an XFS superblock, the run a normal, noisy mount because we are
>   * really going to mount it and want to know about errors.
>   */
>  static void
>  xfs_sb_quiet_read_verify(
>  	struct xfs_buf	*bp)
>  {
>  	struct xfs_sb	sb;
>  
>  	xfs_sb_from_disk(&sb, XFS_BUF_TO_SBP(bp));
>  
>  	if (sb.sb_magicnum == XFS_SB_MAGIC) {
>  		/* XFS filesystem, verify noisily! */
>  		xfs_sb_read_verify(bp);
>  		return;
>  	}
>  	/* quietly fail */
> -	xfs_buf_ioerror(bp, EFSCORRUPTED);
> +	xfs_buf_ioerror(bp, EINVAL);
>  }
>  
>  static void
>  xfs_sb_write_verify(
>  	struct xfs_buf	*bp)
>  {
>  	xfs_sb_verify(bp);
>  }
>  
>  const struct xfs_buf_ops xfs_sb_buf_ops = {
>  	.verify_read = xfs_sb_read_verify,
>  	.verify_write = xfs_sb_write_verify,
>  };
>  
>  static const struct xfs_buf_ops xfs_sb_quiet_buf_ops = {
>  	.verify_read = xfs_sb_quiet_read_verify,
>  	.verify_write = xfs_sb_write_verify,
>  };
>  
>  /*
> 

Re: [PATCH] xfs: return -EINVAL instead of -EUCLEAN when mounting non-xfs

[Theodore Ts'o]

On Sat, Dec 29, 2012 at 05:32:12PM -0600, Eric Sandeen wrote:
> yeah, that should work; great minds think alike ;)  Our patches
> crossed in the ether I guess.
> 
> XFS uses EWRONGFS as an alias for EINVAL internally in these cases,
> so maybe we should stick with that for consistency, *shrug*

I keep thinking we should try expanding the number of errno's so that
the file system can give more fs-specific error codes, such that
eventually, user programs could print out error messages that would
make a lot more sense to users.  What we'd have to do is to define the
new errno's, and then wait for the new error_message() strings to
propagate out to a new glibc release, and only then have the kernel
start using the new errno values.  It would be a pain in the tuckus to
do, but in the long run the end result would be a lot better for end
users and system administrators.

					- Ted

Re: [PATCH] xfs: return -EINVAL instead of -EUCLEAN when mounting non-xfs

[Christoph Hellwig]

On Sun, Dec 30, 2012 at 02:16:50AM +0300, Sergei Trofimovich wrote:
> It fixes boot panic when trying to boot from btrfs filesystem.
> kernel tries to mount as xfs and gets fatal -EUCLEAN:
> 
> [    0.170000] VFS: Cannot open root device "ubda" or unknown-block(98,0): error -117
> [    0.170000] Please append a correct "root=" boot option; here are the available partitions:
> [    0.170000] 6200         1048576 ubda  driver: uml-blkdev
> [    0.170000] Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(98,0)
> 
> init/do_mounts.c expects only -EINVAL as 'retry another' option.
> Fixes regression introduced by commit 98021821a502db347bd9c7671beeee6e8ce07ea6

Looks reasonable, but  think xfs_readsb should simply be changed to
turn all EFSCORRUPTED returns into EINVAL if loud is not set.  The
place that changes the errno value would also be a perfect place to
comment why we are doing this in the code so that this knowledge doesn't
get lost.

Re: [PATCH] xfs: return -EINVAL instead of -EUCLEAN when mounting non-xfs

[Ben Myers]

Hey,

On Thu, Jan 03, 2013 at 01:19:29PM -0500, Christoph Hellwig wrote:
> On Sun, Dec 30, 2012 at 02:16:50AM +0300, Sergei Trofimovich wrote:
> > It fixes boot panic when trying to boot from btrfs filesystem.
> > kernel tries to mount as xfs and gets fatal -EUCLEAN:
> > 
> > [    0.170000] VFS: Cannot open root device "ubda" or unknown-block(98,0): error -117
> > [    0.170000] Please append a correct "root=" boot option; here are the available partitions:
> > [    0.170000] 6200         1048576 ubda  driver: uml-blkdev
> > [    0.170000] Kernel panic - not syncing: VFS: Unable to mount root fs on unknown-block(98,0)
> > 
> > init/do_mounts.c expects only -EINVAL as 'retry another' option.
> > Fixes regression introduced by commit 98021821a502db347bd9c7671beeee6e8ce07ea6
> 
> Looks reasonable, but  think xfs_readsb should simply be changed to
> turn all EFSCORRUPTED returns into EINVAL if loud is not set.

I think there are some EFSCORRUPTED returns that should not result in 'retry
another' behavior.  E.g. mounting an xfs filesystem where super block magic is
good but you have a corrupted log probably shouldn't result in a retry with
ext3.  It is useful to be able to distinguish between that special case of
sb->sb_magic != XFS_SB_MAGIC and a truly corrupted filesystem.

Here EWRONGFS (EINVAL) in the quiet case is better than EFSCORRUPTED, but
unfortunately we still can't tell EWRONGFS from other EINVAL returns in
b_error.  Adding a new errno as Ted suggested might make some sense.

Regards,
	Ben