diff mbox

[v2] repair: handle reading superblock from image on larger sector size filesystem

Message ID 1491413638-9590-1-git-send-email-zlang@redhat.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Zorro Lang April 5, 2017, 5:33 p.m. UTC
Due to xfs_repair uses direct IO, sometimes it can't read superblock
from an image file has smaller sector size than host filesystem.
Especially that superblock doesn't align with host filesystem's
sector size.

Fortunately, xfsprogs already has code to do "isa_file" check in
xfs_repair.c, it turns off O_DIRECT after phase1 if the sector
size is less than the host filesystem's geometry. So move it upward
phase1 directly.

Signed-off-by: Zorro Lang <zlang@redhat.com>
---

Hi,

V2 did below changes:
1) remove all changes in V1.
2) move "isa_file" double check over than phase1.

Thanks,
Zorro

 repair/xfs_repair.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

Comments

Eric Sandeen April 5, 2017, 5:40 p.m. UTC | #1
On 4/5/17 12:33 PM, Zorro Lang wrote:
> Due to xfs_repair uses direct IO, sometimes it can't read superblock
> from an image file has smaller sector size than host filesystem.
> Especially that superblock doesn't align with host filesystem's
> sector size.
> 
> Fortunately, xfsprogs already has code to do "isa_file" check in
> xfs_repair.c, it turns off O_DIRECT after phase1 if the sector
> size is less than the host filesystem's geometry. So move it upward
> phase1 directly.

Hm, I think the problem with this is that the code you moved
reads the primary superblock into the psb structure:

rval = get_sb(&psb, 0, XFS_MAX_SECTORSIZE, 0);

but that's now tested before it's read:

                if (psb.sb_sectsize < geom.sectsize) {

In other words, we rely on the primary superblock geometry
to make a determination about turning off O_DIRECT; with your
patch it sure looks like it's testing an uninitialized variable,
no?

-Eric

> Signed-off-by: Zorro Lang <zlang@redhat.com>
> ---
> 
> Hi,
> 
> V2 did below changes:
> 1) remove all changes in V1.
> 2) move "isa_file" double check over than phase1.
> 
> Thanks,
> Zorro
> 
>  repair/xfs_repair.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
> index b07567b..0525c6d 100644
> --- a/repair/xfs_repair.c
> +++ b/repair/xfs_repair.c
> @@ -657,24 +657,6 @@ main(int argc, char **argv)
>  	timestamp(PHASE_START, 0, NULL);
>  	timestamp(PHASE_END, 0, NULL);
>  
> -	/* do phase1 to make sure we have a superblock */
> -	phase1(temp_mp);
> -	timestamp(PHASE_END, 1, NULL);
> -
> -	if (no_modify && primary_sb_modified)  {
> -		do_warn(_("Primary superblock would have been modified.\n"
> -			  "Cannot proceed further in no_modify mode.\n"
> -			  "Exiting now.\n"));
> -		exit(1);
> -	}
> -
> -	rval = get_sb(&psb, 0, XFS_MAX_SECTORSIZE, 0);
> -	if (rval != XR_OK) {
> -		do_warn(_("Primary superblock bad after phase 1!\n"
> -			  "Exiting now.\n"));
> -		exit(1);
> -	}
> -
>  	/* -f forces this, but let's be nice and autodetect it, as well. */
>  	if (!isa_file) {
>  		int		fd = libxfs_device_to_fd(x.ddev);
> @@ -717,6 +699,24 @@ main(int argc, char **argv)
>  		}
>  	}
>  
> +	/* do phase1 to make sure we have a superblock */
> +	phase1(temp_mp);
> +	timestamp(PHASE_END, 1, NULL);
> +
> +	if (no_modify && primary_sb_modified)  {
> +		do_warn(_("Primary superblock would have been modified.\n"
> +			  "Cannot proceed further in no_modify mode.\n"
> +			  "Exiting now.\n"));
> +		exit(1);
> +	}
> +
> +	rval = get_sb(&psb, 0, XFS_MAX_SECTORSIZE, 0);
> +	if (rval != XR_OK) {
> +		do_warn(_("Primary superblock bad after phase 1!\n"
> +			  "Exiting now.\n"));
> +		exit(1);
> +	}
> +
>  	/*
>  	 * Prepare the mount structure. Point the log reference to our local
>  	 * copy so it's available to the various phases. The log bits are
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index b07567b..0525c6d 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -657,24 +657,6 @@  main(int argc, char **argv)
 	timestamp(PHASE_START, 0, NULL);
 	timestamp(PHASE_END, 0, NULL);
 
-	/* do phase1 to make sure we have a superblock */
-	phase1(temp_mp);
-	timestamp(PHASE_END, 1, NULL);
-
-	if (no_modify && primary_sb_modified)  {
-		do_warn(_("Primary superblock would have been modified.\n"
-			  "Cannot proceed further in no_modify mode.\n"
-			  "Exiting now.\n"));
-		exit(1);
-	}
-
-	rval = get_sb(&psb, 0, XFS_MAX_SECTORSIZE, 0);
-	if (rval != XR_OK) {
-		do_warn(_("Primary superblock bad after phase 1!\n"
-			  "Exiting now.\n"));
-		exit(1);
-	}
-
 	/* -f forces this, but let's be nice and autodetect it, as well. */
 	if (!isa_file) {
 		int		fd = libxfs_device_to_fd(x.ddev);
@@ -717,6 +699,24 @@  main(int argc, char **argv)
 		}
 	}
 
+	/* do phase1 to make sure we have a superblock */
+	phase1(temp_mp);
+	timestamp(PHASE_END, 1, NULL);
+
+	if (no_modify && primary_sb_modified)  {
+		do_warn(_("Primary superblock would have been modified.\n"
+			  "Cannot proceed further in no_modify mode.\n"
+			  "Exiting now.\n"));
+		exit(1);
+	}
+
+	rval = get_sb(&psb, 0, XFS_MAX_SECTORSIZE, 0);
+	if (rval != XR_OK) {
+		do_warn(_("Primary superblock bad after phase 1!\n"
+			  "Exiting now.\n"));
+		exit(1);
+	}
+
 	/*
 	 * Prepare the mount structure. Point the log reference to our local
 	 * copy so it's available to the various phases. The log bits are