[PATCH 0/8] xfsprogs: random fixes

[PATCH 0/8] xfsprogs: random fixes

[Darrick J. Wong]

Hi all,

Here's the usual pile of xfsprogs cleanups and fixes.  We start with
some documentation updates and manpages for missing commands.  Next, we
add a command to xfs_db to compute per-AG reservations to aid in
debugging per-AG reservation failures during mount.  Finally, we fix
some overflows and counting errors in xfs_io and xfs_repair.

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=random-fixes

fstests git tree:
https://git.kernel.org/cgit/linux/kernel/git/djwong/xfstests-dev.git/log/?h=random-fixes

[PATCH 1/8] man: list xfs_io lsattr inode flag letters

[Darrick J. Wong]

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

The section of the xfs_io manpage for the 'chattr' command says to refer
to xfsctl(3) for information on the flags.  The inode flag information
was moved to ioctl_xfs_fssetxattr(2) ages ago, and it never actually
mapped the inode flag letters to inode flag bits, so fix the link and
add such a mapping to the xfs_io manpage.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 man/man8/xfs_io.8 |   89 +++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 83 insertions(+), 6 deletions(-)


diff --git a/man/man8/xfs_io.8 b/man/man8/xfs_io.8
index c69b295d..f5431a8c 100644
--- a/man/man8/xfs_io.8
+++ b/man/man8/xfs_io.8
@@ -794,18 +794,95 @@ for all directory entries below the currently open file
 can be used to restrict the output to directories only).
 This is a depth first descent, it does not follow symlinks and
 it also does not cross mount points.
+
+The current inode flag letters are documented below.
+Please refer to the
+.BR ioctl_xfs_fsgetxattr "(2)"
+documentation for more details about what they mean.
+
+.PD 0
+.RS
+.TP 0.5i
+.B r
+realtime file (XFS_XFLAG_REALTIME)
+
+.TP
+.B p
+prealloc (XFS_XFLAG_PREALLOC)
+
+.TP
+.B i
+immutable (XFS_XFLAG_IMMUTABLE)
+
+.TP
+.B a
+append only (XFS_XFLAG_APPEND)
+
+.TP
+.B s
+synchronous file writes (XFS_XFLAG_SYNC)
+
+.TP
+.B A
+noatime (XFS_XFLAG_NOATIME)
+
+.TP
+.B d
+nodump (XFS_XFLAG_NODUMP)
+
+.TP
+.B t
+inherit realtime flag (XFS_XFLAG_RTINHERIT)"
+
+.TP
+.B P
+inherit project id (XFS_XFLAG_PROJINHERIT)
+
+.TP
+.B n
+no symlink creation (XFS_XFLAG_NOSYMLINKS)
+
+.TP
+.B e
+extent size hint (XFS_XFLAG_EXTSIZE)
+
+.TP
+.B E
+inherit extent size hint (XFS_XFLAG_EXTSZINHERIT)
+
+.TP
+.B f
+nodefrag (XFS_XFLAG_NODEFRAG)
+
+.TP
+.B S
+filestream allocator (XFS_XFLAG_FILESTREAM)
+
+.TP
+.B x
+direct access persistent memory (XFS_XFLAG_DAX)
+
+.TP
+.B C
+copy on write extent hint (XFS_XFLAG_COWEXTSIZE)
+
+.TP
+.B X
+has extended attributes (XFS_XFLAG_HASATTR)
+.RE
+
 .TP
 .BR chattr " [ " \-R " | " \-D " ] [ " + / \-riasAdtPneEfSxC " ]"
 Change extended inode flags on the currently open file. The
 .B \-R
 and
 .B \-D
-options have the same meaning as above. The mapping between each
-letter and the inode flags (refer to
-.BR xfsctl (3)
-for the full list) is available via the
-.B help
-command.
+options have the same meaning as above.
+
+See the
+.B lsattr
+command above for the list of inode flag letters.
+
 .TP
 .BI "flink " path
 Link the currently open file descriptor into the filesystem namespace.

[PATCH 2/8] man: document the xfs_db btheight command

[Darrick J. Wong]

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

Document the btheight command in xfs_db.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 man/man8/xfs_db.8 |   39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)


diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
index a1ee3514..53e34983 100644
--- a/man/man8/xfs_db.8
+++ b/man/man8/xfs_db.8
@@ -346,6 +346,45 @@ If the cursor points at an inode, dump the extended attribute block mapping btre
 Dump all keys and pointers in intermediate btree nodes, and all records in leaf btree nodes.
 .RE
 .TP
+.BI "btheight [\-b " blksz "] [\-n " recs "] [\-w " max "|\-w " min "] btree types..."
+For a given number of btree records and a btree type, report the number of
+records and blocks for each level of the btree, and the total number of blocks.
+The btree type must be given after the options.
+
+A raw btree geometry can be provided in the format
+"record_bytes:key_bytes:ptr_bytes:header_type", where header_type is one of
+"short", "long", "shortcrc", or "longcrc".
+
+The supported btree types are:
+.IR bnobt ,
+.IR cntbt ,
+.IR inobt ,
+.IR finobt ,
+.IR bmapbt ,
+.IR refcountbt ,
+and
+.IR rmapbt .
+
+Options are as follows:
+.RS 1.0i
+.TP 0.4i
+.B \-b
+is used to override the btree block size.
+The default is the filesystem block size.
+.TP
+.B \-n
+is used to specify the number of records to store.
+This argument is required.
+.TP
+.B \-w max
+shows only the best case scenario, which is when the btree blocks are
+maximally loaded.
+.TP
+.B \-w min
+shows only the worst case scenario, which is when the btree blocks are
+half full.
+.RE
+.TP
 .B check
 See the
 .B blockget

[PATCH 3/8] man: reformat xfs_quota commands in the manpage for testing

[Darrick J. Wong]

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

Reformat the xfs_quota commands listed in the xfs_quota.8 manpage so
that we can implement a fstest that checks that each command actually
has a section in the manpage.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 man/man8/xfs_quota.8 |   15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)


diff --git a/man/man8/xfs_quota.8 b/man/man8/xfs_quota.8
index d9c5f649..e6fe7cd1 100644
--- a/man/man8/xfs_quota.8
+++ b/man/man8/xfs_quota.8
@@ -228,8 +228,7 @@ option sends the output to
 .I file
 instead of stdout.
 .HP
-.B
-free
+.B free
 [
 .B \-bir
 ] [
@@ -398,8 +397,7 @@ option reports information without the header line. The
 .B \-t
 option performs a terse report.
 .HP
-.B
-state
+.B state
 [
 .B \-gpu
 ] [
@@ -420,8 +418,7 @@ instead of stdout. The
 .B \-a
 option reports state on all filesystems and not just the current path.
 .HP
-.B
-limit
+.B limit
 [
 .BR \-g " | " \-p " | " \-u
 ]
@@ -519,8 +516,7 @@ identified by the current path.
 Quota must not be enabled on the filesystem, else this operation will
 report an error.
 .HP
-.B
-dump
+.B dump
 [
 .BR \-g " | " \-p " | " \-u
 ] [
@@ -547,8 +543,7 @@ The file must be in the format produced by the
 .B dump
 command.
 .HP
-.B
-quot
+.B quot
 [
 .BR \-g " | " \-p " | " \-u
 ] [

[PATCH 4/8] man: document some missing xfs_db commands

[Darrick J. Wong]

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

The 'attr_set', 'attr_remove', and 'logformat' commands in xfs_db were
not documented.  Add sections about them to the manpage.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/attrset.c      |    4 ++--
 man/man8/xfs_db.8 |   58 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 2 deletions(-)


diff --git a/db/attrset.c b/db/attrset.c
index 56972506..8eecf465 100644
--- a/db/attrset.c
+++ b/db/attrset.c
@@ -23,11 +23,11 @@ static void		attrset_help(void);
 
 static const cmdinfo_t	attr_set_cmd =
 	{ "attr_set", "aset", attr_set_f, 1, -1, 0,
-	  N_("[-r|-s|-p|-u] [-n] [-R|-C] [-v n] name"),
+	  N_("[-r|-s|-u] [-n] [-R|-C] [-v n] name"),
 	  N_("set the named attribute on the current inode"), attrset_help };
 static const cmdinfo_t	attr_remove_cmd =
 	{ "attr_remove", "aremove", attr_remove_f, 1, -1, 0,
-	  N_("[-r|-s|-p|-u] [-n] name"),
+	  N_("[-r|-s|-u] [-n] name"),
 	  N_("remove the named attribute from the current inode"), attrset_help };
 
 static void
diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
index 53e34983..9f1ff761 100644
--- a/man/man8/xfs_db.8
+++ b/man/man8/xfs_db.8
@@ -179,6 +179,57 @@ Set current address to the AGI block for allocation group
 .IR agno .
 If no argument is given, use the current allocation group.
 .TP
+.BI "attr_remove [\-r|\-u|\-s] [\-n] " name
+Remove the specified extended attribute from the current file.
+.RS 1.0i
+.TP 0.4i
+.B \-r
+Sets the attribute in the root namespace.
+Only one namespace option can be specified.
+.TP
+.B \-u
+Sets the attribute in the user namespace.
+Only one namespace option can be specified.
+.TP
+.B \-s
+Sets the attribute in the secure namespace.
+Only one namespace option can be specified.
+.TP
+.B \-n
+Do not enable 'noattr2' mode on V4 filesystems.
+.RE
+.TP
+.BI "attr_set [\-r|\-u|\-s] [\-n] [\-R|\-C] [\-v " namelen "] " name
+Sets an extended attribute on the current file with the given name.
+.RS 1.0i
+.TP 0.4i
+.B \-r
+Sets the attribute in the root namespace.
+Only one namespace option can be specified.
+.TP
+.B \-u
+Sets the attribute in the user namespace.
+Only one namespace option can be specified.
+.TP
+.B \-s
+Sets the attribute in the secure namespace.
+Only one namespace option can be specified.
+.TP
+.B \-n
+Do not enable 'noattr2' mode on V4 filesystems.
+.TP
+.B \-R
+Replace the attribute.
+The command will fail if the attribute does not already exist.
+.TP
+.B \-C
+Create the attribute.
+The command will fail if the attribute already exists.
+.TP
+.B \-v
+Set the attribute value to a string of this length containing the letter 'v'.
+.RE
+.TP
 .B b
 See the
 .B back
@@ -737,6 +788,13 @@ Start logging output to
 .IR filename ,
 stop logging, or print the current logging status.
 .TP
+.BI "logformat [\-c " cycle "] [\-s " sunit "]"
+Reformats the log to the specified log cycle and log stripe unit.
+This has the effect of clearing the log destructively.
+If the log cycle is not specified, the log is reformatted to the current cycle.
+If the log stripe unit is not specified, the stripe unit from the filesystem
+superblock is used.
+.TP
 .B logres
 Print transaction reservation size information for each transaction type.
 This makes it easier to find discrepancies in the reservation calculations

[PATCH 5/8] xfs_db: dump per-AG reservations

[Darrick J. Wong]

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

Add a new 'agresv' command to print the size and free blocks count of an
AG along with the size and usage of the per-AG reservation.  This
command can be used to aid in diagnosing why a particular filesystem
fails the mount time per-AG space reservation, and to figure out how
much space needs to be freed from a given AG to fix the problem.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 db/info.c                |  104 ++++++++++++++++++++++++++++++++++++++++++++++
 libxfs/libxfs_api_defs.h |    5 ++
 man/man8/xfs_db.8        |    5 ++
 3 files changed, 114 insertions(+)


diff --git a/db/info.c b/db/info.c
index e5f1c2dd..fc5ccfe7 100644
--- a/db/info.c
+++ b/db/info.c
@@ -8,6 +8,7 @@
 #include "init.h"
 #include "output.h"
 #include "libfrog/fsgeom.h"
+#include "libfrog/logging.h"
 
 static void
 info_help(void)
@@ -45,8 +46,111 @@ static const struct cmdinfo info_cmd = {
 	.help =		info_help,
 };
 
+static void
+agresv_help(void)
+{
+	dbprintf(_(
+"\n"
+" Print the size and per-AG reservation information some allocation groups.\n"
+"\n"
+" Specific allocation group numbers can be provided as command line arguments.\n"
+" If no arguments are provided, all allocation groups are iterated.\n"
+"\n"
+));
+
+}
+
+static void
+print_agresv_info(
+	xfs_agnumber_t	agno)
+{
+	struct xfs_buf	*bp;
+	struct xfs_agf	*agf;
+	xfs_extlen_t	ask = 0;
+	xfs_extlen_t	used = 0;
+	xfs_extlen_t	free = 0;
+	xfs_extlen_t	length = 0;
+	int		error;
+
+	error = -libxfs_refcountbt_calc_reserves(mp, NULL, agno, &ask, &used);
+	if (error)
+		xfrog_perror(error, "refcountbt");
+	error = -libxfs_finobt_calc_reserves(mp, NULL, agno, &ask, &used);
+	if (error)
+		xfrog_perror(error, "finobt");
+	error = -libxfs_rmapbt_calc_reserves(mp, NULL, agno, &ask, &used);
+	if (error)
+		xfrog_perror(error, "rmapbt");
+
+	error = -libxfs_read_agf(mp, NULL, agno, 0, &bp);
+	if (error)
+		xfrog_perror(error, "AGF");
+	agf = XFS_BUF_TO_AGF(bp);
+	length = be32_to_cpu(agf->agf_length);
+	free = be32_to_cpu(agf->agf_freeblks) +
+	       be32_to_cpu(agf->agf_flcount);
+	libxfs_putbuf(bp);
+
+	printf("AG %d: length: %u free: %u reserved: %u used: %u",
+			agno, length, free, ask, used);
+	if (ask - used > free)
+		printf(" <not enough space>");
+	printf("\n");
+}
+
+static int
+agresv_f(
+	int			argc,
+	char			**argv)
+{
+	xfs_agnumber_t		agno;
+	int			i;
+
+	if (argc > 1) {
+		for (i = 1; i < argc; i++) {
+			long	a;
+			char	*p;
+
+			errno = 0;
+			a = strtol(argv[i], &p, 0);
+			if (p == argv[i])
+				errno = ERANGE;
+			if (errno) {
+				perror(argv[i]);
+				continue;
+			}
+
+			if (a < 0 || a >= mp->m_sb.sb_agcount) {
+				fprintf(stderr, "%ld: Not a AG.\n", a);
+				continue;
+			}
+
+			print_agresv_info(a);
+		}
+		return 0;
+	}
+
+	for (agno = 0; agno < mp->m_sb.sb_agcount; agno++)
+		print_agresv_info(agno);
+
+	return 0;
+}
+
+static const struct cmdinfo agresv_cmd = {
+	.name =		"agresv",
+	.altname =	NULL,
+	.cfunc =	agresv_f,
+	.argmin =	0,
+	.argmax =	-1,
+	.canpush =	0,
+	.args =		NULL,
+	.oneline =	N_("print AG reservation stats"),
+	.help =		agresv_help,
+};
+
 void
 info_init(void)
 {
 	add_command(&info_cmd);
+	add_command(&agresv_cmd);
 }
diff --git a/libxfs/libxfs_api_defs.h b/libxfs/libxfs_api_defs.h
index eed63ace..cc7304ad 100644
--- a/libxfs/libxfs_api_defs.h
+++ b/libxfs/libxfs_api_defs.h
@@ -173,4 +173,9 @@
 #define xfs_ag_init_headers		libxfs_ag_init_headers
 #define xfs_buf_delwri_submit		libxfs_buf_delwri_submit
 
+#define xfs_refcountbt_calc_reserves	libxfs_refcountbt_calc_reserves
+#define xfs_finobt_calc_reserves	libxfs_finobt_calc_reserves
+#define xfs_rmapbt_calc_reserves	libxfs_rmapbt_calc_reserves
+#define xfs_read_agf			libxfs_read_agf
+
 #endif /* __LIBXFS_API_DEFS_H__ */
diff --git a/man/man8/xfs_db.8 b/man/man8/xfs_db.8
index 9f1ff761..7f73d458 100644
--- a/man/man8/xfs_db.8
+++ b/man/man8/xfs_db.8
@@ -179,6 +179,11 @@ Set current address to the AGI block for allocation group
 .IR agno .
 If no argument is given, use the current allocation group.
 .TP
+.BI "agresv [" agno ]
+Displays the length, free block count, per-AG reservation size, and per-AG
+reservation usage for a given AG.
+If no argument is given, display information for all AGs.
+.TP
 .BI "attr_remove [\-r|\-u|\-s] [\-n] " name
 Remove the specified extended attribute from the current file.
 .RS 1.0i

[PATCH 6/8] xfs_io: fix copy_file_range length argument overflow

[Darrick J. Wong]

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

Don't let the length argument overflow size_t.  This is mostly a problem
on 32-bit platforms.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 io/copy_file_range.c |   15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)


diff --git a/io/copy_file_range.c b/io/copy_file_range.c
index 800b98da..fb5702e1 100644
--- a/io/copy_file_range.c
+++ b/io/copy_file_range.c
@@ -71,6 +71,7 @@ copy_range_f(int argc, char **argv)
 {
 	long long src_off = 0;
 	long long dst_off = 0;
+	long long llen;
 	size_t len = 0;
 	bool len_specified = false;
 	int opt;
@@ -99,11 +100,21 @@ copy_range_f(int argc, char **argv)
 			}
 			break;
 		case 'l':
-			len = cvtnum(fsblocksize, fssectsize, optarg);
-			if (len == -1LL) {
+			llen = cvtnum(fsblocksize, fssectsize, optarg);
+			if (llen == -1LL) {
 				printf(_("invalid length -- %s\n"), optarg);
 				return 0;
 			}
+			/*
+			 * If size_t can't hold what's in llen, report a
+			 * length overflow.
+			 */
+			if ((size_t)llen != llen) {
+				errno = EOVERFLOW;
+				perror("copy_range");
+				return 0;
+			}
+			len = llen;
 			len_specified = true;
 			break;
 		case 'f':

[PATCH 7/8] xfs_io: fix pwrite/pread length truncation on 32-bit systems

[Darrick J. Wong]

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

The pwrite and pread commands in xfs_io accept an operation length that
can be any quantity that fits in a long long int; and loops to handle
the cases where the operation length is larger than the IO buffer.

Weirdly, the do_ functions contain code to shorten the operation to the
IO buffer size but the @count parameter is size_t, which means that for
a large argument on a 32-bit system, we rip off the upper bits of the
length, turning your 8GB write into a 0 byte write, which does nothing.

This was found by running generic/175 and observing that the 8G test
file it creates has zero length after the operation:

wrote 0/8589934592 bytes at offset 0
0.000000 bytes, 0 ops; 0.0001 sec (0.000000 bytes/sec and 0.0000 ops/sec)

Fix this by pushing long long count all the way through the call stack.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 io/pread.c  |    4 ++--
 io/pwrite.c |    6 +++---
 2 files changed, 5 insertions(+), 5 deletions(-)


diff --git a/io/pread.c b/io/pread.c
index 1b4352be..d52e21d9 100644
--- a/io/pread.c
+++ b/io/pread.c
@@ -164,7 +164,7 @@ static ssize_t
 do_preadv(
 	int		fd,
 	off64_t		offset,
-	size_t		count)
+	long long	count)
 {
 	int		vecs = 0;
 	ssize_t		oldlen = 0;
@@ -199,7 +199,7 @@ static ssize_t
 do_pread(
 	int		fd,
 	off64_t		offset,
-	size_t		count,
+	long long	count,
 	size_t		buffer_size)
 {
 	if (!vectors)
diff --git a/io/pwrite.c b/io/pwrite.c
index ccf14be9..1c28612f 100644
--- a/io/pwrite.c
+++ b/io/pwrite.c
@@ -54,8 +54,8 @@ static ssize_t
 do_pwritev(
 	int		fd,
 	off64_t		offset,
-	size_t		count,
-	int 		pwritev2_flags)
+	long long	count,
+	int		pwritev2_flags)
 {
 	int vecs = 0;
 	ssize_t oldlen = 0;
@@ -97,7 +97,7 @@ static ssize_t
 do_pwrite(
 	int		fd,
 	off64_t		offset,
-	size_t		count,
+	long long	count,
 	size_t		buffer_size,
 	int		pwritev2_flags)
 {

[PATCH 8/8] xfs_repair: fix totally broken unit conversion in directory invalidation

[Darrick J. Wong]

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

Your humble author forgot that xfs_dablk_t has the same units as
xfs_fileoff_t, and totally screwed up the directory buffer invalidation
loop in dir_binval.  Not only is there an off-by-one error in the loop
conditional, but the unit conversions are wrong.

Fix all this stupidity by adding a for loop macro to take care of these
details for us so that everyone can iterate all logical directory blocks
(xfs_dir2_db_t) that start within a given bmbt record.

The pre-5.5 xfs_da_get_buf implementation mostly hides the off-by-one
error because dir_binval turns on "don't complain if no mapping" mode,
but on dirblocksize > fsblocksize filesystems the incorrect units can
cause us to miss invalidating some blocks, which can lead to other
buffer cache errors later.

Fixes: f9c559f4e4fb4 ("xfs_repair: invalidate dirty dir buffers when we zap a  directory")
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/phase6.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)


diff --git a/repair/phase6.c b/repair/phase6.c
index 76709f73..0874b649 100644
--- a/repair/phase6.c
+++ b/repair/phase6.c
@@ -1273,7 +1273,7 @@ dir_binval(
 	struct xfs_ifork	*ifp;
 	struct xfs_da_geometry	*geo;
 	struct xfs_buf		*bp;
-	xfs_dablk_t		dabno, end_dabno;
+	xfs_dablk_t		dabno;
 	int			error = 0;
 
 	if (ip->i_d.di_format != XFS_DINODE_FMT_EXTENTS &&
@@ -1283,11 +1283,9 @@ dir_binval(
 	geo = tp->t_mountp->m_dir_geo;
 	ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
 	for_each_xfs_iext(ifp, &icur, &rec) {
-		dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
-				geo->fsbcount - 1);
-		end_dabno = xfs_dir2_db_to_da(geo, rec.br_startoff +
-				rec.br_blockcount);
-		for (; dabno <= end_dabno; dabno += geo->fsbcount) {
+		for (dabno = roundup(rec.br_startoff, geo->fsbcount);
+		     dabno < rec.br_startoff + rec.br_blockcount;
+		     dabno += geo->fsbcount) {
 			bp = NULL;
 			error = -libxfs_da_get_buf(tp, ip, dabno, &bp,
 					whichfork);

Re: [PATCH 1/8] man: list xfs_io lsattr inode flag letters

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/8] man: document the xfs_db btheight command

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 3/8] man: reformat xfs_quota commands in the manpage for testing

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 4/8] man: document some missing xfs_db commands

[Christoph Hellwig]

On Thu, Jan 23, 2020 at 04:16:56PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The 'attr_set', 'attr_remove', and 'logformat' commands in xfs_db were
> not documented.  Add sections about them to the manpage.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 5/8] xfs_db: dump per-AG reservations

[Christoph Hellwig]

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 6/8] xfs_io: fix copy_file_range length argument overflow

[Christoph Hellwig]

On Thu, Jan 23, 2020 at 04:17:11PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Don't let the length argument overflow size_t.  This is mostly a problem
> on 32-bit platforms.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 7/8] xfs_io: fix pwrite/pread length truncation on 32-bit systems

[Christoph Hellwig]

On Thu, Jan 23, 2020 at 04:17:17PM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> The pwrite and pread commands in xfs_io accept an operation length that
> can be any quantity that fits in a long long int; and loops to handle
> the cases where the operation length is larger than the IO buffer.
> 
> Weirdly, the do_ functions contain code to shorten the operation to the
> IO buffer size but the @count parameter is size_t, which means that for
> a large argument on a 32-bit system, we rip off the upper bits of the
> length, turning your 8GB write into a 0 byte write, which does nothing.
> 
> This was found by running generic/175 and observing that the 8G test
> file it creates has zero length after the operation:
> 
> wrote 0/8589934592 bytes at offset 0
> 0.000000 bytes, 0 ops; 0.0001 sec (0.000000 bytes/sec and 0.0000 ops/sec)
> 
> Fix this by pushing long long count all the way through the call stack.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 8/8] xfs_repair: fix totally broken unit conversion in directory invalidation

[Christoph Hellwig]

On Thu, Jan 23, 2020 at 04:17:23PM -0800, Darrick J. Wong wrote:
> Fix all this stupidity by adding a for loop macro to take care of these
> details for us so that everyone can iterate all logical directory blocks
> (xfs_dir2_db_t) that start within a given bmbt record.

No more magic macro in here (thankfully).  But the rest looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 8/8] xfs_repair: fix totally broken unit conversion in directory invalidation

[Darrick J. Wong]

On Sat, Jan 25, 2020 at 03:19:27PM -0800, Christoph Hellwig wrote:
> On Thu, Jan 23, 2020 at 04:17:23PM -0800, Darrick J. Wong wrote:
> > Fix all this stupidity by adding a for loop macro to take care of these
> > details for us so that everyone can iterate all logical directory blocks
> > (xfs_dir2_db_t) that start within a given bmbt record.
> 
> No more magic macro in here (thankfully).  But the rest looks good:

Oops, yeah... Eric, if you decide to take this patch, could you please
drop that second paragraph about the macro?

> Reviewed-by: Christoph Hellwig <hch@lst.de>

Thank you for the reviews!

--D

[PATCH 9/8] xfs_io: fix integer over/underflow handling in timespec_from_string

[Darrick J. Wong]

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

When we're filling out the struct timespec, make sure we detect when the
string value cannot be represented by a (potentially 32-bit) seconds
field in struct timespec.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxcmd/input.c |   23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/libxcmd/input.c b/libxcmd/input.c
index d232d4f3..137856e3 100644
--- a/libxcmd/input.c
+++ b/libxcmd/input.c
@@ -183,19 +183,30 @@ timestr(
 
 int
 timespec_from_string(
-	const char	* secs,
-	const char	* nsecs,
-	struct timespec	* ts)
+	const char		*secs,
+	const char		*nsecs,
+	struct timespec		*ts)
 {
-	char* p;
+	char			*p;
+	unsigned long long int	ll;
+
 	if (!secs || !nsecs || !ts)
 		return 1;
-	ts->tv_sec = strtoull(secs, &p, 0);
+
+	ll = strtoull(secs, &p, 0);
 	if (*p)
 		return 1;
-	ts->tv_nsec = strtoull(nsecs, &p, 0);
+	ts->tv_sec = ll;
+	if ((unsigned long long int)ts->tv_sec != ll)
+		return 1;
+
+	ll = strtoull(nsecs, &p, 0);
 	if (*p)
 		return 1;
+	ts->tv_nsec = ll;
+	if ((unsigned long long int)ts->tv_nsec != ll)
+		return 1;
+
 	return 0;
 }
 

[PATCH 10/8] libxfs: remove duplicate attr function declarations

[Darrick J. Wong]

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

Remove these function declarations since they're in libxfs/xfs_attr.h
and are therefore redundant.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/libxfs_priv.h |    8 --------
 1 file changed, 8 deletions(-)

diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
index 03edf0d3..fe08f96b 100644
--- a/libxfs/libxfs_priv.h
+++ b/libxfs/libxfs_priv.h
@@ -607,14 +607,6 @@ static inline int test_and_set_bit(int nr, volatile unsigned long *addr)
 }
 
 /* Keep static checkers quiet about nonstatic functions by exporting */
-int xfs_inode_hasattr(struct xfs_inode *ip);
-int xfs_attr_get_ilocked(struct xfs_inode *ip, struct xfs_da_args *args);
-int xfs_attr_get(struct xfs_inode *ip, const unsigned char *name,
-                unsigned char **value, int *valuelenp, int flags);
-int xfs_attr_set(struct xfs_inode *dp, const unsigned char *name,
-                 unsigned char *value, int valuelen, int flags);
-int xfs_attr_remove(struct xfs_inode *dp, const unsigned char *name, int flags);
-
 int xfs_rtbuf_get(struct xfs_mount *mp, struct xfs_trans *tp,
 		  xfs_rtblock_t block, int issum, struct xfs_buf **bpp);
 int xfs_rtcheck_range(struct xfs_mount *mp, struct xfs_trans *tp,

[PATCH 11/8] xfs_repair: don't corrupt a attr fork da3 node when clearing forw/back

[Darrick J. Wong]

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

In process_longform_attr, we enforce that the root block of the
attribute index must have both forw or back pointers set to zero.
Unfortunately, the code that nulls out the pointers is not aware that
the root block could be in da3 node format.

This leads to corruption of da3 root node blocks because the functions
that convert attr3 leaf headers to and from the ondisk structures
perform some interpretation of firstused on what they think is an attr1
leaf block.

Found by using xfs/402 to fuzz hdr.info.hdr.forw.

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

diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index 9a44f610..9d2a40f7 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -952,6 +952,33 @@ _("wrong FS UUID, inode %" PRIu64 " attr block %" PRIu64 "\n"),
 	return 0;
 }
 
+/*
+ * Zap the forw/back links in an attribute block.  Be careful, because the
+ * root block could be an attr leaf block or a da node block.
+ */
+static inline void
+clear_attr_forw_back(
+	struct xfs_buf			*bp,
+	struct xfs_attr3_icleaf_hdr	*leafhdr)
+{
+	struct xfs_mount		*mp = bp->b_mount;
+
+	if (leafhdr->magic == XFS_DA_NODE_MAGIC ||
+	    leafhdr->magic == XFS_DA3_NODE_MAGIC) {
+		struct xfs_da3_icnode_hdr	da3_hdr;
+
+		xfs_da3_node_hdr_from_disk(mp, &da3_hdr, bp->b_addr);
+		da3_hdr.forw = 0;
+		da3_hdr.back = 0;
+		xfs_da3_node_hdr_to_disk(mp, bp->b_addr, &da3_hdr);
+		return;
+	}
+
+	leafhdr->forw = 0;
+	leafhdr->back = 0;
+	xfs_attr3_leaf_hdr_to_disk(mp->m_attr_geo, bp->b_addr, leafhdr);
+}
+
 /*
  * Start processing for a leaf or fuller btree.
  * A leaf directory is one where the attribute fork is too big for
@@ -1028,10 +1055,7 @@ process_longform_attr(
 	_("clearing forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"),
 				ino);
 			repairlinks = 1;
-			leafhdr.forw = 0;
-			leafhdr.back = 0;
-			xfs_attr3_leaf_hdr_to_disk(mp->m_attr_geo,
-						   leaf, &leafhdr);
+			clear_attr_forw_back(bp, &leafhdr);
 		} else  {
 			do_warn(
 	_("would clear forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"), ino);

Re: [PATCH 10/8] libxfs: remove duplicate attr function declarations

[Christoph Hellwig]

On Thu, Jan 30, 2020 at 10:13:30AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Remove these function declarations since they're in libxfs/xfs_attr.h
> and are therefore redundant.

Looks fine - in fact I have these removals in my attr series, just
piecemail..

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 9/8] xfs_io: fix integer over/underflow handling in timespec_from_string

[Christoph Hellwig]

On Tue, Jan 28, 2020 at 07:56:48AM -0800, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we're filling out the struct timespec, make sure we detect when the
> string value cannot be represented by a (potentially 32-bit) seconds
> field in struct timespec.

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 11/8] xfs_repair: don't corrupt a attr fork da3 node when clearing forw/back

[Christoph Hellwig]

While this looks functionally correct, I think the structure in
here is weird.  In libxfs we usually check that magic number first
and then branch out into helper that deal with the leaf vs node
format.  That is we don't do the xfs_attr3_leaf_hdr_from_disk call
for node format attrs, and also check the forward/backward pointers
based on the actual ichdr.  Maybe this code should follow that
structure?

Re: [PATCH 10/8] libxfs: remove duplicate attr function declarations

[Eric Sandeen]

On 1/30/20 12:13 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Remove these function declarations since they're in libxfs/xfs_attr.h
> and are therefore redundant.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Is it worth keeping this exporting hack around to make static checkers
happy if it's just one more thing to keep up to date in userspace?

-Eric

Re: [PATCH 11/8] xfs_repair: don't corrupt a attr fork da3 node when clearing forw/back

[Darrick J. Wong]

On Thu, Jan 30, 2020 at 10:22:30AM -0800, Christoph Hellwig wrote:
> While this looks functionally correct, I think the structure in
> here is weird.  In libxfs we usually check that magic number first
> and then branch out into helper that deal with the leaf vs node
> format.  That is we don't do the xfs_attr3_leaf_hdr_from_disk call
> for node format attrs, and also check the forward/backward pointers
> based on the actual ichdr.  Maybe this code should follow that
> structure?

Yeah, I'll refactor the case blocks into two functions.

--D

Re: [PATCH 8/8] xfs_repair: fix totally broken unit conversion in directory invalidation

[Eric Sandeen]

On 1/26/20 4:11 PM, Darrick J. Wong wrote:
> On Sat, Jan 25, 2020 at 03:19:27PM -0800, Christoph Hellwig wrote:
>> On Thu, Jan 23, 2020 at 04:17:23PM -0800, Darrick J. Wong wrote:
>>> Fix all this stupidity by adding a for loop macro to take care of these
>>> details for us so that everyone can iterate all logical directory blocks
>>> (xfs_dir2_db_t) that start within a given bmbt record.
>>
>> No more magic macro in here (thankfully).  But the rest looks good:
> 
> Oops, yeah... Eric, if you decide to take this patch, could you please
> drop that second paragraph about the macro?

Done

>> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Thank you for the reviews!
> 
> --D
> 

Re: [PATCH 10/8] libxfs: remove duplicate attr function declarations

[Darrick J. Wong]

On Thu, Jan 30, 2020 at 12:28:43PM -0600, Eric Sandeen wrote:
> On 1/30/20 12:13 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Remove these function declarations since they're in libxfs/xfs_attr.h
> > and are therefore redundant.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Is it worth keeping this exporting hack around to make static checkers
> happy if it's just one more thing to keep up to date in userspace?

Probably?  It depends on how much you like culling known false positives
when you run smatch/sparse against xfsprogs.

(I for one don't mind not having to remember that stuff...)

--D

> -Eric

[PATCH v2 11/8] xfs_repair: don't corrupt a attr fork da3 node when clearing forw/back

[Darrick J. Wong]

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

In process_longform_attr, we enforce that the root block of the
attribute index must have both forw or back pointers set to zero.
Unfortunately, the code that nulls out the pointers is not aware that
the root block could be in da3 node format.

This leads to corruption of da3 root node blocks because the functions
that convert attr3 leaf headers to and from the ondisk structures
perform some interpretation of firstused on what they think is an attr1
leaf block.

Found by using xfs/402 to fuzz hdr.info.hdr.forw.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: split the da node code into a separate function since that code path
exits out the bottom of the case statement.
---
 repair/attr_repair.c |   98 +++++++++++++++++++++++++++++++++++---------------
 1 file changed, 68 insertions(+), 30 deletions(-)

diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index 9a44f610..8b66a06d 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -952,6 +952,52 @@ _("wrong FS UUID, inode %" PRIu64 " attr block %" PRIu64 "\n"),
 	return 0;
 }
 
+static int
+process_longform_da_root(
+	struct xfs_mount	*mp,
+	xfs_ino_t		ino,
+	struct xfs_dinode	*dip,
+	struct blkmap		*blkmap,
+	int			*repair,
+	struct xfs_buf		*bp)
+{
+	struct xfs_da3_icnode_hdr	da3_hdr;
+	int			repairlinks = 0;
+	int			error;
+
+	xfs_da3_node_hdr_from_disk(mp, &da3_hdr, bp->b_addr);
+	/*
+	 * check sibling pointers in leaf block or root block 0 before
+	 * we have to release the btree block
+	 */
+	if (da3_hdr.forw != 0 || da3_hdr.back != 0)  {
+		if (!no_modify)  {
+			do_warn(
+_("clearing forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"),
+				ino);
+
+			repairlinks = 1;
+			da3_hdr.forw = 0;
+			da3_hdr.back = 0;
+			xfs_da3_node_hdr_to_disk(mp, bp->b_addr, &da3_hdr);
+		} else  {
+			do_warn(
+_("would clear forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"), ino);
+		}
+	}
+
+	/* must do this now, to release block 0 before the traversal */
+	if ((*repair || repairlinks) && !no_modify) {
+		*repair = 1;
+		libxfs_writebuf(bp, 0);
+	} else
+		libxfs_putbuf(bp);
+	error = process_node_attr(mp, ino, dip, blkmap); /* + repair */
+	if (error)
+		*repair = 0;
+	return error;
+}
+
 /*
  * Start processing for a leaf or fuller btree.
  * A leaf directory is one where the attribute fork is too big for
@@ -975,7 +1021,6 @@ process_longform_attr(
 	xfs_dahash_t	next_hashval;
 	int		repairlinks = 0;
 	struct xfs_attr3_icleaf_hdr leafhdr;
-	int		error;
 
 	*repair = 0;
 
@@ -1019,25 +1064,6 @@ process_longform_attr(
 	leaf = bp->b_addr;
 	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &leafhdr, leaf);
 
-	/* check sibling pointers in leaf block or root block 0 before
-	* we have to release the btree block
-	*/
-	if (leafhdr.forw != 0 || leafhdr.back != 0)  {
-		if (!no_modify)  {
-			do_warn(
-	_("clearing forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"),
-				ino);
-			repairlinks = 1;
-			leafhdr.forw = 0;
-			leafhdr.back = 0;
-			xfs_attr3_leaf_hdr_to_disk(mp->m_attr_geo,
-						   leaf, &leafhdr);
-		} else  {
-			do_warn(
-	_("would clear forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"), ino);
-		}
-	}
-
 	/*
 	 * use magic number to tell us what type of attribute this is.
 	 * it's possible to have a node or leaf attribute in either an
@@ -1046,6 +1072,26 @@ process_longform_attr(
 	switch (leafhdr.magic) {
 	case XFS_ATTR_LEAF_MAGIC:	/* leaf-form attribute */
 	case XFS_ATTR3_LEAF_MAGIC:
+		/*
+		 * check sibling pointers in leaf block or root block 0 before
+		 * we have to release the btree block
+		 */
+		if (leafhdr.forw != 0 || leafhdr.back != 0)  {
+			if (!no_modify)  {
+				do_warn(
+	_("clearing forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"),
+					ino);
+				repairlinks = 1;
+				leafhdr.forw = 0;
+				leafhdr.back = 0;
+				xfs_attr3_leaf_hdr_to_disk(mp->m_attr_geo,
+						leaf, &leafhdr);
+			} else  {
+				do_warn(
+	_("would clear forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"), ino);
+			}
+		}
+
 		if (process_leaf_attr_block(mp, leaf, 0, ino, blkmap,
 				0, &next_hashval, repair)) {
 			*repair = 0;
@@ -1058,16 +1104,8 @@ process_longform_attr(
 
 	case XFS_DA_NODE_MAGIC:		/* btree-form attribute */
 	case XFS_DA3_NODE_MAGIC:
-		/* must do this now, to release block 0 before the traversal */
-		if ((*repair || repairlinks) && !no_modify) {
-			*repair = 1;
-			libxfs_writebuf(bp, 0);
-		} else
-			libxfs_putbuf(bp);
-		error = process_node_attr(mp, ino, dip, blkmap); /* + repair */
-		if (error)
-			*repair = 0;
-		return error;
+		return process_longform_da_root(mp, ino, dip, blkmap, repair,
+				bp);
 	default:
 		do_warn(
 	_("bad attribute leaf magic # %#x for dir ino %" PRIu64 "\n"),

Re: [PATCH 10/8] libxfs: remove duplicate attr function declarations

[Eric Sandeen]

On 1/30/20 12:40 PM, Darrick J. Wong wrote:
> On Thu, Jan 30, 2020 at 12:28:43PM -0600, Eric Sandeen wrote:
>> On 1/30/20 12:13 PM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>
>>> Remove these function declarations since they're in libxfs/xfs_attr.h
>>> and are therefore redundant.
>>>
>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> Is it worth keeping this exporting hack around to make static checkers
>> happy if it's just one more thing to keep up to date in userspace?
> 
> Probably?  It depends on how much you like culling known false positives
> when you run smatch/sparse against xfsprogs.
> 
> (I for one don't mind not having to remember that stuff...)

Ok.  I'm wondering how you guys happened to notice the dups.

Seems like

bool xfs_verify_rtbno(struct xfs_mount *mp, xfs_rtblock_t rtbno);

can be removed as well, FWIW.

-Eric

Re: [PATCH v2 11/8] xfs_repair: don't corrupt a attr fork da3 node when clearing forw/back

[Christoph Hellwig]

Looks sensible, but I think we want the helpers for both the node and
leaf case, something like this untested patch:

diff --git a/repair/attr_repair.c b/repair/attr_repair.c
index 9a44f610..0c26f0e6 100644
--- a/repair/attr_repair.c
+++ b/repair/attr_repair.c
@@ -952,6 +952,98 @@ _("wrong FS UUID, inode %" PRIu64 " attr block %" PRIu64 "\n"),
 	return 0;
 }
 
+static int
+process_leaf_da_root(
+	struct xfs_mount	*mp,
+	xfs_ino_t		ino,
+	struct xfs_dinode	*dip,
+	struct blkmap		*blkmap,
+	int			*repair,
+	struct xfs_buf		*bp)
+{
+	struct xfs_attr3_icleaf_hdr leafhdr;
+	xfs_dahash_t		next_hashval;
+	int			repairlinks = 0;
+
+	/*
+	 * Check sibling pointers in block 0 before we have to release the btree
+	 * block.
+	 */
+	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &leafhdr, bp->b_addr);
+	if (leafhdr.forw != 0 || leafhdr.back != 0)  {
+		if (!no_modify)  {
+			do_warn(
+	_("clearing forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"),
+				ino);
+			repairlinks = 1;
+			leafhdr.forw = 0;
+			leafhdr.back = 0;
+			xfs_attr3_leaf_hdr_to_disk(mp->m_attr_geo, bp->b_addr,
+					&leafhdr);
+		} else  {
+			do_warn(
+	_("would clear forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"), ino);
+		}
+	}
+
+	if (process_leaf_attr_block(mp, bp->b_addr, 0, ino, blkmap, 0,
+			&next_hashval, repair)) {
+		*repair = 0;
+		/* the block is bad.  lose the attribute fork. */
+		libxfs_putbuf(bp);
+		return 1;
+	}
+
+	*repair = *repair || repairlinks;
+	return 0;
+}
+
+static int
+process_node_da_root(
+	struct xfs_mount	*mp,
+	xfs_ino_t		ino,
+	struct xfs_dinode	*dip,
+	struct blkmap		*blkmap,
+	int			*repair,
+	struct xfs_buf		*bp)
+{
+	struct xfs_da3_icnode_hdr	da3_hdr;
+	int			repairlinks = 0;
+	int			error;
+
+	/*
+	 * Check sibling pointers in block 0 before we have to release the btree
+	 * block.
+	 */
+	xfs_da3_node_hdr_from_disk(mp, &da3_hdr, bp->b_addr);
+	if (da3_hdr.forw != 0 || da3_hdr.back != 0)  {
+		if (!no_modify)  {
+			do_warn(
+_("clearing forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"),
+				ino);
+
+			repairlinks = 1;
+			da3_hdr.forw = 0;
+			da3_hdr.back = 0;
+			xfs_da3_node_hdr_to_disk(mp, bp->b_addr, &da3_hdr);
+		} else  {
+			do_warn(
+_("would clear forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"), ino);
+		}
+	}
+
+	/* must do this now, to release block 0 before the traversal */
+	if ((*repair || repairlinks) && !no_modify) {
+		*repair = 1;
+		libxfs_writebuf(bp, 0);
+	} else
+		libxfs_putbuf(bp);
+	error = process_node_attr(mp, ino, dip, blkmap); /* + repair */
+	if (error)
+		*repair = 0;
+	return error;
+}
+
 /*
  * Start processing for a leaf or fuller btree.
  * A leaf directory is one where the attribute fork is too big for
@@ -963,19 +1055,15 @@ _("wrong FS UUID, inode %" PRIu64 " attr block %" PRIu64 "\n"),
  */
 static int
 process_longform_attr(
-	xfs_mount_t	*mp,
-	xfs_ino_t	ino,
-	xfs_dinode_t	*dip,
-	blkmap_t	*blkmap,
-	int		*repair)	/* out - 1 if something was fixed */
+	struct xfs_mount	*mp,
+	xfs_ino_t		ino,
+	struct xfs_dinode	*dip,
+	blkmap_t		*blkmap,
+	int			*repair) /* out - 1 if something was fixed */
 {
-	xfs_attr_leafblock_t	*leaf;
-	xfs_fsblock_t	bno;
-	xfs_buf_t	*bp;
-	xfs_dahash_t	next_hashval;
-	int		repairlinks = 0;
-	struct xfs_attr3_icleaf_hdr leafhdr;
-	int		error;
+	xfs_fsblock_t		bno;
+	struct xfs_buf		*bp;
+	struct xfs_da_blkinfo   *info;
 
 	*repair = 0;
 
@@ -1015,77 +1103,35 @@ process_longform_attr(
 		return 1;
 	}
 
-	/* verify leaf block */
-	leaf = bp->b_addr;
-	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &leafhdr, leaf);
-
-	/* check sibling pointers in leaf block or root block 0 before
-	* we have to release the btree block
-	*/
-	if (leafhdr.forw != 0 || leafhdr.back != 0)  {
-		if (!no_modify)  {
-			do_warn(
-	_("clearing forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"),
-				ino);
-			repairlinks = 1;
-			leafhdr.forw = 0;
-			leafhdr.back = 0;
-			xfs_attr3_leaf_hdr_to_disk(mp->m_attr_geo,
-						   leaf, &leafhdr);
-		} else  {
-			do_warn(
-	_("would clear forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"), ino);
-		}
-	}
-
 	/*
 	 * use magic number to tell us what type of attribute this is.
 	 * it's possible to have a node or leaf attribute in either an
 	 * extent format or btree format attribute fork.
 	 */
-	switch (leafhdr.magic) {
+	info = bp->b_addr;
+	switch (be16_to_cpu(info->magic)) {
 	case XFS_ATTR_LEAF_MAGIC:	/* leaf-form attribute */
 	case XFS_ATTR3_LEAF_MAGIC:
-		if (process_leaf_attr_block(mp, leaf, 0, ino, blkmap,
-				0, &next_hashval, repair)) {
-			*repair = 0;
-			/* the block is bad.  lose the attribute fork. */
-			libxfs_putbuf(bp);
-			return(1);
-		}
-		*repair = *repair || repairlinks;
-		break;
-
+		return process_leaf_da_root(mp, ino, dip, blkmap, repair, bp);
 	case XFS_DA_NODE_MAGIC:		/* btree-form attribute */
 	case XFS_DA3_NODE_MAGIC:
-		/* must do this now, to release block 0 before the traversal */
-		if ((*repair || repairlinks) && !no_modify) {
-			*repair = 1;
-			libxfs_writebuf(bp, 0);
-		} else
-			libxfs_putbuf(bp);
-		error = process_node_attr(mp, ino, dip, blkmap); /* + repair */
-		if (error)
-			*repair = 0;
-		return error;
+		return process_node_da_root(mp, ino, dip, blkmap, repair, bp);
 	default:
 		do_warn(
 	_("bad attribute leaf magic # %#x for dir ino %" PRIu64 "\n"),
-			be16_to_cpu(leaf->hdr.info.magic), ino);
+			be16_to_cpu(info->magic), ino);
 		libxfs_putbuf(bp);
 		*repair = 0;
-		return(1);
+		return 1;
 	}
 
 	if (*repair && !no_modify)
 		libxfs_writebuf(bp, 0);
 	else
 		libxfs_putbuf(bp);
-
-	return(0);  /* repair may be set */
+	return 0;  /* repair may be set */
 }
 
-
 static int
 xfs_acl_from_disk(
 	struct xfs_mount	*mp,

Re: [PATCH v2 11/8] xfs_repair: don't corrupt a attr fork da3 node when clearing forw/back

[Darrick J. Wong]

On Thu, Jan 30, 2020 at 10:03:15PM -0800, Christoph Hellwig wrote:
> Looks sensible, but I think we want the helpers for both the node and
> leaf case, something like this untested patch:

Ok, I was about to repost with more or less the same code.

--D

> diff --git a/repair/attr_repair.c b/repair/attr_repair.c
> index 9a44f610..0c26f0e6 100644
> --- a/repair/attr_repair.c
> +++ b/repair/attr_repair.c
> @@ -952,6 +952,98 @@ _("wrong FS UUID, inode %" PRIu64 " attr block %" PRIu64 "\n"),
>  	return 0;
>  }
>  
> +static int
> +process_leaf_da_root(
> +	struct xfs_mount	*mp,
> +	xfs_ino_t		ino,
> +	struct xfs_dinode	*dip,
> +	struct blkmap		*blkmap,
> +	int			*repair,
> +	struct xfs_buf		*bp)
> +{
> +	struct xfs_attr3_icleaf_hdr leafhdr;
> +	xfs_dahash_t		next_hashval;
> +	int			repairlinks = 0;
> +
> +	/*
> +	 * Check sibling pointers in block 0 before we have to release the btree
> +	 * block.
> +	 */
> +	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &leafhdr, bp->b_addr);
> +	if (leafhdr.forw != 0 || leafhdr.back != 0)  {
> +		if (!no_modify)  {
> +			do_warn(
> +	_("clearing forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"),
> +				ino);
> +			repairlinks = 1;
> +			leafhdr.forw = 0;
> +			leafhdr.back = 0;
> +			xfs_attr3_leaf_hdr_to_disk(mp->m_attr_geo, bp->b_addr,
> +					&leafhdr);
> +		} else  {
> +			do_warn(
> +	_("would clear forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"), ino);
> +		}
> +	}
> +
> +	if (process_leaf_attr_block(mp, bp->b_addr, 0, ino, blkmap, 0,
> +			&next_hashval, repair)) {
> +		*repair = 0;
> +		/* the block is bad.  lose the attribute fork. */
> +		libxfs_putbuf(bp);
> +		return 1;
> +	}
> +
> +	*repair = *repair || repairlinks;
> +	return 0;
> +}
> +
> +static int
> +process_node_da_root(
> +	struct xfs_mount	*mp,
> +	xfs_ino_t		ino,
> +	struct xfs_dinode	*dip,
> +	struct blkmap		*blkmap,
> +	int			*repair,
> +	struct xfs_buf		*bp)
> +{
> +	struct xfs_da3_icnode_hdr	da3_hdr;
> +	int			repairlinks = 0;
> +	int			error;
> +
> +	/*
> +	 * Check sibling pointers in block 0 before we have to release the btree
> +	 * block.
> +	 */
> +	xfs_da3_node_hdr_from_disk(mp, &da3_hdr, bp->b_addr);
> +	if (da3_hdr.forw != 0 || da3_hdr.back != 0)  {
> +		if (!no_modify)  {
> +			do_warn(
> +_("clearing forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"),
> +				ino);
> +
> +			repairlinks = 1;
> +			da3_hdr.forw = 0;
> +			da3_hdr.back = 0;
> +			xfs_da3_node_hdr_to_disk(mp, bp->b_addr, &da3_hdr);
> +		} else  {
> +			do_warn(
> +_("would clear forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"), ino);
> +		}
> +	}
> +
> +	/* must do this now, to release block 0 before the traversal */
> +	if ((*repair || repairlinks) && !no_modify) {
> +		*repair = 1;
> +		libxfs_writebuf(bp, 0);
> +	} else
> +		libxfs_putbuf(bp);
> +	error = process_node_attr(mp, ino, dip, blkmap); /* + repair */
> +	if (error)
> +		*repair = 0;
> +	return error;
> +}
> +
>  /*
>   * Start processing for a leaf or fuller btree.
>   * A leaf directory is one where the attribute fork is too big for
> @@ -963,19 +1055,15 @@ _("wrong FS UUID, inode %" PRIu64 " attr block %" PRIu64 "\n"),
>   */
>  static int
>  process_longform_attr(
> -	xfs_mount_t	*mp,
> -	xfs_ino_t	ino,
> -	xfs_dinode_t	*dip,
> -	blkmap_t	*blkmap,
> -	int		*repair)	/* out - 1 if something was fixed */
> +	struct xfs_mount	*mp,
> +	xfs_ino_t		ino,
> +	struct xfs_dinode	*dip,
> +	blkmap_t		*blkmap,
> +	int			*repair) /* out - 1 if something was fixed */
>  {
> -	xfs_attr_leafblock_t	*leaf;
> -	xfs_fsblock_t	bno;
> -	xfs_buf_t	*bp;
> -	xfs_dahash_t	next_hashval;
> -	int		repairlinks = 0;
> -	struct xfs_attr3_icleaf_hdr leafhdr;
> -	int		error;
> +	xfs_fsblock_t		bno;
> +	struct xfs_buf		*bp;
> +	struct xfs_da_blkinfo   *info;
>  
>  	*repair = 0;
>  
> @@ -1015,77 +1103,35 @@ process_longform_attr(
>  		return 1;
>  	}
>  
> -	/* verify leaf block */
> -	leaf = bp->b_addr;
> -	xfs_attr3_leaf_hdr_from_disk(mp->m_attr_geo, &leafhdr, leaf);
> -
> -	/* check sibling pointers in leaf block or root block 0 before
> -	* we have to release the btree block
> -	*/
> -	if (leafhdr.forw != 0 || leafhdr.back != 0)  {
> -		if (!no_modify)  {
> -			do_warn(
> -	_("clearing forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"),
> -				ino);
> -			repairlinks = 1;
> -			leafhdr.forw = 0;
> -			leafhdr.back = 0;
> -			xfs_attr3_leaf_hdr_to_disk(mp->m_attr_geo,
> -						   leaf, &leafhdr);
> -		} else  {
> -			do_warn(
> -	_("would clear forw/back pointers in block 0 for attributes in inode %" PRIu64 "\n"), ino);
> -		}
> -	}
> -
>  	/*
>  	 * use magic number to tell us what type of attribute this is.
>  	 * it's possible to have a node or leaf attribute in either an
>  	 * extent format or btree format attribute fork.
>  	 */
> -	switch (leafhdr.magic) {
> +	info = bp->b_addr;
> +	switch (be16_to_cpu(info->magic)) {
>  	case XFS_ATTR_LEAF_MAGIC:	/* leaf-form attribute */
>  	case XFS_ATTR3_LEAF_MAGIC:
> -		if (process_leaf_attr_block(mp, leaf, 0, ino, blkmap,
> -				0, &next_hashval, repair)) {
> -			*repair = 0;
> -			/* the block is bad.  lose the attribute fork. */
> -			libxfs_putbuf(bp);
> -			return(1);
> -		}
> -		*repair = *repair || repairlinks;
> -		break;
> -
> +		return process_leaf_da_root(mp, ino, dip, blkmap, repair, bp);
>  	case XFS_DA_NODE_MAGIC:		/* btree-form attribute */
>  	case XFS_DA3_NODE_MAGIC:
> -		/* must do this now, to release block 0 before the traversal */
> -		if ((*repair || repairlinks) && !no_modify) {
> -			*repair = 1;
> -			libxfs_writebuf(bp, 0);
> -		} else
> -			libxfs_putbuf(bp);
> -		error = process_node_attr(mp, ino, dip, blkmap); /* + repair */
> -		if (error)
> -			*repair = 0;
> -		return error;
> +		return process_node_da_root(mp, ino, dip, blkmap, repair, bp);
>  	default:
>  		do_warn(
>  	_("bad attribute leaf magic # %#x for dir ino %" PRIu64 "\n"),
> -			be16_to_cpu(leaf->hdr.info.magic), ino);
> +			be16_to_cpu(info->magic), ino);
>  		libxfs_putbuf(bp);
>  		*repair = 0;
> -		return(1);
> +		return 1;
>  	}
>  
>  	if (*repair && !no_modify)
>  		libxfs_writebuf(bp, 0);
>  	else
>  		libxfs_putbuf(bp);
> -
> -	return(0);  /* repair may be set */
> +	return 0;  /* repair may be set */
>  }
>  
> -
>  static int
>  xfs_acl_from_disk(
>  	struct xfs_mount	*mp,

Re: [PATCH v2 11/8] xfs_repair: don't corrupt a attr fork da3 node when clearing forw/back

[Christoph Hellwig]

On Tue, Feb 04, 2020 at 03:14:42PM -0800, Darrick J. Wong wrote:
> On Thu, Jan 30, 2020 at 10:03:15PM -0800, Christoph Hellwig wrote:
> > Looks sensible, but I think we want the helpers for both the node and
> > leaf case, something like this untested patch:
> 
> Ok, I was about to repost with more or less the same code.

Just send your version as the official patch, especially if you actually
tested it..