[PATCH] mkfs: Refuse to initialize a misaligned device if not forced using libblkid

[PATCH] mkfs: Refuse to initialize a misaligned device if not forced using libblkid

[Carlos Maiolino]

This is the first patch proposal to fix the problem about the usage of
4k sector devices when the device is not properly aligned

The patch make mkfs to refuse to initialize a xfs filesystem if the -f
option is not passed at the command line, and forces a 512b sector size
if the user opt to force the device initialization

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 mkfs/xfs_mkfs.c |   24 ++++++++++++++++++------
 1 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index f527f3d..495c70d 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -369,8 +369,14 @@ out:
 	return ret;
 }
 
-static void blkid_get_topology(const char *device, int *sunit, int *swidth, int *sectorsize)
+static void blkid_get_topology(
+	const char *device,
+	int *sunit,
+	int *swidth,
+	int *sectorsize,
+	int force_overwrite)
 {
+
 	blkid_topology tp;
 	blkid_probe pr;
 	unsigned long val;
@@ -409,6 +415,12 @@ static void blkid_get_topology(const char *device, int *sunit, int *swidth, int
 		fprintf(stderr,
 			_("warning: device is not properly aligned %s\n"),
 			device);
+
+		if(!force_overwrite){
+			fprintf(stderr, _("Use -f to force usage of a misaligned device\n"));
+			exit(EXIT_FAILURE);
+		}
+	*sectorsize = BBSIZE;  /* Force a 512b sector size if the device is misaligned*/
 	}
 
 	blkid_free_probe(pr);
@@ -421,19 +433,19 @@ out_free_probe:
 		device);
 }
 
-static void get_topology(libxfs_init_t *xi, struct fs_topology *ft)
+static void get_topology(libxfs_init_t *xi, struct fs_topology *ft, int force_overwrite)
 {
 	if (!xi->disfile) {
 		const char *dfile = xi->volname ? xi->volname : xi->dname;
 
 		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth,
-				   &ft->sectorsize);
+				   &ft->sectorsize, force_overwrite);
 	}
 
 	if (xi->rtname && !xi->risfile) {
 		int dummy;
 
-		blkid_get_topology(xi->rtname, &dummy, &ft->rtswidth, &dummy);
+		blkid_get_topology(xi->rtname, &dummy, &ft->rtswidth, &dummy, force_overwrite);
 	}
 }
 #else /* ENABLE_BLKID */
@@ -460,7 +472,7 @@ check_overwrite(
 	return 0;
 }
 
-static void get_topology(libxfs_init_t *xi, struct fs_topology *ft)
+static void get_topology(libxfs_init_t *xi, struct fs_topology *ft, int force_overwrite)
 {
 	char *dfile = xi->volname ? xi->volname : xi->dname;
 	int bsz = BBSIZE;
@@ -1625,7 +1637,7 @@ main(
 	}
 
 	memset(&ft, 0, sizeof(ft));
-	get_topology(&xi, &ft);
+	get_topology(&xi, &ft, force_overwrite);
 
 	if (ft.sectoralign) {
 		/*
-- 
1.7.6.4

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

Re: [PATCH] mkfs: Refuse to initialize a misaligned device if not forced using libblkid

[Christoph Hellwig]

On Thu, Dec 01, 2011 at 10:44:25AM -0200, Carlos Maiolino wrote:
> This is the first patch proposal to fix the problem about the usage of
> 4k sector devices when the device is not properly aligned
> 
> The patch make mkfs to refuse to initialize a xfs filesystem if the -f
> option is not passed at the command line, and forces a 512b sector size
> if the user opt to force the device initialization
> 
> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  mkfs/xfs_mkfs.c |   24 ++++++++++++++++++------
>  1 files changed, 18 insertions(+), 6 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index f527f3d..495c70d 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -369,8 +369,14 @@ out:
>  	return ret;
>  }
>  
> -static void blkid_get_topology(const char *device, int *sunit, int *swidth, int *sectorsize)
> +static void blkid_get_topology(
> +	const char *device,
> +	int *sunit,
> +	int *swidth,
> +	int *sectorsize,
> +	int force_overwrite)
>  {
> +
>  	blkid_topology tp;
>  	blkid_probe pr;
>  	unsigned long val;
> @@ -409,6 +415,12 @@ static void blkid_get_topology(const char *device, int *sunit, int *swidth, int
>  		fprintf(stderr,
>  			_("warning: device is not properly aligned %s\n"),
>  			device);
> +
> +		if(!force_overwrite){
> +			fprintf(stderr, _("Use -f to force usage of a misaligned device\n"));
> +			exit(EXIT_FAILURE);
> +		}
> +	*sectorsize = BBSIZE;  /* Force a 512b sector size if the device is misaligned*/
>  	}

style issues:

	- always use whitespaces beforew and after braces, except in
	  function calls e.g.

		if (!force_overwrite) {

avoid 80 character lines if possible, e.g.


		fprintf(stderr,
	_("Use -f to force usage of a misaligned device\n"));

and

	/* Force a 512b sector size if the device is misaligned */
	*sectorsize = BBSIZE;

> -		blkid_get_topology(xi->rtname, &dummy, &ft->rtswidth, &dummy);
> +		blkid_get_topology(xi->rtname, &dummy, &ft->rtswidth, &dummy, force_overwrite);

needs linebreak, too.

>  	}
>  }
>  #else /* ENABLE_BLKID */
> @@ -460,7 +472,7 @@ check_overwrite(
>  	return 0;
>  }
>  
> -static void get_topology(libxfs_init_t *xi, struct fs_topology *ft)
> +static void get_topology(libxfs_init_t *xi, struct fs_topology *ft, int force_overwrite)

same here

otherwise this looks good to me.

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

[PATCH] mkfs: Refuse to initialize a misaligned device if not forced using libblkid

[Carlos Maiolino]

The patch fix the problem about the usage of 4k sector devices when the
device is not properly aligned. It makes mkfs to refuse to initialize a
xfs filesystem if the -f option is not passed at the command line, and
forces a 512b sector size if the user chooses to force the device
initialization.

Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
---
 mkfs/xfs_mkfs.c |   35 +++++++++++++++++++++++++++++------
 1 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index f527f3d..159bf60 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -369,8 +369,14 @@ out:
 	return ret;
 }
 
-static void blkid_get_topology(const char *device, int *sunit, int *swidth, int *sectorsize)
+static void blkid_get_topology(
+	const char *device,
+	int *sunit,
+	int *swidth,
+	int *sectorsize,
+	int force_overwrite)
 {
+
 	blkid_topology tp;
 	blkid_probe pr;
 	unsigned long val;
@@ -409,6 +415,15 @@ static void blkid_get_topology(const char *device, int *sunit, int *swidth, int
 		fprintf(stderr,
 			_("warning: device is not properly aligned %s\n"),
 			device);
+
+		if(!force_overwrite) {
+			fprintf(stderr,
+			_("Use -f to force usage of a misaligned device\n"));
+
+			exit(EXIT_FAILURE);
+		}
+	/* Force a 512b sector size if the device is misaligned*/
+	*sectorsize = BBSIZE;
 	}
 
 	blkid_free_probe(pr);
@@ -421,19 +436,23 @@ out_free_probe:
 		device);
 }
 
-static void get_topology(libxfs_init_t *xi, struct fs_topology *ft)
+static void get_topology(
+	libxfs_init_t *xi,
+	struct fs_topology *ft,
+	int force_overwrite)
 {
 	if (!xi->disfile) {
 		const char *dfile = xi->volname ? xi->volname : xi->dname;
 
 		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth,
-				   &ft->sectorsize);
+				   &ft->sectorsize, force_overwrite);
 	}
 
 	if (xi->rtname && !xi->risfile) {
 		int dummy;
 
-		blkid_get_topology(xi->rtname, &dummy, &ft->rtswidth, &dummy);
+		blkid_get_topology(xi->rtname, &dummy, &ft->rtswidth,
+				   &dummy, force_overwrite);
 	}
 }
 #else /* ENABLE_BLKID */
@@ -460,8 +479,12 @@ check_overwrite(
 	return 0;
 }
 
-static void get_topology(libxfs_init_t *xi, struct fs_topology *ft)
+static void get_topology(
+	libxfs_init_t *xi,
+	struct fs_topology *ft,
+	int force_overwrite)
 {
+
 	char *dfile = xi->volname ? xi->volname : xi->dname;
 	int bsz = BBSIZE;
 
@@ -1625,7 +1648,7 @@ main(
 	}
 
 	memset(&ft, 0, sizeof(ft));
-	get_topology(&xi, &ft);
+	get_topology(&xi, &ft, force_overwrite);
 
 	if (ft.sectoralign) {
 		/*
-- 
1.7.6.4

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

Re: [PATCH] mkfs: Refuse to initialize a misaligned device if not forced using libblkid

[Eric Sandeen]

On 12/2/11 6:14 AM, Carlos Maiolino wrote:
> The patch fix the problem about the usage of 4k sector devices when the
> device is not properly aligned. It makes mkfs to refuse to initialize a
> xfs filesystem if the -f option is not passed at the command line, and
> forces a 512b sector size if the user chooses to force the device
> initialization.

[editor]

In mkfs, ensure that the partition is aligned to the device's
physical sector size, so that we do not break atomic IO guarantees
due to read modify write for IOs that span physical blocks.

If an override is forced, set the sector size to the device's
logical sector size as a next-best backup plan.

[/editor]

I think that reads better?  (And I think it is correct?)

> Signed-off-by: Carlos Maiolino <cmaiolino@redhat.com>
> ---
>  mkfs/xfs_mkfs.c |   35 +++++++++++++++++++++++++++++------
>  1 files changed, 29 insertions(+), 6 deletions(-)
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index f527f3d..159bf60 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -369,8 +369,14 @@ out:
>  	return ret;
>  }
>  
> -static void blkid_get_topology(const char *device, int *sunit, int *swidth, int *sectorsize)
> +static void blkid_get_topology(
> +	const char *device,
> +	int *sunit,
> +	int *swidth,
> +	int *sectorsize,
> +	int force_overwrite)
>  {
> +
>  	blkid_topology tp;
>  	blkid_probe pr;
>  	unsigned long val;
> @@ -409,6 +415,15 @@ static void blkid_get_topology(const char *device, int *sunit, int *swidth, int
>  		fprintf(stderr,
>  			_("warning: device is not properly aligned %s\n"),
>  			device);
> +
> +		if(!force_overwrite) {
> +			fprintf(stderr,
> +			_("Use -f to force usage of a misaligned device\n"));
> +
> +			exit(EXIT_FAILURE);
> +		}

I think it might be better to send in *offset rather than force_overwrite;
that way the _caller_ can decide what to do with a misaligned partition.
I suppose by that token maybe the warning should be moved out too.

> +	/* Force a 512b sector size if the device is misaligned*/

space before "*/" - but perhaps the caller should do this.
This function should just get the topology, not handle commandline
switches, exits, etc IMHO.

> +	*sectorsize = BBSIZE;
>  	}
>  
>  	blkid_free_probe(pr);
> @@ -421,19 +436,23 @@ out_free_probe:
>  		device);
>  }
>  
> -static void get_topology(libxfs_init_t *xi, struct fs_topology *ft)
> +static void get_topology(
> +	libxfs_init_t *xi,
> +	struct fs_topology *ft,
> +	int force_overwrite)
>  {
>  	if (!xi->disfile) {
>  		const char *dfile = xi->volname ? xi->volname : xi->dname;
>  
>  		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth,
> -				   &ft->sectorsize);
> +				   &ft->sectorsize, force_overwrite);
>  	}
>  
>  	if (xi->rtname && !xi->risfile) {
>  		int dummy;
>  
> -		blkid_get_topology(xi->rtname, &dummy, &ft->rtswidth, &dummy);
> +		blkid_get_topology(xi->rtname, &dummy, &ft->rtswidth,
> +				   &dummy, force_overwrite);
>  	}

so here I'd check alignment, and decide what to do, i.e. something like:

if (!force && (data_offset || rt_offset)) {
	sprintf(" ... bad alignment!\n");
	sprintf("use -f to force this geometry\n");
	exit(EXIT_FAILURE);
}

-Eric

>  }
>  #else /* ENABLE_BLKID */
> @@ -460,8 +479,12 @@ check_overwrite(
>  	return 0;
>  }
>  
> -static void get_topology(libxfs_init_t *xi, struct fs_topology *ft)
> +static void get_topology(
> +	libxfs_init_t *xi,
> +	struct fs_topology *ft,
> +	int force_overwrite)
>  {
> +
>  	char *dfile = xi->volname ? xi->volname : xi->dname;
>  	int bsz = BBSIZE;
>  
> @@ -1625,7 +1648,7 @@ main(
>  	}
>  
>  	memset(&ft, 0, sizeof(ft));
> -	get_topology(&xi, &ft);
> +	get_topology(&xi, &ft, force_overwrite);
>  
>  	if (ft.sectoralign) {
>  		/*

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