[PATCH 00/12] xfsprogs-5.3: various fixes

[PATCH 00/12] xfsprogs-5.3: various fixes

[Darrick J. Wong]

Hi all,

Fix various problems in xfsprogs:

Patch 1 fixes libxfs-diff to handle files that are in libxfs/ in xfsprogs
but still in fs/xfs/ in the kernel.

Patch 2 moves the topology function declarations into a separate header
file since they're no longer libxcmd functionality.

Patch 3 teaches the xfs geometry wrapper function to try the v4 ioctl
calls if the v5 one fails.

Patches 4-7 document the ioctls introduced in 5.2.

Patch 8 removes the nearly empty "convert.h" file from db/ to eliminate
the possibility of confusion with include/convert.h.

Patch 9 adds a new "btheight" command to xfs_db so that we can calculate
the size of each level of a theoretical btree.

Patches 10-11 refactor db and repair to use inode geometry values instead
of recalculating them.

Patch 12 quiets down repair with regards to clearing reflink flags.

If you're going to start using this mess, you probably ought to just
pull from my git trees, which are linked below.

This is an extraordinary way to destroy everything.  Enjoy!
Comments and questions are, as always, welcome.

--D

xfsprogs git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=xfsprogs-5.3-fixes

[PATCH 01/12] libxfs-diff: try harder to find the kernel equivalent libxfs files

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Now that we're syncing userspace libxfs/ files with kernel fs/xfs/
files, teach the diff tool to try fs/xfs/xfs_foo.c if
fs/xfs/libxfs/xfs_foo.c doesn't exist.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tools/libxfs-diff |    1 +
 1 file changed, 1 insertion(+)


diff --git a/tools/libxfs-diff b/tools/libxfs-diff
index fa57c004..c18ad487 100755
--- a/tools/libxfs-diff
+++ b/tools/libxfs-diff
@@ -22,5 +22,6 @@ dir="$(readlink -m "${dir}/..")"
 
 for i in libxfs/xfs*.[ch]; do
 	kfile="${dir}/$i"
+	test -f "${kfile}" || kfile="$(echo "${kfile}" | sed -e 's|libxfs/||g')"
 	diff -Naurpw --label "$i" <(sed -e '/#include/d' "$i") --label "${kfile}" <(sed -e '/#include/d' "${kfile}")
 done

[PATCH 02/12] libxfs: move topology declarations into separate header

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

The topology functions live in libfrog now, which means their
declarations don't belong in libxcmd.h.  Create new header file for
them.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 include/libxcmd.h  |   31 -------------------------------
 include/topology.h |   39 +++++++++++++++++++++++++++++++++++++++
 libfrog/topology.c |    1 +
 mkfs/xfs_mkfs.c    |    2 +-
 repair/sb.c        |    1 +
 5 files changed, 42 insertions(+), 32 deletions(-)
 create mode 100644 include/topology.h


diff --git a/include/libxcmd.h b/include/libxcmd.h
index 20e5d834..7b889b0a 100644
--- a/include/libxcmd.h
+++ b/include/libxcmd.h
@@ -10,35 +10,4 @@
 #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(
-	const char	*device);
-
-
-
 #endif	/* __LIBXCMD_H__ */
diff --git a/include/topology.h b/include/topology.h
new file mode 100644
index 00000000..61ede23a
--- /dev/null
+++ b/include/topology.h
@@ -0,0 +1,39 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2000-2005 Silicon Graphics, Inc.
+ * All Rights Reserved.
+ */
+
+#ifndef __TOPOLOGY_H__
+#define __TOPOLOGY_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(
+	const char	*device);
+
+#endif	/* __TOPOLOGY_H__ */
diff --git a/libfrog/topology.c b/libfrog/topology.c
index cac164f3..e2f87415 100644
--- a/libfrog/topology.c
+++ b/libfrog/topology.c
@@ -10,6 +10,7 @@
 #  include <blkid/blkid.h>
 #endif /* ENABLE_BLKID */
 #include "xfs_multidisk.h"
+#include "topology.h"
 
 #define TERABYTES(count, blog)	((uint64_t)(count) << (40 - (blog)))
 #define GIGABYTES(count, blog)	((uint64_t)(count) << (30 - (blog)))
diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 0bdf6ec3..d05a6898 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -9,7 +9,7 @@
 #include "xfs_multidisk.h"
 #include "libxcmd.h"
 #include "fsgeom.h"
-
+#include "topology.h"
 
 #define TERABYTES(count, blog)	((uint64_t)(count) << (40 - (blog)))
 #define GIGABYTES(count, blog)	((uint64_t)(count) << (30 - (blog)))
diff --git a/repair/sb.c b/repair/sb.c
index 119bf219..547969f7 100644
--- a/repair/sb.c
+++ b/repair/sb.c
@@ -12,6 +12,7 @@
 #include "protos.h"
 #include "err_protos.h"
 #include "xfs_multidisk.h"
+#include "topology.h"
 
 #define BSIZE	(1024 * 1024)
 

[PATCH 03/12] libfrog: try the v4 fs geometry ioctl after failing the v5 ioctl

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

If a v5 fs geometry query fails, try the v4 interface before falling
back to the v1 interface.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libfrog/fsgeom.c |    4 ++++
 1 file changed, 4 insertions(+)


diff --git a/libfrog/fsgeom.c b/libfrog/fsgeom.c
index a3b748f8..06e4e663 100644
--- a/libfrog/fsgeom.c
+++ b/libfrog/fsgeom.c
@@ -84,6 +84,10 @@ xfrog_geometry(
 	if (!ret)
 		return 0;
 
+	ret = ioctl(fd, XFS_IOC_FSGEOMETRY_V4, fsgeo);
+	if (!ret)
+		return 0;
+
 	return ioctl(fd, XFS_IOC_FSGEOMETRY_V1, fsgeo);
 }
 

[PATCH 04/12] man: document the new v5 fs geometry ioctl structures

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Amend the fs geometry ioctl documentation to cover the new v5 structure.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libfrog/fsgeom.c                   |    4 ++++
 man/man2/ioctl_xfs_fsop_geometry.2 |    8 ++++++++
 2 files changed, 12 insertions(+)


diff --git a/libfrog/fsgeom.c b/libfrog/fsgeom.c
index 06e4e663..159738c5 100644
--- a/libfrog/fsgeom.c
+++ b/libfrog/fsgeom.c
@@ -88,6 +88,10 @@ xfrog_geometry(
 	if (!ret)
 		return 0;
 
+	ret = ioctl(fd, XFS_IOC_FSGEOMETRY_V4, fsgeo);
+	if (!ret)
+		return 0;
+
 	return ioctl(fd, XFS_IOC_FSGEOMETRY_V1, fsgeo);
 }
 
diff --git a/man/man2/ioctl_xfs_fsop_geometry.2 b/man/man2/ioctl_xfs_fsop_geometry.2
index 68e3387d..365bda8b 100644
--- a/man/man2/ioctl_xfs_fsop_geometry.2
+++ b/man/man2/ioctl_xfs_fsop_geometry.2
@@ -12,6 +12,8 @@ ioctl_xfs_fsop_geometry \- report XFS filesystem layout and features
 .PP
 .BI "int ioctl(int " fd ", XFS_IOC_FSOP_GEOMETRY, struct xfs_fsop_geom*" arg );
 .br
+.BI "int ioctl(int " fd ", XFS_IOC_FSOP_GEOMETRY_V4, struct xfs_fsop_geom_v4 *" arg );
+.br
 .BI "int ioctl(int " fd ", XFS_IOC_FSOP_GEOMETRY_V1, struct xfs_fsop_geom_v1 *" arg );
 .SH DESCRIPTION
 Report the details of an XFS filesystem layout, features, and other descriptive items.
@@ -43,6 +45,9 @@ struct xfs_fsop_geom {
 	/* struct xfs_fsop_geom_v1 stops here. */
 
 	__u32         logsunit;
+	/* struct xfs_fsop_geom_v4 stops here. */
+
+	__u64         reserved[18];
 };
 .fi
 .in
@@ -124,6 +129,9 @@ underlying log device, in filesystem blocks.
 This field is meaningful only if the flag
 .B  XFS_FSOP_GEOM_FLAGS_LOGV2
 is set.
+.PP
+.I reserved
+is set to zero.
 .SH FILESYSTEM FEATURE FLAGS
 Filesystem features are reported to userspace as a combination the following
 flags:

[PATCH 05/12] man: document new fs summary counter scrub command

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Update the scrub ioctl documentation to include the new fs summary
counter scrubber.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 man/man2/ioctl_xfs_scrub_metadata.2 |    5 +++++
 1 file changed, 5 insertions(+)


diff --git a/man/man2/ioctl_xfs_scrub_metadata.2 b/man/man2/ioctl_xfs_scrub_metadata.2
index 1e80ee71..046e3e36 100644
--- a/man/man2/ioctl_xfs_scrub_metadata.2
+++ b/man/man2/ioctl_xfs_scrub_metadata.2
@@ -159,6 +159,11 @@ corruption.
 .TP
 .B XFS_SCRUB_TYPE_PQUOTA
 Examine all user, group, or project quota records for corruption.
+
+.TP
+.B XFS_SCRUB_TYPE_FSCOUNTERS
+Examine all filesystem summary counters (free blocks, inode count, free inode
+count) for errors.
 .RE
 
 .PD 1

[PATCH 06/12] man: document the new allocation group geometry ioctl

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Document the new ioctl to describe an allocation group's geometry.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 man/man2/ioctl_xfs_ag_geometry.2 |   74 ++++++++++++++++++++++++++++++++++++++
 man/man3/xfsctl.3                |    6 +++
 2 files changed, 80 insertions(+)
 create mode 100644 man/man2/ioctl_xfs_ag_geometry.2


diff --git a/man/man2/ioctl_xfs_ag_geometry.2 b/man/man2/ioctl_xfs_ag_geometry.2
new file mode 100644
index 00000000..5dfe0d08
--- /dev/null
+++ b/man/man2/ioctl_xfs_ag_geometry.2
@@ -0,0 +1,74 @@
+.\" Copyright (c) 2019, Oracle.  All rights reserved.
+.\"
+.\" %%%LICENSE_START(GPLv2+_DOC_FULL)
+.\" SPDX-License-Identifier: GPL-2.0+
+.\" %%%LICENSE_END
+.TH IOCTL-XFS-AG-GEOMETRY 2 2019-04-11 "XFS"
+.SH NAME
+ioctl_xfs_ag_geometry \- query XFS allocation group geometry information
+.SH SYNOPSIS
+.br
+.B #include <xfs/xfs_fs.h>
+.PP
+.BI "int ioctl(int " fd ", XFS_IOC_AG_GEOMETRY, struct xfs_ag_geometry *" arg );
+.SH DESCRIPTION
+This XFS ioctl retrieves the geometry information for a given allocation group.
+The geometry information is conveyed in a structure of the following form:
+.PP
+.in +4n
+.nf
+struct xfs_ag_geometry {
+	uint32_t  ag_number;
+	uint32_t  ag_length;
+	uint32_t  ag_freeblks;
+	uint32_t  ag_icount;
+	uint32_t  ag_ifree;
+	uint32_t  ag_sick;
+	uint32_t  ag_checked;
+	uint32_t  ag_reserved32;
+	uint64_t  ag_reserved[12];
+};
+.fi
+.in
+.TP
+.I ag_number
+The number of allocation group that the caller wishes to learn about.
+.TP
+.I ag_length
+Length of the allocation group, in units of filesystem blocks.
+.TP
+.I ag_freeblks
+Number of free blocks in the allocation group, in units of filesystem blocks.
+.TP
+.I ag_icount
+Number of inode records allocated in this allocation group.
+.TP
+.I ag_ifree
+Number of unused inode records (of the space allocated) in this allocation
+group.
+.TP
+.IR ag_reserved " and " ag_reserved32
+Will be set to zero.
+.SH RETURN VALUE
+On error, \-1 is returned, and
+.I errno
+is set to indicate the error.
+.PP
+.SH ERRORS
+Error codes can be one of, but are not limited to, the following:
+.TP
+.B EFSBADCRC
+Metadata checksum validation failed while performing the query.
+.TP
+.B EFSCORRUPTED
+Metadata corruption was encountered while performing the query.
+.TP
+.B EINVAL
+The specified allocation group number is not valid for this filesystem.
+.TP
+.B EIO
+An I/O error was encountered while performing the query.
+.SH CONFORMING TO
+This API is specific to XFS filesystem on the Linux kernel.
+.SH SEE ALSO
+.BR ioctl (2)
diff --git a/man/man3/xfsctl.3 b/man/man3/xfsctl.3
index 7e6588b8..dfebd12d 100644
--- a/man/man3/xfsctl.3
+++ b/man/man3/xfsctl.3
@@ -336,6 +336,12 @@ See
 .BR ioctl_xfs_fsop_geometry (2)
 for more information.
 
+.TP
+.B XFS_IOC_AG_GEOMETRY
+See
+.BR ioctl_xfs_ag_geometry (2)
+for more information.
+
 .TP
 .BR XFS_IOC_FSBULKSTAT " or " XFS_IOC_FSBULKSTAT_SINGLE
 See

[PATCH 07/12] man: document the new health reporting fields in various ioctls

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Update the manpages to conver the new health reporting fields in the
fs geometry, ag geometry, and bulkstat ioctls.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 man/man2/ioctl_xfs_ag_geometry.2   |   48 +++++++++++++++++++++++++++++++
 man/man2/ioctl_xfs_fsbulkstat.2    |   52 +++++++++++++++++++++++++++++++++
 man/man2/ioctl_xfs_fsop_geometry.2 |   56 +++++++++++++++++++++++++++++++++++-
 3 files changed, 154 insertions(+), 2 deletions(-)


diff --git a/man/man2/ioctl_xfs_ag_geometry.2 b/man/man2/ioctl_xfs_ag_geometry.2
index 5dfe0d08..cf6aec1d 100644
--- a/man/man2/ioctl_xfs_ag_geometry.2
+++ b/man/man2/ioctl_xfs_ag_geometry.2
@@ -49,6 +49,54 @@ group.
 .TP
 .IR ag_reserved " and " ag_reserved32
 Will be set to zero.
+.PP
+The fields
+.IR ag_sick " and " ag_checked
+indicate the relative health of various allocation group metadata:
+.IP \[bu] 2
+If a given sick flag is set in
+.IR ag_sick ,
+then that piece of metadata has been observed to be damaged.
+The same bit will be set in
+.IR ag_checked .
+.IP \[bu]
+If a given sick flag is set in
+.I ag_checked
+and is not set in
+.IR ag_sick ,
+then that piece of metadata has been checked and is not faulty.
+.IP \[bu]
+If a given sick flag is not set in
+.IR ag_checked ,
+then no conclusion can be made.
+.PP
+The following flags apply to these fields:
+.RS 0.4i
+.TP
+.B XFS_AG_GEOM_SICK_SB
+Allocation group superblock.
+.TP
+.B XFS_AG_GEOM_SICK_AGF
+Free space header.
+.TP
+.B XFS_AG_GEOM_SICK_AGFL
+Free space reserve list.
+.TP
+.B XFS_AG_GEOM_SICK_AGI
+Inode header.
+.TP
+.BR XFS_AG_GEOM_SICK_BNOBT " or " XFS_AG_GEOM_SICK_CNTBT
+Free space btrees.
+.TP
+.BR XFS_AG_GEOM_SICK_INOBT " or " XFS_AG_GEOM_SICK_FINOBT
+Inode btrees.
+.TP
+.B XFS_AG_GEOM_SICK_RMAPBT
+Reverse mapping btree.
+.TP
+.B XFS_AG_GEOM_SICK_REFCNTBT
+Reference count btree.
+.RE
 .SH RETURN VALUE
 On error, \-1 is returned, and
 .I errno
diff --git a/man/man2/ioctl_xfs_fsbulkstat.2 b/man/man2/ioctl_xfs_fsbulkstat.2
index a8b22dc4..3e13cfa8 100644
--- a/man/man2/ioctl_xfs_fsbulkstat.2
+++ b/man/man2/ioctl_xfs_fsbulkstat.2
@@ -94,7 +94,9 @@ struct xfs_bstat {
 	__u16             bs_projid_lo;
 	__u16             bs_forkoff;
 	__u16             bs_projid_hi;
-	unsigned char     bs_pad[6];
+	uint16_t          bs_sick;
+	uint16_t          bs_checked;
+	unsigned char     bs_pad[2];
 	__u32             bs_cowextsize;
 	__u32             bs_dmevmask;
 	__u16             bs_dmstate;
@@ -184,6 +186,54 @@ is unused on Linux.
 .I bs_aextents
 is the number of storage mappings associated with this file's extended
 attributes.
+.PP
+The fields
+.IR bs_sick " and " bs_checked
+indicate the relative health of various allocation group metadata:
+.IP \[bu] 2
+If a given sick flag is set in
+.IR bs_sick ,
+then that piece of metadata has been observed to be damaged.
+The same bit should be set in
+.IR bs_checked .
+.IP \[bu]
+If a given sick flag is set in
+.I bs_checked
+but is not set in
+.IR bs_sick ,
+then that piece of metadata has been checked and is not faulty.
+.IP \[bu]
+If a given sick flag is not set in
+.IR bs_checked ,
+then no conclusion can be made.
+.PP
+The following flags apply to these fields:
+.RS 0.4i
+.TP
+.B XFS_BS_SICK_INODE
+The inode's record itself.
+.TP
+.B XFS_BS_SICK_BMBTD
+File data extent mappings.
+.TP
+.B XFS_BS_SICK_BMBTA
+Extended attribute extent mappings.
+.TP
+.B XFS_BS_SICK_BMBTC
+Copy on Write staging extent mappings.
+.TP
+.B XFS_BS_SICK_DIR
+Directory information.
+.TP
+.B XFS_BS_SICK_XATTR
+Extended attribute data.
+.TP
+.B XFS_BS_SICK_SYMLINK
+Symbolic link target.
+.TP
+.B XFS_BS_SICK_PARENT
+Parent pointers.
+.RE
 .SH RETURN VALUE
 On error, \-1 is returned, and
 .I errno
diff --git a/man/man2/ioctl_xfs_fsop_geometry.2 b/man/man2/ioctl_xfs_fsop_geometry.2
index 365bda8b..a35bbaeb 100644
--- a/man/man2/ioctl_xfs_fsop_geometry.2
+++ b/man/man2/ioctl_xfs_fsop_geometry.2
@@ -47,7 +47,9 @@ struct xfs_fsop_geom {
 	__u32         logsunit;
 	/* struct xfs_fsop_geom_v4 stops here. */
 
-	__u64         reserved[18];
+	__u32         sick;
+	__u32         checked;
+	__u64         reserved[17];
 };
 .fi
 .in
@@ -130,6 +132,13 @@ This field is meaningful only if the flag
 .B  XFS_FSOP_GEOM_FLAGS_LOGV2
 is set.
 .PP
+The fields
+.IR sick " and " checked
+indicate the relative health of various whole-filesystem metadata.
+Please see the section
+.B XFS METADATA HEALTH REPORTING
+for more details.
+.PP
 .I reserved
 is set to zero.
 .SH FILESYSTEM FEATURE FLAGS
@@ -203,6 +212,51 @@ Filesystem stores reverse mappings of blocks to owners.
 .B XFS_FSOP_GEOM_FLAGS_REFLINK
 Filesystem supports sharing blocks between files.
 .RE
+.SH XFS METADATA HEALTH REPORTING
+.PP
+The online filesystem checking utility scans metadata and records what it
+finds in the kernel incore state.
+The following scheme is used for userspace to read the incore health status
+of the filesystem:
+
+.IP \[bu] 2
+If a given sick flag is set in
+.IR sick ,
+then that piece of metadata has been observed to be damaged.
+The same bit should be set in
+.IR checked .
+.IP \[bu]
+If a given sick flag is set in
+.I checked
+but is not set in
+.IR sick ,
+then that piece of metadata has been checked and is not faulty.
+.IP \[bu]
+If a given sick flag is not set in
+.IR checked ,
+then no conclusion can be made.
+.PP
+The following flags apply to these fields:
+.RS 0.4i
+.TP
+.B XFS_FSOP_GEOM_SICK_COUNTERS
+Inode and space summary counters.
+.TP
+.B XFS_FSOP_GEOM_SICK_UQUOTA
+User quota information.
+.TP
+.B XFS_FSOP_GEOM_SICK_GQUOTA
+Group quota information.
+.TP
+.B XFS_FSOP_GEOM_SICK_PQUOTA
+Project quota information.
+.TP
+.B XFS_FSOP_GEOM_SICK_RT_BITMAP
+Free space bitmap for the realtime device.
+.TP
+.B XFS_FSOP_GEOM_SICK_RT_SUMMARY
+Free space summary for the realtime device.
+.RE
 
 .SH RETURN VALUE
 On error, \-1 is returned, and

[PATCH 08/12] xfs_db: remove db/convert.h

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

db/convert.h conflicts with include/convert.h and since the former only
has one declaration in it anyway, just get rid of it.  We'll need this
in the next patch to avoid an ugly include mess.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/Makefile  |    4 ++--
 db/command.c |    1 -
 db/command.h |    1 +
 db/convert.c |    1 -
 db/convert.h |    7 -------
 5 files changed, 3 insertions(+), 11 deletions(-)
 delete mode 100644 db/convert.h


diff --git a/db/Makefile b/db/Makefile
index 8fecfc1c..0941b32e 100644
--- a/db/Makefile
+++ b/db/Makefile
@@ -8,13 +8,13 @@ include $(TOPDIR)/include/builddefs
 LTCOMMAND = xfs_db
 
 HFILES = addr.h agf.h agfl.h agi.h attr.h attrshort.h bit.h block.h bmap.h \
-	btblock.h bmroot.h check.h command.h convert.h crc.h debug.h \
+	btblock.h bmroot.h check.h command.h crc.h debug.h \
 	dir2.h dir2sf.h dquot.h echo.h faddr.h field.h \
 	flist.h fprint.h frag.h freesp.h hash.h help.h init.h inode.h input.h \
 	io.h logformat.h malloc.h metadump.h output.h print.h quit.h sb.h \
 	sig.h strvec.h text.h type.h write.h attrset.h symlink.h fsmap.h \
 	fuzz.h
-CFILES = $(HFILES:.h=.c) btdump.c info.c
+CFILES = $(HFILES:.h=.c) btdump.c convert.c info.c
 LSRCFILES = xfs_admin.sh xfs_ncheck.sh xfs_metadump.sh
 
 LLDLIBS	= $(LIBXFS) $(LIBXLOG) $(LIBFROG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD)
diff --git a/db/command.c b/db/command.c
index c7c52342..89a78f03 100644
--- a/db/command.c
+++ b/db/command.c
@@ -11,7 +11,6 @@
 #include "bmap.h"
 #include "check.h"
 #include "command.h"
-#include "convert.h"
 #include "debug.h"
 #include "type.h"
 #include "echo.h"
diff --git a/db/command.h b/db/command.h
index eacfd465..2f9a7e16 100644
--- a/db/command.h
+++ b/db/command.h
@@ -28,5 +28,6 @@ extern int		command(int argc, char **argv);
 extern const cmdinfo_t	*find_command(const char *cmd);
 extern void		init_commands(void);
 
+extern void		convert_init(void);
 extern void		btdump_init(void);
 extern void		info_init(void);
diff --git a/db/convert.c b/db/convert.c
index 01a08823..e1466057 100644
--- a/db/convert.c
+++ b/db/convert.c
@@ -6,7 +6,6 @@
 
 #include "libxfs.h"
 #include "command.h"
-#include "convert.h"
 #include "output.h"
 #include "init.h"
 
diff --git a/db/convert.h b/db/convert.h
deleted file mode 100644
index 3660cabe..00000000
--- a/db/convert.h
+++ /dev/null
@@ -1,7 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Copyright (c) 2000-2001,2005 Silicon Graphics, Inc.
- * All Rights Reserved.
- */
-
-extern void	convert_init(void);

[PATCH 09/12] xfs_db: add a function to compute btree geometry

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Add a new command to xfs_db that uses a btree type and a record count
to report the best and worst case height and level size.  This can be
used to estimate how overhead a metadata index will add to a filesystem.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/Makefile              |    2 
 db/btheight.c            |  308 ++++++++++++++++++++++++++++++++++++++++++++++
 db/command.c             |    1 
 db/command.h             |    1 
 libxfs/libxfs_api_defs.h |    2 
 5 files changed, 313 insertions(+), 1 deletion(-)
 create mode 100644 db/btheight.c


diff --git a/db/Makefile b/db/Makefile
index 0941b32e..ed9f56c2 100644
--- a/db/Makefile
+++ b/db/Makefile
@@ -14,7 +14,7 @@ HFILES = addr.h agf.h agfl.h agi.h attr.h attrshort.h bit.h block.h bmap.h \
 	io.h logformat.h malloc.h metadump.h output.h print.h quit.h sb.h \
 	sig.h strvec.h text.h type.h write.h attrset.h symlink.h fsmap.h \
 	fuzz.h
-CFILES = $(HFILES:.h=.c) btdump.c convert.c info.c
+CFILES = $(HFILES:.h=.c) btdump.c btheight.c convert.c info.c
 LSRCFILES = xfs_admin.sh xfs_ncheck.sh xfs_metadump.sh
 
 LLDLIBS	= $(LIBXFS) $(LIBXLOG) $(LIBFROG) $(LIBUUID) $(LIBRT) $(LIBPTHREAD)
diff --git a/db/btheight.c b/db/btheight.c
new file mode 100644
index 00000000..1b53bf61
--- /dev/null
+++ b/db/btheight.c
@@ -0,0 +1,308 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2019 Oracle.  All Rights Reserved.
+ * Author: Darrick J. Wong <darrick.wong@oracle.com>
+ */
+#include "libxfs.h"
+#include "command.h"
+#include "output.h"
+#include "init.h"
+#include "io.h"
+#include "type.h"
+#include "input.h"
+#include "convert.h"
+
+static int refc_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
+{
+	return libxfs_refcountbt_maxrecs(blocklen, leaf != 0);
+}
+
+static int rmap_maxrecs(struct xfs_mount *mp, int blocklen, int leaf)
+{
+	return libxfs_rmapbt_maxrecs(blocklen, leaf);
+}
+
+struct btmap {
+	const char	*tag;
+	int		(*maxrecs)(struct xfs_mount *mp, int blocklen,
+				   int leaf);
+} maps[] = {
+	{"bnobt", libxfs_allocbt_maxrecs},
+	{"cntbt", libxfs_allocbt_maxrecs},
+	{"inobt", libxfs_inobt_maxrecs},
+	{"finobt", libxfs_inobt_maxrecs},
+	{"bmapbt", libxfs_bmbt_maxrecs},
+	{"refcountbt", refc_maxrecs},
+	{"rmapbt", rmap_maxrecs},
+};
+
+static void
+btheight_help(void)
+{
+	struct btmap	*m;
+	int		i;
+
+	dbprintf(_(
+"\n"
+" For a given number of btree records and a btree type, report the number of\n"
+" records and blocks for each level of the btree, and the total btree size.\n"
+" The btree type must be given after the options.  A raw btree geometry can\n"
+" be provided in the format \"record_bytes:key_bytes:ptr_bytes:header_type\"\n"
+" where header_type is one of \"short\", \"long\", \"shortcrc\", or \"longcrc\".\n"
+"\n"
+" Options:\n"
+"   -b -- Override the btree block size.\n"
+"   -n -- Number of records we want to store.\n"
+"   -w max -- Show only the best case scenario.\n"
+"   -w min -- Show only the worst case scenario.\n"
+"\n"
+" Supported btree types:\n"
+"   "
+));
+	for (i = 0, m = maps; i < ARRAY_SIZE(maps); i++, m++)
+		printf("%s ", m->tag);
+	printf("\n");
+}
+
+static void
+calc_height(
+	unsigned long long	nr_records,
+	uint			*records_per_block)
+{
+	unsigned int		level = 0;
+	unsigned long long	total_blocks = 0;
+	unsigned long long	blocks;
+	char			*levels_suffix = "s";
+	char			*totblocks_suffix = "s";
+
+	while (nr_records) {
+		unsigned int	level_rpb = records_per_block[level != 0];
+		char		*recs_suffix = "s";
+		char		*blocks_suffix = "s";
+
+		blocks = (nr_records + level_rpb - 1) / level_rpb;
+
+		if (nr_records == 1)
+			recs_suffix = "";
+		if (blocks == 1)
+			blocks_suffix = "";
+
+		printf(_("level %u: %llu record%s, %llu block%s\n"),
+				level, nr_records, recs_suffix, blocks,
+				blocks_suffix);
+
+		total_blocks += blocks;
+		nr_records = blocks == 1 ? 0 : blocks;
+		level++;
+	}
+
+	if (level == 1)
+		levels_suffix = "";
+	if (total_blocks == 1)
+		totblocks_suffix = "";
+
+	printf(_("%u level%s, %llu block%s total\n"), level, levels_suffix,
+			total_blocks, totblocks_suffix);
+}
+
+static int
+construct_records_per_block(
+	char		*tag,
+	int		blocksize,
+	unsigned int	*records_per_block)
+{
+	char		*toktag;
+	struct btmap	*m;
+	unsigned int	record_size, key_size, ptr_size;
+	char		*p;
+	int		i, ret;
+
+	for (i = 0, m = maps; i < ARRAY_SIZE(maps); i++, m++) {
+		if (!strcmp(m->tag, tag)) {
+			records_per_block[0] = m->maxrecs(mp, blocksize, 1);
+			records_per_block[1] = m->maxrecs(mp, blocksize, 0);
+			return 0;
+		}
+	}
+
+	toktag = strdup(tag);
+	ret = -1;
+
+	p = strtok(toktag, ":");
+	if (!p) {
+		fprintf(stderr, _("%s: record size not found.\n"), tag);
+		goto out;
+	}
+	record_size = cvt_u16(p, 0);
+	if (errno) {
+		perror(p);
+		goto out;
+	}
+
+	p = strtok(NULL, ":");
+	if (!p) {
+		fprintf(stderr, _("%s: key size not found.\n"), tag);
+		goto out;
+	}
+	key_size = cvt_u16(p, 0);
+	if (errno) {
+		perror(p);
+		goto out;
+	}
+
+	p = strtok(NULL, ":");
+	if (!p) {
+		fprintf(stderr, _("%s: pointer size not found.\n"), tag);
+		goto out;
+	}
+	ptr_size = cvt_u16(p, 0);
+	if (errno) {
+		perror(p);
+		goto out;
+	}
+
+	p = strtok(NULL, ":");
+	if (!p) {
+		fprintf(stderr, _("%s: header type not found.\n"), tag);
+		goto out;
+	}
+	if (!strcmp(p, "short"))
+		blocksize -= XFS_BTREE_SBLOCK_LEN;
+	else if (!strcmp(p, "shortcrc"))
+		blocksize -= XFS_BTREE_SBLOCK_CRC_LEN;
+	else if (!strcmp(p, "long"))
+		blocksize -= XFS_BTREE_LBLOCK_LEN;
+	else if (!strcmp(p, "longcrc"))
+		blocksize -= XFS_BTREE_LBLOCK_CRC_LEN;
+	else {
+		fprintf(stderr, _("%s: unrecognized btree header type."),
+				p);
+		goto out;
+	}
+
+	p = strtok(NULL, ":");
+	if (p) {
+		fprintf(stderr,
+			_("%s: unrecognized raw btree geometry."),
+				tag);
+		goto out;
+	}
+
+	records_per_block[0] = blocksize / record_size;
+	records_per_block[1] = blocksize / (key_size + ptr_size);
+	ret = 0;
+out:
+	free(toktag);
+	return ret;
+}
+
+#define REPORT_DEFAULT	(-1U)
+#define REPORT_MAX	(1 << 0)
+#define REPORT_MIN	(1 << 1)
+
+static void
+report(
+	char			*tag,
+	unsigned int		report_what,
+	unsigned long long	nr_records,
+	unsigned int		blocksize)
+{
+	unsigned int		records_per_block[2];
+	int			ret;
+
+	ret = construct_records_per_block(tag, blocksize, records_per_block);
+	if (ret) {
+		printf(_("%s: Unable to determine records per block.\n"),
+				tag);
+		return;
+	}
+
+	if (report_what & REPORT_MAX) {
+		printf(
+_("%s: best case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
+				tag, blocksize, records_per_block[0],
+				records_per_block[1]);
+
+		calc_height(nr_records, records_per_block);
+	}
+
+	if (report_what & REPORT_MIN) {
+		records_per_block[0] /= 2;
+		records_per_block[1] /= 2;
+
+		printf(
+_("%s: worst case per %u-byte block: %u records (leaf) / %u keyptrs (node)\n"),
+				tag, blocksize, records_per_block[0],
+				records_per_block[1]);
+
+		calc_height(nr_records, records_per_block);
+	}
+}
+
+static int
+btheight_f(
+	int		argc,
+	char		**argv)
+{
+	long long	blocksize = mp->m_sb.sb_blocksize;
+	uint64_t	nr_records = 0;
+	int		report_what = REPORT_DEFAULT;
+	int		i, c;
+
+	while ((c = getopt(argc, argv, "b:n:w:")) != -1) {
+		switch (c) {
+		case 'b':
+			errno = 0;
+			blocksize = cvtnum(mp->m_sb.sb_blocksize,
+					mp->m_sb.sb_sectsize,
+					optarg);
+			if (errno) {
+				perror(optarg);
+				return 0;
+			}
+			break;
+		case 'n':
+			nr_records = cvt_u64(optarg, 0);
+			if (errno) {
+				perror(optarg);
+				return 0;
+			}
+			break;
+		case 'w':
+			if (!strcmp(optarg, "min"))
+				report_what = REPORT_MIN;
+			else if (!strcmp(optarg, "max"))
+				report_what = REPORT_MAX;
+			else {
+				btheight_help();
+				return 0;
+			}
+			break;
+		default:
+			btheight_help();
+			return 0;
+		}
+	}
+
+	if (argc == optind || blocksize <= 0 || blocksize > INT_MAX ||
+	    nr_records == 0) {
+		btheight_help();
+		return 0;
+	}
+
+	for (i = optind; i < argc; i++)
+		report(argv[i], report_what, nr_records, blocksize);
+
+	return 0;
+}
+
+static const cmdinfo_t btheight_cmd =
+	{ "btheight", "b", btheight_f, 1, -1, 0,
+	  "[-b blksz] [-n recs] [-w max|-w min] btree types...",
+	  N_("compute btree heights"), btheight_help };
+
+void
+btheight_init(void)
+{
+	add_command(&btheight_cmd);
+}
diff --git a/db/command.c b/db/command.c
index 89a78f03..0fb44efa 100644
--- a/db/command.c
+++ b/db/command.c
@@ -113,6 +113,7 @@ init_commands(void)
 	block_init();
 	bmap_init();
 	btdump_init();
+	btheight_init();
 	check_init();
 	convert_init();
 	crc_init();
diff --git a/db/command.h b/db/command.h
index 2f9a7e16..b8499de0 100644
--- a/db/command.h
+++ b/db/command.h
@@ -31,3 +31,4 @@ extern void		init_commands(void);
 extern void		convert_init(void);
 extern void		btdump_init(void);
 extern void		info_init(void);
+extern void		btheight_init(void);
diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index 7bd373f7..0ae21318 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -125,6 +125,8 @@
 #define xfs_dir_ino_validate		libxfs_dir_ino_validate
 #define xfs_initialize_perag_data	libxfs_initialize_perag_data
 #define xfs_inobt_maxrecs		libxfs_inobt_maxrecs
+#define xfs_rmapbt_maxrecs		libxfs_rmapbt_maxrecs
+#define xfs_refcountbt_maxrecs		libxfs_refcountbt_maxrecs
 #define xfs_iread_extents		libxfs_iread_extents
 #define xfs_log_calc_minimum_size	libxfs_log_calc_minimum_size
 #define xfs_perag_get			libxfs_perag_get

[PATCH 10/12] xfs_db: use precomputed inode geometry values

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Use the precomputed inode geometry values instead of open-coding them.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/inode.c      |    8 +++-----
 repair/dinode.c |    2 +-
 2 files changed, 4 insertions(+), 6 deletions(-)


diff --git a/db/inode.c b/db/inode.c
index 73dd118d..d8d69ffb 100644
--- a/db/inode.c
+++ b/db/inode.c
@@ -657,16 +657,14 @@ set_cur_inode(
 	    igeo->inoalign_mask) {
 		xfs_agblock_t	chunk_agbno;
 		xfs_agblock_t	offset_agbno;
-		int		blks_per_cluster;
 
-		blks_per_cluster = igeo->inode_cluster_size >>
-							mp->m_sb.sb_blocklog;
 		offset_agbno = agbno & igeo->inoalign_mask;
 		chunk_agbno = agbno - offset_agbno;
 		cluster_agbno = chunk_agbno +
-			((offset_agbno / blks_per_cluster) * blks_per_cluster);
+			((offset_agbno / M_IGEO(mp)->blocks_per_cluster) *
+			 M_IGEO(mp)->blocks_per_cluster);
 		offset += ((agbno - cluster_agbno) * mp->m_sb.sb_inopblock);
-		numblks = XFS_FSB_TO_BB(mp, blks_per_cluster);
+		numblks = XFS_FSB_TO_BB(mp, M_IGEO(mp)->blocks_per_cluster);
 	} else
 		cluster_agbno = agbno;
 
diff --git a/repair/dinode.c b/repair/dinode.c
index 56992dd2..f5e88cc3 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -768,7 +768,7 @@ get_agino_buf(
 	 * we must find the buffer for its cluster, add the appropriate
 	 * offset, and return that.
 	 */
-	cluster_size = max(igeo->inode_cluster_size, mp->m_sb.sb_blocksize);
+	cluster_size = igeo->inode_cluster_size;
 	ino_per_cluster = cluster_size / mp->m_sb.sb_inodesize;
 	cluster_agino = agino & ~(ino_per_cluster - 1);
 	cluster_blks = XFS_FSB_TO_DADDR(mp, max(1,

[PATCH 11/12] xfs_repair: use precomputed inode geometry values

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Use the precomputed inode geometry values instead of open-coding them.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/dino_chunks.c |   22 +++++++++++-----------
 repair/dinode.c      |   13 ++++---------
 repair/globals.c     |    1 -
 repair/globals.h     |    1 -
 repair/prefetch.c    |   21 ++++++++-------------
 repair/xfs_repair.c  |    2 --
 6 files changed, 23 insertions(+), 37 deletions(-)


diff --git a/repair/dino_chunks.c b/repair/dino_chunks.c
index 323a355e..00b67468 100644
--- a/repair/dino_chunks.c
+++ b/repair/dino_chunks.c
@@ -608,7 +608,6 @@ process_inode_chunk(
 	xfs_ino_t		ino;
 	int			dirty = 0;
 	int			isa_dir = 0;
-	int			blks_per_cluster;
 	int			cluster_count;
 	int			bp_index;
 	int			cluster_offset;
@@ -620,10 +619,7 @@ process_inode_chunk(
 	*bogus = 0;
 	ASSERT(igeo->ialloc_blks > 0);
 
-	blks_per_cluster = M_IGEO(mp)->inode_cluster_size >> mp->m_sb.sb_blocklog;
-	if (blks_per_cluster == 0)
-		blks_per_cluster = 1;
-	cluster_count = XFS_INODES_PER_CHUNK / inodes_per_cluster;
+	cluster_count = XFS_INODES_PER_CHUNK / M_IGEO(mp)->inodes_per_cluster;
 	if (cluster_count == 0)
 		cluster_count = 1;
 
@@ -662,13 +658,16 @@ process_inode_chunk(
 
 		bplist[bp_index] = libxfs_readbuf(mp->m_dev,
 					XFS_AGB_TO_DADDR(mp, agno, agbno),
-					XFS_FSB_TO_BB(mp, blks_per_cluster), 0,
+					XFS_FSB_TO_BB(mp,
+						M_IGEO(mp)->blocks_per_cluster),
+					0,
 					&xfs_inode_buf_ops);
 		if (!bplist[bp_index]) {
 			do_warn(_("cannot read inode %" PRIu64 ", disk block %" PRId64 ", cnt %d\n"),
 				XFS_AGINO_TO_INO(mp, agno, first_irec->ino_startnum),
 				XFS_AGB_TO_DADDR(mp, agno, agbno),
-				XFS_FSB_TO_BB(mp, blks_per_cluster));
+				XFS_FSB_TO_BB(mp,
+					M_IGEO(mp)->blocks_per_cluster));
 			while (bp_index > 0) {
 				bp_index--;
 				libxfs_putbuf(bplist[bp_index]);
@@ -684,8 +683,9 @@ process_inode_chunk(
 		bplist[bp_index]->b_ops = &xfs_inode_buf_ops;
 
 next_readbuf:
-		irec_offset += mp->m_sb.sb_inopblock * blks_per_cluster;
-		agbno += blks_per_cluster;
+		irec_offset += mp->m_sb.sb_inopblock *
+				M_IGEO(mp)->blocks_per_cluster;
+		agbno += M_IGEO(mp)->blocks_per_cluster;
 	}
 	agbno = XFS_AGINO_TO_AGBNO(mp, first_irec->ino_startnum);
 
@@ -745,7 +745,7 @@ process_inode_chunk(
 				ASSERT(ino_rec->ino_startnum == agino + 1);
 				irec_offset = 0;
 			}
-			if (cluster_offset == inodes_per_cluster) {
+			if (cluster_offset == M_IGEO(mp)->inodes_per_cluster) {
 				bp_index++;
 				cluster_offset = 0;
 			}
@@ -964,7 +964,7 @@ process_inode_chunk(
 			ASSERT(ino_rec->ino_startnum == agino + 1);
 			irec_offset = 0;
 		}
-		if (cluster_offset == inodes_per_cluster) {
+		if (cluster_offset == M_IGEO(mp)->inodes_per_cluster) {
 			bp_index++;
 			cluster_offset = 0;
 		}
diff --git a/repair/dinode.c b/repair/dinode.c
index f5e88cc3..8af2cb25 100644
--- a/repair/dinode.c
+++ b/repair/dinode.c
@@ -755,8 +755,6 @@ get_agino_buf(
 	struct xfs_dinode	**dipp)
 {
 	struct xfs_buf		*bp;
-	int			cluster_size;
-	int			ino_per_cluster;
 	xfs_agino_t		cluster_agino;
 	xfs_daddr_t		cluster_daddr;
 	xfs_daddr_t		cluster_blks;
@@ -768,18 +766,15 @@ get_agino_buf(
 	 * we must find the buffer for its cluster, add the appropriate
 	 * offset, and return that.
 	 */
-	cluster_size = igeo->inode_cluster_size;
-	ino_per_cluster = cluster_size / mp->m_sb.sb_inodesize;
-	cluster_agino = agino & ~(ino_per_cluster - 1);
-	cluster_blks = XFS_FSB_TO_DADDR(mp, max(1,
-			igeo->inode_cluster_size >> mp->m_sb.sb_blocklog));
+	cluster_agino = agino & ~(igeo->inodes_per_cluster - 1);
+	cluster_blks = XFS_FSB_TO_DADDR(mp, igeo->blocks_per_cluster);
 	cluster_daddr = XFS_AGB_TO_DADDR(mp, agno,
 			XFS_AGINO_TO_AGBNO(mp, cluster_agino));
 
 #ifdef XR_INODE_TRACE
 	printf("cluster_size %d ipc %d clusagino %d daddr %lld sectors %lld\n",
-		cluster_size, ino_per_cluster, cluster_agino, cluster_daddr,
-		cluster_blks);
+		M_IGEO(mp)->inode_cluster_size, M_IGEO(mp)->inodes_per_cluster,
+		cluster_agino, cluster_daddr, cluster_blks);
 #endif
 
 	bp = libxfs_readbuf(mp->m_dev, cluster_daddr, cluster_blks,
diff --git a/repair/globals.c b/repair/globals.c
index ae9d55b4..dcd79ea4 100644
--- a/repair/globals.c
+++ b/repair/globals.c
@@ -81,7 +81,6 @@ xfs_agblock_t	inobt_root;
 /* configuration vars -- fs geometry dependent */
 
 int		inodes_per_block;
-int		inodes_per_cluster;
 unsigned int	glob_agcount;
 int		chunks_pblock;	/* # of 64-ino chunks per allocation */
 int		max_symlink_blocks;
diff --git a/repair/globals.h b/repair/globals.h
index 05121d4f..008bdd90 100644
--- a/repair/globals.h
+++ b/repair/globals.h
@@ -122,7 +122,6 @@ extern xfs_agblock_t	inobt_root;
 /* configuration vars -- fs geometry dependent */
 
 extern int		inodes_per_block;
-extern int		inodes_per_cluster;
 extern unsigned int	glob_agcount;
 extern int		chunks_pblock;	/* # of 64-ino chunks per allocation */
 extern int		max_symlink_blocks;
diff --git a/repair/prefetch.c b/repair/prefetch.c
index 2fecfd68..5a725a51 100644
--- a/repair/prefetch.c
+++ b/repair/prefetch.c
@@ -710,17 +710,12 @@ pf_queuing_worker(
 	int			num_inos;
 	ino_tree_node_t		*irec;
 	ino_tree_node_t		*cur_irec;
-	int			blks_per_cluster;
 	xfs_agblock_t		bno;
 	int			i;
 	int			err;
 	uint64_t		sparse;
 	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
 
-	blks_per_cluster = igeo->inode_cluster_size >> mp->m_sb.sb_blocklog;
-	if (blks_per_cluster == 0)
-		blks_per_cluster = 1;
-
 	for (i = 0; i < PF_THREAD_COUNT; i++) {
 		err = pthread_create(&args->io_threads[i], NULL,
 				pf_io_worker, args);
@@ -786,21 +781,22 @@ pf_queuing_worker(
 			struct xfs_buf_map	map;
 
 			map.bm_bn = XFS_AGB_TO_DADDR(mp, args->agno, bno);
-			map.bm_len = XFS_FSB_TO_BB(mp, blks_per_cluster);
+			map.bm_len = XFS_FSB_TO_BB(mp,
+					M_IGEO(mp)->blocks_per_cluster);
 
 			/*
 			 * Queue I/O for each non-sparse cluster. We can check
 			 * sparse state in cluster sized chunks as cluster size
 			 * is the min. granularity of sparse irec regions.
 			 */
-			if ((sparse & ((1ULL << inodes_per_cluster) - 1)) == 0)
+			if ((sparse & ((1ULL << M_IGEO(mp)->inodes_per_cluster) - 1)) == 0)
 				pf_queue_io(args, &map, 1,
 					    (cur_irec->ino_isa_dir != 0) ?
 					     B_DIR_INODE : B_INODE);
 
-			bno += blks_per_cluster;
-			num_inos += inodes_per_cluster;
-			sparse >>= inodes_per_cluster;
+			bno += igeo->blocks_per_cluster;
+			num_inos += igeo->inodes_per_cluster;
+			sparse >>= igeo->inodes_per_cluster;
 		} while (num_inos < igeo->ialloc_inos);
 	}
 
@@ -903,9 +899,8 @@ start_inode_prefetch(
 
 	max_queue = libxfs_bcache->c_maxcount / thread_count / 8;
 	if (igeo->inode_cluster_size > mp->m_sb.sb_blocksize)
-		max_queue = max_queue *
-			(igeo->inode_cluster_size >> mp->m_sb.sb_blocklog) /
-			igeo->ialloc_blks;
+		max_queue = max_queue * igeo->blocks_per_cluster /
+				igeo->ialloc_blks;
 
 	sem_init(&args->ra_count, 0, max_queue);
 
diff --git a/repair/xfs_repair.c b/repair/xfs_repair.c
index 9f4f2611..c7f3bfbc 100644
--- a/repair/xfs_repair.c
+++ b/repair/xfs_repair.c
@@ -762,8 +762,6 @@ main(int argc, char **argv)
 
 	chunks_pblock = mp->m_sb.sb_inopblock / XFS_INODES_PER_CHUNK;
 	max_symlink_blocks = libxfs_symlink_blocks(mp, XFS_SYMLINK_MAXLEN);
-	inodes_per_cluster = max(mp->m_sb.sb_inopblock,
-			igeo->inode_cluster_size >> mp->m_sb.sb_inodelog);
 
 	/*
 	 * Automatic striding for high agcount filesystems.

[PATCH 12/12] xfs_repair: reduce the amount of "clearing reflink flag" messages

[Darrick J. Wong]

From: Darrick J. Wong <darrick.wong@oracle.com>

Clearing the reflink flag on files that don't share blocks is an
optimization, not a repair, so it's not critical to log a message every
single time we clear a flag.  Only log one message that we're clearing
these flags unless verbose mode is enabled.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/rmap.c |   34 +++++++++++++++++++++++++++++++---
 1 file changed, 31 insertions(+), 3 deletions(-)


diff --git a/repair/rmap.c b/repair/rmap.c
index 47828a06..24251e9f 100644
--- a/repair/rmap.c
+++ b/repair/rmap.c
@@ -1170,6 +1170,36 @@ record_inode_reflink_flag(
 		(unsigned long long)lino, (unsigned long long)irec->ino_was_rl);
 }
 
+/*
+ * Inform the user that we're clearing the reflink flag on an inode that
+ * doesn't actually share any blocks.  This is an optimization (the kernel
+ * skips refcount checks for non-reflink files) and not a corruption repair,
+ * so we don't need to log every time we clear a flag unless verbose mode is
+ * enabled.
+ */
+static void
+warn_clearing_reflink(
+	xfs_ino_t		ino)
+{
+	static bool		warned = false;
+	static pthread_mutex_t	lock = PTHREAD_MUTEX_INITIALIZER;
+
+	if (verbose) {
+		do_warn( _("clearing reflink flag on inode %"PRIu64"\n"), ino);
+		return;
+	}
+
+	if (warned)
+		return;
+
+	pthread_mutex_lock(&lock);
+	if (!warned) {
+		do_warn( _("clearing reflink flag on inodes when possible\n"));
+		warned = true;
+	}
+	pthread_mutex_unlock(&lock);
+}
+
 /*
  * Fix an inode's reflink flag.
  */
@@ -1188,9 +1218,7 @@ fix_inode_reflink_flag(
 _("setting reflink flag on inode %"PRIu64"\n"),
 			XFS_AGINO_TO_INO(mp, agno, agino));
 	else if (!no_modify) /* && !set */
-		do_warn(
-_("clearing reflink flag on inode %"PRIu64"\n"),
-			XFS_AGINO_TO_INO(mp, agno, agino));
+		warn_clearing_reflink(XFS_AGINO_TO_INO(mp, agno, agino));
 	if (no_modify)
 		return 0;
 

Re: [PATCH 01/12] libxfs-diff: try harder to find the kernel equivalent libxfs files

[Dave Chinner]

On Tue, Aug 20, 2019 at 01:31:17PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Now that we're syncing userspace libxfs/ files with kernel fs/xfs/
> files, teach the diff tool to try fs/xfs/xfs_foo.c if
> fs/xfs/libxfs/xfs_foo.c doesn't exist.

I'd prefer we have a strategy that moves fs/xfs files to
fs/xfs/libxfs once they are synced instead of breaking the "files
in libxfs/ are the same in both user and kernel space" rule we set
for libxfs...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 01/12] libxfs-diff: try harder to find the kernel equivalent libxfs files

[Darrick J. Wong]

On Fri, Aug 30, 2019 at 03:38:12PM +1000, Dave Chinner wrote:
> On Tue, Aug 20, 2019 at 01:31:17PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Now that we're syncing userspace libxfs/ files with kernel fs/xfs/
> > files, teach the diff tool to try fs/xfs/xfs_foo.c if
> > fs/xfs/libxfs/xfs_foo.c doesn't exist.
> 
> I'd prefer we have a strategy that moves fs/xfs files to
> fs/xfs/libxfs once they are synced instead of breaking the "files
> in libxfs/ are the same in both user and kernel space" rule we set
> for libxfs...

Ok, I will kill this patch.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 02/12] libxfs: move topology declarations into separate header

[Dave Chinner]

On Tue, Aug 20, 2019 at 01:31:24PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The topology functions live in libfrog now, which means their
> declarations don't belong in libxcmd.h.  Create new header file for
> them.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  include/libxcmd.h  |   31 -------------------------------
>  include/topology.h |   39 +++++++++++++++++++++++++++++++++++++++
>  libfrog/topology.c |    1 +
>  mkfs/xfs_mkfs.c    |    2 +-
>  repair/sb.c        |    1 +
>  5 files changed, 42 insertions(+), 32 deletions(-)
>  create mode 100644 include/topology.h

I like the idea, but I'm wondering if we should have a similar
setup to libxfs header files here.

i.e. the header file lives in libfrog/, and the install-headers make
command creates include/libxfrog and links them for the build. That
way the includes become namespaced like:

#include "libxfrog/topology,h"

and we don't pollute include with random header files from all
different parts of xfsprogs...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 03/12] libfrog: try the v4 fs geometry ioctl after failing the v5 ioctl

[Dave Chinner]

On Tue, Aug 20, 2019 at 01:31:30PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> If a v5 fs geometry query fails, try the v4 interface before falling
> back to the v1 interface.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  libfrog/fsgeom.c |    4 ++++
>  1 file changed, 4 insertions(+)

looks good to me.

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 04/12] man: document the new v5 fs geometry ioctl structures

[Dave Chinner]

On Tue, Aug 20, 2019 at 01:31:36PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Amend the fs geometry ioctl documentation to cover the new v5 structure.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  libfrog/fsgeom.c                   |    4 ++++
>  man/man2/ioctl_xfs_fsop_geometry.2 |    8 ++++++++
>  2 files changed, 12 insertions(+)
> 
> 
> diff --git a/libfrog/fsgeom.c b/libfrog/fsgeom.c
> index 06e4e663..159738c5 100644
> --- a/libfrog/fsgeom.c
> +++ b/libfrog/fsgeom.c
> @@ -88,6 +88,10 @@ xfrog_geometry(
>  	if (!ret)
>  		return 0;
>  
> +	ret = ioctl(fd, XFS_IOC_FSGEOMETRY_V4, fsgeo);
> +	if (!ret)
> +		return 0;
> +
>  	return ioctl(fd, XFS_IOC_FSGEOMETRY_V1, fsgeo);
>  }

This hunk is in the previous patch.

>  
> diff --git a/man/man2/ioctl_xfs_fsop_geometry.2 b/man/man2/ioctl_xfs_fsop_geometry.2
> index 68e3387d..365bda8b 100644
> --- a/man/man2/ioctl_xfs_fsop_geometry.2
> +++ b/man/man2/ioctl_xfs_fsop_geometry.2
> @@ -12,6 +12,8 @@ ioctl_xfs_fsop_geometry \- report XFS filesystem layout and features
>  .PP
>  .BI "int ioctl(int " fd ", XFS_IOC_FSOP_GEOMETRY, struct xfs_fsop_geom*" arg );
>  .br
> +.BI "int ioctl(int " fd ", XFS_IOC_FSOP_GEOMETRY_V4, struct xfs_fsop_geom_v4 *" arg );
> +.br
>  .BI "int ioctl(int " fd ", XFS_IOC_FSOP_GEOMETRY_V1, struct xfs_fsop_geom_v1 *" arg );
>  .SH DESCRIPTION
>  Report the details of an XFS filesystem layout, features, and other descriptive items.
> @@ -43,6 +45,9 @@ struct xfs_fsop_geom {
>  	/* struct xfs_fsop_geom_v1 stops here. */
>  
>  	__u32         logsunit;
> +	/* struct xfs_fsop_geom_v4 stops here. */
> +
> +	__u64         reserved[18];
>  };
>  .fi
>  .in

And this looks like a stray, too.

The man page changes look fine, though :P

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 05/12] man: document new fs summary counter scrub command

[Dave Chinner]

On Tue, Aug 20, 2019 at 01:31:42PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Update the scrub ioctl documentation to include the new fs summary
> counter scrubber.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  man/man2/ioctl_xfs_scrub_metadata.2 |    5 +++++
>  1 file changed, 5 insertions(+)

looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 06/12] man: document the new allocation group geometry ioctl

[Dave Chinner]

On Tue, Aug 20, 2019 at 01:31:48PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Document the new ioctl to describe an allocation group's geometry.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  man/man2/ioctl_xfs_ag_geometry.2 |   74 ++++++++++++++++++++++++++++++++++++++
>  man/man3/xfsctl.3                |    6 +++
>  2 files changed, 80 insertions(+)
>  create mode 100644 man/man2/ioctl_xfs_ag_geometry.2
> 
> 
> diff --git a/man/man2/ioctl_xfs_ag_geometry.2 b/man/man2/ioctl_xfs_ag_geometry.2
> new file mode 100644
> index 00000000..5dfe0d08
> --- /dev/null
> +++ b/man/man2/ioctl_xfs_ag_geometry.2
> @@ -0,0 +1,74 @@
> +.\" Copyright (c) 2019, Oracle.  All rights reserved.
> +.\"
> +.\" %%%LICENSE_START(GPLv2+_DOC_FULL)
> +.\" SPDX-License-Identifier: GPL-2.0+
> +.\" %%%LICENSE_END
> +.TH IOCTL-XFS-AG-GEOMETRY 2 2019-04-11 "XFS"
> +.SH NAME
> +ioctl_xfs_ag_geometry \- query XFS allocation group geometry information
> +.SH SYNOPSIS
> +.br
> +.B #include <xfs/xfs_fs.h>
> +.PP
> +.BI "int ioctl(int " fd ", XFS_IOC_AG_GEOMETRY, struct xfs_ag_geometry *" arg );
> +.SH DESCRIPTION
> +This XFS ioctl retrieves the geometry information for a given allocation group.
> +The geometry information is conveyed in a structure of the following form:
> +.PP
> +.in +4n
> +.nf
> +struct xfs_ag_geometry {
> +	uint32_t  ag_number;
> +	uint32_t  ag_length;
> +	uint32_t  ag_freeblks;
> +	uint32_t  ag_icount;
> +	uint32_t  ag_ifree;
> +	uint32_t  ag_sick;
> +	uint32_t  ag_checked;
> +	uint32_t  ag_reserved32;
> +	uint64_t  ag_reserved[12];

Where's the flags field for feature versioning? Please don't tell me
we merged an ioctl structure without a flags or version field in
it...

> +};
> +.fi
> +.in
> +.TP
> +.I ag_number
> +The number of allocation group that the caller wishes to learn about.

"the index of"....

"The number of" is easily confused with a quantity....

Is this an input or an output?

> +.TP
> +.I ag_length
> +Length of the allocation group, in units of filesystem blocks.

The length of the AG is returned in this field, in units....

Same for the rest...

> +.TP
> +.I ag_freeblks
> +Number of free blocks in the allocation group, in units of filesystem blocks.
> +.TP
> +.I ag_icount
> +Number of inode records allocated in this allocation group.
> +.TP
> +.I ag_ifree
> +Number of unused inode records (of the space allocated) in this allocation
> +group.
> +.TP
> +.IR ag_reserved " and " ag_reserved32
> +Will be set to zero.

It would be better to say "all reserved feilds will be set to zero
on return" so that we don't have to change this every time we rev
the structure....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 07/12] man: document the new health reporting fields in various ioctls

[Dave Chinner]

On Tue, Aug 20, 2019 at 01:31:54PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Update the manpages to conver the new health reporting fields in the
> fs geometry, ag geometry, and bulkstat ioctls.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  man/man2/ioctl_xfs_ag_geometry.2   |   48 +++++++++++++++++++++++++++++++
>  man/man2/ioctl_xfs_fsbulkstat.2    |   52 +++++++++++++++++++++++++++++++++
>  man/man2/ioctl_xfs_fsop_geometry.2 |   56 +++++++++++++++++++++++++++++++++++-
>  3 files changed, 154 insertions(+), 2 deletions(-)

Looks (!sick and checked).

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 08/12] xfs_db: remove db/convert.h

[Dave Chinner]

On Tue, Aug 20, 2019 at 01:32:00PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> db/convert.h conflicts with include/convert.h and since the former only
> has one declaration in it anyway, just get rid of it.  We'll need this
> in the next patch to avoid an ugly include mess.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  db/Makefile  |    4 ++--
>  db/command.c |    1 -
>  db/command.h |    1 +
>  db/convert.c |    1 -
>  db/convert.h |    7 -------
>  5 files changed, 3 insertions(+), 11 deletions(-)
>  delete mode 100644 db/convert.h

Looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 09/12] xfs_db: add a function to compute btree geometry

[Dave Chinner]

On Tue, Aug 20, 2019 at 01:32:10PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add a new command to xfs_db that uses a btree type and a record count
> to report the best and worst case height and level size.  This can be
> used to estimate how overhead a metadata index will add to a filesystem.
                      ^ much

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Nothing else stands out as obviously incorrect, so with the above
commit meesage fixup,

Reviewed-by: Dave Chinner <dchinner@redhat.com>

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 10/12] xfs_db: use precomputed inode geometry values

[Dave Chinner]

On Tue, Aug 20, 2019 at 01:32:17PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Use the precomputed inode geometry values instead of open-coding them.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

looks good.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 11/12] xfs_repair: use precomputed inode geometry values

[Dave Chinner]

On Tue, Aug 20, 2019 at 01:32:23PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Use the precomputed inode geometry values instead of open-coding them.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  repair/dino_chunks.c |   22 +++++++++++-----------
>  repair/dinode.c      |   13 ++++---------
>  repair/globals.c     |    1 -
>  repair/globals.h     |    1 -
>  repair/prefetch.c    |   21 ++++++++-------------
>  repair/xfs_repair.c  |    2 --
>  6 files changed, 23 insertions(+), 37 deletions(-)

two minor nits:

> diff --git a/repair/prefetch.c b/repair/prefetch.c
> index 2fecfd68..5a725a51 100644
> --- a/repair/prefetch.c
> +++ b/repair/prefetch.c
> @@ -710,17 +710,12 @@ pf_queuing_worker(
>  	int			num_inos;
>  	ino_tree_node_t		*irec;
>  	ino_tree_node_t		*cur_irec;
> -	int			blks_per_cluster;
>  	xfs_agblock_t		bno;
>  	int			i;
>  	int			err;
>  	uint64_t		sparse;
>  	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
>  
> -	blks_per_cluster = igeo->inode_cluster_size >> mp->m_sb.sb_blocklog;
> -	if (blks_per_cluster == 0)
> -		blks_per_cluster = 1;
> -
>  	for (i = 0; i < PF_THREAD_COUNT; i++) {
>  		err = pthread_create(&args->io_threads[i], NULL,
>  				pf_io_worker, args);
> @@ -786,21 +781,22 @@ pf_queuing_worker(
>  			struct xfs_buf_map	map;
>  
>  			map.bm_bn = XFS_AGB_TO_DADDR(mp, args->agno, bno);
> -			map.bm_len = XFS_FSB_TO_BB(mp, blks_per_cluster);
> +			map.bm_len = XFS_FSB_TO_BB(mp,
> +					M_IGEO(mp)->blocks_per_cluster);

	igeo->blocks_per_cluster

>  
>  			/*
>  			 * Queue I/O for each non-sparse cluster. We can check
>  			 * sparse state in cluster sized chunks as cluster size
>  			 * is the min. granularity of sparse irec regions.
>  			 */
> -			if ((sparse & ((1ULL << inodes_per_cluster) - 1)) == 0)
> +			if ((sparse & ((1ULL << M_IGEO(mp)->inodes_per_cluster) - 1)) == 0)

	igeo->inodes_per_cluster and 80 columns....

Otherwise,

Reviewed-by: Dave Chinner <dchinner@redhat.com>

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 12/12] xfs_repair: reduce the amount of "clearing reflink flag" messages

[Dave Chinner]

On Tue, Aug 20, 2019 at 01:32:29PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Clearing the reflink flag on files that don't share blocks is an
> optimization, not a repair, so it's not critical to log a message every
> single time we clear a flag.  Only log one message that we're clearing
> these flags unless verbose mode is enabled.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Using a mutex for this is kinda overkill, but <shrug>

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 02/12] libxfs: move topology declarations into separate header

[Darrick J. Wong]

> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > The topology functions live in libfrog now, which means their
> > declarations don't belong in libxcmd.h.  Create new header file for
> > them.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  include/libxcmd.h  |   31 -------------------------------
> >  include/topology.h |   39 +++++++++++++++++++++++++++++++++++++++
> >  libfrog/topology.c |    1 +
> >  mkfs/xfs_mkfs.c    |    2 +-
> >  repair/sb.c        |    1 +
> >  5 files changed, 42 insertions(+), 32 deletions(-)
> >  create mode 100644 include/topology.h
> 
> I like the idea, but I'm wondering if we should have a similar
> setup to libxfs header files here.
> 
> i.e. the header file lives in libfrog/, and the install-headers make
> command creates include/libxfrog and links them for the build. That
> way the includes become namespaced like:
> 
> #include "libxfrog/topology,h"
> 
> and we don't pollute include with random header files from all
> different parts of xfsprogs...

What if I leave topology.h in libfrog/ and then create an
include/libfrog.h that pulls in all the libfrog header files like
libxfs.h does, and then put -I$(TOPDIR)/libfrog in GCFLAGS?

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 04/12] man: document the new v5 fs geometry ioctl structures

[Darrick J. Wong]

On Fri, Aug 30, 2019 at 03:44:59PM +1000, Dave Chinner wrote:
> On Tue, Aug 20, 2019 at 01:31:36PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Amend the fs geometry ioctl documentation to cover the new v5 structure.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  libfrog/fsgeom.c                   |    4 ++++
> >  man/man2/ioctl_xfs_fsop_geometry.2 |    8 ++++++++
> >  2 files changed, 12 insertions(+)
> > 
> > 
> > diff --git a/libfrog/fsgeom.c b/libfrog/fsgeom.c
> > index 06e4e663..159738c5 100644
> > --- a/libfrog/fsgeom.c
> > +++ b/libfrog/fsgeom.c
> > @@ -88,6 +88,10 @@ xfrog_geometry(
> >  	if (!ret)
> >  		return 0;
> >  
> > +	ret = ioctl(fd, XFS_IOC_FSGEOMETRY_V4, fsgeo);
> > +	if (!ret)
> > +		return 0;
> > +
> >  	return ioctl(fd, XFS_IOC_FSGEOMETRY_V1, fsgeo);
> >  }
> 
> This hunk is in the previous patch.

Dunno where that came from, and it's not reflected in my git repo at
this point ... ?   Weird.

> >  
> > diff --git a/man/man2/ioctl_xfs_fsop_geometry.2 b/man/man2/ioctl_xfs_fsop_geometry.2
> > index 68e3387d..365bda8b 100644
> > --- a/man/man2/ioctl_xfs_fsop_geometry.2
> > +++ b/man/man2/ioctl_xfs_fsop_geometry.2
> > @@ -12,6 +12,8 @@ ioctl_xfs_fsop_geometry \- report XFS filesystem layout and features
> >  .PP
> >  .BI "int ioctl(int " fd ", XFS_IOC_FSOP_GEOMETRY, struct xfs_fsop_geom*" arg );
> >  .br
> > +.BI "int ioctl(int " fd ", XFS_IOC_FSOP_GEOMETRY_V4, struct xfs_fsop_geom_v4 *" arg );
> > +.br
> >  .BI "int ioctl(int " fd ", XFS_IOC_FSOP_GEOMETRY_V1, struct xfs_fsop_geom_v1 *" arg );
> >  .SH DESCRIPTION
> >  Report the details of an XFS filesystem layout, features, and other descriptive items.
> > @@ -43,6 +45,9 @@ struct xfs_fsop_geom {
> >  	/* struct xfs_fsop_geom_v1 stops here. */
> >  
> >  	__u32         logsunit;
> > +	/* struct xfs_fsop_geom_v4 stops here. */
> > +
> > +	__u64         reserved[18];
> >  };
> >  .fi
> >  .in
> 
> And this looks like a stray, too.

That's not a stray, that's part of the manpage update to reflect the
extra space at the end of the structure.

--D

> The man page changes look fine, though :P
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 06/12] man: document the new allocation group geometry ioctl

[Darrick J. Wong]

On Fri, Aug 30, 2019 at 03:53:47PM +1000, Dave Chinner wrote:
> On Tue, Aug 20, 2019 at 01:31:48PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Document the new ioctl to describe an allocation group's geometry.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  man/man2/ioctl_xfs_ag_geometry.2 |   74 ++++++++++++++++++++++++++++++++++++++
> >  man/man3/xfsctl.3                |    6 +++
> >  2 files changed, 80 insertions(+)
> >  create mode 100644 man/man2/ioctl_xfs_ag_geometry.2
> > 
> > 
> > diff --git a/man/man2/ioctl_xfs_ag_geometry.2 b/man/man2/ioctl_xfs_ag_geometry.2
> > new file mode 100644
> > index 00000000..5dfe0d08
> > --- /dev/null
> > +++ b/man/man2/ioctl_xfs_ag_geometry.2
> > @@ -0,0 +1,74 @@
> > +.\" Copyright (c) 2019, Oracle.  All rights reserved.
> > +.\"
> > +.\" %%%LICENSE_START(GPLv2+_DOC_FULL)
> > +.\" SPDX-License-Identifier: GPL-2.0+
> > +.\" %%%LICENSE_END
> > +.TH IOCTL-XFS-AG-GEOMETRY 2 2019-04-11 "XFS"
> > +.SH NAME
> > +ioctl_xfs_ag_geometry \- query XFS allocation group geometry information
> > +.SH SYNOPSIS
> > +.br
> > +.B #include <xfs/xfs_fs.h>
> > +.PP
> > +.BI "int ioctl(int " fd ", XFS_IOC_AG_GEOMETRY, struct xfs_ag_geometry *" arg );
> > +.SH DESCRIPTION
> > +This XFS ioctl retrieves the geometry information for a given allocation group.
> > +The geometry information is conveyed in a structure of the following form:
> > +.PP
> > +.in +4n
> > +.nf
> > +struct xfs_ag_geometry {
> > +	uint32_t  ag_number;
> > +	uint32_t  ag_length;
> > +	uint32_t  ag_freeblks;
> > +	uint32_t  ag_icount;
> > +	uint32_t  ag_ifree;
> > +	uint32_t  ag_sick;
> > +	uint32_t  ag_checked;
> > +	uint32_t  ag_reserved32;
> > +	uint64_t  ag_reserved[12];
> 
> Where's the flags field for feature versioning? Please don't tell me
> we merged an ioctl structure without a flags or version field in
> it...

Yes, we did, though the "reserved fields are always zeroed" enables us
to retroactively define this to v0 of the structure.

> > +};
> > +.fi
> > +.in
> > +.TP
> > +.I ag_number
> > +The number of allocation group that the caller wishes to learn about.
> 
> "the index of"....
> 
> "The number of" is easily confused with a quantity....
> 
> Is this an input or an output?

Purely an input.

"The caller must set this field to the index of the allocation group
that the caller wishes to learn about." ?

> > +.TP
> > +.I ag_length
> > +Length of the allocation group, in units of filesystem blocks.
> 
> The length of the AG is returned in this field, in units....
> 
> Same for the rest...

Ok.

> > +.TP
> > +.I ag_freeblks
> > +Number of free blocks in the allocation group, in units of filesystem blocks.
> > +.TP
> > +.I ag_icount
> > +Number of inode records allocated in this allocation group.
> > +.TP
> > +.I ag_ifree
> > +Number of unused inode records (of the space allocated) in this allocation
> > +group.
> > +.TP
> > +.IR ag_reserved " and " ag_reserved32
> > +Will be set to zero.
> 
> It would be better to say "all reserved feilds will be set to zero
> on return" so that we don't have to change this every time we rev
> the structure....

Ok.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 11/12] xfs_repair: use precomputed inode geometry values

[Darrick J. Wong]

On Fri, Aug 30, 2019 at 04:17:34PM +1000, Dave Chinner wrote:
> On Tue, Aug 20, 2019 at 01:32:23PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Use the precomputed inode geometry values instead of open-coding them.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  repair/dino_chunks.c |   22 +++++++++++-----------
> >  repair/dinode.c      |   13 ++++---------
> >  repair/globals.c     |    1 -
> >  repair/globals.h     |    1 -
> >  repair/prefetch.c    |   21 ++++++++-------------
> >  repair/xfs_repair.c  |    2 --
> >  6 files changed, 23 insertions(+), 37 deletions(-)
> 
> two minor nits:
> 
> > diff --git a/repair/prefetch.c b/repair/prefetch.c
> > index 2fecfd68..5a725a51 100644
> > --- a/repair/prefetch.c
> > +++ b/repair/prefetch.c
> > @@ -710,17 +710,12 @@ pf_queuing_worker(
> >  	int			num_inos;
> >  	ino_tree_node_t		*irec;
> >  	ino_tree_node_t		*cur_irec;
> > -	int			blks_per_cluster;
> >  	xfs_agblock_t		bno;
> >  	int			i;
> >  	int			err;
> >  	uint64_t		sparse;
> >  	struct xfs_ino_geometry	*igeo = M_IGEO(mp);
> >  
> > -	blks_per_cluster = igeo->inode_cluster_size >> mp->m_sb.sb_blocklog;
> > -	if (blks_per_cluster == 0)
> > -		blks_per_cluster = 1;
> > -
> >  	for (i = 0; i < PF_THREAD_COUNT; i++) {
> >  		err = pthread_create(&args->io_threads[i], NULL,
> >  				pf_io_worker, args);
> > @@ -786,21 +781,22 @@ pf_queuing_worker(
> >  			struct xfs_buf_map	map;
> >  
> >  			map.bm_bn = XFS_AGB_TO_DADDR(mp, args->agno, bno);
> > -			map.bm_len = XFS_FSB_TO_BB(mp, blks_per_cluster);
> > +			map.bm_len = XFS_FSB_TO_BB(mp,
> > +					M_IGEO(mp)->blocks_per_cluster);
> 
> 	igeo->blocks_per_cluster
> 
> >  
> >  			/*
> >  			 * Queue I/O for each non-sparse cluster. We can check
> >  			 * sparse state in cluster sized chunks as cluster size
> >  			 * is the min. granularity of sparse irec regions.
> >  			 */
> > -			if ((sparse & ((1ULL << inodes_per_cluster) - 1)) == 0)
> > +			if ((sparse & ((1ULL << M_IGEO(mp)->inodes_per_cluster) - 1)) == 0)
> 
> 	igeo->inodes_per_cluster and 80 columns....

Will fix.

--D

> Otherwise,
> 
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 02/12] libxfs: move topology declarations into separate header

[Dave Chinner]

On Fri, Aug 30, 2019 at 01:34:02PM -0700, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > The topology functions live in libfrog now, which means their
> > > declarations don't belong in libxcmd.h.  Create new header file for
> > > them.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  include/libxcmd.h  |   31 -------------------------------
> > >  include/topology.h |   39 +++++++++++++++++++++++++++++++++++++++
> > >  libfrog/topology.c |    1 +
> > >  mkfs/xfs_mkfs.c    |    2 +-
> > >  repair/sb.c        |    1 +
> > >  5 files changed, 42 insertions(+), 32 deletions(-)
> > >  create mode 100644 include/topology.h
> > 
> > I like the idea, but I'm wondering if we should have a similar
> > setup to libxfs header files here.
> > 
> > i.e. the header file lives in libfrog/, and the install-headers make
> > command creates include/libxfrog and links them for the build. That
> > way the includes become namespaced like:
> > 
> > #include "libxfrog/topology,h"
> > 
> > and we don't pollute include with random header files from all
> > different parts of xfsprogs...
> 
> What if I leave topology.h in libfrog/ and then create an
> include/libfrog.h that pulls in all the libfrog header files like
> libxfs.h does, and then put -I$(TOPDIR)/libfrog in GCFLAGS?

libxfs is a basket case - it's done that way so we don't need all
the whacky games we play to shim the kernel functionality correctly
in every xfsprogs file that needs libxfs functionality.

libxfrog is very different - we have progs that just need topology
or number conversion, but not both. I'd prefer for libxfrog we only
include the headers we require, that way avoiding defining things we
don't actually need...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 06/12] man: document the new allocation group geometry ioctl

[Dave Chinner]

On Fri, Aug 30, 2019 at 01:48:49PM -0700, Darrick J. Wong wrote:
> On Fri, Aug 30, 2019 at 03:53:47PM +1000, Dave Chinner wrote:
> > On Tue, Aug 20, 2019 at 01:31:48PM -0700, Darrick J. Wong wrote:
> > > +	uint64_t  ag_reserved[12];
> > 
> > Where's the flags field for feature versioning? Please don't tell me
> > we merged an ioctl structure without a flags or version field in
> > it...
> 
> Yes, we did, though the "reserved fields are always zeroed" enables us
> to retroactively define this to v0 of the structure.

OK, but this is an input/output structure, not an output-only
structure, so the flags field needs to cover what features the
caller might be expecting the kernel to return, too.,,

> > > +};
> > > +.fi
> > > +.in
> > > +.TP
> > > +.I ag_number
> > > +The number of allocation group that the caller wishes to learn about.
> > 
> > "the index of"....
> > 
> > "The number of" is easily confused with a quantity....
> > 
> > Is this an input or an output?
> 
> Purely an input.
> 
> "The caller must set this field to the index of the allocation group
> that the caller wishes to learn about." ?

*nod*.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 02/12] libxfs: move topology declarations into separate header

[Darrick J. Wong]

On Tue, Sep 03, 2019 at 08:33:46AM +1000, Dave Chinner wrote:
> On Fri, Aug 30, 2019 at 01:34:02PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > The topology functions live in libfrog now, which means their
> > > > declarations don't belong in libxcmd.h.  Create new header file for
> > > > them.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > >  include/libxcmd.h  |   31 -------------------------------
> > > >  include/topology.h |   39 +++++++++++++++++++++++++++++++++++++++
> > > >  libfrog/topology.c |    1 +
> > > >  mkfs/xfs_mkfs.c    |    2 +-
> > > >  repair/sb.c        |    1 +
> > > >  5 files changed, 42 insertions(+), 32 deletions(-)
> > > >  create mode 100644 include/topology.h
> > > 
> > > I like the idea, but I'm wondering if we should have a similar
> > > setup to libxfs header files here.
> > > 
> > > i.e. the header file lives in libfrog/, and the install-headers make
> > > command creates include/libxfrog and links them for the build. That
> > > way the includes become namespaced like:
> > > 
> > > #include "libxfrog/topology,h"
> > > 
> > > and we don't pollute include with random header files from all
> > > different parts of xfsprogs...
> > 
> > What if I leave topology.h in libfrog/ and then create an
> > include/libfrog.h that pulls in all the libfrog header files like
> > libxfs.h does, and then put -I$(TOPDIR)/libfrog in GCFLAGS?
> 
> libxfs is a basket case - it's done that way so we don't need all
> the whacky games we play to shim the kernel functionality correctly
> in every xfsprogs file that needs libxfs functionality.
> 
> libxfrog is very different - we have progs that just need topology
> or number conversion, but not both. I'd prefer for libxfrog we only
> include the headers we require, that way avoiding defining things we
> don't actually need...

Ok, will rework this tomorrow.

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 06/12] man: document the new allocation group geometry ioctl

[Darrick J. Wong]

On Tue, Sep 04, 2019 at 08:36:57AM +1000, Dave Chinner wrote:
> On Fri, Aug 30, 2019 at 01:48:49PM -0700, Darrick J. Wong wrote:
> > On Fri, Aug 30, 2019 at 03:53:47PM +1000, Dave Chinner wrote:
> > > On Tue, Aug 20, 2019 at 01:31:48PM -0700, Darrick J. Wong wrote:
> > > > +	uint64_t  ag_reserved[12];
> > > 
> > > Where's the flags field for feature versioning? Please don't tell me
> > > we merged an ioctl structure without a flags or version field in
> > > it...
> > 
> > Yes, we did, though the "reserved fields are always zeroed" enables us
> > to retroactively define this to v0 of the structure.
> 
> OK, but this is an input/output structure, not an output-only
> structure, so the flags field needs to cover what features the
> caller might be expecting the kernel to return, too.,,

What do you think of the v2 "xfs: define a flags field for the AG
geometry ioctl structure" patch, then?

--D

> > > > +};
> > > > +.fi
> > > > +.in
> > > > +.TP
> > > > +.I ag_number
> > > > +The number of allocation group that the caller wishes to learn about.
> > > 
> > > "the index of"....
> > > 
> > > "The number of" is easily confused with a quantity....
> > > 
> > > Is this an input or an output?
> > 
> > Purely an input.
> > 
> > "The caller must set this field to the index of the allocation group
> > that the caller wishes to learn about." ?
> 
> *nod*.
> 
> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

Re: [PATCH 02/12] libxfs: move topology declarations into separate header

[Darrick J. Wong]

On Tue, Sep 03, 2019 at 08:33:46AM +1000, Dave Chinner wrote:
> On Fri, Aug 30, 2019 at 01:34:02PM -0700, Darrick J. Wong wrote:
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > 
> > > > The topology functions live in libfrog now, which means their
> > > > declarations don't belong in libxcmd.h.  Create new header file for
> > > > them.
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > >  include/libxcmd.h  |   31 -------------------------------
> > > >  include/topology.h |   39 +++++++++++++++++++++++++++++++++++++++
> > > >  libfrog/topology.c |    1 +
> > > >  mkfs/xfs_mkfs.c    |    2 +-
> > > >  repair/sb.c        |    1 +
> > > >  5 files changed, 42 insertions(+), 32 deletions(-)
> > > >  create mode 100644 include/topology.h
> > > 
> > > I like the idea, but I'm wondering if we should have a similar
> > > setup to libxfs header files here.
> > > 
> > > i.e. the header file lives in libfrog/, and the install-headers make
> > > command creates include/libxfrog and links them for the build. That
> > > way the includes become namespaced like:
> > > 
> > > #include "libxfrog/topology,h"
> > > 
> > > and we don't pollute include with random header files from all
> > > different parts of xfsprogs...
> > 
> > What if I leave topology.h in libfrog/ and then create an
> > include/libfrog.h that pulls in all the libfrog header files like
> > libxfs.h does, and then put -I$(TOPDIR)/libfrog in GCFLAGS?
> 
> libxfs is a basket case - it's done that way so we don't need all
> the whacky games we play to shim the kernel functionality correctly
> in every xfsprogs file that needs libxfs functionality.
> 
> libxfrog is very different - we have progs that just need topology
> or number conversion, but not both. I'd prefer for libxfrog we only
> include the headers we require, that way avoiding defining things we
> don't actually need...

Ok.  I will go clean that up too....

--D

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com