mkfs.gfs2: Set sunit and swidth from probed io limits

This patch uses the values provided by libblkid to align resource groups to RAID stripes. The strategy we're using here is to give the start of each rgrp an alignment to the stripe width and add an offset of one stripe unit for the next rgrp and so on. This should ensure that the rgrp headers are spread evenly over the array to minimise contention on the bitmap blocks.

One challenge here was to avoid creating large gaps between rgrps and at the end of the device due to the alignment padding. We get around this by calculating the start and length of the next rgrp before fixing the length of the current rgrp and extending it (or shrinking the final one) as appropriate.

In order for this to work some relationships between block and stripe sizes have been enforced: the stripe width must be a multiple of the stripe unit and the stripe unit must be a multiple of the block size.

With this patch, specifying an rg size on the command line still gives aligned rgrps but gaps will still be present.

Signed-off-by Andrew Price <[email protected]>

---
 gfs2/mkfs/main_mkfs.c | 136 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 92 insertions(+), 44 deletions(-)

gfs2/mkfs/main_mkfs.c +0 -0

index 12a259f..058e4fa 100644
@@ -580,26 +580,62 @@ static int writerg(int fd, const struct rgrp_tree *rgt, const unsigned bsize)
 	return 0;

gfs2/mkfs/main_mkfs.c +85 -43

+static int place_rgrps(struct gfs2_sbd *sdp, const struct mkfs_opts *opts, const struct mkfs_dev *dev)
 {
 	struct rgrp_tree *rgt = NULL;
 	uint64_t rgaddr = 0;
-	unsigned int i = 0;
+	uint64_t nextaddr = 0;
+	uint64_t rglen = (sdp->rgsize << 20) / sdp->bsize;
+	const uint64_t maxrgsz = (GFS2_MAX_RGSIZE << 20) / sdp->bsize;
+	const uint64_t minrgsz = (GFS2_MIN_RGSIZE << 20) / sdp->bsize;
+	unsigned sunit_blocks = opts->sunit / sdp->bsize;
+	unsigned swidth_blocks = opts->swidth / opts->bsize;
+	unsigned stripe_offset = 0;
 	int err = 0;
 
-	sdp->device.length -= sdp->sb_addr + 1;
-	sdp->new_rgrps = how_many_rgrps(sdp, &sdp->device, opts->got_rgsize);
-	rgaddr = sdp->sb_addr + 1;
+	sdp->new_rgrps = 0;
+	rgaddr = align_block(sdp->sb_addr + 1, swidth_blocks, sdp->bsize);
 
-	for (i = 0; i < sdp->new_rgrps; i++) {
-		/* TODO: align to RAID stripes, etc. */
+	while (rgaddr > 0) {
 		rgt = rgrp_insert(&sdp->rgtree, rgaddr);
 		if (rgt == NULL)
 			return -1;
-		if (i == 0)
-			rgt->length = sdp->device.length - ((sdp->new_rgrps - 1) * (sdp->device.length / sdp->new_rgrps));
+
+		stripe_offset += sunit_blocks;
+		if (stripe_offset >= swidth_blocks)
+			stripe_offset = 0;
+
+		/* The next rg might not fit into the remaining space so calculate it now
+		   in order to make decisions about the current rg */
+		nextaddr = align_block(rgaddr + rglen, swidth_blocks, sdp->bsize) + stripe_offset;
+		if (!opts->got_rgsize && (nextaddr - rgaddr) <= maxrgsz)
+			/* Use up gap left by alignment if possible */
+			rgt->length = nextaddr - rgaddr;
 		else
-			rgt->length = sdp->device.length / sdp->new_rgrps;
+			rgt->length = rglen;
+
+		/* If the next rg would overflow the device, either shrink it or expand
+		   the current rg to use the remaining space */
+		if (nextaddr + rglen > sdp->device.length) {
+			/* Squeeze the last 1 or 2 rgs into the remaining space */
+			if ((nextaddr < sdp->device.length) && (sdp->device.length - nextaddr >= minrgsz)) {
+				rglen = sdp->device.length - nextaddr;
+			} else {
+				if (sdp->device.length - rgaddr <= maxrgsz)
+					rgt->length = sdp->device.length - rgaddr;
+				else
+					rgt->length = maxrgsz;
+				/* This is the last rg */
+				nextaddr = 0;
+			}
+		}
 
 		/* Build the rindex entry */
 		rgt->ri.ri_length = rgblocks2bitblocks(sdp->bsize, rgt->length, &rgt->ri.ri_data);

@@ -614,6 +650,11 @@ static int place_rgrps(struct gfs2_sbd *sdp, const struct mkfs_opts *opts)
 		rgt->rg.rg_header.mh_format = GFS2_FORMAT_RG;
 		rgt->rg.rg_free = rgt->ri.ri_data;
 
+		if (opts->debug) {
+			gfs2_rindex_print(&rgt->ri);
+			printf(" stripe_offset: %u\n", stripe_offset);
+		}
+
 		/* TODO: This call allocates buffer heads and bitmap pointers
 		 * in rgt. We really shouldn't need to do that. */
 		err = gfs2_compute_bitstructs(sdp->bsize, rgt);

@@ -628,8 +669,9 @@ static int place_rgrps(struct gfs2_sbd *sdp, const struct mkfs_opts *opts)
 			perror(_("Failed to write resource group"));
 			return -1;
 		}
+		sdp->new_rgrps++;
 		sdp->blks_total += rgt->ri.ri_data;
-		rgaddr += rgt->length;
+		rgaddr = nextaddr;
 	}
 
 	sdp->rgrps = sdp->new_rgrps;

@@ -637,7 +679,7 @@ static int place_rgrps(struct gfs2_sbd *sdp, const struct mkfs_opts *opts)
 	return 0;
 }
 
-static void sbd_init(struct gfs2_sbd *sdp, struct mkfs_opts *opts, struct mkfs_dev *dev)
+static void sbd_init(struct gfs2_sbd *sdp, struct mkfs_opts *opts, struct mkfs_dev *dev, unsigned bsize)
 {
 	memset(sdp, 0, sizeof(struct gfs2_sbd));
 	sdp->time = time(NULL);

@@ -647,7 +689,7 @@ static void sbd_init(struct gfs2_sbd *sdp, struct mkfs_opts *opts, struct mkfs_d
 	sdp->jsize = opts->jsize;
 	sdp->md.journals = opts->journals;
 	sdp->device_fd = dev->fd;
-	sdp->bsize = choose_blocksize(opts, dev);
+	sdp->bsize = bsize;
 
 	if (compute_constants(sdp)) {
 		perror(_("Failed to compute file system constants"));

@@ -666,19 +708,6 @@ static void sbd_init(struct gfs2_sbd *sdp, struct mkfs_opts *opts, struct mkfs_d
 	}
 	strcpy(sdp->lockproto, opts->lockproto);
 	strcpy(sdp->locktable, opts->locktable);
-	if (opts->debug) {
-		printf(_("Calculated file system options:\n"));
-		printf("  bsize = %u\n", sdp->bsize);
-		printf("  qcsize = %u\n", sdp->qcsize);
-		printf("  jsize = %u\n", sdp->jsize);
-		printf("  journals = %u\n", sdp->md.journals);
-		printf("  proto = %s\n", sdp->lockproto);
-		printf("  rgsize = %u\n", sdp->rgsize);
-		printf("  table = %s\n", sdp->locktable);
-		printf("  fssize = %"PRIu64"\n", opts->fssize);
-		printf("  sunit = %lu\n", opts->sunit);
-		printf("  swidth = %lu\n", opts->swidth);
-	}
 }
 
 static int probe_contents(struct mkfs_dev *dev)

@@ -764,6 +793,24 @@ static void open_dev(const char *path, struct mkfs_dev *dev)
 		exit(1);
 }
 
+static void opts_set_stripe(struct mkfs_opts *opts, const struct mkfs_dev *dev, unsigned bsize)
+{
+	if (!opts->got_swidth && dev->optimal_io_size > dev->physical_sector_size) {
+		opts->swidth = dev->optimal_io_size;
+		opts->got_swidth = 1;
+	}
+
+	if (!opts->got_sunit && dev->minimum_io_size > dev->physical_sector_size) {
+		opts->sunit = dev->minimum_io_size;
+		opts->got_sunit = 1;
+	}
+
+	if (opts->got_sunit && (opts->sunit % bsize) != 0) {
+		fprintf(stderr, "Stripe unit (%lu) is not a multiple of the block size (%u)\n", opts->sunit, bsize);
+		exit(1);
+	}
+}
+
 void main_mkfs(int argc, char *argv[])
 {
 	struct gfs2_sbd sbd;

@@ -771,28 +818,15 @@ void main_mkfs(int argc, char *argv[])
 	struct mkfs_dev dev;
 	int error;
 	unsigned char uuid[16];
+	unsigned bsize;
 
 	opts_init(&opts);
 	opts_get(argc, argv, &opts);
 	opts_check(&opts);
 
 	open_dev(opts.device, &dev);
-	if (!opts.got_swidth) {
-		if (dev.optimal_io_size > 0)
-			opts.swidth = dev.optimal_io_size;
-		else
-			opts.swidth = dev.logical_sector_size;
-	}
-
-	if (!opts.got_sunit) {
-		if (dev.minimum_io_size > 0)
-			opts.sunit = dev.minimum_io_size;
-		else
-			opts.sunit = dev.logical_sector_size;
-	}
-
-	if (opts.debug)
-		printf("Resource group alignment: %"PRIu64" bytes\n", opts.swidth);
+	bsize = choose_blocksize(&opts, &dev);
+	opts_set_stripe(&opts, &dev, bsize);
 
 	if (S_ISREG(dev.stat.st_mode)) {
 		opts.got_bsize = 1; /* Use default block size for regular files */

@@ -800,7 +834,21 @@ void main_mkfs(int argc, char *argv[])
 
 	warn_of_destruction(opts.device);
 
-	sbd_init(&sbd, &opts, &dev);
+	sbd_init(&sbd, &opts, &dev, bsize);
+	if (opts.debug) {
+		printf(_("Calculated file system options:\n"));
+		printf("  bsize = %u\n", sbd.bsize);
+		printf("  qcsize = %u\n", sbd.qcsize);
+		printf("  jsize = %u\n", sbd.jsize);
+		printf("  journals = %u\n", sbd.md.journals);
+		printf("  proto = %s\n", sbd.lockproto);
+		printf("  rgsize = %u\n", sbd.rgsize);
+		printf("  table = %s\n", sbd.locktable);
+		printf("  fssize = %"PRIu64"\n", opts.fssize);
+		printf("  sunit = %lu\n", opts.sunit);
+		printf("  swidth = %lu\n", opts.swidth);
+		printf("  rgrp align = %lu+%lu blocks\n", opts.swidth/sbd.bsize, opts.sunit/sbd.bsize);
+	}
 
 	if (opts.confirm && !opts.override)
 		are_you_sure();

@@ -808,7 +856,7 @@ void main_mkfs(int argc, char *argv[])
 	if (!S_ISREG(dev.stat.st_mode) && opts.discard)
 		discard_blocks(dev.fd, sbd.bsize * sbd.device.length, opts.debug);
 
-	error = place_rgrps(&sbd, &opts);
+	error = place_rgrps(&sbd, &opts, &dev);
 	if (error) {
 		fprintf(stderr, _("Failed to build resource groups\n"));
 		exit(1);

Hi,

Quoted 33 lines · Andrew Price

This patch uses the values provided by libblkid to align resource groups to RAID stripes. The strategy we're using here is to give the start of each rgrp an alignment to the stripe width and add an offset of one stripe unit for the next rgrp and so on. This should ensure that the rgrp headers are spread evenly over the array to minimise contention on the bitmap blocks.

One challenge here was to avoid creating large gaps between rgrps and at the end of the device due to the alignment padding. We get around this by calculating the start and length of the next rgrp before fixing the length of the current rgrp and extending it (or shrinking the final one) as appropriate.

In order for this to work some relationships between block and stripe sizes have been enforced: the stripe width must be a multiple of the stripe unit and the stripe unit must be a multiple of the block size.

With this patch, specifying an rg size on the command line still gives aligned rgrps but gaps will still be present.

Signed-off-by Andrew Price <[email protected]>

---
  gfs2/mkfs/main_mkfs.c | 136 ++++++++++++++++++++++++++++++++++----------------
  1 file changed, 92 insertions(+), 44 deletions(-)

gfs2/mkfs/main_mkfs.c +0 -0

index 12a259f..058e4fa 100644
@@ -580,26 +580,62 @@ static int writerg(int fd, const struct rgrp_tree *rgt, const unsigned bsize)
  	return 0;
  }
 

gfs2/mkfs/main_mkfs.c static int writerg(int fd, const struct rgrp_tree *rgt, const unsigned bsize) · line 589

Quoted 8 lines

gfs2/mkfs/main_mkfs.c +7 -1

-static int place_rgrps(struct gfs2_sbd *sdp, const struct mkfs_opts *opts)
+static uint64_t align_block(const uint64_t base, const uint64_t align, const uint32_t bsize)
+{
+	if ((align > 0) && ((base % align) > 0))
+		return (base - (base % align)) + align;
+	return base;
+}
+

This doesn't appear to use bsize. Is align always going to be a power of two?

Steve.

Quoted 211 lines

gfs2/mkfs/main_mkfs.c +85 -43

+static int place_rgrps(struct gfs2_sbd *sdp, const struct mkfs_opts *opts, const struct mkfs_dev *dev)
  {
  	struct rgrp_tree *rgt = NULL;
  	uint64_t rgaddr = 0;
-	unsigned int i = 0;
+	uint64_t nextaddr = 0;
+	uint64_t rglen = (sdp->rgsize << 20) / sdp->bsize;
+	const uint64_t maxrgsz = (GFS2_MAX_RGSIZE << 20) / sdp->bsize;
+	const uint64_t minrgsz = (GFS2_MIN_RGSIZE << 20) / sdp->bsize;
+	unsigned sunit_blocks = opts->sunit / sdp->bsize;
+	unsigned swidth_blocks = opts->swidth / opts->bsize;
+	unsigned stripe_offset = 0;
  	int err = 0;
 
-	sdp->device.length -= sdp->sb_addr + 1;
-	sdp->new_rgrps = how_many_rgrps(sdp, &sdp->device, opts->got_rgsize);
-	rgaddr = sdp->sb_addr + 1;
+	sdp->new_rgrps = 0;
+	rgaddr = align_block(sdp->sb_addr + 1, swidth_blocks, sdp->bsize);
 
-	for (i = 0; i < sdp->new_rgrps; i++) {
-		/* TODO: align to RAID stripes, etc. */
+	while (rgaddr > 0) {
  		rgt = rgrp_insert(&sdp->rgtree, rgaddr);
  		if (rgt == NULL)
  			return -1;
-		if (i == 0)
-			rgt->length = sdp->device.length - ((sdp->new_rgrps - 1) * (sdp->device.length / sdp->new_rgrps));
+
+		stripe_offset += sunit_blocks;
+		if (stripe_offset >= swidth_blocks)
+			stripe_offset = 0;
+
+		/* The next rg might not fit into the remaining space so calculate it now
+		   in order to make decisions about the current rg */
+		nextaddr = align_block(rgaddr + rglen, swidth_blocks, sdp->bsize) + stripe_offset;
+		if (!opts->got_rgsize && (nextaddr - rgaddr) <= maxrgsz)
+			/* Use up gap left by alignment if possible */
+			rgt->length = nextaddr - rgaddr;
  		else
-			rgt->length = sdp->device.length / sdp->new_rgrps;
+			rgt->length = rglen;
+
+		/* If the next rg would overflow the device, either shrink it or expand
+		   the current rg to use the remaining space */
+		if (nextaddr + rglen > sdp->device.length) {
+			/* Squeeze the last 1 or 2 rgs into the remaining space */
+			if ((nextaddr < sdp->device.length) && (sdp->device.length - nextaddr >= minrgsz)) {
+				rglen = sdp->device.length - nextaddr;
+			} else {
+				if (sdp->device.length - rgaddr <= maxrgsz)
+					rgt->length = sdp->device.length - rgaddr;
+				else
+					rgt->length = maxrgsz;
+				/* This is the last rg */
+				nextaddr = 0;
+			}
+		}
 
  		/* Build the rindex entry */
  		rgt->ri.ri_length = rgblocks2bitblocks(sdp->bsize, rgt->length, &rgt->ri.ri_data);

@@ -614,6 +650,11 @@ static int place_rgrps(struct gfs2_sbd *sdp, const struct mkfs_opts *opts)
  		rgt->rg.rg_header.mh_format = GFS2_FORMAT_RG;
  		rgt->rg.rg_free = rgt->ri.ri_data;
 
+		if (opts->debug) {
+			gfs2_rindex_print(&rgt->ri);
+			printf(" stripe_offset: %u\n", stripe_offset);
+		}
+
  		/* TODO: This call allocates buffer heads and bitmap pointers
  		 * in rgt. We really shouldn't need to do that. */
  		err = gfs2_compute_bitstructs(sdp->bsize, rgt);

@@ -628,8 +669,9 @@ static int place_rgrps(struct gfs2_sbd *sdp, const struct mkfs_opts *opts)
  			perror(_("Failed to write resource group"));
  			return -1;
  		}
+		sdp->new_rgrps++;
  		sdp->blks_total += rgt->ri.ri_data;
-		rgaddr += rgt->length;
+		rgaddr = nextaddr;
  	}
 
  	sdp->rgrps = sdp->new_rgrps;

@@ -637,7 +679,7 @@ static int place_rgrps(struct gfs2_sbd *sdp, const struct mkfs_opts *opts)
  	return 0;
  }
 
-static void sbd_init(struct gfs2_sbd *sdp, struct mkfs_opts *opts, struct mkfs_dev *dev)
+static void sbd_init(struct gfs2_sbd *sdp, struct mkfs_opts *opts, struct mkfs_dev *dev, unsigned bsize)
  {
  	memset(sdp, 0, sizeof(struct gfs2_sbd));
  	sdp->time = time(NULL);

@@ -647,7 +689,7 @@ static void sbd_init(struct gfs2_sbd *sdp, struct mkfs_opts *opts, struct mkfs_d
  	sdp->jsize = opts->jsize;
  	sdp->md.journals = opts->journals;
  	sdp->device_fd = dev->fd;
-	sdp->bsize = choose_blocksize(opts, dev);
+	sdp->bsize = bsize;
 
  	if (compute_constants(sdp)) {
  		perror(_("Failed to compute file system constants"));

@@ -666,19 +708,6 @@ static void sbd_init(struct gfs2_sbd *sdp, struct mkfs_opts *opts, struct mkfs_d
  	}
  	strcpy(sdp->lockproto, opts->lockproto);
  	strcpy(sdp->locktable, opts->locktable);
-	if (opts->debug) {
-		printf(_("Calculated file system options:\n"));
-		printf("  bsize = %u\n", sdp->bsize);
-		printf("  qcsize = %u\n", sdp->qcsize);
-		printf("  jsize = %u\n", sdp->jsize);
-		printf("  journals = %u\n", sdp->md.journals);
-		printf("  proto = %s\n", sdp->lockproto);
-		printf("  rgsize = %u\n", sdp->rgsize);
-		printf("  table = %s\n", sdp->locktable);
-		printf("  fssize = %"PRIu64"\n", opts->fssize);
-		printf("  sunit = %lu\n", opts->sunit);
-		printf("  swidth = %lu\n", opts->swidth);
-	}
  }
 
  static int probe_contents(struct mkfs_dev *dev)

@@ -764,6 +793,24 @@ static void open_dev(const char *path, struct mkfs_dev *dev)
  		exit(1);
  }
 
+static void opts_set_stripe(struct mkfs_opts *opts, const struct mkfs_dev *dev, unsigned bsize)
+{
+	if (!opts->got_swidth && dev->optimal_io_size > dev->physical_sector_size) {
+		opts->swidth = dev->optimal_io_size;
+		opts->got_swidth = 1;
+	}
+
+	if (!opts->got_sunit && dev->minimum_io_size > dev->physical_sector_size) {
+		opts->sunit = dev->minimum_io_size;
+		opts->got_sunit = 1;
+	}
+
+	if (opts->got_sunit && (opts->sunit % bsize) != 0) {
+		fprintf(stderr, "Stripe unit (%lu) is not a multiple of the block size (%u)\n", opts->sunit, bsize);
+		exit(1);
+	}
+}
+
  void main_mkfs(int argc, char *argv[])
  {
  	struct gfs2_sbd sbd;

@@ -771,28 +818,15 @@ void main_mkfs(int argc, char *argv[])
  	struct mkfs_dev dev;
  	int error;
  	unsigned char uuid[16];
+	unsigned bsize;
 
  	opts_init(&opts);
  	opts_get(argc, argv, &opts);
  	opts_check(&opts);
 
  	open_dev(opts.device, &dev);
-	if (!opts.got_swidth) {
-		if (dev.optimal_io_size > 0)
-			opts.swidth = dev.optimal_io_size;
-		else
-			opts.swidth = dev.logical_sector_size;
-	}
-
-	if (!opts.got_sunit) {
-		if (dev.minimum_io_size > 0)
-			opts.sunit = dev.minimum_io_size;
-		else
-			opts.sunit = dev.logical_sector_size;
-	}
-
-	if (opts.debug)
-		printf("Resource group alignment: %"PRIu64" bytes\n", opts.swidth);
+	bsize = choose_blocksize(&opts, &dev);
+	opts_set_stripe(&opts, &dev, bsize);
 
  	if (S_ISREG(dev.stat.st_mode)) {
  		opts.got_bsize = 1; /* Use default block size for regular files */

@@ -800,7 +834,21 @@ void main_mkfs(int argc, char *argv[])
 
  	warn_of_destruction(opts.device);
 
-	sbd_init(&sbd, &opts, &dev);
+	sbd_init(&sbd, &opts, &dev, bsize);
+	if (opts.debug) {
+		printf(_("Calculated file system options:\n"));
+		printf("  bsize = %u\n", sbd.bsize);
+		printf("  qcsize = %u\n", sbd.qcsize);
+		printf("  jsize = %u\n", sbd.jsize);
+		printf("  journals = %u\n", sbd.md.journals);
+		printf("  proto = %s\n", sbd.lockproto);
+		printf("  rgsize = %u\n", sbd.rgsize);
+		printf("  table = %s\n", sbd.locktable);
+		printf("  fssize = %"PRIu64"\n", opts.fssize);
+		printf("  sunit = %lu\n", opts.sunit);
+		printf("  swidth = %lu\n", opts.swidth);
+		printf("  rgrp align = %lu+%lu blocks\n", opts.swidth/sbd.bsize, opts.sunit/sbd.bsize);
+	}
 
  	if (opts.confirm && !opts.override)
  		are_you_sure();

@@ -808,7 +856,7 @@ void main_mkfs(int argc, char *argv[])
  	if (!S_ISREG(dev.stat.st_mode) && opts.discard)
  		discard_blocks(dev.fd, sbd.bsize * sbd.device.length, opts.debug);
 
-	error = place_rgrps(&sbd, &opts);
+	error = place_rgrps(&sbd, &opts, &dev);
  	if (error) {
  		fprintf(stderr, _("Failed to build resource groups\n"));
  		exit(1);

Hi,

| +			/* Squeeze the last 1 or 2 rgs into the remaining space */
| +			if ((nextaddr < sdp->device.length) && (sdp->device.length - nextaddr >=
| minrgsz)) {
| +				rglen = sdp->device.length - nextaddr;
| +			} else {
| +				if (sdp->device.length - rgaddr <= maxrgsz)
| +					rgt->length = sdp->device.length - rgaddr;
| +				else
| +					rgt->length = maxrgsz;
| +				/* This is the last rg */
| +				nextaddr = 0;

In GFS1, we allowed mix-and-match resource group sizes, but we originally designed mkfs.gfs2 to ensure that all rgrps were the same uniform size. This usually means some space is wasted at the end of the last resource group.

We did this primarily so that fsck.gfs2 could more easily detect and repair damaged resource groups and rindex values. At the time it was designed, I got the buy-in of a bunch of developers and we all agreed to it. Since that time, I've had to change fsck.gfs2 to take more drastic measures to repair damaged resource groups, due to the fact that gfs2_convert can convert a GFS1 file system to GFS2, and thus, we can still end up with non-uniform resource groups. Many customers were adding storage and doing multiple gfs_grow ops, which resulted in metadata sets where the rgrps and rindex were complete chaos.

Still, my assumption has always been: If the file system was made by mkfs.gfs2, all resource groups (but the first one) are identical in size.

I think gfs2_grow takes some steps to ensure that new rgrps are also created using the same size as the current resource groups. If we don't enforce that rule, the rindex could once again become chaos, which means our chances of rgrp and rindex repair get worse.

Do we still want to enforce this rule?

With the improved rgrp repair algorithms in fsck.gfs2, it may not be necessary anymore. I'm not trying to be dogmatic; I'm looking for opinions here.

Regards,

Bob Peterson
Red Hat File Systems

Hi,

| +			/* Squeeze the last 1 or 2 rgs into the remaining space */
| +			if ((nextaddr < sdp->device.length) && (sdp->device.length - nextaddr >=
| minrgsz)) {
| +				rglen = sdp->device.length - nextaddr;
| +			} else {
| +				if (sdp->device.length - rgaddr <= maxrgsz)
| +					rgt->length = sdp->device.length - rgaddr;
| +				else
| +					rgt->length = maxrgsz;
| +				/* This is the last rg */
| +				nextaddr = 0;

In GFS1, we allowed mix-and-match resource group sizes, but we originally designed mkfs.gfs2 to ensure that all rgrps were the same uniform size. This usually means some space is wasted at the end of the last resource group.

We did this primarily so that fsck.gfs2 could more easily detect and repair damaged resource groups and rindex values. At the time it was designed, I got the buy-in of a bunch of developers and we all agreed to it. Since that time, I've had to change fsck.gfs2 to take more drastic measures to repair damaged resource groups, due to the fact that gfs2_convert can convert a GFS1 file system to GFS2, and thus, we can still end up with non-uniform resource groups. Many customers were adding storage and doing multiple gfs_grow ops, which resulted in metadata sets where the rgrps and rindex were complete chaos.

Still, my assumption has always been: If the file system was made by mkfs.gfs2, all resource groups (but the first one) are identical in size.

I think gfs2_grow takes some steps to ensure that new rgrps are also created using the same size as the current resource groups. If we don't enforce that rule, the rindex could once again become chaos, which means our chances of rgrp and rindex repair get worse.

Do we still want to enforce this rule?

With the improved rgrp repair algorithms in fsck.gfs2, it may not be necessary anymore. I'm not trying to be dogmatic; I'm looking for opinions here.

Regards,

Bob Peterson
Red Hat File Systems

It has never been valid to assume that all the rgrps are the same size. It may be useful as a hint, but we should not be relying on that being true. Obviously it makes sense to try and keep them to an even spacing where possible but we must allow for them to be placed and sized independently as required for alignment, etc.

There are some restrictions on rgrp length as they need to be aligned such as to give an integer number of bitmap bytes, and in fact it would be good if that could be further enforced to ensure that all rgrp bitmaps are an integer number of 64 bit words in length (so a multiple of 32 blocks, excluding the headers). So depending on the various restrictions, there may be a few unused blocks between rgrps in some cases.

It would be worth abstracting the details about alignment from mkfs in due course, so that fsck also has access to the same information about where rgrps are likely to have been put, I think.

Some further thoughts:

- Would it be useful to introduce a flag to show the source of an rgrp
(whether mkfs or gfs2_grow) ?
- Would it be useful to add a creation date stamp to each rgrp so that
we can see when things have happened in the past?

Hi,

| +static unsigned rgsize_for_data(uint64_t blksreq, unsigned bsize)
| +{
| +        const uint32_t blks_rgrp = GFS2_NBBY * (bsize - sizeof(struct
| gfs2_rgrp));
| +        const uint32_t blks_meta = GFS2_NBBY * (bsize - sizeof(struct
| gfs2_meta_header));
| +	unsigned bitblocks = 1;
| +	if (blksreq > blks_rgrp)
| +		bitblocks += ((blksreq - blks_rgrp) + blks_meta) / blks_meta;

It looks like this may be rounding up to blks_meta, so shouldn't this be:

   +		bitblocks += ((blksreq - blks_rgrp) + blks_meta - 1) / blks_meta;

Add an 'align' extended option for enabling and disabling resource group alignment and update the mkfs.gfs2 man page to document the new extended options.

Signed-off-by Andrew Price <[email protected]>

---
 gfs2/man/mkfs.gfs2.8  | 31 +++++++++++++++++++++++++++++++
 gfs2/mkfs/main_mkfs.c | 47 +++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 74 insertions(+), 4 deletions(-)

gfs2/man/mkfs.gfs2.8 +31 -0

index 4613305..ceb6f38 100644
@@ -48,6 +48,37 @@ storage).
 This option prevents gfs2_mkfs from asking for confirmation before writing
 the filesystem.
 .TP
+\fB-o\fP
+Specify extended options. Multiple options can be separated by commas. Valid extended options are:
+.RS 1.0i
+.TP
+.BI help
+Display an extended options help summary, then exit.
+.TP
+.BI sunit= bytes
+This is used to specify the stripe unit for a RAID device or striped logical
+volume.  This option ensures that resource groups will be stripe unit aligned
+and overrides the stripe unit value obtained by probing the device. This value
+must be a multiple of the file system block size and must be specified with the
+.I swidth
+option.
+.TP
+.BI swidth= bytes
+This is used to specify the stripe width for a RAID device or striped logical
+volume.  This option ensures that resource groups will be stripe aligned and overrides the stripe width value obtained by probing the device. This value must be a multiple of the
+.I sunit
+option and must also be specified with it.
+.TP
+.BI align= [0|1]
+Disable or enable the alignment of resource groups. The default behaviour is to
+align resource groups to the stripe width and stripe unit values obtained from
+probing the device or specified with the
+.I swidth
+and
+.I sunit
+extended options.
+.RE
+.TP
 \fB-p\fP \fILockProtoName\fR 
 LockProtoName is the name of the  locking  protocol to use.  Acceptable
 locking protocols are \fIlock_dlm\fR (for shared storage) or if you are

gfs2/mkfs/main_mkfs.c +1 -1

index dcfb032..fb4b5ca 100644
@@ -57,7 +57,7 @@ static void print_usage(const char *prog_name)
 	    "-j", _("<number>"), _("Number of journals"),
 	    "-K", NULL,          _("Don't try to discard unused blocks"),
 	    "-O", NULL,          _("Don't ask for confirmation"),
-	    "-o", _("<key>[=<value>][,...]"), _("Specify extended options"),
+	    "-o", _("<key>[=<value>][,...]"), _("Specify extended options. See '-o help'."),
 	    "-p", _("<name>"),   _("Name of the locking protocol"),
 	    "-q", NULL,          _("Don't print anything"),
 	    "-r", _("<size>"),   _("Size of resource groups, in megabytes"),

@@ -80,6 +80,22 @@ static void print_usage(const char *prog_name)
 	}

gfs2/mkfs/main_mkfs.c +34 -3

+		NULL, NULL
+	};
+	printf(_("Extended options:\n"));
+	for (i = 0; options[i] != NULL; i+=2) {
+		printf("%15s  %-22s\n", options[i], options[i+1]);
+	}
+}
+
 struct mkfs_opts {
 	unsigned bsize;
 	unsigned qcsize;

@@ -111,6 +127,7 @@ struct mkfs_opts {
 	unsigned expert:1;
 	unsigned debug:1;
 	unsigned confirm:1;
+	unsigned align:1;
 };
 
 /**

@@ -165,6 +182,7 @@ static void opts_init(struct mkfs_opts *opts)
 	opts->lockproto = "lock_dlm";
 	opts->locktable = "";
 	opts->confirm = 1;
+	opts->align = 1;
 }
 
 #ifndef BLKDISCARD

@@ -250,6 +268,18 @@ static unsigned long parse_ulong(struct mkfs_opts *opts, const char *key, const
 	return (unsigned long)l;
 }
 
+static unsigned parse_bool(struct mkfs_opts *opts, const char *key, const char *val)
+{
+	if (strnlen(val, 2) == 1) {
+		if (*val == '0')
+			return 0;
+		if (*val == '1')
+			return 1;
+	}
+	fprintf(stderr, _("Option '%s' must be either 1 or 0\n"), key);
+	exit(-1);
+}
+
 static void opt_parse_extended(char *str, struct mkfs_opts *opts)
 {
 	char *opt;

@@ -266,6 +296,11 @@ static void opt_parse_extended(char *str, struct mkfs_opts *opts)
 		} else if (strcmp("swidth", key) == 0) {
 			opts->swidth = parse_ulong(opts, "swidth", val);
 			opts->got_swidth = 1;
+		} else if (strcmp("align", key) == 0) {
+			opts->align = parse_bool(opts, "align", val);
+		} else if (strcmp("help", key) == 0) {
+			print_ext_opts();
+			exit(0);
 		} else {
 			fprintf(stderr, _("Invalid option '%s'\n"), key);
 			exit(-1);

@@ -603,7 +638,7 @@ static uint64_t align_block(const uint64_t base, const uint64_t align, const uin
 static void rgs_init(struct mkfs_rgs *rgs, struct mkfs_opts *opts, struct mkfs_dev *dev, struct gfs2_sbd *sdp)
 {
 	memset(rgs, 0, sizeof(*rgs));
-	if (opts->got_sunit) {
+	if (opts->align && opts->got_sunit) {
 		if ((opts->sunit % sdp->bsize) != 0) {
 			fprintf(stderr, _("Stripe unit (%lu) must be a multiple of block size (%u)\n"),
 			        opts->sunit, sdp->bsize);

@@ -616,7 +651,7 @@ static void rgs_init(struct mkfs_rgs *rgs, struct mkfs_opts *opts, struct mkfs_d
 			rgs->align = opts->swidth / sdp->bsize;
 			rgs->align_off = opts->sunit / sdp->bsize;
 		}
-	} else {
+	} else if (opts->align) {
 		if ((dev->minimum_io_size > dev->physical_sector_size) &&
 		    (dev->optimal_io_size > dev->physical_sector_size)) {
 			rgs->align = dev->optimal_io_size / sdp->bsize;

@@ -871,7 +906,11 @@ void main_mkfs(int argc, char *argv[])
 		printf("  fssize = %"PRIu64"\n", opts.fssize);
 		printf("  sunit = %lu\n", opts.sunit);
 		printf("  swidth = %lu\n", opts.swidth);
-		printf("  rgrp align = %lu+%lu blocks\n", rgs.align, rgs.align_off);
+		printf("  rgrp align = ");
+		if (opts.align)
+			printf("%lu+%lu blocks\n", rgs.align, rgs.align_off);
+		else
+			printf("(disabled)\n");
 	}
 
 	warn_of_destruction(opts.device);

Hi,

Quoted 69 lines · Andrew Price

Add an 'align' extended option for enabling and disabling resource group alignment and update the mkfs.gfs2 man page to document the new extended options.

Signed-off-by Andrew Price <[email protected]>

---
  gfs2/man/mkfs.gfs2.8  | 31 +++++++++++++++++++++++++++++++
  gfs2/mkfs/main_mkfs.c | 47 +++++++++++++++++++++++++++++++++++++++++++----
  2 files changed, 74 insertions(+), 4 deletions(-)

gfs2/man/mkfs.gfs2.8 +31 -0

index 4613305..ceb6f38 100644
@@ -48,6 +48,37 @@ storage).
  This option prevents gfs2_mkfs from asking for confirmation before writing
  the filesystem.
  .TP
+\fB-o\fP
+Specify extended options. Multiple options can be separated by commas. Valid extended options are:
+.RS 1.0i
+.TP
+.BI help
+Display an extended options help summary, then exit.
+.TP
+.BI sunit= bytes
+This is used to specify the stripe unit for a RAID device or striped logical
+volume.  This option ensures that resource groups will be stripe unit aligned
+and overrides the stripe unit value obtained by probing the device. This value
+must be a multiple of the file system block size and must be specified with the
+.I swidth
+option.
+.TP
+.BI swidth= bytes
+This is used to specify the stripe width for a RAID device or striped logical
+volume.  This option ensures that resource groups will be stripe aligned and overrides the stripe width value obtained by probing the device. This value must be a multiple of the
+.I sunit
+option and must also be specified with it.
+.TP
+.BI align= [0|1]
+Disable or enable the alignment of resource groups. The default behaviour is to
+align resource groups to the stripe width and stripe unit values obtained from
+probing the device or specified with the
+.I swidth
+and
+.I sunit
+extended options.
+.RE
+.TP
  \fB-p\fP \fILockProtoName\fR
  LockProtoName is the name of the  locking  protocol to use.  Acceptable
  locking protocols are \fIlock_dlm\fR (for shared storage) or if you are

gfs2/mkfs/main_mkfs.c +1 -1

index dcfb032..fb4b5ca 100644
@@ -57,7 +57,7 @@ static void print_usage(const char *prog_name)
  	    "-j", _("<number>"), _("Number of journals"),
  	    "-K", NULL,          _("Don't try to discard unused blocks"),
  	    "-O", NULL,          _("Don't ask for confirmation"),
-	    "-o", _("<key>[=<value>][,...]"), _("Specify extended options"),
+	    "-o", _("<key>[=<value>][,...]"), _("Specify extended options. See '-o help'."),
  	    "-p", _("<name>"),   _("Name of the locking protocol"),
  	    "-q", NULL,          _("Don't print anything"),
  	    "-r", _("<size>"),   _("Size of resource groups, in megabytes"),

@@ -80,6 +80,22 @@ static void print_usage(const char *prog_name)
  	}
  }
 

gfs2/mkfs/main_mkfs.c static void print_usage(const char *prog_name) · line 90

Quoted 8 lines

gfs2/mkfs/main_mkfs.c +8 -0

+static void print_ext_opts(void)
+{
+	int i;
+	const char *options[] = {
+		"help", _("Display this help, then exit"),
+		"swidth=N",  _("Specify the stripe width of the device, overriding probed values"),
+		"sunit=N", _("Specify the stripe unit of the device, overriding probed values"),
+		"align=[0|1]", _("Disable or enable alignment of resource groups"),

I don't think that this is going to work with gettext since the _() implies a function call,

Steve.

Quoted 89 lines

gfs2/mkfs/main_mkfs.c +34 -3

+		NULL, NULL
+	};
+	printf(_("Extended options:\n"));
+	for (i = 0; options[i] != NULL; i+=2) {
+		printf("%15s  %-22s\n", options[i], options[i+1]);
+	}
+}
+
  struct mkfs_opts {
  	unsigned bsize;
  	unsigned qcsize;

@@ -111,6 +127,7 @@ struct mkfs_opts {
  	unsigned expert:1;
  	unsigned debug:1;
  	unsigned confirm:1;
+	unsigned align:1;
  };
 
  /**

@@ -165,6 +182,7 @@ static void opts_init(struct mkfs_opts *opts)
  	opts->lockproto = "lock_dlm";
  	opts->locktable = "";
  	opts->confirm = 1;
+	opts->align = 1;
  }
 
  #ifndef BLKDISCARD

@@ -250,6 +268,18 @@ static unsigned long parse_ulong(struct mkfs_opts *opts, const char *key, const
  	return (unsigned long)l;
  }
 
+static unsigned parse_bool(struct mkfs_opts *opts, const char *key, const char *val)
+{
+	if (strnlen(val, 2) == 1) {
+		if (*val == '0')
+			return 0;
+		if (*val == '1')
+			return 1;
+	}
+	fprintf(stderr, _("Option '%s' must be either 1 or 0\n"), key);
+	exit(-1);
+}
+
  static void opt_parse_extended(char *str, struct mkfs_opts *opts)
  {
  	char *opt;

@@ -266,6 +296,11 @@ static void opt_parse_extended(char *str, struct mkfs_opts *opts)
  		} else if (strcmp("swidth", key) == 0) {
  			opts->swidth = parse_ulong(opts, "swidth", val);
  			opts->got_swidth = 1;
+		} else if (strcmp("align", key) == 0) {
+			opts->align = parse_bool(opts, "align", val);
+		} else if (strcmp("help", key) == 0) {
+			print_ext_opts();
+			exit(0);
  		} else {
  			fprintf(stderr, _("Invalid option '%s'\n"), key);
  			exit(-1);

@@ -603,7 +638,7 @@ static uint64_t align_block(const uint64_t base, const uint64_t align, const uin
  static void rgs_init(struct mkfs_rgs *rgs, struct mkfs_opts *opts, struct mkfs_dev *dev, struct gfs2_sbd *sdp)
  {
  	memset(rgs, 0, sizeof(*rgs));
-	if (opts->got_sunit) {
+	if (opts->align && opts->got_sunit) {
  		if ((opts->sunit % sdp->bsize) != 0) {
  			fprintf(stderr, _("Stripe unit (%lu) must be a multiple of block size (%u)\n"),
  			        opts->sunit, sdp->bsize);

@@ -616,7 +651,7 @@ static void rgs_init(struct mkfs_rgs *rgs, struct mkfs_opts *opts, struct mkfs_d
  			rgs->align = opts->swidth / sdp->bsize;
  			rgs->align_off = opts->sunit / sdp->bsize;
  		}
-	} else {
+	} else if (opts->align) {
  		if ((dev->minimum_io_size > dev->physical_sector_size) &&
  		    (dev->optimal_io_size > dev->physical_sector_size)) {
  			rgs->align = dev->optimal_io_size / sdp->bsize;

@@ -871,7 +906,11 @@ void main_mkfs(int argc, char *argv[])
  		printf("  fssize = %"PRIu64"\n", opts.fssize);
  		printf("  sunit = %lu\n", opts.sunit);
  		printf("  swidth = %lu\n", opts.swidth);
-		printf("  rgrp align = %lu+%lu blocks\n", rgs.align, rgs.align_off);
+		printf("  rgrp align = ");
+		if (opts.align)
+			printf("%lu+%lu blocks\n", rgs.align, rgs.align_off);
+		else
+			printf("(disabled)\n");
  	}
 
  	warn_of_destruction(opts.device);

Quoted 72 lines · Steven Whitehouse

Hi,

Quoted 69 lines · Andrew Price

Add an 'align' extended option for enabling and disabling resource group alignment and update the mkfs.gfs2 man page to document the new extended options.

Signed-off-by Andrew Price <[email protected]>

---
  gfs2/man/mkfs.gfs2.8  | 31 +++++++++++++++++++++++++++++++
  gfs2/mkfs/main_mkfs.c | 47 +++++++++++++++++++++++++++++++++++++++++++----
  2 files changed, 74 insertions(+), 4 deletions(-)

gfs2/man/mkfs.gfs2.8 +31 -0

index 4613305..ceb6f38 100644
@@ -48,6 +48,37 @@ storage).
  This option prevents gfs2_mkfs from asking for confirmation before writing
  the filesystem.
  .TP
+\fB-o\fP
+Specify extended options. Multiple options can be separated by commas. Valid extended options are:
+.RS 1.0i
+.TP
+.BI help
+Display an extended options help summary, then exit.
+.TP
+.BI sunit= bytes
+This is used to specify the stripe unit for a RAID device or striped logical
+volume.  This option ensures that resource groups will be stripe unit aligned
+and overrides the stripe unit value obtained by probing the device. This value
+must be a multiple of the file system block size and must be specified with the
+.I swidth
+option.
+.TP
+.BI swidth= bytes
+This is used to specify the stripe width for a RAID device or striped logical
+volume.  This option ensures that resource groups will be stripe aligned and overrides the stripe width value obtained by probing the device. This value must be a multiple of the
+.I sunit
+option and must also be specified with it.
+.TP
+.BI align= [0|1]
+Disable or enable the alignment of resource groups. The default behaviour is to
+align resource groups to the stripe width and stripe unit values obtained from
+probing the device or specified with the
+.I swidth
+and
+.I sunit
+extended options.
+.RE
+.TP
  \fB-p\fP \fILockProtoName\fR
  LockProtoName is the name of the  locking  protocol to use.  Acceptable
  locking protocols are \fIlock_dlm\fR (for shared storage) or if you are

gfs2/mkfs/main_mkfs.c +1 -1

index dcfb032..fb4b5ca 100644
@@ -57,7 +57,7 @@ static void print_usage(const char *prog_name)
  	    "-j", _("<number>"), _("Number of journals"),
  	    "-K", NULL,          _("Don't try to discard unused blocks"),
  	    "-O", NULL,          _("Don't ask for confirmation"),
-	    "-o", _("<key>[=<value>][,...]"), _("Specify extended options"),
+	    "-o", _("<key>[=<value>][,...]"), _("Specify extended options. See '-o help'."),
  	    "-p", _("<name>"),   _("Name of the locking protocol"),
  	    "-q", NULL,          _("Don't print anything"),
  	    "-r", _("<size>"),   _("Size of resource groups, in megabytes"),

@@ -80,6 +80,22 @@ static void print_usage(const char *prog_name)
  	}
  }
 

Quoted 10 lines

gfs2/mkfs/main_mkfs.c static void print_usage(const char *prog_name) · line 90

Quoted 8 lines

gfs2/mkfs/main_mkfs.c +8 -0

+static void print_ext_opts(void)
+{
+	int i;
+	const char *options[] = {
+		"help", _("Display this help, then exit"),
+		"swidth=N",  _("Specify the stripe width of the device, overriding probed values"),
+		"sunit=N", _("Specify the stripe unit of the device, overriding probed values"),
+		"align=[0|1]", _("Disable or enable alignment of resource groups"),

I don't think that this is going to work with gettext since the _() implies a function call,

_() is #defined as gettext() but I'm not sure why it shouldn't work. gettext() is being called as the array is initialised, returning char pointers which get stored in the array.

Andy

Quoted 94 lines

Steve.

Quoted 89 lines

gfs2/mkfs/main_mkfs.c +34 -3

+		NULL, NULL
+	};
+	printf(_("Extended options:\n"));
+	for (i = 0; options[i] != NULL; i+=2) {
+		printf("%15s  %-22s\n", options[i], options[i+1]);
+	}
+}
+
  struct mkfs_opts {
  	unsigned bsize;
  	unsigned qcsize;

@@ -111,6 +127,7 @@ struct mkfs_opts {
  	unsigned expert:1;
  	unsigned debug:1;
  	unsigned confirm:1;
+	unsigned align:1;
  };
 
  /**

@@ -165,6 +182,7 @@ static void opts_init(struct mkfs_opts *opts)
  	opts->lockproto = "lock_dlm";
  	opts->locktable = "";
  	opts->confirm = 1;
+	opts->align = 1;
  }
 
  #ifndef BLKDISCARD

@@ -250,6 +268,18 @@ static unsigned long parse_ulong(struct mkfs_opts *opts, const char *key, const
  	return (unsigned long)l;
  }
 
+static unsigned parse_bool(struct mkfs_opts *opts, const char *key, const char *val)
+{
+	if (strnlen(val, 2) == 1) {
+		if (*val == '0')
+			return 0;
+		if (*val == '1')
+			return 1;
+	}
+	fprintf(stderr, _("Option '%s' must be either 1 or 0\n"), key);
+	exit(-1);
+}
+
  static void opt_parse_extended(char *str, struct mkfs_opts *opts)
  {
  	char *opt;

@@ -266,6 +296,11 @@ static void opt_parse_extended(char *str, struct mkfs_opts *opts)
  		} else if (strcmp("swidth", key) == 0) {
  			opts->swidth = parse_ulong(opts, "swidth", val);
  			opts->got_swidth = 1;
+		} else if (strcmp("align", key) == 0) {
+			opts->align = parse_bool(opts, "align", val);
+		} else if (strcmp("help", key) == 0) {
+			print_ext_opts();
+			exit(0);
  		} else {
  			fprintf(stderr, _("Invalid option '%s'\n"), key);
  			exit(-1);

@@ -603,7 +638,7 @@ static uint64_t align_block(const uint64_t base, const uint64_t align, const uin
  static void rgs_init(struct mkfs_rgs *rgs, struct mkfs_opts *opts, struct mkfs_dev *dev, struct gfs2_sbd *sdp)
  {
  	memset(rgs, 0, sizeof(*rgs));
-	if (opts->got_sunit) {
+	if (opts->align && opts->got_sunit) {
  		if ((opts->sunit % sdp->bsize) != 0) {
  			fprintf(stderr, _("Stripe unit (%lu) must be a multiple of block size (%u)\n"),
  			        opts->sunit, sdp->bsize);

@@ -616,7 +651,7 @@ static void rgs_init(struct mkfs_rgs *rgs, struct mkfs_opts *opts, struct mkfs_d
  			rgs->align = opts->swidth / sdp->bsize;
  			rgs->align_off = opts->sunit / sdp->bsize;
  		}
-	} else {
+	} else if (opts->align) {
  		if ((dev->minimum_io_size > dev->physical_sector_size) &&
  		    (dev->optimal_io_size > dev->physical_sector_size)) {
  			rgs->align = dev->optimal_io_size / sdp->bsize;

@@ -871,7 +906,11 @@ void main_mkfs(int argc, char *argv[])
  		printf("  fssize = %"PRIu64"\n", opts.fssize);
  		printf("  sunit = %lu\n", opts.sunit);
  		printf("  swidth = %lu\n", opts.swidth);
-		printf("  rgrp align = %lu+%lu blocks\n", rgs.align, rgs.align_off);
+		printf("  rgrp align = ");
+		if (opts.align)
+			printf("%lu+%lu blocks\n", rgs.align, rgs.align_off);
+		else
+			printf("(disabled)\n");
  	}
 
  	warn_of_destruction(opts.device);

Hi,

----- Original Message -----
| + for (i = 0; options[i] != NULL; i+=2) {

Just a nit here, but I'd recommend adding spaces, as per our standard: "i += 2"

| + printf("%15s %-22s\n", options[i], options[i+1]);

Same here: "i + 1"

Regards,

Bob Peterson
Red Hat File Systems