All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] xfs_repair: improved secondary sb search
@ 2016-02-09 17:13 Bill O'Donnell
  2016-02-09 17:13 ` [PATCH 1/2] libxcmd: generalize topology functions Bill O'Donnell
  2016-02-09 17:13 ` [PATCH 2/2] xfs_repair: new secondary superblock search method Bill O'Donnell
  0 siblings, 2 replies; 5+ messages in thread
From: Bill O'Donnell @ 2016-02-09 17:13 UTC (permalink / raw)
  To: xfs


Hello -

Current xfs_repair uses a brute force approach to find a valid
secondary sb. This series optimizes the secondary sb search, using
similar method to determine geometry as that of xfs_mkfs. If the
faster method fails in its search, fall back to original brute force
slower search.

patch 1 has some general infrastructure adjustments, moving general
topology functions from mkfs to libxcmd.

patch 2 adds the new secondary superblock search method.

Questions, comments are welcome.

Thanks-
Bill

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* [PATCH 1/2] libxcmd: generalize topology functions
  2016-02-09 17:13 [PATCH 0/2] xfs_repair: improved secondary sb search Bill O'Donnell
@ 2016-02-09 17:13 ` Bill O'Donnell
  2016-02-09 17:13 ` [PATCH 2/2] xfs_repair: new secondary superblock search method Bill O'Donnell
  1 sibling, 0 replies; 5+ messages in thread
From: Bill O'Donnell @ 2016-02-09 17:13 UTC (permalink / raw)
  To: xfs

Move general topology functions from xfs_mkfs to new topology
collection in libxcmd.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 include/Makefile   |   1 +
 include/libxcmd.h  |  55 +++++++++
 libxcmd/Makefile   |   2 +-
 libxcmd/topology.c | 348 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 mkfs/Makefile      |   2 +-
 mkfs/xfs_mkfs.c    | 321 +-----------------------------------------------
 6 files changed, 407 insertions(+), 322 deletions(-)
 create mode 100644 include/libxcmd.h
 create mode 100644 libxcmd/topology.c

diff --git a/include/Makefile b/include/Makefile
index 6148756..2671f07 100644
--- a/include/Makefile
+++ b/include/Makefile
@@ -20,6 +20,7 @@ include $(TOPDIR)/include/builddefs
 
 LIBHFILES = libxfs.h \
 	libxlog.h \
+	libxcmd.h \
 	atomic.h \
 	bitops.h \
 	cache.h \
diff --git a/include/libxcmd.h b/include/libxcmd.h
new file mode 100644
index 0000000..df7046e
--- /dev/null
+++ b/include/libxcmd.h
@@ -0,0 +1,55 @@
+/*
+ * Copyright (c) 2000-2005 Silicon Graphics, Inc.
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#ifndef __LIBXCMD_H__
+#define __LIBXCMD_H__
+
+#include "libxfs.h"
+#include <sys/time.h>
+
+/*
+ * Device topology information.
+ */
+typedef struct fs_topology {
+	int	dsunit;		/* stripe unit - data subvolume */
+	int	dswidth;	/* stripe width - data subvolume */
+	int	rtswidth;	/* stripe width - rt subvolume */
+	int	lsectorsize;	/* logical sector size &*/
+	int	psectorsize;	/* physical sector size */
+} fs_topology_t;
+
+extern void get_topology(
+	libxfs_init_t		*xi,
+	struct fs_topology	*ft,
+	int			force_overwrite);
+
+extern void
+calc_default_ag_geometry(
+	int             blocklog,
+	__uint64_t      dblocks,
+	int             multidisk,
+	__uint64_t      *agsize,
+	__uint64_t      *agcount);
+
+extern int
+check_overwrite(
+	char		*device);
+
+
+
+#endif	/* __LIBXCMD_H__ */
diff --git a/libxcmd/Makefile b/libxcmd/Makefile
index 7701ed9..aab8d6d 100644
--- a/libxcmd/Makefile
+++ b/libxcmd/Makefile
@@ -10,7 +10,7 @@ LT_CURRENT = 0
 LT_REVISION = 0
 LT_AGE = 0
 
-CFILES = command.c input.c paths.c projects.c help.c quit.c
+CFILES = command.c input.c paths.c projects.c help.c quit.c topology.c
 
 ifeq ($(HAVE_GETMNTENT),yes)
 LCFLAGS += -DHAVE_GETMNTENT
diff --git a/libxcmd/topology.c b/libxcmd/topology.c
new file mode 100644
index 0000000..0eeea28
--- /dev/null
+++ b/libxcmd/topology.c
@@ -0,0 +1,348 @@
+/*
+ * Copyright (c) 2000-2001,2005 Silicon Graphics, Inc.
+ * All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write the Free Software Foundation,
+ * Inc.,  51 Franklin St, Fifth Floor, Boston, MA  02110-1301  USA
+ */
+
+#include "libxfs.h"
+#ifdef ENABLE_BLKID
+#  include <blkid/blkid.h>
+#endif /* ENABLE_BLKID */
+#include "../mkfs/xfs_mkfs.h"
+
+#define TERABYTES(count, blog)	((__uint64_t)(count) << (40 - (blog)))
+#define GIGABYTES(count, blog)	((__uint64_t)(count) << (30 - (blog)))
+#define MEGABYTES(count, blog)	((__uint64_t)(count) << (20 - (blog)))
+
+void
+calc_default_ag_geometry(
+	int		blocklog,
+	__uint64_t	dblocks,
+	int		multidisk,
+	__uint64_t	*agsize,
+	__uint64_t	*agcount)
+{
+	__uint64_t	blocks = 0;
+	int		shift = 0;
+
+	/*
+	 * First handle the high extreme - the point at which we will
+	 * always use the maximum AG size.
+	 *
+	 * This applies regardless of storage configuration.
+	 */
+	if (dblocks >= TERABYTES(32, blocklog)) {
+		blocks = XFS_AG_MAX_BLOCKS(blocklog);
+		goto done;
+	}
+
+	/*
+	 * For the remainder we choose an AG size based on the
+	 * number of data blocks available, trying to keep the
+	 * number of AGs relatively small (especially compared
+	 * to the original algorithm).  AG count is calculated
+	 * based on the preferred AG size, not vice-versa - the
+	 * count can be increased by growfs, so prefer to use
+	 * smaller counts at mkfs time.
+	 *
+	 * For a single underlying storage device between 128MB
+	 * and 4TB in size, just use 4 AGs, otherwise scale up
+	 * smoothly between min/max AG sizes.
+	 */
+
+	if (!multidisk && dblocks >= MEGABYTES(128, blocklog)) {
+		if (dblocks >= TERABYTES(4, blocklog)) {
+			blocks = XFS_AG_MAX_BLOCKS(blocklog);
+			goto done;
+		}
+		shift = 2;
+	} else if (dblocks > GIGABYTES(512, blocklog))
+		shift = 5;
+	else if (dblocks > GIGABYTES(8, blocklog))
+		shift = 4;
+	else if (dblocks >= MEGABYTES(128, blocklog))
+		shift = 3;
+	else if (dblocks >= MEGABYTES(64, blocklog))
+		shift = 2;
+	else if (dblocks >= MEGABYTES(32, blocklog))
+		shift = 1;
+	else
+		shift = 0;
+	/*
+	 * If dblocks is not evenly divisible by the number of
+	 * desired AGs, round "blocks" up so we don't lose the
+	 * last bit of the filesystem. The same principle applies
+	 * to the AG count, so we don't lose the last AG!
+	 */
+	blocks = dblocks >> shift;
+	if (dblocks & xfs_mask32lo(shift)) {
+		if (blocks < XFS_AG_MAX_BLOCKS(blocklog))
+		    blocks++;
+	}
+done:
+	*agsize = blocks;
+	*agcount = dblocks / blocks + (dblocks % blocks != 0);
+}
+
+/*
+ * Device topology information.
+ */
+struct fs_topology {
+	int	dsunit;		/* stripe unit - data subvolume */
+	int	dswidth;	/* stripe width - data subvolume */
+	int	rtswidth;	/* stripe width - rt subvolume */
+	int	lsectorsize;	/* logical sector size &*/
+	int	psectorsize;	/* physical sector size */
+};
+
+/*
+ * Check for existing filesystem or partition table on device.
+ * Returns:
+ *	 1 for existing fs or partition
+ *	 0 for nothing found
+ *	-1 for internal error
+ */
+#ifdef ENABLE_BLKID
+int
+check_overwrite(
+	char		*device)
+{
+	const char	*type;
+	blkid_probe	pr = NULL;
+	int		ret;
+	int		fd;
+	long long	size;
+	int		bsz;
+
+	if (!device || !*device)
+		return 0;
+
+	ret = -1; /* will reset on success of all setup calls */
+
+	fd = open(device, O_RDONLY);
+	if (fd < 0)
+		goto out;
+	platform_findsizes(device, fd, &size, &bsz);
+	close(fd);
+
+	/* nothing to overwrite on a 0-length device */
+	if (size == 0) {
+		ret = 0;
+		goto out;
+	}
+
+	pr = blkid_new_probe_from_filename(device);
+	if (!pr)
+		goto out;
+
+	ret = blkid_probe_enable_partitions(pr, 1);
+	if (ret < 0)
+		goto out;
+
+	ret = blkid_do_fullprobe(pr);
+	if (ret < 0)
+		goto out;
+
+	/*
+	 * Blkid returns 1 for nothing found and 0 when it finds a signature,
+	 * but we want the exact opposite, so reverse the return value here.
+	 *
+	 * In addition print some useful diagnostics about what actually is
+	 * on the device.
+	 */
+	if (ret) {
+		ret = 0;
+		goto out;
+	}
+
+	if (!blkid_probe_lookup_value(pr, "TYPE", &type, NULL)) {
+		fprintf(stderr,
+			_("%s: %s appears to contain an existing "
+			"filesystem (%s).\n"), progname, device, type);
+	} else if (!blkid_probe_lookup_value(pr, "PTTYPE", &type, NULL)) {
+		fprintf(stderr,
+			_("%s: %s appears to contain a partition "
+			"table (%s).\n"), progname, device, type);
+	} else {
+		fprintf(stderr,
+			_("%s: %s appears to contain something weird "
+			"according to blkid\n"), progname, device);
+	}
+	ret = 1;
+
+out:
+	if (pr)
+		blkid_free_probe(pr);
+	if (ret == -1)
+		fprintf(stderr,
+			_("%s: probe of %s failed, cannot detect "
+			  "existing filesystem.\n"), progname, device);
+	return ret;
+}
+
+static void blkid_get_topology(
+	const char	*device,
+	int		*sunit,
+	int		*swidth,
+	int		*lsectorsize,
+	int		*psectorsize,
+	int		force_overwrite)
+{
+
+	blkid_topology tp;
+	blkid_probe pr;
+	unsigned long val;
+	struct stat statbuf;
+
+	/* can't get topology info from a file */
+	if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode))
+		return;
+
+	pr = blkid_new_probe_from_filename(device);
+	if (!pr)
+		return;
+
+	tp = blkid_probe_get_topology(pr);
+	if (!tp)
+		goto out_free_probe;
+
+	val = blkid_topology_get_logical_sector_size(tp);
+	*lsectorsize = val;
+	val = blkid_topology_get_physical_sector_size(tp);
+	*psectorsize = val;
+	val = blkid_topology_get_minimum_io_size(tp);
+	*sunit = val;
+	val = blkid_topology_get_optimal_io_size(tp);
+	*swidth = val;
+
+	/*
+	 * If the reported values are the same as the physical sector size
+	 * do not bother to report anything.  It will only cause warnings
+	 * if people specify larger stripe units or widths manually.
+	 */
+	if (*sunit == *psectorsize || *swidth == *psectorsize) {
+		*sunit = 0;
+		*swidth = 0;
+	}
+
+	/*
+	 * Blkid reports the information in terms of bytes, but we want it in
+	 * terms of 512 bytes blocks (only to convert it to bytes later..)
+	 */
+	*sunit = *sunit >> 9;
+	*swidth = *swidth >> 9;
+
+	if (blkid_topology_get_alignment_offset(tp) != 0) {
+		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);
+		}
+		/* Do not use physical sector size if the device is misaligned */
+		*psectorsize = *lsectorsize;
+	}
+
+	blkid_free_probe(pr);
+	return;
+
+out_free_probe:
+	blkid_free_probe(pr);
+	fprintf(stderr,
+		_("warning: unable to probe device topology for device %s\n"),
+		device);
+}
+#else /* ifdef ENABLE_BLKID */
+/*
+ * Without blkid, we can't do a good check for signatures.
+ * So instead of some messy attempts, just disable any checks
+ * and always return 'nothing found'.
+ */
+#  warning BLKID is disabled, so signature detection and block device\
+ access are not working!
+
+int
+check_overwrite(
+	char		*device)
+{
+	return 1;
+}
+
+static void blkid_get_topology(
+	const char	*device,
+	int		*sunit,
+	int		*swidth,
+	int		*lsectorsize,
+	int		*psectorsize,
+	int		force_overwrite)
+{
+	/*
+	 * Shouldn't make any difference (no blkid = no block device access),
+	 * but make sure this dummy replacement returns with at least some
+	 * sanity.
+	 */
+	*lsectorsize = *psectorsize = 512;
+}
+
+#endif /* ENABLE_BLKID */
+
+
+void get_topology(
+	libxfs_init_t		*xi,
+	struct fs_topology	*ft,
+	int			force_overwrite)
+{
+	struct stat statbuf;
+	char *dfile = xi->volname ? xi->volname : xi->dname;
+
+	/*
+	 * If our target is a regular file, use platform_findsizes
+	 * to try to obtain the underlying filesystem's requirements
+	 * for direct IO; we'll set our sector size to that if possible.
+	 */
+	if (xi->disfile ||
+	    (!stat(dfile, &statbuf) && S_ISREG(statbuf.st_mode))) {
+		int fd;
+		int flags = O_RDONLY;
+		long long dummy;
+
+		/* with xi->disfile we may not have the file yet! */
+		if (xi->disfile)
+			flags |= O_CREAT;
+
+		fd = open(dfile, flags, 0666);
+		if (fd >= 0) {
+			platform_findsizes(dfile, fd, &dummy, &ft->lsectorsize);
+			close(fd);
+			ft->psectorsize = ft->lsectorsize;
+		} else
+			ft->psectorsize = ft->lsectorsize = BBSIZE;
+	} else {
+		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth,
+				   &ft->lsectorsize, &ft->psectorsize,
+				   force_overwrite);
+	}
+
+	if (xi->rtname && !xi->risfile) {
+		int sunit, lsectorsize, psectorsize;
+
+		blkid_get_topology(xi->rtname, &sunit, &ft->rtswidth,
+				   &lsectorsize, &psectorsize, force_overwrite);
+	}
+}
diff --git a/mkfs/Makefile b/mkfs/Makefile
index 570ab07..2997398 100644
--- a/mkfs/Makefile
+++ b/mkfs/Makefile
@@ -10,7 +10,7 @@ LTCOMMAND = mkfs.xfs
 HFILES = xfs_mkfs.h
 CFILES = maxtrres.c proto.c xfs_mkfs.c
 
-LLDLIBS += $(LIBBLKID) $(LIBXFS) $(LIBUUID) $(LIBRT) $(LIBPTHREAD)
+LLDLIBS += $(LIBBLKID) $(LIBXFS) $(LIBUUID) $(LIBRT) $(LIBPTHREAD) $(LIBXCMD)
 LTDEPENDENCIES += $(LIBXFS)
 LLDFLAGS = -static-libtool-libs
 
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 700d12c..2ca7e5c 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -22,17 +22,7 @@
 #  include <blkid/blkid.h>
 #endif /* ENABLE_BLKID */
 #include "xfs_mkfs.h"
-
-/*
- * Device topology information.
- */
-struct fs_topology {
-	int	dsunit;		/* stripe unit - data subvolume */
-	int	dswidth;	/* stripe width - data subvolume */
-	int	rtswidth;	/* stripe width - rt subvolume */
-	int	lsectorsize;	/* logical sector size &*/
-	int	psectorsize;	/* physical sector size */
-};
+#include "libxcmd.h"
 
 /*
  * Prototypes for internal functions.
@@ -295,244 +285,6 @@ calc_stripe_factors(
 	}
 }
 
-/*
- * Check for existing filesystem or partition table on device.
- * Returns:
- *	 1 for existing fs or partition
- *	 0 for nothing found
- *	-1 for internal error
- */
-#ifdef ENABLE_BLKID
-static int
-check_overwrite(
-	char		*device)
-{
-	const char	*type;
-	blkid_probe	pr = NULL;
-	int		ret;
-	int		fd;
-	long long	size;
-	int		bsz;
-
-	if (!device || !*device)
-		return 0;
-
-	ret = -1; /* will reset on success of all setup calls */
-
-	fd = open(device, O_RDONLY);
-	if (fd < 0)
-		goto out;
-	platform_findsizes(device, fd, &size, &bsz);
-	close(fd);
-
-	/* nothing to overwrite on a 0-length device */
-	if (size == 0) {
-		ret = 0;
-		goto out;
-	}
-
-	pr = blkid_new_probe_from_filename(device);
-	if (!pr)
-		goto out;
-
-	ret = blkid_probe_enable_partitions(pr, 1);
-	if (ret < 0)
-		goto out;
-
-	ret = blkid_do_fullprobe(pr);
-	if (ret < 0)
-		goto out;
-
-	/*
-	 * Blkid returns 1 for nothing found and 0 when it finds a signature,
-	 * but we want the exact opposite, so reverse the return value here.
-	 *
-	 * In addition print some useful diagnostics about what actually is
-	 * on the device.
-	 */
-	if (ret) {
-		ret = 0;
-		goto out;
-	}
-
-	if (!blkid_probe_lookup_value(pr, "TYPE", &type, NULL)) {
-		fprintf(stderr,
-			_("%s: %s appears to contain an existing "
-			"filesystem (%s).\n"), progname, device, type);
-	} else if (!blkid_probe_lookup_value(pr, "PTTYPE", &type, NULL)) {
-		fprintf(stderr,
-			_("%s: %s appears to contain a partition "
-			"table (%s).\n"), progname, device, type);
-	} else {
-		fprintf(stderr,
-			_("%s: %s appears to contain something weird "
-			"according to blkid\n"), progname, device);
-	}
-	ret = 1;
-
-out:
-	if (pr)
-		blkid_free_probe(pr);
-	if (ret == -1)
-		fprintf(stderr,
-			_("%s: probe of %s failed, cannot detect "
-			  "existing filesystem.\n"), progname, device);
-	return ret;
-}
-
-static void blkid_get_topology(
-	const char	*device,
-	int		*sunit,
-	int		*swidth,
-	int		*lsectorsize,
-	int		*psectorsize,
-	int		force_overwrite)
-{
-
-	blkid_topology tp;
-	blkid_probe pr;
-	unsigned long val;
-	struct stat statbuf;
-
-	/* can't get topology info from a file */
-	if (!stat(device, &statbuf) && S_ISREG(statbuf.st_mode))
-		return;
-
-	pr = blkid_new_probe_from_filename(device);
-	if (!pr)
-		return;
-
-	tp = blkid_probe_get_topology(pr);
-	if (!tp)
-		goto out_free_probe;
-
-	val = blkid_topology_get_logical_sector_size(tp);
-	*lsectorsize = val;
-	val = blkid_topology_get_physical_sector_size(tp);
-	*psectorsize = val;
-	val = blkid_topology_get_minimum_io_size(tp);
-	*sunit = val;
-	val = blkid_topology_get_optimal_io_size(tp);
-	*swidth = val;
-
-	/*
-	 * If the reported values are the same as the physical sector size
-	 * do not bother to report anything.  It will only cause warnings
-	 * if people specify larger stripe units or widths manually.
-	 */
-	if (*sunit == *psectorsize || *swidth == *psectorsize) {
-		*sunit = 0;
-		*swidth = 0;
-	}
-
-	/*
-	 * Blkid reports the information in terms of bytes, but we want it in
-	 * terms of 512 bytes blocks (only to convert it to bytes later..)
-	 */
-	*sunit = *sunit >> 9;
-	*swidth = *swidth >> 9;
-
-	if (blkid_topology_get_alignment_offset(tp) != 0) {
-		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);
-		}
-		/* Do not use physical sector size if the device is misaligned */
-		*psectorsize = *lsectorsize;
-	}
-
-	blkid_free_probe(pr);
-	return;
-
-out_free_probe:
-	blkid_free_probe(pr);
-	fprintf(stderr,
-		_("warning: unable to probe device topology for device %s\n"),
-		device);
-}
-#else /* ifdef ENABLE_BLKID */
-/*
- * Without blkid, we can't do a good check for signatures.
- * So instead of some messy attempts, just disable any checks
- * and always return 'nothing found'.
- */
-#  warning BLKID is disabled, so signature detection and block device\
- access are not working!
-static int
-check_overwrite(
-	char		*device)
-{
-	return 1;
-}
-
-static void blkid_get_topology(
-	const char	*device,
-	int		*sunit,
-	int		*swidth,
-	int		*lsectorsize,
-	int		*psectorsize,
-	int		force_overwrite)
-{
-	/*
-	 * Shouldn't make any difference (no blkid = no block device access),
-	 * but make sure this dummy replacement returns with at least some
-	 * sanity.
-	 */
-	*lsectorsize = *psectorsize = 512;
-}
-
-#endif /* ENABLE_BLKID */
-
-static void get_topology(
-	libxfs_init_t		*xi,
-	struct fs_topology	*ft,
-	int			force_overwrite)
-{
-	struct stat statbuf;
-	char *dfile = xi->volname ? xi->volname : xi->dname;
-
-	/*
-	 * If our target is a regular file, use platform_findsizes
-	 * to try to obtain the underlying filesystem's requirements
-	 * for direct IO; we'll set our sector size to that if possible.
-	 */
-	if (xi->disfile ||
-	    (!stat(dfile, &statbuf) && S_ISREG(statbuf.st_mode))) {
-		int fd;
-		int flags = O_RDONLY;
-		long long dummy;
-
-		/* with xi->disfile we may not have the file yet! */
-		if (xi->disfile)
-			flags |= O_CREAT;
-
-		fd = open(dfile, flags, 0666);
-		if (fd >= 0) {
-			platform_findsizes(dfile, fd, &dummy, &ft->lsectorsize);
-			close(fd);
-			ft->psectorsize = ft->lsectorsize;
-		} else
-			ft->psectorsize = ft->lsectorsize = BBSIZE;
-	} else {
-		blkid_get_topology(dfile, &ft->dsunit, &ft->dswidth,
-				   &ft->lsectorsize, &ft->psectorsize,
-				   force_overwrite);
-	}
-
-	if (xi->rtname && !xi->risfile) {
-		int sunit, lsectorsize, psectorsize;
-
-		blkid_get_topology(xi->rtname, &sunit, &ft->rtswidth,
-				   &lsectorsize, &psectorsize, force_overwrite);
-	}
-}
-
 static void
 fixup_log_stripe_unit(
 	int		lsflag,
@@ -640,77 +392,6 @@ calc_default_imaxpct(
 	return 1;
 }
 
-
-void
-calc_default_ag_geometry(
-	int		blocklog,
-	__uint64_t	dblocks,
-	int		multidisk,
-	__uint64_t	*agsize,
-	__uint64_t	*agcount)
-{
-	__uint64_t	blocks = 0;
-	int		shift = 0;
-
-	/*
-	 * First handle the high extreme - the point at which we will
-	 * always use the maximum AG size.
-	 *
-	 * This applies regardless of storage configuration.
-	 */
-	if (dblocks >= TERABYTES(32, blocklog)) {
-		blocks = XFS_AG_MAX_BLOCKS(blocklog);
-		goto done;
-	}
-
-	/*
-	 * For the remainder we choose an AG size based on the
-	 * number of data blocks available, trying to keep the
-	 * number of AGs relatively small (especially compared
-	 * to the original algorithm).  AG count is calculated
-	 * based on the preferred AG size, not vice-versa - the
-	 * count can be increased by growfs, so prefer to use
-	 * smaller counts at mkfs time.
-	 *
-	 * For a single underlying storage device between 128MB
-	 * and 4TB in size, just use 4 AGs, otherwise scale up
-	 * smoothly between min/max AG sizes.
-	 */
-
-	if (!multidisk && dblocks >= MEGABYTES(128, blocklog)) {
-		if (dblocks >= TERABYTES(4, blocklog)) {
-			blocks = XFS_AG_MAX_BLOCKS(blocklog);
-			goto done;
-		}
-		shift = 2;
-	} else if (dblocks > GIGABYTES(512, blocklog))
-		shift = 5;
-	else if (dblocks > GIGABYTES(8, blocklog))
-		shift = 4;
-	else if (dblocks >= MEGABYTES(128, blocklog))
-		shift = 3;
-	else if (dblocks >= MEGABYTES(64, blocklog))
-		shift = 2;
-	else if (dblocks >= MEGABYTES(32, blocklog))
-		shift = 1;
-	else
-		shift = 0;
-	/*
-	 * If dblocks is not evenly divisible by the number of
-	 * desired AGs, round "blocks" up so we don't lose the
-	 * last bit of the filesystem. The same principle applies
-	 * to the AG count, so we don't lose the last AG!
-	 */
-	blocks = dblocks >> shift;
-	if (dblocks & xfs_mask32lo(shift)) {
-		if (blocks < XFS_AG_MAX_BLOCKS(blocklog))
-		    blocks++;
-	}
-done:
-	*agsize = blocks;
-	*agcount = dblocks / blocks + (dblocks % blocks != 0);
-}
-
 static void
 validate_ag_geometry(
 	int		blocklog,
-- 
2.5.0

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* [PATCH 2/2] xfs_repair: new secondary superblock search method
  2016-02-09 17:13 [PATCH 0/2] xfs_repair: improved secondary sb search Bill O'Donnell
  2016-02-09 17:13 ` [PATCH 1/2] libxcmd: generalize topology functions Bill O'Donnell
@ 2016-02-09 17:13 ` Bill O'Donnell
  2016-02-09 19:05   ` Eric Sandeen
  1 sibling, 1 reply; 5+ messages in thread
From: Bill O'Donnell @ 2016-02-09 17:13 UTC (permalink / raw)
  To: xfs

Optimize secondary sb search, using similar method to find
fs geometry as that of xfs_mkfs. If this faster method fails
in finding a secondary sb, fall back to original brute force
slower search.

Signed-off-by: Bill O'Donnell <billodo@redhat.com>
---
 Makefile           |  2 +-
 include/libxcmd.h  |  4 +++-
 libxcmd/topology.c | 28 +++++++++++++++++++++++++++-
 repair/Makefile    |  4 ++--
 repair/phase1.c    | 25 +++++++++++++++++++++++--
 repair/protos.h    |  2 +-
 repair/sb.c        | 27 +++++++++++++++++++++------
 7 files changed, 78 insertions(+), 14 deletions(-)

diff --git a/Makefile b/Makefile
index fca0a42..1d60d9c 100644
--- a/Makefile
+++ b/Makefile
@@ -80,7 +80,7 @@ fsr: libhandle
 growfs: libxcmd
 io: libxcmd libhandle
 quota: libxcmd
-repair: libxlog
+repair: libxlog libxcmd
 copy: libxlog
 
 ifeq ($(HAVE_BUILDDEFS), yes)
diff --git a/include/libxcmd.h b/include/libxcmd.h
index df7046e..b140adb 100644
--- a/include/libxcmd.h
+++ b/include/libxcmd.h
@@ -50,6 +50,8 @@ extern int
 check_overwrite(
 	char		*device);
 
-
+extern int guess_default_geometry(__uint64_t *agsize,
+				  __uint64_t *agcount,
+				  libxfs_init_t x);
 
 #endif	/* __LIBXCMD_H__ */
diff --git a/libxcmd/topology.c b/libxcmd/topology.c
index 0eeea28..4a8ab8b 100644
--- a/libxcmd/topology.c
+++ b/libxcmd/topology.c
@@ -302,7 +302,6 @@ static void blkid_get_topology(
 
 #endif /* ENABLE_BLKID */
 
-
 void get_topology(
 	libxfs_init_t		*xi,
 	struct fs_topology	*ft,
@@ -346,3 +345,30 @@ void get_topology(
 				   &lsectorsize, &psectorsize, force_overwrite);
 	}
 }
+
+/*
+ * Use this macro before we have superblock and mount structure
+ */
+#define	DTOBT(d)	((xfs_rfsblock_t)((d) >> (blocklog - BBSHIFT)))
+
+int guess_default_geometry(__uint64_t *agsize, __uint64_t *agcount, libxfs_init_t x) {
+	struct fs_topology ft;
+	int blocklog;
+	__uint64_t      dblocks;
+	int             multidisk;
+        memset(&ft, 0, sizeof(ft));
+        get_topology(&x, &ft, 1);
+
+        /*
+	 * get geometry from get_topology result.
+	 * Use default block size (2^12)
+	 */
+        blocklog = 12;
+        multidisk = ft.dswidth | ft.dsunit;
+	printf("x.dsize = %lu\n",(long unsigned int)x.dsize);
+        dblocks = DTOBT(x.dsize);
+        calc_default_ag_geometry(blocklog, dblocks, multidisk,
+                                 agsize, agcount);
+
+	return(blocklog);
+}
diff --git a/repair/Makefile b/repair/Makefile
index 251722b..d24ab1f 100644
--- a/repair/Makefile
+++ b/repair/Makefile
@@ -20,8 +20,8 @@ CFILES = agheader.c attr_repair.c avl.c avl64.c bmap.c btree.c \
 	progress.c prefetch.c rt.c sb.c scan.c threads.c \
 	versions.c xfs_repair.c
 
-LLDLIBS = $(LIBXFS) $(LIBXLOG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD)
-LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG)
+LLDLIBS = $(LIBBLKID) $(LIBXFS) $(LIBXLOG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD) $(LIBXCMD)
+LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG) $(LIBXCMD)
 LLDFLAGS = -static-libtool-libs
 
 default: depend $(LTCOMMAND)
diff --git a/repair/phase1.c b/repair/phase1.c
index 126d0b3..c27033d 100644
--- a/repair/phase1.c
+++ b/repair/phase1.c
@@ -21,6 +21,7 @@
 #include "agheader.h"
 #include "protos.h"
 #include "err_protos.h"
+#include "libxcmd.h"
 
 static void
 no_sb(void)
@@ -47,6 +48,9 @@ alloc_ag_buf(int size)
  */
 #define MAX_SECTSIZE		(512 * 1024)
 
+extern libxfs_init_t x;
+
+
 /* ARGSUSED */
 void
 phase1(xfs_mount_t *mp)
@@ -54,6 +58,10 @@ phase1(xfs_mount_t *mp)
 	xfs_sb_t		*sb;
 	char			*ag_bp;
 	int			rval;
+	int			blocklog;
+	__uint64_t		agcount;
+	__uint64_t		agsize;
+	__uint64_t		skipsize;
 
 	do_log(_("Phase 1 - find and verify superblock...\n"));
 
@@ -65,6 +73,18 @@ phase1(xfs_mount_t *mp)
 	lost_quotas = 0;
 
 	/*
+	 * divine the geometry ;)
+	 */
+	blocklog = guess_default_geometry(&agsize, &agcount, x);
+
+	/*
+	 * use found ag geometry to quickly find secondary sb
+	 */
+	skipsize = agsize << blocklog;
+	do_log("agsize = %d  agcount = %d  skipsize = %d\n",
+	       (int)agsize, (int)agcount, (int)skipsize);
+
+	/*
 	 * get AG 0 into ag header buf
 	 */
 	ag_bp = alloc_ag_buf(MAX_SECTSIZE);
@@ -80,14 +100,15 @@ phase1(xfs_mount_t *mp)
 	if (rval != XR_OK)  {
 		do_warn(_("bad primary superblock - %s !!!\n"),
 			err_string(rval));
-		if (!find_secondary_sb(sb))
+
+		if (!find_secondary_sb(sb, skipsize))
 			no_sb();
 		primary_sb_modified = 1;
 	} else if ((rval = verify_set_primary_sb(sb, 0,
 					&primary_sb_modified)) != XR_OK)  {
 		do_warn(_("couldn't verify primary superblock - %s !!!\n"),
 			err_string(rval));
-		if (!find_secondary_sb(sb))
+		if (!find_secondary_sb(sb, skipsize))
 			no_sb();
 		primary_sb_modified = 1;
 	}
diff --git a/repair/protos.h b/repair/protos.h
index 9d5a2a6..d96373e 100644
--- a/repair/protos.h
+++ b/repair/protos.h
@@ -31,7 +31,7 @@ int	get_sb(xfs_sb_t			*sbp,
 void	write_primary_sb(xfs_sb_t	*sbp,
 			int		size);
 
-int	find_secondary_sb(xfs_sb_t	*sb);
+int	find_secondary_sb(xfs_sb_t	*sb, __uint64_t skipsize);
 
 struct fs_geometry;
 void	get_sb_geometry(struct fs_geometry	*geo,
diff --git a/repair/sb.c b/repair/sb.c
index 4eef14a..4386479 100644
--- a/repair/sb.c
+++ b/repair/sb.c
@@ -87,8 +87,7 @@ copy_sb(xfs_sb_t *source, xfs_sb_t *dest)
 /*
  * find a secondary superblock, copy it into the sb buffer
  */
-int
-find_secondary_sb(xfs_sb_t *rsb)
+static int __find_secondary_sb(xfs_sb_t *rsb, __uint64_t skipsize )
 {
 	xfs_off_t	off;
 	xfs_sb_t	*sb;
@@ -101,7 +100,6 @@ find_secondary_sb(xfs_sb_t *rsb)
 	int		bsize;
 
 	do_warn(_("\nattempting to find secondary superblock...\n"));
-
 	sb = (xfs_sb_t *)memalign(libxfs_device_alignment(), BSIZE);
 	if (!sb) {
 		do_error(
@@ -117,7 +115,7 @@ find_secondary_sb(xfs_sb_t *rsb)
 	/*
 	 * skip first sector since we know that's bad
 	 */
-	for (done = 0, off = XFS_AG_MIN_BYTES; !done ; off += bsize)  {
+	for (done = 0, off = skipsize; !done ; off += bsize)  {
 		/*
 		 * read disk 1 MByte at a time.
 		 */
@@ -130,7 +128,6 @@ find_secondary_sb(xfs_sb_t *rsb)
 		}
 
 		do_warn(".");
-
 		/*
 		 * check the buffer 512 bytes at a time since
 		 * we don't know how big the sectors really are.
@@ -164,11 +161,29 @@ find_secondary_sb(xfs_sb_t *rsb)
 			}
 		}
 	}
-
 	free(sb);
 	return(retval);
 }
 
+int
+find_secondary_sb(xfs_sb_t *rsb, __uint64_t skipsize)
+{
+	int		retval;
+
+	/*
+	 * Attempt to find secondary sb with a coarse approach,
+	 * using a large skipsize (agsize in bytes). Failing that,
+	 * fallback to the fine-grained approach using min agsize.
+	 */
+	retval = __find_secondary_sb(rsb, skipsize);
+	if (!retval)
+		/*
+		 * fallback: use minimum agsize for skipsize
+		 */
+		retval = __find_secondary_sb(rsb, XFS_AG_MIN_BYTES);
+	return(retval);
+}
+
 /*
  * Calculate what the inode alignment field ought to be based on internal
  * superblock info and determine if it is valid.
-- 
2.5.0

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] xfs_repair: new secondary superblock search method
  2016-02-09 17:13 ` [PATCH 2/2] xfs_repair: new secondary superblock search method Bill O'Donnell
@ 2016-02-09 19:05   ` Eric Sandeen
  2016-02-09 19:14     ` Darrick J. Wong
  0 siblings, 1 reply; 5+ messages in thread
From: Eric Sandeen @ 2016-02-09 19:05 UTC (permalink / raw)
  To: xfs

On 2/9/16 11:13 AM, Bill O'Donnell wrote:
> Optimize secondary sb search, using similar method to find
> fs geometry as that of xfs_mkfs. If this faster method fails
> in finding a secondary sb, fall back to original brute force
> slower search.
> 
> Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> ---
>  Makefile           |  2 +-
>  include/libxcmd.h  |  4 +++-
>  libxcmd/topology.c | 28 +++++++++++++++++++++++++++-
>  repair/Makefile    |  4 ++--
>  repair/phase1.c    | 25 +++++++++++++++++++++++--
>  repair/protos.h    |  2 +-
>  repair/sb.c        | 27 +++++++++++++++++++++------
>  7 files changed, 78 insertions(+), 14 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index fca0a42..1d60d9c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -80,7 +80,7 @@ fsr: libhandle
>  growfs: libxcmd
>  io: libxcmd libhandle
>  quota: libxcmd
> -repair: libxlog
> +repair: libxlog libxcmd
>  copy: libxlog
>  
>  ifeq ($(HAVE_BUILDDEFS), yes)
> diff --git a/include/libxcmd.h b/include/libxcmd.h
> index df7046e..b140adb 100644
> --- a/include/libxcmd.h
> +++ b/include/libxcmd.h
> @@ -50,6 +50,8 @@ extern int
>  check_overwrite(
>  	char		*device);
>  
> -
> +extern int guess_default_geometry(__uint64_t *agsize,
> +				  __uint64_t *agcount,
> +				  libxfs_init_t x);
>  
>  #endif	/* __LIBXCMD_H__ */
> diff --git a/libxcmd/topology.c b/libxcmd/topology.c
> index 0eeea28..4a8ab8b 100644
> --- a/libxcmd/topology.c
> +++ b/libxcmd/topology.c
> @@ -302,7 +302,6 @@ static void blkid_get_topology(
>  
>  #endif /* ENABLE_BLKID */
>  
> -
>  void get_topology(
>  	libxfs_init_t		*xi,
>  	struct fs_topology	*ft,
> @@ -346,3 +345,30 @@ void get_topology(
>  				   &lsectorsize, &psectorsize, force_overwrite);
>  	}
>  }
> +
> +/*
> + * Use this macro before we have superblock and mount structure
> + */
> +#define	DTOBT(d)	((xfs_rfsblock_t)((d) >> (blocklog - BBSHIFT)))

Hm, I hate this macro.  ;)  I know it lives in xfs_mkfs.c too... depending
on a local variable is ick.  Since you only use it once below, it might be best
to just open-code it, see below.

> +
> +int guess_default_geometry(__uint64_t *agsize, __uint64_t *agcount, libxfs_init_t x) {

Move the "{" down to this line, please, and wrap the args so it doesn't go
over 80 chars.

> +	struct fs_topology ft;
> +	int blocklog;
> +	__uint64_t      dblocks;
                  ^tab here please
> +	int             multidisk;
           ^tabs here please

... and a newline here to separate variable declarations...

> +        memset(&ft, 0, sizeof(ft));

... from this call.  Tab this call in...


> +        get_topology(&x, &ft, 1);
   ^^^^ as well as this one.

> +
> +        /*
   ^ tab here too
> +	 * get geometry from get_topology result.
> +	 * Use default block size (2^12)
> +	 */
> +        blocklog = 12;
> +        multidisk = ft.dswidth | ft.dsunit;
> +	printf("x.dsize = %lu\n",(long unsigned int)x.dsize);

drop the debug printf

> +        dblocks = DTOBT(x.dsize);

maybe best to just:

           dblocks = x.dsize >> (blocklog - BBSHIFT); 

> +        calc_default_ag_geometry(blocklog, dblocks, multidisk,
> +                                 agsize, agcount);

general whitespace problems above; use tabs not spaces please,
as appropriate.

> +
> +	return(blocklog);

while we're at it it'd be nice to make return not a function; 
just "return blocklog;"

> +}
> diff --git a/repair/Makefile b/repair/Makefile
> index 251722b..d24ab1f 100644
> --- a/repair/Makefile
> +++ b/repair/Makefile
> @@ -20,8 +20,8 @@ CFILES = agheader.c attr_repair.c avl.c avl64.c bmap.c btree.c \
>  	progress.c prefetch.c rt.c sb.c scan.c threads.c \
>  	versions.c xfs_repair.c
>  
> -LLDLIBS = $(LIBXFS) $(LIBXLOG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD)
> -LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG)
> +LLDLIBS = $(LIBBLKID) $(LIBXFS) $(LIBXLOG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD) $(LIBXCMD)
> +LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG) $(LIBXCMD)
>  LLDFLAGS = -static-libtool-libs
>  
>  default: depend $(LTCOMMAND)
> diff --git a/repair/phase1.c b/repair/phase1.c
> index 126d0b3..c27033d 100644
> --- a/repair/phase1.c
> +++ b/repair/phase1.c
> @@ -21,6 +21,7 @@
>  #include "agheader.h"
>  #include "protos.h"
>  #include "err_protos.h"
> +#include "libxcmd.h"
>  
>  static void
>  no_sb(void)
> @@ -47,6 +48,9 @@ alloc_ag_buf(int size)
>   */
>  #define MAX_SECTSIZE		(512 * 1024)
>  
> +extern libxfs_init_t x;

It would be better to 

#include "libxlog.h"

after the inclusion of libxfs.h at the top of this file, and you won't
need this extern.  That's how the main file/function gets "x" for
xfs_repair.  but actually, see more below.

> +
> +
>  /* ARGSUSED */
>  void
>  phase1(xfs_mount_t *mp)
> @@ -54,6 +58,10 @@ phase1(xfs_mount_t *mp)
>  	xfs_sb_t		*sb;
>  	char			*ag_bp;
>  	int			rval;
> +	int			blocklog;
> +	__uint64_t		agcount;
> +	__uint64_t		agsize;
> +	__uint64_t		skipsize;
>  
>  	do_log(_("Phase 1 - find and verify superblock...\n"));
>  
> @@ -65,6 +73,18 @@ phase1(xfs_mount_t *mp)
>  	lost_quotas = 0;
>  
>  	/*
> +	 * divine the geometry 
> +	 */
> +	blocklog = guess_default_geometry(&agsize, &agcount, x);
> +
> +	/*
> +	 * use found ag geometry to quickly find secondary sb
> +	 */
> +	skipsize = agsize << blocklog;
> +	do_log("agsize = %d  agcount = %d  skipsize = %d\n",
> +	       (int)agsize, (int)agcount, (int)skipsize);

Since this is just a guess, I wouldn't log.  It may well
print out geometry that is not correct, and that would be confusing.

And actually - looking at how you have refactored find_secondary_sb(),
you can move the above 12 or so lines into find_secondary_sb(),
and you don't need to pre-compute skipsize etc here.  You can do it
all in find_secondary_sb() when it gets called.

Then this file (phase1.c) can be completely unchanged, and all the
magic happens in repair/sb.c.

> +
> +	/*
>  	 * get AG 0 into ag header buf
>  	 */
>  	ag_bp = alloc_ag_buf(MAX_SECTSIZE);
> @@ -80,14 +100,15 @@ phase1(xfs_mount_t *mp)
>  	if (rval != XR_OK)  {
>  		do_warn(_("bad primary superblock - %s !!!\n"),
>  			err_string(rval));
> -		if (!find_secondary_sb(sb))
> +
> +		if (!find_secondary_sb(sb, skipsize))
>  			no_sb();
>  		primary_sb_modified = 1;
>  	} else if ((rval = verify_set_primary_sb(sb, 0,
>  					&primary_sb_modified)) != XR_OK)  {
>  		do_warn(_("couldn't verify primary superblock - %s !!!\n"),
>  			err_string(rval));
> -		if (!find_secondary_sb(sb))
> +		if (!find_secondary_sb(sb, skipsize))
>  			no_sb();
>  		primary_sb_modified = 1;
>  	}
> diff --git a/repair/protos.h b/repair/protos.h
> index 9d5a2a6..d96373e 100644
> --- a/repair/protos.h
> +++ b/repair/protos.h
> @@ -31,7 +31,7 @@ int	get_sb(xfs_sb_t			*sbp,
>  void	write_primary_sb(xfs_sb_t	*sbp,
>  			int		size);
>  
> -int	find_secondary_sb(xfs_sb_t	*sb);
> +int	find_secondary_sb(xfs_sb_t	*sb, __uint64_t skipsize);

this would be unchanged as well.

>  
>  struct fs_geometry;
>  void	get_sb_geometry(struct fs_geometry	*geo,
> diff --git a/repair/sb.c b/repair/sb.c
> index 4eef14a..4386479 100644
> --- a/repair/sb.c/d
> +++ b/repair/sb.c
> @@ -87,8 +87,7 @@ copy_sb(xfs_sb_t *source, xfs_sb_t *dest)
>  /*
>   * find a secondary superblock, copy it into the sb buffer

It'd be good to document "skipsize" here - units, and purpose.

>   */
> -int
> -find_secondary_sb(xfs_sb_t *rsb)
> +static int __find_secondary_sb(xfs_sb_t *rsb, __uint64_t skipsize )

no space between skipsize and ")" please.

>  {
>  	xfs_off_t	off;
>  	xfs_sb_t	*sb;
> @@ -101,7 +100,6 @@ find_secondary_sb(xfs_sb_t *rsb)
>  	int		bsize;
>  
>  	do_warn(_("\nattempting to find secondary superblock...\n"));
> -
>  	sb = (xfs_sb_t *)memalign(libxfs_device_alignment(), BSIZE);
>  	if (!sb) {
>  		do_error(
> @@ -117,7 +115,7 @@ find_secondary_sb(xfs_sb_t *rsb)
>  	/*
>  	 * skip first sector since we know that's bad
>  	 */
> -	for (done = 0, off = XFS_AG_MIN_BYTES; !done ; off += bsize)  {
> +	for (done = 0, off = skipsize; !done ; off += bsize)  {

Ok, so you jump out "skipsize" for the first read, but after that you
only move the offset by bsize, which is BSIZE (or 1MB).

That works OK if the 2nd superblock (at skipsize) is OK, but if it's
not, you'll start from there doing smaller reads like before, and
then this loop isn't really any more efficient than it was.

If skipsize is set, you need to start out 1x skipsize, then read
at 2x skipsize, 3x skipsize, etc.  So you need to change the
loop increment as well.

i.e. fast path: start at skipsize, advance by skipsize each loop

slow path: start at XFS_AG_MIN_BYTES, advance by BSIZE each loop

If you make a 4t sparse file, mkfs it, and corrupt the first *2*
superblocks, then point repair at it you'll see what I mean.

>  		/*
>  		 * read disk 1 MByte at a time.
>  		 */
> @@ -130,7 +128,6 @@ find_secondary_sb(xfs_sb_t *rsb)
>  		}
>  
>  		do_warn(".");
> -
>  		/*
>  		 * check the buffer 512 bytes at a time since
>  		 * we don't know how big the sectors really are.
> @@ -164,11 +161,29 @@ find_secondary_sb(xfs_sb_t *rsb)
>  			}
>  		}
>  	}
> -
>  	free(sb);
>  	return(retval);
>  }
>  
> +int
> +find_secondary_sb(xfs_sb_t *rsb, __uint64_t skipsize)
> +{
> +	int		retval;
> +
> +	/*
> +	 * Attempt to find secondary sb with a coarse approach,
> +	 * using a large skipsize (agsize in bytes). Failing that,
> +	 * fallback to the fine-grained approach using min agsize.
> +	 */
> +	retval = __find_secondary_sb(rsb, skipsize);
> +	if (!retval)

With the comment below, it's safest to wrap these multiple lines after
the conditional in { .... }

> +		/*
> +		 * fallback: use minimum agsize for skipsize
> +		 */
> +		retval = __find_secondary_sb(rsb, XFS_AG_MIN_BYTES);
> +	return(retval);

again drop the () on the return, please.

> +}
> +
>  /*
>   * Calculate what the inode alignment field ought to be based on internal
>   * superblock info and determine if it is valid.
> 

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

^ permalink raw reply	[flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] xfs_repair: new secondary superblock search method
  2016-02-09 19:05   ` Eric Sandeen
@ 2016-02-09 19:14     ` Darrick J. Wong
  0 siblings, 0 replies; 5+ messages in thread
From: Darrick J. Wong @ 2016-02-09 19:14 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: xfs

On Tue, Feb 09, 2016 at 01:05:12PM -0600, Eric Sandeen wrote:
> On 2/9/16 11:13 AM, Bill O'Donnell wrote:
> > Optimize secondary sb search, using similar method to find
> > fs geometry as that of xfs_mkfs. If this faster method fails
> > in finding a secondary sb, fall back to original brute force
> > slower search.
> > 
> > Signed-off-by: Bill O'Donnell <billodo@redhat.com>
> > ---
> >  Makefile           |  2 +-
> >  include/libxcmd.h  |  4 +++-
> >  libxcmd/topology.c | 28 +++++++++++++++++++++++++++-
> >  repair/Makefile    |  4 ++--
> >  repair/phase1.c    | 25 +++++++++++++++++++++++--
> >  repair/protos.h    |  2 +-
> >  repair/sb.c        | 27 +++++++++++++++++++++------
> >  7 files changed, 78 insertions(+), 14 deletions(-)
> > 
> > diff --git a/Makefile b/Makefile
> > index fca0a42..1d60d9c 100644
> > --- a/Makefile
> > +++ b/Makefile
> > @@ -80,7 +80,7 @@ fsr: libhandle
> >  growfs: libxcmd
> >  io: libxcmd libhandle
> >  quota: libxcmd
> > -repair: libxlog
> > +repair: libxlog libxcmd
> >  copy: libxlog
> >  
> >  ifeq ($(HAVE_BUILDDEFS), yes)
> > diff --git a/include/libxcmd.h b/include/libxcmd.h
> > index df7046e..b140adb 100644
> > --- a/include/libxcmd.h
> > +++ b/include/libxcmd.h
> > @@ -50,6 +50,8 @@ extern int
> >  check_overwrite(
> >  	char		*device);
> >  
> > -
> > +extern int guess_default_geometry(__uint64_t *agsize,
> > +				  __uint64_t *agcount,
> > +				  libxfs_init_t x);
> >  
> >  #endif	/* __LIBXCMD_H__ */
> > diff --git a/libxcmd/topology.c b/libxcmd/topology.c
> > index 0eeea28..4a8ab8b 100644
> > --- a/libxcmd/topology.c
> > +++ b/libxcmd/topology.c
> > @@ -302,7 +302,6 @@ static void blkid_get_topology(
> >  
> >  #endif /* ENABLE_BLKID */
> >  
> > -
> >  void get_topology(
> >  	libxfs_init_t		*xi,
> >  	struct fs_topology	*ft,
> > @@ -346,3 +345,30 @@ void get_topology(
> >  				   &lsectorsize, &psectorsize, force_overwrite);
> >  	}
> >  }
> > +
> > +/*
> > + * Use this macro before we have superblock and mount structure
> > + */
> > +#define	DTOBT(d)	((xfs_rfsblock_t)((d) >> (blocklog - BBSHIFT)))
> 
> Hm, I hate this macro.  ;)  I know it lives in xfs_mkfs.c too... depending
> on a local variable is ick.  Since you only use it once below, it might be best
> to just open-code it, see below.
> 
> > +
> > +int guess_default_geometry(__uint64_t *agsize, __uint64_t *agcount, libxfs_init_t x) {
> 
> Move the "{" down to this line, please, and wrap the args so it doesn't go
> over 80 chars.

Also, please put the function name at the start of a line like most of the
other XFS code:

int
guess_default_geometry(
	__uint64_t	*agsize,
	__uint64_t	*agcount,
	libxfs_init_t	x)
{
	/* ... */
}

This makes it wayyyy easier to grep-hunt for function definitions.

(Try wading through the mishmash of ext4/e2fsprogs until your brain fades...)

--D

> 
> > +	struct fs_topology ft;
> > +	int blocklog;
> > +	__uint64_t      dblocks;
>                   ^tab here please
> > +	int             multidisk;
>            ^tabs here please
> 
> ... and a newline here to separate variable declarations...
> 
> > +        memset(&ft, 0, sizeof(ft));
> 
> ... from this call.  Tab this call in...
> 
> 
> > +        get_topology(&x, &ft, 1);
>    ^^^^ as well as this one.
> 
> > +
> > +        /*
>    ^ tab here too
> > +	 * get geometry from get_topology result.
> > +	 * Use default block size (2^12)
> > +	 */
> > +        blocklog = 12;
> > +        multidisk = ft.dswidth | ft.dsunit;
> > +	printf("x.dsize = %lu\n",(long unsigned int)x.dsize);
> 
> drop the debug printf
> 
> > +        dblocks = DTOBT(x.dsize);
> 
> maybe best to just:
> 
>            dblocks = x.dsize >> (blocklog - BBSHIFT); 
> 
> > +        calc_default_ag_geometry(blocklog, dblocks, multidisk,
> > +                                 agsize, agcount);
> 
> general whitespace problems above; use tabs not spaces please,
> as appropriate.
> 
> > +
> > +	return(blocklog);
> 
> while we're at it it'd be nice to make return not a function; 
> just "return blocklog;"
> 
> > +}
> > diff --git a/repair/Makefile b/repair/Makefile
> > index 251722b..d24ab1f 100644
> > --- a/repair/Makefile
> > +++ b/repair/Makefile
> > @@ -20,8 +20,8 @@ CFILES = agheader.c attr_repair.c avl.c avl64.c bmap.c btree.c \
> >  	progress.c prefetch.c rt.c sb.c scan.c threads.c \
> >  	versions.c xfs_repair.c
> >  
> > -LLDLIBS = $(LIBXFS) $(LIBXLOG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD)
> > -LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG)
> > +LLDLIBS = $(LIBBLKID) $(LIBXFS) $(LIBXLOG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD) $(LIBXCMD)
> > +LTDEPENDENCIES = $(LIBXFS) $(LIBXLOG) $(LIBXCMD)
> >  LLDFLAGS = -static-libtool-libs
> >  
> >  default: depend $(LTCOMMAND)
> > diff --git a/repair/phase1.c b/repair/phase1.c
> > index 126d0b3..c27033d 100644
> > --- a/repair/phase1.c
> > +++ b/repair/phase1.c
> > @@ -21,6 +21,7 @@
> >  #include "agheader.h"
> >  #include "protos.h"
> >  #include "err_protos.h"
> > +#include "libxcmd.h"
> >  
> >  static void
> >  no_sb(void)
> > @@ -47,6 +48,9 @@ alloc_ag_buf(int size)
> >   */
> >  #define MAX_SECTSIZE		(512 * 1024)
> >  
> > +extern libxfs_init_t x;
> 
> It would be better to 
> 
> #include "libxlog.h"
> 
> after the inclusion of libxfs.h at the top of this file, and you won't
> need this extern.  That's how the main file/function gets "x" for
> xfs_repair.  but actually, see more below.
> 
> > +
> > +
> >  /* ARGSUSED */
> >  void
> >  phase1(xfs_mount_t *mp)
> > @@ -54,6 +58,10 @@ phase1(xfs_mount_t *mp)
> >  	xfs_sb_t		*sb;
> >  	char			*ag_bp;
> >  	int			rval;
> > +	int			blocklog;
> > +	__uint64_t		agcount;
> > +	__uint64_t		agsize;
> > +	__uint64_t		skipsize;
> >  
> >  	do_log(_("Phase 1 - find and verify superblock...\n"));
> >  
> > @@ -65,6 +73,18 @@ phase1(xfs_mount_t *mp)
> >  	lost_quotas = 0;
> >  
> >  	/*
> > +	 * divine the geometry 
> > +	 */
> > +	blocklog = guess_default_geometry(&agsize, &agcount, x);
> > +
> > +	/*
> > +	 * use found ag geometry to quickly find secondary sb
> > +	 */
> > +	skipsize = agsize << blocklog;
> > +	do_log("agsize = %d  agcount = %d  skipsize = %d\n",
> > +	       (int)agsize, (int)agcount, (int)skipsize);
> 
> Since this is just a guess, I wouldn't log.  It may well
> print out geometry that is not correct, and that would be confusing.
> 
> And actually - looking at how you have refactored find_secondary_sb(),
> you can move the above 12 or so lines into find_secondary_sb(),
> and you don't need to pre-compute skipsize etc here.  You can do it
> all in find_secondary_sb() when it gets called.
> 
> Then this file (phase1.c) can be completely unchanged, and all the
> magic happens in repair/sb.c.
> 
> > +
> > +	/*
> >  	 * get AG 0 into ag header buf
> >  	 */
> >  	ag_bp = alloc_ag_buf(MAX_SECTSIZE);
> > @@ -80,14 +100,15 @@ phase1(xfs_mount_t *mp)
> >  	if (rval != XR_OK)  {
> >  		do_warn(_("bad primary superblock - %s !!!\n"),
> >  			err_string(rval));
> > -		if (!find_secondary_sb(sb))
> > +
> > +		if (!find_secondary_sb(sb, skipsize))
> >  			no_sb();
> >  		primary_sb_modified = 1;
> >  	} else if ((rval = verify_set_primary_sb(sb, 0,
> >  					&primary_sb_modified)) != XR_OK)  {
> >  		do_warn(_("couldn't verify primary superblock - %s !!!\n"),
> >  			err_string(rval));
> > -		if (!find_secondary_sb(sb))
> > +		if (!find_secondary_sb(sb, skipsize))
> >  			no_sb();
> >  		primary_sb_modified = 1;
> >  	}
> > diff --git a/repair/protos.h b/repair/protos.h
> > index 9d5a2a6..d96373e 100644
> > --- a/repair/protos.h
> > +++ b/repair/protos.h
> > @@ -31,7 +31,7 @@ int	get_sb(xfs_sb_t			*sbp,
> >  void	write_primary_sb(xfs_sb_t	*sbp,
> >  			int		size);
> >  
> > -int	find_secondary_sb(xfs_sb_t	*sb);
> > +int	find_secondary_sb(xfs_sb_t	*sb, __uint64_t skipsize);
> 
> this would be unchanged as well.
> 
> >  
> >  struct fs_geometry;
> >  void	get_sb_geometry(struct fs_geometry	*geo,
> > diff --git a/repair/sb.c b/repair/sb.c
> > index 4eef14a..4386479 100644
> > --- a/repair/sb.c/d
> > +++ b/repair/sb.c
> > @@ -87,8 +87,7 @@ copy_sb(xfs_sb_t *source, xfs_sb_t *dest)
> >  /*
> >   * find a secondary superblock, copy it into the sb buffer
> 
> It'd be good to document "skipsize" here - units, and purpose.
> 
> >   */
> > -int
> > -find_secondary_sb(xfs_sb_t *rsb)
> > +static int __find_secondary_sb(xfs_sb_t *rsb, __uint64_t skipsize )
> 
> no space between skipsize and ")" please.
> 
> >  {
> >  	xfs_off_t	off;
> >  	xfs_sb_t	*sb;
> > @@ -101,7 +100,6 @@ find_secondary_sb(xfs_sb_t *rsb)
> >  	int		bsize;
> >  
> >  	do_warn(_("\nattempting to find secondary superblock...\n"));
> > -
> >  	sb = (xfs_sb_t *)memalign(libxfs_device_alignment(), BSIZE);
> >  	if (!sb) {
> >  		do_error(
> > @@ -117,7 +115,7 @@ find_secondary_sb(xfs_sb_t *rsb)
> >  	/*
> >  	 * skip first sector since we know that's bad
> >  	 */
> > -	for (done = 0, off = XFS_AG_MIN_BYTES; !done ; off += bsize)  {
> > +	for (done = 0, off = skipsize; !done ; off += bsize)  {
> 
> Ok, so you jump out "skipsize" for the first read, but after that you
> only move the offset by bsize, which is BSIZE (or 1MB).
> 
> That works OK if the 2nd superblock (at skipsize) is OK, but if it's
> not, you'll start from there doing smaller reads like before, and
> then this loop isn't really any more efficient than it was.
> 
> If skipsize is set, you need to start out 1x skipsize, then read
> at 2x skipsize, 3x skipsize, etc.  So you need to change the
> loop increment as well.
> 
> i.e. fast path: start at skipsize, advance by skipsize each loop
> 
> slow path: start at XFS_AG_MIN_BYTES, advance by BSIZE each loop
> 
> If you make a 4t sparse file, mkfs it, and corrupt the first *2*
> superblocks, then point repair at it you'll see what I mean.
> 
> >  		/*
> >  		 * read disk 1 MByte at a time.
> >  		 */
> > @@ -130,7 +128,6 @@ find_secondary_sb(xfs_sb_t *rsb)
> >  		}
> >  
> >  		do_warn(".");
> > -
> >  		/*
> >  		 * check the buffer 512 bytes at a time since
> >  		 * we don't know how big the sectors really are.
> > @@ -164,11 +161,29 @@ find_secondary_sb(xfs_sb_t *rsb)
> >  			}
> >  		}
> >  	}
> > -
> >  	free(sb);
> >  	return(retval);
> >  }
> >  
> > +int
> > +find_secondary_sb(xfs_sb_t *rsb, __uint64_t skipsize)
> > +{
> > +	int		retval;
> > +
> > +	/*
> > +	 * Attempt to find secondary sb with a coarse approach,
> > +	 * using a large skipsize (agsize in bytes). Failing that,
> > +	 * fallback to the fine-grained approach using min agsize.
> > +	 */
> > +	retval = __find_secondary_sb(rsb, skipsize);
> > +	if (!retval)
> 
> With the comment below, it's safest to wrap these multiple lines after
> the conditional in { .... }
> 
> > +		/*
> > +		 * fallback: use minimum agsize for skipsize
> > +		 */
> > +		retval = __find_secondary_sb(rsb, XFS_AG_MIN_BYTES);
> > +	return(retval);
> 
> again drop the () on the return, please.
> 
> > +}
> > +
> >  /*
> >   * Calculate what the inode alignment field ought to be based on internal
> >   * superblock info and determine if it is valid.
> > 
> 
> _______________________________________________
> 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] 5+ messages in thread
end of thread, other threads:[~2016-02-09 19:14 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-09 17:13 [PATCH 0/2] xfs_repair: improved secondary sb search Bill O'Donnell
2016-02-09 17:13 ` [PATCH 1/2] libxcmd: generalize topology functions Bill O'Donnell
2016-02-09 17:13 ` [PATCH 2/2] xfs_repair: new secondary superblock search method Bill O'Donnell
2016-02-09 19:05   ` Eric Sandeen
2016-02-09 19:14     ` Darrick J. Wong

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.