[PATCH v3 00/10] xfsprogs-5.0: fix various problems

[PATCH v3 00/10] xfsprogs-5.0: fix various problems

[Darrick J. Wong]

Hi all,

Here are some fixes for xfsprogs 5.0:

Patch 1 fixes the problem of xfs_scrub's support scripts not getting
rebuilt when file path definitions change.

Patch 2 fixes a problem xfs_info had in finding mounts.

Patch 3 fixes a crash in xfs_repair where accidentally create duplicate
rmapbt records for blocks that are initially allocated to the free space
btrees but then are freed back to the AGFL. while rebuilding the rmap
btree.

Patches 4-5 fix a problem where xfs_repair phase 6 will fail to flush
partially corrected inode core data to the inode cluster buffer because
phase 6 hasn't yet fixed every thing that's wrong with the inode.  This
patch is part of a continuing series of fixes for Arkadiusz Miśkiewicz's
broken filesystem.

Patch 6 fixes some static checker complaints about strncpy usage.

Patches 7-10 refactor the userspace buffer and inode log item handling
so that the item lifetimes make more sense -- they don't need to be kept
around after a transaction commits (or cancels), and we certainly don't
need to be leaking them because we got lazy about cleaning them up.

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.0-fixes

[PATCH 01/10] scrub: fix Makefile targets which depend on builddefs

[Darrick J. Wong]

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

Add Makefile dependencies for targets that require variables set in
builddefs.  Although most of the required variables are file paths
defined during the ./configure process, we cannot simply use
AC_CONFIG_FILES to generate the scripts because that macro only expands
one level deep and its documentation says that it's only to be used for
generating makefiles, not build targets themselves.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 scrub/Makefile |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)


diff --git a/scrub/Makefile b/scrub/Makefile
index f4710d3e..882da8fd 100644
--- a/scrub/Makefile
+++ b/scrub/Makefile
@@ -3,7 +3,8 @@
 #
 
 TOPDIR = ..
-include $(TOPDIR)/include/builddefs
+builddefs=$(TOPDIR)/include/builddefs
+include $(builddefs)
 
 # On linux we get fsmap from the system or define it ourselves
 # so include this based on platform type.  If this reverts to only
@@ -101,27 +102,27 @@ LDIRT = $(XFS_SCRUB_ALL_PROG) *.service *.cron
 
 default: depend $(LTCOMMAND) $(XFS_SCRUB_ALL_PROG) $(OPTIONAL_TARGETS)
 
-xfs_scrub_all: xfs_scrub_all.in
+xfs_scrub_all: xfs_scrub_all.in $(builddefs)
 	@echo "    [SED]    $@"
 	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" \
 		   -e "s|@pkg_version@|$(PKG_VERSION)|g" \
 		   -e "s|@scrub_args@|$(XFS_SCRUB_ARGS)|g" < $< > $@
 	$(Q)chmod a+x $@
 
-phase5.o unicrash.o xfs.o: $(TOPDIR)/include/builddefs
+phase5.o unicrash.o xfs.o: $(builddefs)
 
 include $(BUILDRULES)
 
 install: $(INSTALL_SCRUB)
 
-%.service: %.service.in
+%.service: %.service.in $(builddefs)
 	@echo "    [SED]    $@"
 	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" \
 		   -e "s|@scrub_args@|$(XFS_SCRUB_ARGS)|g" \
 		   -e "s|@pkg_lib_dir@|$(PKG_LIB_DIR)|g" \
 		   -e "s|@pkg_name@|$(PKG_NAME)|g" < $< > $@
 
-%.cron: %.cron.in
+%.cron: %.cron.in $(builddefs)
 	@echo "    [SED]    $@"
 	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" < $< > $@
 

Re: [PATCH 01/10] scrub: fix Makefile targets which depend on builddefs

[Eric Sandeen]

On 4/22/19 10:44 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add Makefile dependencies for targets that require variables set in
> builddefs.  Although most of the required variables are file paths
> defined during the ./configure process, we cannot simply use
> AC_CONFIG_FILES to generate the scripts because that macro only expands
> one level deep and its documentation says that it's only to be used for
> generating makefiles, not build targets themselves.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  scrub/Makefile |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/scrub/Makefile b/scrub/Makefile
> index f4710d3e..882da8fd 100644
> --- a/scrub/Makefile
> +++ b/scrub/Makefile
> @@ -3,7 +3,8 @@
>  #
>  
>  TOPDIR = ..
> -include $(TOPDIR)/include/builddefs
> +builddefs=$(TOPDIR)/include/builddefs
> +include $(builddefs)
>  
>  # On linux we get fsmap from the system or define it ourselves
>  # so include this based on platform type.  If this reverts to only
> @@ -101,27 +102,27 @@ LDIRT = $(XFS_SCRUB_ALL_PROG) *.service *.cron
>  
>  default: depend $(LTCOMMAND) $(XFS_SCRUB_ALL_PROG) $(OPTIONAL_TARGETS)
>  
> -xfs_scrub_all: xfs_scrub_all.in
> +xfs_scrub_all: xfs_scrub_all.in $(builddefs)
>  	@echo "    [SED]    $@"
>  	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" \
>  		   -e "s|@pkg_version@|$(PKG_VERSION)|g" \
>  		   -e "s|@scrub_args@|$(XFS_SCRUB_ARGS)|g" < $< > $@
>  	$(Q)chmod a+x $@
>  
> -phase5.o unicrash.o xfs.o: $(TOPDIR)/include/builddefs
> +phase5.o unicrash.o xfs.o: $(builddefs)
>  
>  include $(BUILDRULES)
>  
>  install: $(INSTALL_SCRUB)
>  
> -%.service: %.service.in
> +%.service: %.service.in $(builddefs)
>  	@echo "    [SED]    $@"
>  	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" \
>  		   -e "s|@scrub_args@|$(XFS_SCRUB_ARGS)|g" \
>  		   -e "s|@pkg_lib_dir@|$(PKG_LIB_DIR)|g" \
>  		   -e "s|@pkg_name@|$(PKG_NAME)|g" < $< > $@
>  
> -%.cron: %.cron.in
> +%.cron: %.cron.in $(builddefs)
>  	@echo "    [SED]    $@"
>  	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" < $< > $@
>  
> 

Re: [PATCH 01/10] scrub: fix Makefile targets which depend on builddefs

[Bill O'Donnell]

On Mon, Apr 22, 2019 at 08:44:56AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Add Makefile dependencies for targets that require variables set in
> builddefs.  Although most of the required variables are file paths
> defined during the ./configure process, we cannot simply use
> AC_CONFIG_FILES to generate the scripts because that macro only expands
> one level deep and its documentation says that it's only to be used for
> generating makefiles, not build targets themselves.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

looks good.
Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
>  scrub/Makefile |   11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/scrub/Makefile b/scrub/Makefile
> index f4710d3e..882da8fd 100644
> --- a/scrub/Makefile
> +++ b/scrub/Makefile
> @@ -3,7 +3,8 @@
>  #
>  
>  TOPDIR = ..
> -include $(TOPDIR)/include/builddefs
> +builddefs=$(TOPDIR)/include/builddefs
> +include $(builddefs)
>  
>  # On linux we get fsmap from the system or define it ourselves
>  # so include this based on platform type.  If this reverts to only
> @@ -101,27 +102,27 @@ LDIRT = $(XFS_SCRUB_ALL_PROG) *.service *.cron
>  
>  default: depend $(LTCOMMAND) $(XFS_SCRUB_ALL_PROG) $(OPTIONAL_TARGETS)
>  
> -xfs_scrub_all: xfs_scrub_all.in
> +xfs_scrub_all: xfs_scrub_all.in $(builddefs)
>  	@echo "    [SED]    $@"
>  	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" \
>  		   -e "s|@pkg_version@|$(PKG_VERSION)|g" \
>  		   -e "s|@scrub_args@|$(XFS_SCRUB_ARGS)|g" < $< > $@
>  	$(Q)chmod a+x $@
>  
> -phase5.o unicrash.o xfs.o: $(TOPDIR)/include/builddefs
> +phase5.o unicrash.o xfs.o: $(builddefs)
>  
>  include $(BUILDRULES)
>  
>  install: $(INSTALL_SCRUB)
>  
> -%.service: %.service.in
> +%.service: %.service.in $(builddefs)
>  	@echo "    [SED]    $@"
>  	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" \
>  		   -e "s|@scrub_args@|$(XFS_SCRUB_ARGS)|g" \
>  		   -e "s|@pkg_lib_dir@|$(PKG_LIB_DIR)|g" \
>  		   -e "s|@pkg_name@|$(PKG_NAME)|g" < $< > $@
>  
> -%.cron: %.cron.in
> +%.cron: %.cron.in $(builddefs)
>  	@echo "    [SED]    $@"
>  	$(Q)$(SED) -e "s|@sbindir@|$(PKG_SBIN_DIR)|g" < $< > $@
>  
> 

[PATCH 02/10] xfs_info: use findmnt to handle mounted block devices

[Darrick J. Wong]

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

Use findmnt to determine if the passed-in argument is associated with a
mount point, and if so, use spaceman to query the mounted filesystem.
If the user passed in a file, try to find out if it's a loop mounted
live filesystem and if so query the live filesystem.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 debian/control       |    2 +-
 spaceman/xfs_info.sh |   22 +++++++++++++++++++---
 2 files changed, 20 insertions(+), 4 deletions(-)


diff --git a/debian/control b/debian/control
index f4f807b0..0b3205f5 100644
--- a/debian/control
+++ b/debian/control
@@ -8,7 +8,7 @@ Standards-Version: 4.0.0
 Homepage: https://xfs.wiki.kernel.org/
 
 Package: xfsprogs
-Depends: ${shlibs:Depends}, ${misc:Depends}, python3:any
+Depends: ${shlibs:Depends}, ${misc:Depends}, python3:any, util-linux
 Provides: fsck-backend
 Suggests: xfsdump, acl, attr, quota
 Breaks: xfsdump (<< 3.0.0)
diff --git a/spaceman/xfs_info.sh b/spaceman/xfs_info.sh
index ecf17f61..1bf6d2c3 100755
--- a/spaceman/xfs_info.sh
+++ b/spaceman/xfs_info.sh
@@ -7,6 +7,14 @@
 OPTS=""
 USAGE="Usage: xfs_info [-V] [-t mtab] [mountpoint|device|file]"
 
+# Try to find a loop device associated with a file.  We only want to return
+# one loopdev (multiple loop devices can attach to a single file) so we grab
+# the last line and return it if it's actually a block device.
+try_find_loop_dev_for_file() {
+	local x="$(losetup -O NAME -j "$1" 2> /dev/null | tail -n 1)"
+	test -b "$x" && echo "$x"
+}
+
 while getopts "t:V" c
 do
 	case $c in
@@ -24,11 +32,19 @@ set -- extra "$@"
 shift $OPTIND
 case $# in
 	1)
-		if [ -b "$1" ] || [ -f "$1" ]; then
-			xfs_db -p xfs_info -c "info" $OPTS "$1"
+		arg="$1"
+
+		# See if we can map the arg to a loop device
+		loopdev="$(try_find_loop_dev_for_file "${arg}")"
+		test -n "${loopdev}" && arg="${loopdev}"
+
+		# If we find a mountpoint for the device, do a live query;
+		# otherwise try reading the fs with xfs_db.
+		if mountpt="$(findmnt -f -n -o TARGET "${arg}" 2> /dev/null)"; then
+			xfs_spaceman -p xfs_info -c "info" $OPTS "${mountpt}"
 			status=$?
 		else
-			xfs_spaceman -p xfs_info -c "info" $OPTS "$1"
+			xfs_db -p xfs_info -c "info" $OPTS "${arg}"
 			status=$?
 		fi
 		;;

Re: [PATCH 02/10] xfs_info: use findmnt to handle mounted block devices

[Eric Sandeen]

On 4/22/19 10:45 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Use findmnt to determine if the passed-in argument is associated with a
> mount point, and if so, use spaceman to query the mounted filesystem.
> If the user passed in a file, try to find out if it's a loop mounted
> live filesystem and if so query the live filesystem.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

older util-linux doesn't have -O either, but that's getting really old
and I guess we'll burn that bridge if we come to it.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  debian/control       |    2 +-
>  spaceman/xfs_info.sh |   22 +++++++++++++++++++---
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/debian/control b/debian/control
> index f4f807b0..0b3205f5 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -8,7 +8,7 @@ Standards-Version: 4.0.0
>  Homepage: https://xfs.wiki.kernel.org/
>  
>  Package: xfsprogs
> -Depends: ${shlibs:Depends}, ${misc:Depends}, python3:any
> +Depends: ${shlibs:Depends}, ${misc:Depends}, python3:any, util-linux
>  Provides: fsck-backend
>  Suggests: xfsdump, acl, attr, quota
>  Breaks: xfsdump (<< 3.0.0)
> diff --git a/spaceman/xfs_info.sh b/spaceman/xfs_info.sh
> index ecf17f61..1bf6d2c3 100755
> --- a/spaceman/xfs_info.sh
> +++ b/spaceman/xfs_info.sh
> @@ -7,6 +7,14 @@
>  OPTS=""
>  USAGE="Usage: xfs_info [-V] [-t mtab] [mountpoint|device|file]"
>  
> +# Try to find a loop device associated with a file.  We only want to return
> +# one loopdev (multiple loop devices can attach to a single file) so we grab
> +# the last line and return it if it's actually a block device.
> +try_find_loop_dev_for_file() {
> +	local x="$(losetup -O NAME -j "$1" 2> /dev/null | tail -n 1)"
> +	test -b "$x" && echo "$x"
> +}
> +
>  while getopts "t:V" c
>  do
>  	case $c in
> @@ -24,11 +32,19 @@ set -- extra "$@"
>  shift $OPTIND
>  case $# in
>  	1)
> -		if [ -b "$1" ] || [ -f "$1" ]; then
> -			xfs_db -p xfs_info -c "info" $OPTS "$1"
> +		arg="$1"
> +
> +		# See if we can map the arg to a loop device
> +		loopdev="$(try_find_loop_dev_for_file "${arg}")"
> +		test -n "${loopdev}" && arg="${loopdev}"
> +
> +		# If we find a mountpoint for the device, do a live query;
> +		# otherwise try reading the fs with xfs_db.
> +		if mountpt="$(findmnt -f -n -o TARGET "${arg}" 2> /dev/null)"; then
> +			xfs_spaceman -p xfs_info -c "info" $OPTS "${mountpt}"
>  			status=$?
>  		else
> -			xfs_spaceman -p xfs_info -c "info" $OPTS "$1"
> +			xfs_db -p xfs_info -c "info" $OPTS "${arg}"
>  			status=$?
>  		fi
>  		;;
> 

Re: [PATCH 02/10] xfs_info: use findmnt to handle mounted block devices

[Bill O'Donnell]

On Mon, Apr 22, 2019 at 08:45:02AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Use findmnt to determine if the passed-in argument is associated with a
> mount point, and if so, use spaceman to query the mounted filesystem.
> If the user passed in a file, try to find out if it's a loop mounted
> live filesystem and if so query the live filesystem.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
>  debian/control       |    2 +-
>  spaceman/xfs_info.sh |   22 +++++++++++++++++++---
>  2 files changed, 20 insertions(+), 4 deletions(-)
> 
> 
> diff --git a/debian/control b/debian/control
> index f4f807b0..0b3205f5 100644
> --- a/debian/control
> +++ b/debian/control
> @@ -8,7 +8,7 @@ Standards-Version: 4.0.0
>  Homepage: https://xfs.wiki.kernel.org/
>  
>  Package: xfsprogs
> -Depends: ${shlibs:Depends}, ${misc:Depends}, python3:any
> +Depends: ${shlibs:Depends}, ${misc:Depends}, python3:any, util-linux
>  Provides: fsck-backend
>  Suggests: xfsdump, acl, attr, quota
>  Breaks: xfsdump (<< 3.0.0)
> diff --git a/spaceman/xfs_info.sh b/spaceman/xfs_info.sh
> index ecf17f61..1bf6d2c3 100755
> --- a/spaceman/xfs_info.sh
> +++ b/spaceman/xfs_info.sh
> @@ -7,6 +7,14 @@
>  OPTS=""
>  USAGE="Usage: xfs_info [-V] [-t mtab] [mountpoint|device|file]"
>  
> +# Try to find a loop device associated with a file.  We only want to return
> +# one loopdev (multiple loop devices can attach to a single file) so we grab
> +# the last line and return it if it's actually a block device.
> +try_find_loop_dev_for_file() {
> +	local x="$(losetup -O NAME -j "$1" 2> /dev/null | tail -n 1)"
> +	test -b "$x" && echo "$x"
> +}
> +
>  while getopts "t:V" c
>  do
>  	case $c in
> @@ -24,11 +32,19 @@ set -- extra "$@"
>  shift $OPTIND
>  case $# in
>  	1)
> -		if [ -b "$1" ] || [ -f "$1" ]; then
> -			xfs_db -p xfs_info -c "info" $OPTS "$1"
> +		arg="$1"
> +
> +		# See if we can map the arg to a loop device
> +		loopdev="$(try_find_loop_dev_for_file "${arg}")"
> +		test -n "${loopdev}" && arg="${loopdev}"
> +
> +		# If we find a mountpoint for the device, do a live query;
> +		# otherwise try reading the fs with xfs_db.
> +		if mountpt="$(findmnt -f -n -o TARGET "${arg}" 2> /dev/null)"; then
> +			xfs_spaceman -p xfs_info -c "info" $OPTS "${mountpt}"
>  			status=$?
>  		else
> -			xfs_spaceman -p xfs_info -c "info" $OPTS "$1"
> +			xfs_db -p xfs_info -c "info" $OPTS "${arg}"
>  			status=$?
>  		fi
>  		;;
> 

[PATCH 03/10] xfs_repair: correctly account for free space btree shrinks when fixing freelist

[Darrick J. Wong]

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

When we fix the freelist at the end of build_agf_agfl in phase 5 of
repair, we need to create incore rmap records for the blocks that get
added to the AGFL.  We can't let the regular freelist fixing code use
the regular on-disk rmapbt update code because the rmapbt isn't fully
set up yet.

Unfortunately, the original code fails to account for the fact that the
free space btrees can shrink when we allocate blocks to fix the
freelist; those blocks are also put on the freelist, but there are
already incore rmaps for all the free space btree blocks.  We must not
create (redundant) incore rmaps for those blocks.  If we do, repair
fails with a complaint that rebuilding the rmapbt failed during phase 5.
xfs/137 on a 1k block size occasionally triggers this bug.

To fix the problem, construct a bitmap of all OWN_AG blocks that we know
about before traversing the AGFL, and only create new incore rmaps for
those AGFL blocks that are not already tracked in the bitmap.

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


diff --git a/repair/rmap.c b/repair/rmap.c
index d0156f9d..19cceca3 100644
--- a/repair/rmap.c
+++ b/repair/rmap.c
@@ -12,6 +12,7 @@
 #include "dinode.h"
 #include "slab.h"
 #include "rmap.h"
+#include "bitmap.h"
 
 #undef RMAP_DEBUG
 
@@ -450,15 +451,16 @@ rmap_store_ag_btree_rec(
 	struct xfs_buf		*agflbp = NULL;
 	struct xfs_trans	*tp;
 	__be32			*agfl_bno, *b;
+	struct xfs_ag_rmap	*ag_rmap = &ag_rmaps[agno];
+	struct bitmap		*own_ag_bitmap = NULL;
 	int			error = 0;
 
 	if (!xfs_sb_version_hasrmapbt(&mp->m_sb))
 		return 0;
 
 	/* Release the ar_rmaps; they were put into the rmapbt during p5. */
-	free_slab(&ag_rmaps[agno].ar_rmaps);
-	error = init_slab(&ag_rmaps[agno].ar_rmaps,
-				  sizeof(struct xfs_rmap_irec));
+	free_slab(&ag_rmap->ar_rmaps);
+	error = init_slab(&ag_rmap->ar_rmaps, sizeof(struct xfs_rmap_irec));
 	if (error)
 		goto err;
 
@@ -478,19 +480,50 @@ rmap_store_ag_btree_rec(
 	 * rmap, we only need to add rmap records for AGFL blocks past
 	 * that point in the AGFL because those blocks are a result of a
 	 * no-rmap no-shrink freelist fixup that we did earlier.
+	 *
+	 * However, some blocks end up on the AGFL because the free space
+	 * btrees shed blocks as a result of allocating space to fix the
+	 * freelist.  We already created in-core rmap records for the free
+	 * space btree blocks, so we must be careful not to create those
+	 * records again.  Create a bitmap of already-recorded OWN_AG rmaps.
 	 */
+	error = init_slab_cursor(ag_rmap->ar_raw_rmaps, rmap_compare, &rm_cur);
+	if (error)
+		goto err;
+	if (!bitmap_init(&own_ag_bitmap)) {
+		error = -ENOMEM;
+		goto err_slab;
+	}
+	while ((rm_rec = pop_slab_cursor(rm_cur)) != NULL) {
+		if (rm_rec->rm_owner != XFS_RMAP_OWN_AG)
+			continue;
+		if (!bitmap_set(own_ag_bitmap, rm_rec->rm_startblock,
+					rm_rec->rm_blockcount)) {
+			error = EFSCORRUPTED;
+			goto err_slab;
+		}
+	}
+	free_slab_cursor(&rm_cur);
+
+	/* Create rmaps for any AGFL blocks that aren't already rmapped. */
 	agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
-	b = agfl_bno + ag_rmaps[agno].ar_flcount;
+	b = agfl_bno + ag_rmap->ar_flcount;
 	while (*b != cpu_to_be32(NULLAGBLOCK) &&
 	       b - agfl_bno < libxfs_agfl_size(mp)) {
-		error = rmap_add_ag_rec(mp, agno, be32_to_cpu(*b), 1,
-				XFS_RMAP_OWN_AG);
-		if (error)
-			goto err;
+		xfs_agblock_t	agbno;
+
+		agbno = be32_to_cpu(*b);
+		if (!bitmap_test(own_ag_bitmap, agbno, 1)) {
+			error = rmap_add_ag_rec(mp, agno, agbno, 1,
+					XFS_RMAP_OWN_AG);
+			if (error)
+				goto err;
+		}
 		b++;
 	}
 	libxfs_putbuf(agflbp);
 	agflbp = NULL;
+	bitmap_free(&own_ag_bitmap);
 
 	/* Merge all the raw rmaps into the main list */
 	error = rmap_fold_raw_recs(mp, agno);
@@ -498,8 +531,7 @@ rmap_store_ag_btree_rec(
 		goto err;
 
 	/* Create cursors to refcount structures */
-	error = init_slab_cursor(ag_rmaps[agno].ar_rmaps, rmap_compare,
-			&rm_cur);
+	error = init_slab_cursor(ag_rmap->ar_rmaps, rmap_compare, &rm_cur);
 	if (error)
 		goto err;
 
@@ -542,6 +574,8 @@ rmap_store_ag_btree_rec(
 err:
 	if (agflbp)
 		libxfs_putbuf(agflbp);
+	if (own_ag_bitmap)
+		bitmap_free(&own_ag_bitmap);
 	return error;
 }
 

Re: [PATCH 03/10] xfs_repair: correctly account for free space btree shrinks when fixing freelist

[Eric Sandeen]

On 4/22/19 10:45 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we fix the freelist at the end of build_agf_agfl in phase 5 of
> repair, we need to create incore rmap records for the blocks that get
> added to the AGFL.  We can't let the regular freelist fixing code use
> the regular on-disk rmapbt update code because the rmapbt isn't fully
> set up yet.
> 
> Unfortunately, the original code fails to account for the fact that the
> free space btrees can shrink when we allocate blocks to fix the
> freelist; those blocks are also put on the freelist, but there are
> already incore rmaps for all the free space btree blocks.  We must not
> create (redundant) incore rmaps for those blocks.  If we do, repair
> fails with a complaint that rebuilding the rmapbt failed during phase 5.
> xfs/137 on a 1k block size occasionally triggers this bug.
> 
> To fix the problem, construct a bitmap of all OWN_AG blocks that we know
> about before traversing the AGFL, and only create new incore rmaps for
> those AGFL blocks that are not already tracked in the bitmap.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  repair/rmap.c |   54 ++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 44 insertions(+), 10 deletions(-)
> 
> 
> diff --git a/repair/rmap.c b/repair/rmap.c
> index d0156f9d..19cceca3 100644
> --- a/repair/rmap.c
> +++ b/repair/rmap.c
> @@ -12,6 +12,7 @@
>  #include "dinode.h"
>  #include "slab.h"
>  #include "rmap.h"
> +#include "bitmap.h"
>  
>  #undef RMAP_DEBUG
>  
> @@ -450,15 +451,16 @@ rmap_store_ag_btree_rec(
>  	struct xfs_buf		*agflbp = NULL;
>  	struct xfs_trans	*tp;
>  	__be32			*agfl_bno, *b;
> +	struct xfs_ag_rmap	*ag_rmap = &ag_rmaps[agno];
> +	struct bitmap		*own_ag_bitmap = NULL;
>  	int			error = 0;
>  
>  	if (!xfs_sb_version_hasrmapbt(&mp->m_sb))
>  		return 0;
>  
>  	/* Release the ar_rmaps; they were put into the rmapbt during p5. */
> -	free_slab(&ag_rmaps[agno].ar_rmaps);
> -	error = init_slab(&ag_rmaps[agno].ar_rmaps,
> -				  sizeof(struct xfs_rmap_irec));
> +	free_slab(&ag_rmap->ar_rmaps);
> +	error = init_slab(&ag_rmap->ar_rmaps, sizeof(struct xfs_rmap_irec));
>  	if (error)
>  		goto err;
>  
> @@ -478,19 +480,50 @@ rmap_store_ag_btree_rec(
>  	 * rmap, we only need to add rmap records for AGFL blocks past
>  	 * that point in the AGFL because those blocks are a result of a
>  	 * no-rmap no-shrink freelist fixup that we did earlier.
> +	 *
> +	 * However, some blocks end up on the AGFL because the free space
> +	 * btrees shed blocks as a result of allocating space to fix the
> +	 * freelist.  We already created in-core rmap records for the free
> +	 * space btree blocks, so we must be careful not to create those
> +	 * records again.  Create a bitmap of already-recorded OWN_AG rmaps.
>  	 */
> +	error = init_slab_cursor(ag_rmap->ar_raw_rmaps, rmap_compare, &rm_cur);
> +	if (error)
> +		goto err;
> +	if (!bitmap_init(&own_ag_bitmap)) {
> +		error = -ENOMEM;
> +		goto err_slab;
> +	}
> +	while ((rm_rec = pop_slab_cursor(rm_cur)) != NULL) {
> +		if (rm_rec->rm_owner != XFS_RMAP_OWN_AG)
> +			continue;
> +		if (!bitmap_set(own_ag_bitmap, rm_rec->rm_startblock,
> +					rm_rec->rm_blockcount)) {
> +			error = EFSCORRUPTED;
> +			goto err_slab;
> +		}
> +	}
> +	free_slab_cursor(&rm_cur);
> +
> +	/* Create rmaps for any AGFL blocks that aren't already rmapped. */
>  	agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
> -	b = agfl_bno + ag_rmaps[agno].ar_flcount;
> +	b = agfl_bno + ag_rmap->ar_flcount;
>  	while (*b != cpu_to_be32(NULLAGBLOCK) &&
>  	       b - agfl_bno < libxfs_agfl_size(mp)) {
> -		error = rmap_add_ag_rec(mp, agno, be32_to_cpu(*b), 1,
> -				XFS_RMAP_OWN_AG);
> -		if (error)
> -			goto err;
> +		xfs_agblock_t	agbno;
> +
> +		agbno = be32_to_cpu(*b);
> +		if (!bitmap_test(own_ag_bitmap, agbno, 1)) {
> +			error = rmap_add_ag_rec(mp, agno, agbno, 1,
> +					XFS_RMAP_OWN_AG);
> +			if (error)
> +				goto err;
> +		}
>  		b++;
>  	}
>  	libxfs_putbuf(agflbp);
>  	agflbp = NULL;
> +	bitmap_free(&own_ag_bitmap);
>  
>  	/* Merge all the raw rmaps into the main list */
>  	error = rmap_fold_raw_recs(mp, agno);
> @@ -498,8 +531,7 @@ rmap_store_ag_btree_rec(
>  		goto err;
>  
>  	/* Create cursors to refcount structures */
> -	error = init_slab_cursor(ag_rmaps[agno].ar_rmaps, rmap_compare,
> -			&rm_cur);
> +	error = init_slab_cursor(ag_rmap->ar_rmaps, rmap_compare, &rm_cur);
>  	if (error)
>  		goto err;
>  
> @@ -542,6 +574,8 @@ rmap_store_ag_btree_rec(
>  err:
>  	if (agflbp)
>  		libxfs_putbuf(agflbp);
> +	if (own_ag_bitmap)
> +		bitmap_free(&own_ag_bitmap);
>  	return error;
>  }
>  
> 

Re: [PATCH 03/10] xfs_repair: correctly account for free space btree shrinks when fixing freelist

[Bill O'Donnell]

On Mon, Apr 22, 2019 at 08:45:09AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we fix the freelist at the end of build_agf_agfl in phase 5 of
> repair, we need to create incore rmap records for the blocks that get
> added to the AGFL.  We can't let the regular freelist fixing code use
> the regular on-disk rmapbt update code because the rmapbt isn't fully
> set up yet.
> 
> Unfortunately, the original code fails to account for the fact that the
> free space btrees can shrink when we allocate blocks to fix the
> freelist; those blocks are also put on the freelist, but there are
> already incore rmaps for all the free space btree blocks.  We must not
> create (redundant) incore rmaps for those blocks.  If we do, repair
> fails with a complaint that rebuilding the rmapbt failed during phase 5.
> xfs/137 on a 1k block size occasionally triggers this bug.
> 
> To fix the problem, construct a bitmap of all OWN_AG blocks that we know
> about before traversing the AGFL, and only create new incore rmaps for
> those AGFL blocks that are not already tracked in the bitmap.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

looks good to me.
Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
>  repair/rmap.c |   54 ++++++++++++++++++++++++++++++++++++++++++++----------
>  1 file changed, 44 insertions(+), 10 deletions(-)
> 
> 
> diff --git a/repair/rmap.c b/repair/rmap.c
> index d0156f9d..19cceca3 100644
> --- a/repair/rmap.c
> +++ b/repair/rmap.c
> @@ -12,6 +12,7 @@
>  #include "dinode.h"
>  #include "slab.h"
>  #include "rmap.h"
> +#include "bitmap.h"
>  
>  #undef RMAP_DEBUG
>  
> @@ -450,15 +451,16 @@ rmap_store_ag_btree_rec(
>  	struct xfs_buf		*agflbp = NULL;
>  	struct xfs_trans	*tp;
>  	__be32			*agfl_bno, *b;
> +	struct xfs_ag_rmap	*ag_rmap = &ag_rmaps[agno];
> +	struct bitmap		*own_ag_bitmap = NULL;
>  	int			error = 0;
>  
>  	if (!xfs_sb_version_hasrmapbt(&mp->m_sb))
>  		return 0;
>  
>  	/* Release the ar_rmaps; they were put into the rmapbt during p5. */
> -	free_slab(&ag_rmaps[agno].ar_rmaps);
> -	error = init_slab(&ag_rmaps[agno].ar_rmaps,
> -				  sizeof(struct xfs_rmap_irec));
> +	free_slab(&ag_rmap->ar_rmaps);
> +	error = init_slab(&ag_rmap->ar_rmaps, sizeof(struct xfs_rmap_irec));
>  	if (error)
>  		goto err;
>  
> @@ -478,19 +480,50 @@ rmap_store_ag_btree_rec(
>  	 * rmap, we only need to add rmap records for AGFL blocks past
>  	 * that point in the AGFL because those blocks are a result of a
>  	 * no-rmap no-shrink freelist fixup that we did earlier.
> +	 *
> +	 * However, some blocks end up on the AGFL because the free space
> +	 * btrees shed blocks as a result of allocating space to fix the
> +	 * freelist.  We already created in-core rmap records for the free
> +	 * space btree blocks, so we must be careful not to create those
> +	 * records again.  Create a bitmap of already-recorded OWN_AG rmaps.
>  	 */
> +	error = init_slab_cursor(ag_rmap->ar_raw_rmaps, rmap_compare, &rm_cur);
> +	if (error)
> +		goto err;
> +	if (!bitmap_init(&own_ag_bitmap)) {
> +		error = -ENOMEM;
> +		goto err_slab;
> +	}
> +	while ((rm_rec = pop_slab_cursor(rm_cur)) != NULL) {
> +		if (rm_rec->rm_owner != XFS_RMAP_OWN_AG)
> +			continue;
> +		if (!bitmap_set(own_ag_bitmap, rm_rec->rm_startblock,
> +					rm_rec->rm_blockcount)) {
> +			error = EFSCORRUPTED;
> +			goto err_slab;
> +		}
> +	}
> +	free_slab_cursor(&rm_cur);
> +
> +	/* Create rmaps for any AGFL blocks that aren't already rmapped. */
>  	agfl_bno = XFS_BUF_TO_AGFL_BNO(mp, agflbp);
> -	b = agfl_bno + ag_rmaps[agno].ar_flcount;
> +	b = agfl_bno + ag_rmap->ar_flcount;
>  	while (*b != cpu_to_be32(NULLAGBLOCK) &&
>  	       b - agfl_bno < libxfs_agfl_size(mp)) {
> -		error = rmap_add_ag_rec(mp, agno, be32_to_cpu(*b), 1,
> -				XFS_RMAP_OWN_AG);
> -		if (error)
> -			goto err;
> +		xfs_agblock_t	agbno;
> +
> +		agbno = be32_to_cpu(*b);
> +		if (!bitmap_test(own_ag_bitmap, agbno, 1)) {
> +			error = rmap_add_ag_rec(mp, agno, agbno, 1,
> +					XFS_RMAP_OWN_AG);
> +			if (error)
> +				goto err;
> +		}
>  		b++;
>  	}
>  	libxfs_putbuf(agflbp);
>  	agflbp = NULL;
> +	bitmap_free(&own_ag_bitmap);
>  
>  	/* Merge all the raw rmaps into the main list */
>  	error = rmap_fold_raw_recs(mp, agno);
> @@ -498,8 +531,7 @@ rmap_store_ag_btree_rec(
>  		goto err;
>  
>  	/* Create cursors to refcount structures */
> -	error = init_slab_cursor(ag_rmaps[agno].ar_rmaps, rmap_compare,
> -			&rm_cur);
> +	error = init_slab_cursor(ag_rmap->ar_rmaps, rmap_compare, &rm_cur);
>  	if (error)
>  		goto err;
>  
> @@ -542,6 +574,8 @@ rmap_store_ag_btree_rec(
>  err:
>  	if (agflbp)
>  		libxfs_putbuf(agflbp);
> +	if (own_ag_bitmap)
> +		bitmap_free(&own_ag_bitmap);
>  	return error;
>  }
>  
> 

[PATCH 04/10] libxfs: retain ifork_ops when flushing inode

[Darrick J. Wong]

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

Retain the ifork ops used to validate the inode so that we can use the
same one to iflush it.  xfs_repair phase 6 can use multiple transactions
to fix various inode problems, which means that the inode might not be
fully fixed when each transaction commits.

This can be a particular problem if there's a shortform directory with
both invalid directory entries and incorrect i8count.  Phase 3 will set
the parent inode to "0" to signal to phase 6 that it needs to reset the
parent and i8count, but phase 6 starts a transaction to junk the bad
entries which fail to commit because the parent is invalid:

fixing i8count in inode 69022994673
Invalid inode number 0x0
xfs_dir_ino_validate: XFS_ERROR_REPORT
Metadata corruption detected at 0x464eb0, inode 0x10121750f1 data fork
xfs_repair: warning - iflush_int failed (-117)

And thus the inode fixes never get written out.

Reported-by: Arkadiusz Miskiewicz <arekm@maven.pl>
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 include/xfs_inode.h |    1 +
 libxfs/rdwr.c       |    1 +
 libxfs/util.c       |    2 +-
 3 files changed, 3 insertions(+), 1 deletion(-)


diff --git a/include/xfs_inode.h b/include/xfs_inode.h
index 79ec3a2d..e1e8b430 100644
--- a/include/xfs_inode.h
+++ b/include/xfs_inode.h
@@ -51,6 +51,7 @@ typedef struct xfs_inode {
 
 	xfs_fsize_t		i_size;		/* in-memory size */
 	const struct xfs_dir_ops *d_ops;	/* directory ops vector */
+	struct xfs_ifork_ops	*i_fork_ops;	/* fork verifiers */
 	struct inode		i_vnode;
 } xfs_inode_t;
 
diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index a00360e7..69d5abb2 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -1391,6 +1391,7 @@ libxfs_iget(
 		return error;
 	}
 
+	ip->i_fork_ops = ifork_ops;
 	if (!libxfs_inode_verify_forks(ip, ifork_ops)) {
 		libxfs_irele(ip);
 		return -EFSCORRUPTED;
diff --git a/libxfs/util.c b/libxfs/util.c
index 4ac151e6..2e3b9d51 100644
--- a/libxfs/util.c
+++ b/libxfs/util.c
@@ -422,7 +422,7 @@ libxfs_iflush_int(xfs_inode_t *ip, xfs_buf_t *bp)
 		VFS_I(ip)->i_version++;
 
 	/* Check the inline fork data before we write out. */
-	if (!libxfs_inode_verify_forks(ip, &xfs_default_ifork_ops))
+	if (!libxfs_inode_verify_forks(ip, ip->i_fork_ops))
 		return -EFSCORRUPTED;
 
 	/*

Re: [PATCH 04/10] libxfs: retain ifork_ops when flushing inode

[Bill O'Donnell]

On Mon, Apr 22, 2019 at 08:45:15AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Retain the ifork ops used to validate the inode so that we can use the
> same one to iflush it.  xfs_repair phase 6 can use multiple transactions
> to fix various inode problems, which means that the inode might not be
> fully fixed when each transaction commits.
> 
> This can be a particular problem if there's a shortform directory with
> both invalid directory entries and incorrect i8count.  Phase 3 will set
> the parent inode to "0" to signal to phase 6 that it needs to reset the
> parent and i8count, but phase 6 starts a transaction to junk the bad
> entries which fail to commit because the parent is invalid:
> 
> fixing i8count in inode 69022994673
> Invalid inode number 0x0
> xfs_dir_ino_validate: XFS_ERROR_REPORT
> Metadata corruption detected at 0x464eb0, inode 0x10121750f1 data fork
> xfs_repair: warning - iflush_int failed (-117)
> 
> And thus the inode fixes never get written out.
> 
> Reported-by: Arkadiusz Miskiewicz <arekm@maven.pl>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
>  include/xfs_inode.h |    1 +
>  libxfs/rdwr.c       |    1 +
>  libxfs/util.c       |    2 +-
>  3 files changed, 3 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/include/xfs_inode.h b/include/xfs_inode.h
> index 79ec3a2d..e1e8b430 100644
> --- a/include/xfs_inode.h
> +++ b/include/xfs_inode.h
> @@ -51,6 +51,7 @@ typedef struct xfs_inode {
>  
>  	xfs_fsize_t		i_size;		/* in-memory size */
>  	const struct xfs_dir_ops *d_ops;	/* directory ops vector */
> +	struct xfs_ifork_ops	*i_fork_ops;	/* fork verifiers */
>  	struct inode		i_vnode;
>  } xfs_inode_t;
>  
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index a00360e7..69d5abb2 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -1391,6 +1391,7 @@ libxfs_iget(
>  		return error;
>  	}
>  
> +	ip->i_fork_ops = ifork_ops;
>  	if (!libxfs_inode_verify_forks(ip, ifork_ops)) {
>  		libxfs_irele(ip);
>  		return -EFSCORRUPTED;
> diff --git a/libxfs/util.c b/libxfs/util.c
> index 4ac151e6..2e3b9d51 100644
> --- a/libxfs/util.c
> +++ b/libxfs/util.c
> @@ -422,7 +422,7 @@ libxfs_iflush_int(xfs_inode_t *ip, xfs_buf_t *bp)
>  		VFS_I(ip)->i_version++;
>  
>  	/* Check the inline fork data before we write out. */
> -	if (!libxfs_inode_verify_forks(ip, &xfs_default_ifork_ops))
> +	if (!libxfs_inode_verify_forks(ip, ip->i_fork_ops))
>  		return -EFSCORRUPTED;
>  
>  	/*
> 

Re: [PATCH 04/10] libxfs: retain ifork_ops when flushing inode

[Eric Sandeen]

On 4/22/19 10:45 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Retain the ifork ops used to validate the inode so that we can use the
> same one to iflush it.  xfs_repair phase 6 can use multiple transactions
> to fix various inode problems, which means that the inode might not be
> fully fixed when each transaction commits.
> 
> This can be a particular problem if there's a shortform directory with
> both invalid directory entries and incorrect i8count.  Phase 3 will set
> the parent inode to "0" to signal to phase 6 that it needs to reset the
> parent and i8count, but phase 6 starts a transaction to junk the bad
> entries which fail to commit because the parent is invalid:
> 
> fixing i8count in inode 69022994673
> Invalid inode number 0x0
> xfs_dir_ino_validate: XFS_ERROR_REPORT
> Metadata corruption detected at 0x464eb0, inode 0x10121750f1 data fork
> xfs_repair: warning - iflush_int failed (-117)
> 
> And thus the inode fixes never get written out.
> 
> Reported-by: Arkadiusz Miskiewicz <arekm@maven.pl>
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

I might have combined this with the next patch, but I guess
I can see the rationale either way.  :)

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  include/xfs_inode.h |    1 +
>  libxfs/rdwr.c       |    1 +
>  libxfs/util.c       |    2 +-
>  3 files changed, 3 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/include/xfs_inode.h b/include/xfs_inode.h
> index 79ec3a2d..e1e8b430 100644
> --- a/include/xfs_inode.h
> +++ b/include/xfs_inode.h
> @@ -51,6 +51,7 @@ typedef struct xfs_inode {
>  
>  	xfs_fsize_t		i_size;		/* in-memory size */
>  	const struct xfs_dir_ops *d_ops;	/* directory ops vector */
> +	struct xfs_ifork_ops	*i_fork_ops;	/* fork verifiers */
>  	struct inode		i_vnode;
>  } xfs_inode_t;
>  
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index a00360e7..69d5abb2 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -1391,6 +1391,7 @@ libxfs_iget(
>  		return error;
>  	}
>  
> +	ip->i_fork_ops = ifork_ops;
>  	if (!libxfs_inode_verify_forks(ip, ifork_ops)) {
>  		libxfs_irele(ip);
>  		return -EFSCORRUPTED;
> diff --git a/libxfs/util.c b/libxfs/util.c
> index 4ac151e6..2e3b9d51 100644
> --- a/libxfs/util.c
> +++ b/libxfs/util.c
> @@ -422,7 +422,7 @@ libxfs_iflush_int(xfs_inode_t *ip, xfs_buf_t *bp)
>  		VFS_I(ip)->i_version++;
>  
>  	/* Check the inline fork data before we write out. */
> -	if (!libxfs_inode_verify_forks(ip, &xfs_default_ifork_ops))
> +	if (!libxfs_inode_verify_forks(ip, ip->i_fork_ops))
>  		return -EFSCORRUPTED;
>  
>  	/*
> 

Re: [PATCH 04/10] libxfs: retain ifork_ops when flushing inode

[Arkadiusz Miśkiewicz]

On 22/04/2019 17:45, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Retain the ifork ops used to validate the inode so that we can use the
> same one to iflush it.  xfs_repair phase 6 can use multiple transactions
> to fix various inode problems, which means that the inode might not be
> fully fixed when each transaction commits.
> 
> This can be a particular problem if there's a shortform directory with
> both invalid directory entries and incorrect i8count.  Phase 3 will set
> the parent inode to "0" to signal to phase 6 that it needs to reset the
> parent and i8count, but phase 6 starts a transaction to junk the bad
> entries which fail to commit because the parent is invalid:
> 
> fixing i8count in inode 69022994673
> Invalid inode number 0x0
> xfs_dir_ino_validate: XFS_ERROR_REPORT
> Metadata corruption detected at 0x464eb0, inode 0x10121750f1 data fork
> xfs_repair: warning - iflush_int failed (-117)
> 
> And thus the inode fixes never get written out.


I just hit something similar again. xfsprogs from current for-next
(b94a69ac being latest commit in it)

[...]
bogus .. inode number (0) in directory inode 36510278809, clearing inode
number
[...]
entry "langs" in dir ino 34365014856 doesn't have a .. entry, will set
it in ino 36510278809.
[...]
setting .. in sf dir inode 36510278809 to 34365014856
Metadata corruption detected at 0x460e1c, inode 0x8802ea499 data fork
xfs_repair: warning - iflush_int failed (-117)
[...]
Phase 7 - verify and correct link counts...
Invalid inode number 0x0
xfs_dir_ino_validate: XFS_ERROR_REPORT
Metadata corruption detected at 0x460da8, inode 0x8802ea499 data fork

fatal error -- couldn't map inode 36510278809, err = 117


Full log output below:

> # /tmp/qq/sbin/xfs_repair -vvv -o bhash=256000 /dev/sdd1
> Phase 1 - find and verify superblock...
>         - reporting progress in intervals of 15 minutes
> Phase 2 - using internal log
>         - zero log...
> zero_log: head block 3022391 tail block 3022391
>         - scan filesystem freespace and inode maps...
>         - 14:26:44: scanning filesystem freespace - 39 of 39 allocation groups done
>         - found root inode chunk
> libxfs_bcache: 0xdb0900
> Max supported entries = 2048000
> Max utilized entries = 40516
> Active entries = 40516
> Hash table size = 256000
> Hits = 0
> Misses = 40517
> Hit ratio =  0.00
> MRU 0 entries =  40516 (100%)
> MRU 1 entries =      0 (  0%)
> MRU 2 entries =      0 (  0%)
> MRU 3 entries =      0 (  0%)
> MRU 4 entries =      0 (  0%)
> MRU 5 entries =      0 (  0%)
> MRU 6 entries =      0 (  0%)
> MRU 7 entries =      0 (  0%)
> MRU 8 entries =      0 (  0%)
> MRU 9 entries =      0 (  0%)
> MRU 10 entries =      0 (  0%)
> MRU 11 entries =      0 (  0%)
> MRU 12 entries =      0 (  0%)
> MRU 13 entries =      0 (  0%)
> MRU 14 entries =      0 (  0%)
> MRU 15 entries =      0 (  0%)
> Dirty MRU 16 entries =      0 (  0%)
> Hash buckets with   0 entries 218473 (  0%)
> Hash buckets with   1 entries  34691 ( 85%)
> Hash buckets with   2 entries   2690 ( 13%)
> Hash buckets with   3 entries    139 (  1%)
> Hash buckets with   4 entries      7 (  0%)
> Phase 3 - for each AG...
>         - scan and clear agi unlinked lists...
>         - 14:26:44: scanning agi unlinked lists - 39 of 39 allocation groups done
>         - process known inodes and perform inode discovery...
>         - agno = 15
>         - agno = 0
>         - agno = 30
>         - agno = 16
>         - agno = 31
>         - agno = 1
>         - agno = 17
>         - agno = 32
> bogus .. inode number (0) in directory inode 36510278809, clearing inode number
> bogus .. inode number (0) in directory inode 36512379414, clearing inode number
>         - agno = 18
>         - agno = 19
>         - agno = 33
>         - agno = 20
>         - agno = 34
>         - agno = 21
>         - agno = 35
>         - agno = 2
>         - agno = 22
>         - agno = 36
>         - agno = 23
>         - agno = 37
>         - agno = 24
>         - agno = 3
>         - agno = 38
>         - agno = 25
>         - agno = 4
>         - agno = 26
>         - agno = 5
>         - agno = 27
>         - agno = 6
>         - agno = 28
>         - agno = 7
>         - agno = 29
>         - agno = 8
>         - agno = 9
>         - agno = 10
>         - agno = 11
>         - agno = 12
>         - agno = 13
>         - agno = 14
>         - 17:27:19: process known inodes and inode discovery - 173158784 of 173158784 inodes done
>         - process newly discovered inodes...
>         - 17:27:19: process newly discovered inodes - 39 of 39 allocation groups done
> libxfs_bcache: 0xdb0900
> Max supported entries = 2048000
> Max utilized entries = 2048000
> Active entries = 2047963
> Hash table size = 256000
> Hits = 33258462
> Misses = 39986202
> Hit ratio = 45.41
> MRU 0 entries =  21826 (  1%)
> MRU 1 entries =      0 (  0%)
> MRU 2 entries =  43908 (  2%)
> MRU 3 entries = 218917 ( 10%)
> MRU 4 entries =  17304 (  0%)
> MRU 5 entries =  45429 (  2%)
> MRU 6 entries = 1097133 ( 53%)
> MRU 7 entries =  68828 (  3%)
> MRU 8 entries =      0 (  0%)
> MRU 9 entries =      0 (  0%)
> MRU 10 entries =      0 (  0%)
> MRU 11 entries = 318994 ( 15%)
> MRU 12 entries = 104225 (  5%)
> MRU 13 entries = 110173 (  5%)
> MRU 14 entries =   1226 (  0%)
> MRU 15 entries =      0 (  0%)
> Dirty MRU 16 entries =      0 (  0%)
> Hash buckets with   0 entries     75 (  0%)
> Hash buckets with   1 entries    669 (  0%)
> Hash buckets with   2 entries   2673 (  0%)
> Hash buckets with   3 entries   7083 (  1%)
> Hash buckets with   4 entries  14564 (  2%)
> Hash buckets with   5 entries  23510 (  5%)
> Hash buckets with   6 entries  31292 (  9%)
> Hash buckets with   7 entries  35847 ( 12%)
> Hash buckets with   8 entries  35808 ( 13%)
> Hash buckets with   9 entries  32245 ( 14%)
> Hash buckets with  10 entries  25460 ( 12%)
> Hash buckets with  11 entries  18399 (  9%)
> Hash buckets with  12 entries  12437 (  7%)
> Hash buckets with  13 entries   7502 (  4%)
> Hash buckets with  14 entries   4257 (  2%)
> Hash buckets with  15 entries   2172 (  1%)
> Hash buckets with  16 entries   1114 (  0%)
> Hash buckets with  17 entries    542 (  0%)
> Hash buckets with  18 entries    200 (  0%)
> Hash buckets with  19 entries     88 (  0%)
> Hash buckets with  20 entries     39 (  0%)
> Hash buckets with  21 entries     14 (  0%)
> Hash buckets with  22 entries      8 (  0%)
> Hash buckets with  23 entries      2 (  0%)
> Phase 4 - check for duplicate blocks...
>         - setting up duplicate extent list...
>         - 17:27:21: setting up duplicate extent list - 39 of 39 allocation groups done
>         - check for inodes claiming duplicate blocks...
>         - agno = 30
>         - agno = 15
>         - agno = 0
>         - agno = 16
>         - agno = 1
>         - agno = 31
>         - agno = 17
> bogus .. inode number (0) in directory inode 36510278809, clearing inode number
> bogus .. inode number (0) in directory inode 36512379414, clearing inode number
>         - agno = 18
>         - agno = 32
>         - agno = 19
>         - agno = 20
>         - agno = 33
>         - agno = 21
>         - agno = 34
>         - agno = 22
>         - agno = 35
>         - agno = 23
>         - agno = 36
>         - agno = 2
>         - agno = 37
>         - agno = 24
>         - agno = 25
>         - agno = 38
>         - agno = 26
>         - agno = 27
>         - agno = 28
>         - agno = 3
>         - agno = 29
>         - agno = 4
>         - agno = 5
>         - agno = 6
>         - agno = 7
>         - agno = 8
>         - agno = 9
>         - agno = 10
>         - agno = 11
>         - agno = 12
>         - agno = 13
>         - agno = 14
>         - 20:44:23: check for inodes claiming duplicate blocks - 173158784 of 173158784 inodes done
> libxfs_bcache: 0xdb0900
> Max supported entries = 2048000
> Max utilized entries = 2048000
> Active entries = 2047999
> Hash table size = 256000
> Hits = 64756246
> Misses = 82886582
> Hit ratio = 43.86
> MRU 0 entries =  19324 (  0%)
> MRU 1 entries =      0 (  0%)
> MRU 2 entries =  47524 (  2%)
> MRU 3 entries = 217973 ( 10%)
> MRU 4 entries =  16829 (  0%)
> MRU 5 entries =  45729 (  2%)
> MRU 6 entries = 1071173 ( 52%)
> MRU 7 entries =      0 (  0%)
> MRU 8 entries =      0 (  0%)
> MRU 9 entries =      0 (  0%)
> MRU 10 entries =      0 (  0%)
> MRU 11 entries = 400426 ( 19%)
> MRU 12 entries =  97317 (  4%)
> MRU 13 entries = 130545 (  6%)
> MRU 14 entries =   1159 (  0%)
> MRU 15 entries =      0 (  0%)
> Dirty MRU 16 entries =      0 (  0%)
> Hash buckets with   0 entries     73 (  0%)
> Hash buckets with   1 entries    664 (  0%)
> Hash buckets with   2 entries   2642 (  0%)
> Hash buckets with   3 entries   7175 (  1%)
> Hash buckets with   4 entries  14586 (  2%)
> Hash buckets with   5 entries  23505 (  5%)
> Hash buckets with   6 entries  31069 (  9%)
> Hash buckets with   7 entries  35990 ( 12%)
> Hash buckets with   8 entries  36151 ( 14%)
> Hash buckets with   9 entries  31999 ( 14%)
> Hash buckets with  10 entries  25381 ( 12%)
> Hash buckets with  11 entries  18361 (  9%)
> Hash buckets with  12 entries  12288 (  7%)
> Hash buckets with  13 entries   7494 (  4%)
> Hash buckets with  14 entries   4346 (  2%)
> Hash buckets with  15 entries   2285 (  1%)
> Hash buckets with  16 entries   1126 (  0%)
> Hash buckets with  17 entries    502 (  0%)
> Hash buckets with  18 entries    211 (  0%)
> Hash buckets with  19 entries     95 (  0%)
> Hash buckets with  20 entries     28 (  0%)
> Hash buckets with  21 entries     16 (  0%)
> Hash buckets with  22 entries     12 (  0%)
> Hash buckets with  23 entries      1 (  0%)
> Phase 5 - rebuild AG headers and trees...
>         - agno = 0
>         - agno = 1
>         - agno = 2
>         - agno = 3
>         - agno = 4
>         - agno = 5
>         - agno = 6
>         - agno = 7
>         - agno = 8
>         - agno = 9
>         - agno = 10
>         - agno = 11
>         - agno = 12
>         - agno = 13
>         - agno = 14
>         - agno = 15
>         - agno = 16
>         - agno = 17
>         - agno = 18
>         - agno = 19
>         - agno = 20
>         - agno = 21
>         - agno = 22
>         - agno = 23
>         - agno = 24
>         - agno = 25
>         - agno = 26
>         - agno = 27
>         - agno = 28
>         - agno = 29
>         - agno = 30
>         - agno = 31
>         - agno = 32
>         - agno = 33
>         - agno = 34
>         - agno = 35
>         - agno = 36
>         - agno = 37
>         - agno = 38
>         - 20:44:35: rebuild AG headers and trees - 39 of 39 allocation groups done
>         - reset superblock...
> libxfs_bcache: 0xdb0900
> Max supported entries = 2048000
> Max utilized entries = 2048000
> Active entries = 2047979
> Hash table size = 256000
> Hits = 64756472
> Misses = 82918282
> Hit ratio = 43.85
> MRU 0 entries =  19304 (  0%)
> MRU 1 entries =      0 (  0%)
> MRU 2 entries =  47524 (  2%)
> MRU 3 entries = 217973 ( 10%)
> MRU 4 entries =  16829 (  0%)
> MRU 5 entries =  45729 (  2%)
> MRU 6 entries = 1071173 ( 52%)
> MRU 7 entries =      0 (  0%)
> MRU 8 entries =      0 (  0%)
> MRU 9 entries =      0 (  0%)
> MRU 10 entries =      0 (  0%)
> MRU 11 entries = 400426 ( 19%)
> MRU 12 entries =  97317 (  4%)
> MRU 13 entries = 130545 (  6%)
> MRU 14 entries =   1159 (  0%)
> MRU 15 entries =      0 (  0%)
> Dirty MRU 16 entries =      0 (  0%)
> Hash buckets with   0 entries     73 (  0%)
> Hash buckets with   1 entries    678 (  0%)
> Hash buckets with   2 entries   2631 (  0%)
> Hash buckets with   3 entries   7189 (  1%)
> Hash buckets with   4 entries  14612 (  2%)
> Hash buckets with   5 entries  23535 (  5%)
> Hash buckets with   6 entries  31083 (  9%)
> Hash buckets with   7 entries  35948 ( 12%)
> Hash buckets with   8 entries  35995 ( 14%)
> Hash buckets with   9 entries  32122 ( 14%)
> Hash buckets with  10 entries  25412 ( 12%)
> Hash buckets with  11 entries  18278 (  9%)
> Hash buckets with  12 entries  12245 (  7%)
> Hash buckets with  13 entries   7570 (  4%)
> Hash buckets with  14 entries   4320 (  2%)
> Hash buckets with  15 entries   2299 (  1%)
> Hash buckets with  16 entries   1131 (  0%)
> Hash buckets with  17 entries    497 (  0%)
> Hash buckets with  18 entries    231 (  0%)
> Hash buckets with  19 entries     95 (  0%)
> Hash buckets with  20 entries     31 (  0%)
> Hash buckets with  21 entries     15 (  0%)
> Hash buckets with  22 entries      9 (  0%)
> Hash buckets with  23 entries      1 (  0%)
> Phase 6 - check inode connectivity...
>         - resetting contents of realtime bitmap and summary inodes
>         - traversing filesystem ...
>         - agno = 0
>         - agno = 1
>         - agno = 2
>         - agno = 3
>         - agno = 4
>         - agno = 5
>         - agno = 6
>         - agno = 7
>         - agno = 8
>         - agno = 9
>         - agno = 10
>         - agno = 11
>         - agno = 12
> entry "servmask" in dir ino 25777842447 doesn't have a .. entry, will set it in ino 36512379414.
>         - agno = 13
>         - agno = 14
>         - agno = 15
>         - agno = 16
> entry "langs" in dir ino 34365014856 doesn't have a .. entry, will set it in ino 36510278809.
>         - agno = 17
>         - agno = 18
>         - agno = 19
>         - agno = 20
>         - agno = 21
>         - agno = 22
>         - agno = 23
>         - agno = 24
>         - agno = 25
>         - agno = 26
>         - agno = 27
>         - agno = 28
>         - agno = 29
>         - agno = 30
>         - agno = 31
>         - agno = 32
>         - agno = 33
>         - agno = 34
>         - agno = 35
>         - agno = 36
>         - agno = 37
>         - agno = 38
> setting .. in sf dir inode 36512379414 to 25777842447
> Metadata corruption detected at 0x460e1c, inode 0x8804eb216 data fork
> xfs_repair: warning - iflush_int failed (-117)
> setting .. in sf dir inode 36510278809 to 34365014856
> Metadata corruption detected at 0x460e1c, inode 0x8802ea499 data fork
> xfs_repair: warning - iflush_int failed (-117)
>         - traversal finished ...
>         - moving disconnected inodes to lost+found ...
> libxfs_bcache: 0xdb0900
> Max supported entries = 2048000
> Max utilized entries = 2048000
> Active entries = 2047971
> Hash table size = 256000
> Hits = 167418187
> Misses = 119244070
> Hit ratio = 58.40
> MRU 0 entries =  80752 (  3%)
> MRU 1 entries =      0 (  0%)
> MRU 2 entries =   1830 (  0%)
> MRU 3 entries = 500840 ( 24%)
> MRU 4 entries =  18369 (  0%)
> MRU 5 entries =      0 (  0%)
> MRU 6 entries = 1110024 ( 54%)
> MRU 7 entries = 133410 (  6%)
> MRU 8 entries =      0 (  0%)
> MRU 9 entries =      0 (  0%)
> MRU 10 entries = 202746 (  9%)
> MRU 11 entries =      0 (  0%)
> MRU 12 entries =      0 (  0%)
> MRU 13 entries =      0 (  0%)
> MRU 14 entries =      0 (  0%)
> MRU 15 entries =      0 (  0%)
> Dirty MRU 16 entries =      0 (  0%)
> Hash buckets with   0 entries     72 (  0%)
> Hash buckets with   1 entries    655 (  0%)
> Hash buckets with   2 entries   2512 (  0%)
> Hash buckets with   3 entries   7015 (  1%)
> Hash buckets with   4 entries  14124 (  2%)
> Hash buckets with   5 entries  23498 (  5%)
> Hash buckets with   6 entries  31301 (  9%)
> Hash buckets with   7 entries  36059 ( 12%)
> Hash buckets with   8 entries  36530 ( 14%)
> Hash buckets with   9 entries  32219 ( 14%)
> Hash buckets with  10 entries  25732 ( 12%)
> Hash buckets with  11 entries  18590 (  9%)
> Hash buckets with  12 entries  12175 (  7%)
> Hash buckets with  13 entries   7484 (  4%)
> Hash buckets with  14 entries   4068 (  2%)
> Hash buckets with  15 entries   2087 (  1%)
> Hash buckets with  16 entries   1073 (  0%)
> Hash buckets with  17 entries    487 (  0%)
> Hash buckets with  18 entries    194 (  0%)
> Hash buckets with  19 entries     81 (  0%)
> Hash buckets with  20 entries     34 (  0%)
> Hash buckets with  21 entries      7 (  0%)
> Hash buckets with  22 entries      2 (  0%)
> Hash buckets with  23 entries      1 (  0%)
> Phase 7 - verify and correct link counts...
> Invalid inode number 0x0
> xfs_dir_ino_validate: XFS_ERROR_REPORT
> Metadata corruption detected at 0x460da8, inode 0x8802ea499 data fork
> 
> fatal error -- couldn't map inode 36510278809, err = 117
> 

-- 
Arkadiusz Miśkiewicz, arekm / ( maven.pl | pld-linux.org )

[PATCH 05/10] libxfs: drop the ifork_ops parameter from _inode_verify_forks

[Darrick J. Wong]

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

Now that the inode remembers its own ifork_ops, we can drop the second
parameter from libxfs_inode_verify_forks.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 include/xfs_inode.h |    3 +--
 libxfs/rdwr.c       |   11 +++++------
 libxfs/util.c       |    2 +-
 3 files changed, 7 insertions(+), 9 deletions(-)


diff --git a/include/xfs_inode.h b/include/xfs_inode.h
index e1e8b430..52d79f3c 100644
--- a/include/xfs_inode.h
+++ b/include/xfs_inode.h
@@ -151,8 +151,7 @@ extern void	libxfs_trans_ichgtime(struct xfs_trans *,
 extern int	libxfs_iflush_int (struct xfs_inode *, struct xfs_buf *);
 
 /* Inode Cache Interfaces */
-extern bool	libxfs_inode_verify_forks(struct xfs_inode *ip,
-				struct xfs_ifork_ops *);
+extern bool	libxfs_inode_verify_forks(struct xfs_inode *ip);
 extern int	libxfs_iget(struct xfs_mount *, struct xfs_trans *, xfs_ino_t,
 				uint, struct xfs_inode **,
 				struct xfs_ifork_ops *);
diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index 69d5abb2..bc2ed38c 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -1338,16 +1338,15 @@ extern kmem_zone_t	*xfs_ili_zone;
  */
 bool
 libxfs_inode_verify_forks(
-	struct xfs_inode	*ip,
-	struct xfs_ifork_ops	*ops)
+	struct xfs_inode	*ip)
 {
 	struct xfs_ifork	*ifp;
 	xfs_failaddr_t		fa;
 
-	if (!ops)
+	if (!ip->i_fork_ops)
 		return true;
 
-	fa = xfs_ifork_verify_data(ip, ops);
+	fa = xfs_ifork_verify_data(ip, ip->i_fork_ops);
 	if (fa) {
 		ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
 		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
@@ -1355,7 +1354,7 @@ libxfs_inode_verify_forks(
 		return false;
 	}
 
-	fa = xfs_ifork_verify_attr(ip, ops);
+	fa = xfs_ifork_verify_attr(ip, ip->i_fork_ops);
 	if (fa) {
 		ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
 		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
@@ -1392,7 +1391,7 @@ libxfs_iget(
 	}
 
 	ip->i_fork_ops = ifork_ops;
-	if (!libxfs_inode_verify_forks(ip, ifork_ops)) {
+	if (!libxfs_inode_verify_forks(ip)) {
 		libxfs_irele(ip);
 		return -EFSCORRUPTED;
 	}
diff --git a/libxfs/util.c b/libxfs/util.c
index 2e3b9d51..ea75fa20 100644
--- a/libxfs/util.c
+++ b/libxfs/util.c
@@ -422,7 +422,7 @@ libxfs_iflush_int(xfs_inode_t *ip, xfs_buf_t *bp)
 		VFS_I(ip)->i_version++;
 
 	/* Check the inline fork data before we write out. */
-	if (!libxfs_inode_verify_forks(ip, ip->i_fork_ops))
+	if (!libxfs_inode_verify_forks(ip))
 		return -EFSCORRUPTED;
 
 	/*

Re: [PATCH 05/10] libxfs: drop the ifork_ops parameter from _inode_verify_forks

[Bill O'Donnell]

On Mon, Apr 22, 2019 at 08:45:21AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Now that the inode remembers its own ifork_ops, we can drop the second
> parameter from libxfs_inode_verify_forks.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
>  include/xfs_inode.h |    3 +--
>  libxfs/rdwr.c       |   11 +++++------
>  libxfs/util.c       |    2 +-
>  3 files changed, 7 insertions(+), 9 deletions(-)
> 
> 
> diff --git a/include/xfs_inode.h b/include/xfs_inode.h
> index e1e8b430..52d79f3c 100644
> --- a/include/xfs_inode.h
> +++ b/include/xfs_inode.h
> @@ -151,8 +151,7 @@ extern void	libxfs_trans_ichgtime(struct xfs_trans *,
>  extern int	libxfs_iflush_int (struct xfs_inode *, struct xfs_buf *);
>  
>  /* Inode Cache Interfaces */
> -extern bool	libxfs_inode_verify_forks(struct xfs_inode *ip,
> -				struct xfs_ifork_ops *);
> +extern bool	libxfs_inode_verify_forks(struct xfs_inode *ip);
>  extern int	libxfs_iget(struct xfs_mount *, struct xfs_trans *, xfs_ino_t,
>  				uint, struct xfs_inode **,
>  				struct xfs_ifork_ops *);
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index 69d5abb2..bc2ed38c 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -1338,16 +1338,15 @@ extern kmem_zone_t	*xfs_ili_zone;
>   */
>  bool
>  libxfs_inode_verify_forks(
> -	struct xfs_inode	*ip,
> -	struct xfs_ifork_ops	*ops)
> +	struct xfs_inode	*ip)
>  {
>  	struct xfs_ifork	*ifp;
>  	xfs_failaddr_t		fa;
>  
> -	if (!ops)
> +	if (!ip->i_fork_ops)
>  		return true;
>  
> -	fa = xfs_ifork_verify_data(ip, ops);
> +	fa = xfs_ifork_verify_data(ip, ip->i_fork_ops);
>  	if (fa) {
>  		ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
>  		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> @@ -1355,7 +1354,7 @@ libxfs_inode_verify_forks(
>  		return false;
>  	}
>  
> -	fa = xfs_ifork_verify_attr(ip, ops);
> +	fa = xfs_ifork_verify_attr(ip, ip->i_fork_ops);
>  	if (fa) {
>  		ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
>  		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> @@ -1392,7 +1391,7 @@ libxfs_iget(
>  	}
>  
>  	ip->i_fork_ops = ifork_ops;
> -	if (!libxfs_inode_verify_forks(ip, ifork_ops)) {
> +	if (!libxfs_inode_verify_forks(ip)) {
>  		libxfs_irele(ip);
>  		return -EFSCORRUPTED;
>  	}
> diff --git a/libxfs/util.c b/libxfs/util.c
> index 2e3b9d51..ea75fa20 100644
> --- a/libxfs/util.c
> +++ b/libxfs/util.c
> @@ -422,7 +422,7 @@ libxfs_iflush_int(xfs_inode_t *ip, xfs_buf_t *bp)
>  		VFS_I(ip)->i_version++;
>  
>  	/* Check the inline fork data before we write out. */
> -	if (!libxfs_inode_verify_forks(ip, ip->i_fork_ops))
> +	if (!libxfs_inode_verify_forks(ip))
>  		return -EFSCORRUPTED;
>  
>  	/*
> 

Re: [PATCH 05/10] libxfs: drop the ifork_ops parameter from _inode_verify_forks

[Eric Sandeen]

On 4/22/19 10:45 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Now that the inode remembers its own ifork_ops, we can drop the second
> parameter from libxfs_inode_verify_forks.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  include/xfs_inode.h |    3 +--
>  libxfs/rdwr.c       |   11 +++++------
>  libxfs/util.c       |    2 +-
>  3 files changed, 7 insertions(+), 9 deletions(-)
> 
> 
> diff --git a/include/xfs_inode.h b/include/xfs_inode.h
> index e1e8b430..52d79f3c 100644
> --- a/include/xfs_inode.h
> +++ b/include/xfs_inode.h
> @@ -151,8 +151,7 @@ extern void	libxfs_trans_ichgtime(struct xfs_trans *,
>  extern int	libxfs_iflush_int (struct xfs_inode *, struct xfs_buf *);
>  
>  /* Inode Cache Interfaces */
> -extern bool	libxfs_inode_verify_forks(struct xfs_inode *ip,
> -				struct xfs_ifork_ops *);
> +extern bool	libxfs_inode_verify_forks(struct xfs_inode *ip);
>  extern int	libxfs_iget(struct xfs_mount *, struct xfs_trans *, xfs_ino_t,
>  				uint, struct xfs_inode **,
>  				struct xfs_ifork_ops *);
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index 69d5abb2..bc2ed38c 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -1338,16 +1338,15 @@ extern kmem_zone_t	*xfs_ili_zone;
>   */
>  bool
>  libxfs_inode_verify_forks(
> -	struct xfs_inode	*ip,
> -	struct xfs_ifork_ops	*ops)
> +	struct xfs_inode	*ip)
>  {
>  	struct xfs_ifork	*ifp;
>  	xfs_failaddr_t		fa;
>  
> -	if (!ops)
> +	if (!ip->i_fork_ops)
>  		return true;
>  
> -	fa = xfs_ifork_verify_data(ip, ops);
> +	fa = xfs_ifork_verify_data(ip, ip->i_fork_ops);
>  	if (fa) {
>  		ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
>  		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "data fork",
> @@ -1355,7 +1354,7 @@ libxfs_inode_verify_forks(
>  		return false;
>  	}
>  
> -	fa = xfs_ifork_verify_attr(ip, ops);
> +	fa = xfs_ifork_verify_attr(ip, ip->i_fork_ops);
>  	if (fa) {
>  		ifp = XFS_IFORK_PTR(ip, XFS_ATTR_FORK);
>  		xfs_inode_verifier_error(ip, -EFSCORRUPTED, "attr fork",
> @@ -1392,7 +1391,7 @@ libxfs_iget(
>  	}
>  
>  	ip->i_fork_ops = ifork_ops;
> -	if (!libxfs_inode_verify_forks(ip, ifork_ops)) {
> +	if (!libxfs_inode_verify_forks(ip)) {
>  		libxfs_irele(ip);
>  		return -EFSCORRUPTED;
>  	}
> diff --git a/libxfs/util.c b/libxfs/util.c
> index 2e3b9d51..ea75fa20 100644
> --- a/libxfs/util.c
> +++ b/libxfs/util.c
> @@ -422,7 +422,7 @@ libxfs_iflush_int(xfs_inode_t *ip, xfs_buf_t *bp)
>  		VFS_I(ip)->i_version++;
>  
>  	/* Check the inline fork data before we write out. */
> -	if (!libxfs_inode_verify_forks(ip, ip->i_fork_ops))
> +	if (!libxfs_inode_verify_forks(ip))
>  		return -EFSCORRUPTED;
>  
>  	/*
> 

[PATCH 06/10] misc: fix strncpy length complaints

[Darrick J. Wong]

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

Fix a number of complaints about feeding sizeof(dest) directly to
strncpy.  We do this by declaring the char arrays to be one larger
than necessary and subtracting one, to ensure that we never overfill
the buffer.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 mkfs/xfs_mkfs.c |   13 +++++++++++--
 quota/edit.c    |    9 ++++++---
 2 files changed, 17 insertions(+), 5 deletions(-)


diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
index 3e2ef92d..db3ad38e 100644
--- a/mkfs/xfs_mkfs.c
+++ b/mkfs/xfs_mkfs.c
@@ -3270,8 +3270,17 @@ finish_superblock_setup(
 	struct xfs_mount	*mp,
 	struct xfs_sb		*sbp)
 {
-	if (cfg->label)
-		strncpy(sbp->sb_fname, cfg->label, sizeof(sbp->sb_fname));
+	if (cfg->label) {
+		size_t		label_len;
+
+		/*
+		 * Labels are null terminated unless the string fits exactly
+		 * in the label field, so assume sb_fname is zeroed and then
+		 * do a memcpy because the destination isn't a normal C string.
+		 */
+		label_len = min(sizeof(sbp->sb_fname), strlen(cfg->label));
+		memcpy(sbp->sb_fname, cfg->label, label_len);
+	}
 
 	sbp->sb_dblocks = cfg->dblocks;
 	sbp->sb_rblocks = cfg->rtblocks;
diff --git a/quota/edit.c b/quota/edit.c
index b10a5b34..f9938b8a 100644
--- a/quota/edit.c
+++ b/quota/edit.c
@@ -368,8 +368,7 @@ restore_file(
 	uint		type)
 {
 	char		buffer[512];
-	char		devbuffer[512];
-	char		*dev = NULL;
+	char		dev[512];
 	uint		mask;
 	int		cnt;
 	uint32_t	id;
@@ -377,7 +376,11 @@ restore_file(
 
 	while (fgets(buffer, sizeof(buffer), fp) != NULL) {
 		if (strncmp("fs = ", buffer, 5) == 0) {
-			dev = strncpy(devbuffer, buffer+5, sizeof(devbuffer));
+			/*
+			 * Copy the device name to dev, strip off the trailing
+			 * newline, and move on to the next line.
+			 */
+			strncpy(dev, buffer + 5, sizeof(dev) - 1);
 			dev[strlen(dev) - 1] = '\0';
 			continue;
 		}

Re: [PATCH 06/10] misc: fix strncpy length complaints

[Eric Sandeen]

On 4/22/19 10:45 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Fix a number of complaints about feeding sizeof(dest) directly to
> strncpy.  We do this by declaring the char arrays to be one larger
> than necessary and subtracting one, to ensure that we never overfill
> the buffer.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Hm, you don't actually do what the changelog says, do you?  No buffer
changes size in this patch.

And FWIW, neither of these was actually a real problem (fgets guarantees
a null in the string) but hey, whatever makes gcc happy I guess?

(I think we lose a char in the quota array, but 511 chars ought to be
enough for anybody right?)

Patch seems fine, though.

> ---
>  mkfs/xfs_mkfs.c |   13 +++++++++++--
>  quota/edit.c    |    9 ++++++---
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 3e2ef92d..db3ad38e 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3270,8 +3270,17 @@ finish_superblock_setup(
>  	struct xfs_mount	*mp,
>  	struct xfs_sb		*sbp)
>  {
> -	if (cfg->label)
> -		strncpy(sbp->sb_fname, cfg->label, sizeof(sbp->sb_fname));
> +	if (cfg->label) {
> +		size_t		label_len;
> +
> +		/*
> +		 * Labels are null terminated unless the string fits exactly
> +		 * in the label field, so assume sb_fname is zeroed and then
> +		 * do a memcpy because the destination isn't a normal C string.
> +		 */
> +		label_len = min(sizeof(sbp->sb_fname), strlen(cfg->label));
> +		memcpy(sbp->sb_fname, cfg->label, label_len);
> +	}
>  
>  	sbp->sb_dblocks = cfg->dblocks;
>  	sbp->sb_rblocks = cfg->rtblocks;
> diff --git a/quota/edit.c b/quota/edit.c
> index b10a5b34..f9938b8a 100644
> --- a/quota/edit.c
> +++ b/quota/edit.c
> @@ -368,8 +368,7 @@ restore_file(
>  	uint		type)
>  {
>  	char		buffer[512];
> -	char		devbuffer[512];
> -	char		*dev = NULL;
> +	char		dev[512];
>  	uint		mask;
>  	int		cnt;
>  	uint32_t	id;
> @@ -377,7 +376,11 @@ restore_file(
>  
>  	while (fgets(buffer, sizeof(buffer), fp) != NULL) {
>  		if (strncmp("fs = ", buffer, 5) == 0) {
> -			dev = strncpy(devbuffer, buffer+5, sizeof(devbuffer));
> +			/*
> +			 * Copy the device name to dev, strip off the trailing
> +			 * newline, and move on to the next line.
> +			 */
> +			strncpy(dev, buffer + 5, sizeof(dev) - 1);
>  			dev[strlen(dev) - 1] = '\0';
>  			continue;
>  		}
> 

Re: [PATCH 06/10] misc: fix strncpy length complaints

[Darrick J. Wong]

On Mon, Apr 22, 2019 at 03:48:03PM -0500, Eric Sandeen wrote:
> On 4/22/19 10:45 AM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Fix a number of complaints about feeding sizeof(dest) directly to
> > strncpy.  We do this by declaring the char arrays to be one larger
> > than necessary and subtracting one, to ensure that we never overfill
> > the buffer.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Hm, you don't actually do what the changelog says, do you?  No buffer
> changes size in this patch.

I'll change it to:

"Fix a number of complaints about feeding sizeof(dest) directly to
strncpy.  We do this by feeding strncpy the length of the buffer minus
one, having checked that the allocated space are long enough."

> And FWIW, neither of these was actually a real problem (fgets guarantees
> a null in the string) but hey, whatever makes gcc happy I guess?
> 
> (I think we lose a char in the quota array, but 511 chars ought to be
> enough for anybody right?)

Well... the quota one is reading lines straight out of a file, right?
So that probably ought to be char buf[PATH_MAX + length of other junk in
the line], though with the current lower limit now nobody's complained
so maybe it's fine....

--D

> Patch seems fine, though.
> 
> > ---
> >  mkfs/xfs_mkfs.c |   13 +++++++++++--
> >  quota/edit.c    |    9 ++++++---
> >  2 files changed, 17 insertions(+), 5 deletions(-)
> > 
> > 
> > diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> > index 3e2ef92d..db3ad38e 100644
> > --- a/mkfs/xfs_mkfs.c
> > +++ b/mkfs/xfs_mkfs.c
> > @@ -3270,8 +3270,17 @@ finish_superblock_setup(
> >  	struct xfs_mount	*mp,
> >  	struct xfs_sb		*sbp)
> >  {
> > -	if (cfg->label)
> > -		strncpy(sbp->sb_fname, cfg->label, sizeof(sbp->sb_fname));
> > +	if (cfg->label) {
> > +		size_t		label_len;
> > +
> > +		/*
> > +		 * Labels are null terminated unless the string fits exactly
> > +		 * in the label field, so assume sb_fname is zeroed and then
> > +		 * do a memcpy because the destination isn't a normal C string.
> > +		 */
> > +		label_len = min(sizeof(sbp->sb_fname), strlen(cfg->label));
> > +		memcpy(sbp->sb_fname, cfg->label, label_len);
> > +	}
> >  
> >  	sbp->sb_dblocks = cfg->dblocks;
> >  	sbp->sb_rblocks = cfg->rtblocks;
> > diff --git a/quota/edit.c b/quota/edit.c
> > index b10a5b34..f9938b8a 100644
> > --- a/quota/edit.c
> > +++ b/quota/edit.c
> > @@ -368,8 +368,7 @@ restore_file(
> >  	uint		type)
> >  {
> >  	char		buffer[512];
> > -	char		devbuffer[512];
> > -	char		*dev = NULL;
> > +	char		dev[512];
> >  	uint		mask;
> >  	int		cnt;
> >  	uint32_t	id;
> > @@ -377,7 +376,11 @@ restore_file(
> >  
> >  	while (fgets(buffer, sizeof(buffer), fp) != NULL) {
> >  		if (strncmp("fs = ", buffer, 5) == 0) {
> > -			dev = strncpy(devbuffer, buffer+5, sizeof(devbuffer));
> > +			/*
> > +			 * Copy the device name to dev, strip off the trailing
> > +			 * newline, and move on to the next line.
> > +			 */
> > +			strncpy(dev, buffer + 5, sizeof(dev) - 1);
> >  			dev[strlen(dev) - 1] = '\0';
> >  			continue;
> >  		}
> > 

Re: [PATCH 06/10] misc: fix strncpy length complaints

[Eric Sandeen]

On 4/22/19 3:57 PM, Darrick J. Wong wrote:
> On Mon, Apr 22, 2019 at 03:48:03PM -0500, Eric Sandeen wrote:
>> On 4/22/19 10:45 AM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>
>>> Fix a number of complaints about feeding sizeof(dest) directly to
>>> strncpy.  We do this by declaring the char arrays to be one larger
>>> than necessary and subtracting one, to ensure that we never overfill
>>> the buffer.
>>>
>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>
>> Hm, you don't actually do what the changelog says, do you?  No buffer
>> changes size in this patch.
> 
> I'll change it to:
> 
> "Fix a number of complaints about feeding sizeof(dest) directly to
> strncpy.  We do this by feeding strncpy the length of the buffer minus
> one, having checked that the allocated space are long enough."

sounds good, I'll just fix that here.

>> And FWIW, neither of these was actually a real problem (fgets guarantees
>> a null in the string) but hey, whatever makes gcc happy I guess?
>>
>> (I think we lose a char in the quota array, but 511 chars ought to be
>> enough for anybody right?)
> 
> Well... the quota one is reading lines straight out of a file, right?
> So that probably ought to be char buf[PATH_MAX + length of other junk in
> the line], though with the current lower limit now nobody's complained
> so maybe it's fine....
> 
> --D
> 
>> Patch seems fine, though.
>>
>>> ---
>>>  mkfs/xfs_mkfs.c |   13 +++++++++++--
>>>  quota/edit.c    |    9 ++++++---
>>>  2 files changed, 17 insertions(+), 5 deletions(-)
>>>
>>>
>>> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
>>> index 3e2ef92d..db3ad38e 100644
>>> --- a/mkfs/xfs_mkfs.c
>>> +++ b/mkfs/xfs_mkfs.c
>>> @@ -3270,8 +3270,17 @@ finish_superblock_setup(
>>>  	struct xfs_mount	*mp,
>>>  	struct xfs_sb		*sbp)
>>>  {
>>> -	if (cfg->label)
>>> -		strncpy(sbp->sb_fname, cfg->label, sizeof(sbp->sb_fname));
>>> +	if (cfg->label) {
>>> +		size_t		label_len;
>>> +
>>> +		/*
>>> +		 * Labels are null terminated unless the string fits exactly
>>> +		 * in the label field, so assume sb_fname is zeroed and then
>>> +		 * do a memcpy because the destination isn't a normal C string.
>>> +		 */
>>> +		label_len = min(sizeof(sbp->sb_fname), strlen(cfg->label));
>>> +		memcpy(sbp->sb_fname, cfg->label, label_len);
>>> +	}
>>>  
>>>  	sbp->sb_dblocks = cfg->dblocks;
>>>  	sbp->sb_rblocks = cfg->rtblocks;
>>> diff --git a/quota/edit.c b/quota/edit.c
>>> index b10a5b34..f9938b8a 100644
>>> --- a/quota/edit.c
>>> +++ b/quota/edit.c
>>> @@ -368,8 +368,7 @@ restore_file(
>>>  	uint		type)
>>>  {
>>>  	char		buffer[512];
>>> -	char		devbuffer[512];
>>> -	char		*dev = NULL;
>>> +	char		dev[512];
>>>  	uint		mask;
>>>  	int		cnt;
>>>  	uint32_t	id;
>>> @@ -377,7 +376,11 @@ restore_file(
>>>  
>>>  	while (fgets(buffer, sizeof(buffer), fp) != NULL) {
>>>  		if (strncmp("fs = ", buffer, 5) == 0) {
>>> -			dev = strncpy(devbuffer, buffer+5, sizeof(devbuffer));
>>> +			/*
>>> +			 * Copy the device name to dev, strip off the trailing
>>> +			 * newline, and move on to the next line.
>>> +			 */
>>> +			strncpy(dev, buffer + 5, sizeof(dev) - 1);
>>>  			dev[strlen(dev) - 1] = '\0';
>>>  			continue;
>>>  		}
>>>
> 

Re: [PATCH 06/10] misc: fix strncpy length complaints

[Eric Sandeen]

On 4/22/19 10:45 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Fix a number of complaints about feeding sizeof(dest) directly to
> strncpy.  We do this by declaring the char arrays to be one larger
> than necessary and subtracting one, to ensure that we never overfill
> the buffer.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

With a changelog edit,

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  mkfs/xfs_mkfs.c |   13 +++++++++++--
>  quota/edit.c    |    9 ++++++---
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 3e2ef92d..db3ad38e 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3270,8 +3270,17 @@ finish_superblock_setup(
>  	struct xfs_mount	*mp,
>  	struct xfs_sb		*sbp)
>  {
> -	if (cfg->label)
> -		strncpy(sbp->sb_fname, cfg->label, sizeof(sbp->sb_fname));
> +	if (cfg->label) {
> +		size_t		label_len;
> +
> +		/*
> +		 * Labels are null terminated unless the string fits exactly
> +		 * in the label field, so assume sb_fname is zeroed and then
> +		 * do a memcpy because the destination isn't a normal C string.
> +		 */
> +		label_len = min(sizeof(sbp->sb_fname), strlen(cfg->label));
> +		memcpy(sbp->sb_fname, cfg->label, label_len);
> +	}
>  
>  	sbp->sb_dblocks = cfg->dblocks;
>  	sbp->sb_rblocks = cfg->rtblocks;
> diff --git a/quota/edit.c b/quota/edit.c
> index b10a5b34..f9938b8a 100644
> --- a/quota/edit.c
> +++ b/quota/edit.c
> @@ -368,8 +368,7 @@ restore_file(
>  	uint		type)
>  {
>  	char		buffer[512];
> -	char		devbuffer[512];
> -	char		*dev = NULL;
> +	char		dev[512];
>  	uint		mask;
>  	int		cnt;
>  	uint32_t	id;
> @@ -377,7 +376,11 @@ restore_file(
>  
>  	while (fgets(buffer, sizeof(buffer), fp) != NULL) {
>  		if (strncmp("fs = ", buffer, 5) == 0) {
> -			dev = strncpy(devbuffer, buffer+5, sizeof(devbuffer));
> +			/*
> +			 * Copy the device name to dev, strip off the trailing
> +			 * newline, and move on to the next line.
> +			 */
> +			strncpy(dev, buffer + 5, sizeof(dev) - 1);
>  			dev[strlen(dev) - 1] = '\0';
>  			continue;
>  		}
> 

Re: [PATCH 06/10] misc: fix strncpy length complaints

[Bill O'Donnell]

On Mon, Apr 22, 2019 at 08:45:28AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Fix a number of complaints about feeding sizeof(dest) directly to
> strncpy.  We do this by declaring the char arrays to be one larger
> than necessary and subtracting one, to ensure that we never overfill
> the buffer.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---

Reviewed-by: Bill O'Donnell <billodo@redhat.com>

>  mkfs/xfs_mkfs.c |   13 +++++++++++--
>  quota/edit.c    |    9 ++++++---
>  2 files changed, 17 insertions(+), 5 deletions(-)
> 
> 
> diff --git a/mkfs/xfs_mkfs.c b/mkfs/xfs_mkfs.c
> index 3e2ef92d..db3ad38e 100644
> --- a/mkfs/xfs_mkfs.c
> +++ b/mkfs/xfs_mkfs.c
> @@ -3270,8 +3270,17 @@ finish_superblock_setup(
>  	struct xfs_mount	*mp,
>  	struct xfs_sb		*sbp)
>  {
> -	if (cfg->label)
> -		strncpy(sbp->sb_fname, cfg->label, sizeof(sbp->sb_fname));
> +	if (cfg->label) {
> +		size_t		label_len;
> +
> +		/*
> +		 * Labels are null terminated unless the string fits exactly
> +		 * in the label field, so assume sb_fname is zeroed and then
> +		 * do a memcpy because the destination isn't a normal C string.
> +		 */
> +		label_len = min(sizeof(sbp->sb_fname), strlen(cfg->label));
> +		memcpy(sbp->sb_fname, cfg->label, label_len);
> +	}
>  
>  	sbp->sb_dblocks = cfg->dblocks;
>  	sbp->sb_rblocks = cfg->rtblocks;
> diff --git a/quota/edit.c b/quota/edit.c
> index b10a5b34..f9938b8a 100644
> --- a/quota/edit.c
> +++ b/quota/edit.c
> @@ -368,8 +368,7 @@ restore_file(
>  	uint		type)
>  {
>  	char		buffer[512];
> -	char		devbuffer[512];
> -	char		*dev = NULL;
> +	char		dev[512];
>  	uint		mask;
>  	int		cnt;
>  	uint32_t	id;
> @@ -377,7 +376,11 @@ restore_file(
>  
>  	while (fgets(buffer, sizeof(buffer), fp) != NULL) {
>  		if (strncmp("fs = ", buffer, 5) == 0) {
> -			dev = strncpy(devbuffer, buffer+5, sizeof(devbuffer));
> +			/*
> +			 * Copy the device name to dev, strip off the trailing
> +			 * newline, and move on to the next line.
> +			 */
> +			strncpy(dev, buffer + 5, sizeof(dev) - 1);
>  			dev[strlen(dev) - 1] = '\0';
>  			continue;
>  		}
> 

[PATCH 07/10] libxfs: refactor buffer item release code

[Darrick J. Wong]

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

Refactor the buffer item release code into a helper, which we will use
in subsequent patches to make the buffer log item lifetime match the
kernel equivalents.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/trans.c |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)


diff --git a/libxfs/trans.c b/libxfs/trans.c
index 9de77c8b..629501f8 100644
--- a/libxfs/trans.c
+++ b/libxfs/trans.c
@@ -505,6 +505,16 @@ libxfs_trans_ordered_buf(
 	return ret;
 }
 
+static void
+xfs_buf_item_put(
+	struct xfs_buf_log_item	*bip)
+{
+	struct xfs_buf		*bp = bip->bli_buf;
+
+	bp->b_log_item = NULL;
+	kmem_zone_free(xfs_buf_item_zone, bip);
+}
+
 void
 libxfs_trans_brelse(
 	xfs_trans_t		*tp,
@@ -846,7 +856,6 @@ buf_item_done(
 
 	bp = bip->bli_buf;
 	ASSERT(bp != NULL);
-	bp->b_log_item = NULL;			/* remove log item */
 	bp->b_transp = NULL;			/* remove xact ptr */
 
 	hold = (bip->bli_flags & XFS_BLI_HOLD);
@@ -861,8 +870,7 @@ buf_item_done(
 		bip->bli_flags &= ~XFS_BLI_HOLD;
 	else
 		libxfs_putbuf(bp);
-	/* release the buf item */
-	kmem_zone_free(xfs_buf_item_zone, bip);
+	xfs_buf_item_put(bip);
 }
 
 static void

Re: [PATCH 07/10] libxfs: refactor buffer item release code

[Eric Sandeen]

On 4/22/19 10:45 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Refactor the buffer item release code into a helper, which we will use
> in subsequent patches to make the buffer log item lifetime match the
> kernel equivalents.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  libxfs/trans.c |   14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
> 
> 
> diff --git a/libxfs/trans.c b/libxfs/trans.c
> index 9de77c8b..629501f8 100644
> --- a/libxfs/trans.c
> +++ b/libxfs/trans.c
> @@ -505,6 +505,16 @@ libxfs_trans_ordered_buf(
>  	return ret;
>  }
>  
> +static void
> +xfs_buf_item_put(
> +	struct xfs_buf_log_item	*bip)
> +{
> +	struct xfs_buf		*bp = bip->bli_buf;
> +
> +	bp->b_log_item = NULL;
> +	kmem_zone_free(xfs_buf_item_zone, bip);
> +}
> +
>  void
>  libxfs_trans_brelse(
>  	xfs_trans_t		*tp,
> @@ -846,7 +856,6 @@ buf_item_done(
>  
>  	bp = bip->bli_buf;
>  	ASSERT(bp != NULL);
> -	bp->b_log_item = NULL;			/* remove log item */
>  	bp->b_transp = NULL;			/* remove xact ptr */
>  
>  	hold = (bip->bli_flags & XFS_BLI_HOLD);
> @@ -861,8 +870,7 @@ buf_item_done(
>  		bip->bli_flags &= ~XFS_BLI_HOLD;
>  	else
>  		libxfs_putbuf(bp);
> -	/* release the buf item */
> -	kmem_zone_free(xfs_buf_item_zone, bip);
> +	xfs_buf_item_put(bip);

In xfs_buf_item_put(), we reach back up from bip to bip->bli_buf, which is
the bp.  This is after we did a libxfs_putbuf(bp) on that bp.  Is there not
a chance of use after free here?  Enough puts and a shaker can run, right?

>  }
>  
>  static void
> 

Re: [PATCH 07/10] libxfs: refactor buffer item release code

[Darrick J. Wong]

On Mon, Apr 22, 2019 at 04:26:58PM -0500, Eric Sandeen wrote:
> On 4/22/19 10:45 AM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > Refactor the buffer item release code into a helper, which we will use
> > in subsequent patches to make the buffer log item lifetime match the
> > kernel equivalents.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  libxfs/trans.c |   14 +++++++++++---
> >  1 file changed, 11 insertions(+), 3 deletions(-)
> > 
> > 
> > diff --git a/libxfs/trans.c b/libxfs/trans.c
> > index 9de77c8b..629501f8 100644
> > --- a/libxfs/trans.c
> > +++ b/libxfs/trans.c
> > @@ -505,6 +505,16 @@ libxfs_trans_ordered_buf(
> >  	return ret;
> >  }
> >  
> > +static void
> > +xfs_buf_item_put(
> > +	struct xfs_buf_log_item	*bip)
> > +{
> > +	struct xfs_buf		*bp = bip->bli_buf;
> > +
> > +	bp->b_log_item = NULL;
> > +	kmem_zone_free(xfs_buf_item_zone, bip);
> > +}
> > +
> >  void
> >  libxfs_trans_brelse(
> >  	xfs_trans_t		*tp,
> > @@ -846,7 +856,6 @@ buf_item_done(
> >  
> >  	bp = bip->bli_buf;
> >  	ASSERT(bp != NULL);
> > -	bp->b_log_item = NULL;			/* remove log item */
> >  	bp->b_transp = NULL;			/* remove xact ptr */
> >  
> >  	hold = (bip->bli_flags & XFS_BLI_HOLD);
> > @@ -861,8 +870,7 @@ buf_item_done(
> >  		bip->bli_flags &= ~XFS_BLI_HOLD;
> >  	else
> >  		libxfs_putbuf(bp);
> > -	/* release the buf item */
> > -	kmem_zone_free(xfs_buf_item_zone, bip);
> > +	xfs_buf_item_put(bip);
> 
> In xfs_buf_item_put(), we reach back up from bip to bip->bli_buf, which is
> the bp.  This is after we did a libxfs_putbuf(bp) on that bp.  Is there not
> a chance of use after free here?  Enough puts and a shaker can run, right?

I think you're right, the xfs_buf_item_put should come before the
libxfs_putbuf.

--D

> >  }
> >  
> >  static void
> > 

Re: [PATCH 07/10] libxfs: refactor buffer item release code

[Eric Sandeen]

On 4/22/19 4:35 PM, Darrick J. Wong wrote:
> On Mon, Apr 22, 2019 at 04:26:58PM -0500, Eric Sandeen wrote:
>> On 4/22/19 10:45 AM, Darrick J. Wong wrote:
>>> From: Darrick J. Wong <darrick.wong@oracle.com>
>>>
>>> Refactor the buffer item release code into a helper, which we will use
>>> in subsequent patches to make the buffer log item lifetime match the
>>> kernel equivalents.
>>>
>>> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
>>> ---
>>>  libxfs/trans.c |   14 +++++++++++---
>>>  1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>>
>>> diff --git a/libxfs/trans.c b/libxfs/trans.c
>>> index 9de77c8b..629501f8 100644
>>> --- a/libxfs/trans.c
>>> +++ b/libxfs/trans.c
>>> @@ -505,6 +505,16 @@ libxfs_trans_ordered_buf(
>>>  	return ret;
>>>  }
>>>  
>>> +static void
>>> +xfs_buf_item_put(
>>> +	struct xfs_buf_log_item	*bip)
>>> +{
>>> +	struct xfs_buf		*bp = bip->bli_buf;
>>> +
>>> +	bp->b_log_item = NULL;
>>> +	kmem_zone_free(xfs_buf_item_zone, bip);
>>> +}
>>> +
>>>  void
>>>  libxfs_trans_brelse(
>>>  	xfs_trans_t		*tp,
>>> @@ -846,7 +856,6 @@ buf_item_done(
>>>  
>>>  	bp = bip->bli_buf;
>>>  	ASSERT(bp != NULL);
>>> -	bp->b_log_item = NULL;			/* remove log item */
>>>  	bp->b_transp = NULL;			/* remove xact ptr */
>>>  
>>>  	hold = (bip->bli_flags & XFS_BLI_HOLD);
>>> @@ -861,8 +870,7 @@ buf_item_done(
>>>  		bip->bli_flags &= ~XFS_BLI_HOLD;
>>>  	else
>>>  		libxfs_putbuf(bp);
>>> -	/* release the buf item */
>>> -	kmem_zone_free(xfs_buf_item_zone, bip);
>>> +	xfs_buf_item_put(bip);
>>
>> In xfs_buf_item_put(), we reach back up from bip to bip->bli_buf, which is
>> the bp.  This is after we did a libxfs_putbuf(bp) on that bp.  Is there not
>> a chance of use after free here?  Enough puts and a shaker can run, right?
> 
> I think you're right, the xfs_buf_item_put should come before the
> libxfs_putbuf.


Yeah in the kernel, it comes before xfs_buf_relse, and in userspace,

#define xfs_buf_relse(bp) libxfs_putbuf(bp)

-Eric

[PATCH v2 07/10] libxfs: refactor buffer item release code

[Darrick J. Wong]

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

Refactor the buffer item release code into a helper, which we will use
in subsequent patches to make the buffer log item lifetime match the
kernel equivalents.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
v2: fix a bug where we put the buf log item after the buf
---
 libxfs/trans.c |   21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/libxfs/trans.c b/libxfs/trans.c
index 9de77c8b..121636b5 100644
--- a/libxfs/trans.c
+++ b/libxfs/trans.c
@@ -505,6 +505,16 @@ libxfs_trans_ordered_buf(
 	return ret;
 }
 
+static void
+xfs_buf_item_put(
+	struct xfs_buf_log_item	*bip)
+{
+	struct xfs_buf		*bp = bip->bli_buf;
+
+	bp->b_log_item = NULL;
+	kmem_zone_free(xfs_buf_item_zone, bip);
+}
+
 void
 libxfs_trans_brelse(
 	xfs_trans_t		*tp,
@@ -846,7 +856,6 @@ buf_item_done(
 
 	bp = bip->bli_buf;
 	ASSERT(bp != NULL);
-	bp->b_log_item = NULL;			/* remove log item */
 	bp->b_transp = NULL;			/* remove xact ptr */
 
 	hold = (bip->bli_flags & XFS_BLI_HOLD);
@@ -857,12 +866,12 @@ buf_item_done(
 #endif
 		libxfs_writebuf_int(bp, 0);
 	}
+
+	bip->bli_flags &= ~XFS_BLI_HOLD;
+	xfs_buf_item_put(bip);
 	if (hold)
-		bip->bli_flags &= ~XFS_BLI_HOLD;
-	else
-		libxfs_putbuf(bp);
-	/* release the buf item */
-	kmem_zone_free(xfs_buf_item_zone, bip);
+		return;
+	libxfs_putbuf(bp);
 }
 
 static void

Re: [PATCH v2 07/10] libxfs: refactor buffer item release code

[Bill O'Donnell]

On Tue, Apr 23, 2019 at 01:51:33PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Refactor the buffer item release code into a helper, which we will use
> in subsequent patches to make the buffer log item lifetime match the
> kernel equivalents.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
> v2: fix a bug where we put the buf log item after the buf
> ---
>  libxfs/trans.c |   21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/libxfs/trans.c b/libxfs/trans.c
> index 9de77c8b..121636b5 100644
> --- a/libxfs/trans.c
> +++ b/libxfs/trans.c
> @@ -505,6 +505,16 @@ libxfs_trans_ordered_buf(
>  	return ret;
>  }
>  
> +static void
> +xfs_buf_item_put(
> +	struct xfs_buf_log_item	*bip)
> +{
> +	struct xfs_buf		*bp = bip->bli_buf;
> +
> +	bp->b_log_item = NULL;
> +	kmem_zone_free(xfs_buf_item_zone, bip);
> +}
> +
>  void
>  libxfs_trans_brelse(
>  	xfs_trans_t		*tp,
> @@ -846,7 +856,6 @@ buf_item_done(
>  
>  	bp = bip->bli_buf;
>  	ASSERT(bp != NULL);
> -	bp->b_log_item = NULL;			/* remove log item */
>  	bp->b_transp = NULL;			/* remove xact ptr */
>  
>  	hold = (bip->bli_flags & XFS_BLI_HOLD);
> @@ -857,12 +866,12 @@ buf_item_done(
>  #endif
>  		libxfs_writebuf_int(bp, 0);
>  	}
> +
> +	bip->bli_flags &= ~XFS_BLI_HOLD;
> +	xfs_buf_item_put(bip);
>  	if (hold)
> -		bip->bli_flags &= ~XFS_BLI_HOLD;
> -	else
> -		libxfs_putbuf(bp);
> -	/* release the buf item */
> -	kmem_zone_free(xfs_buf_item_zone, bip);
> +		return;
> +	libxfs_putbuf(bp);
>  }
>  
>  static void

[PATCH 08/10] libxfs: don't touch buffer log item pointer when flushing inode log item

[Darrick J. Wong]

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

When we're flushing an inode log item, it is not necessary to mess with
the inode cluster buffer's log item because the iflush code paths pass
the inode log item directly.  The unconditional reset causes us to leak
buffer log items.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/trans.c |    2 --
 libxfs/util.c  |    1 -
 2 files changed, 3 deletions(-)


diff --git a/libxfs/trans.c b/libxfs/trans.c
index 629501f8..d562cdc0 100644
--- a/libxfs/trans.c
+++ b/libxfs/trans.c
@@ -826,10 +826,8 @@ inode_item_done(
 	 * of whether the flush succeed or not. If we fail the flush, make sure
 	 * we still release the buffer reference we currently hold.
 	 */
-	bp->b_log_item = iip;
 	error = libxfs_iflush_int(ip, bp);
 	ip->i_transp = NULL;	/* disassociate from transaction */
-	bp->b_log_item = NULL;	/* remove log item */
 	bp->b_transp = NULL;	/* remove xact ptr */
 
 	if (error) {
diff --git a/libxfs/util.c b/libxfs/util.c
index ea75fa20..9fe9a367 100644
--- a/libxfs/util.c
+++ b/libxfs/util.c
@@ -394,7 +394,6 @@ libxfs_iflush_int(xfs_inode_t *ip, xfs_buf_t *bp)
 	xfs_dinode_t		*dip;
 	xfs_mount_t		*mp;
 
-	ASSERT(bp-b_log_item != NULL);
 	ASSERT(ip->i_d.di_format != XFS_DINODE_FMT_BTREE ||
 		ip->i_d.di_nextents > ip->i_df.if_ext_max);
 	ASSERT(ip->i_d.di_version > 1);

Re: [PATCH 08/10] libxfs: don't touch buffer log item pointer when flushing inode log item

[Eric Sandeen]

On 4/22/19 10:45 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we're flushing an inode log item, it is not necessary to mess with
> the inode cluster buffer's log item because the iflush code paths pass
> the inode log item directly.  The unconditional reset causes us to leak
> buffer log items.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  libxfs/trans.c |    2 --
>  libxfs/util.c  |    1 -
>  2 files changed, 3 deletions(-)
> 
> 
> diff --git a/libxfs/trans.c b/libxfs/trans.c
> index 629501f8..d562cdc0 100644
> --- a/libxfs/trans.c
> +++ b/libxfs/trans.c
> @@ -826,10 +826,8 @@ inode_item_done(
>  	 * of whether the flush succeed or not. If we fail the flush, make sure
>  	 * we still release the buffer reference we currently hold.
>  	 */
> -	bp->b_log_item = iip;

Weird, digging WAY back into history, I can't figure out why this was ever done.

Given that nothing ever retrieves b_log_item as an inode log item,
this seems clearly wrong, so, ok, sure!

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

>  	error = libxfs_iflush_int(ip, bp);
>  	ip->i_transp = NULL;	/* disassociate from transaction */
> -	bp->b_log_item = NULL;	/* remove log item */
>  	bp->b_transp = NULL;	/* remove xact ptr */
>  
>  	if (error) {
> diff --git a/libxfs/util.c b/libxfs/util.c
> index ea75fa20..9fe9a367 100644
> --- a/libxfs/util.c
> +++ b/libxfs/util.c
> @@ -394,7 +394,6 @@ libxfs_iflush_int(xfs_inode_t *ip, xfs_buf_t *bp)
>  	xfs_dinode_t		*dip;
>  	xfs_mount_t		*mp;
>  
> -	ASSERT(bp-b_log_item != NULL);
>  	ASSERT(ip->i_d.di_format != XFS_DINODE_FMT_BTREE ||
>  		ip->i_d.di_nextents > ip->i_df.if_ext_max);
>  	ASSERT(ip->i_d.di_version > 1);
> 

Re: [PATCH 08/10] libxfs: don't touch buffer log item pointer when flushing inode log item

[Bill O'Donnell]

On Mon, Apr 22, 2019 at 08:45:40AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we're flushing an inode log item, it is not necessary to mess with
> the inode cluster buffer's log item because the iflush code paths pass
> the inode log item directly.  The unconditional reset causes us to leak
> buffer log items.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

looks fine.
Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
>  libxfs/trans.c |    2 --
>  libxfs/util.c  |    1 -
>  2 files changed, 3 deletions(-)
> 
> 
> diff --git a/libxfs/trans.c b/libxfs/trans.c
> index 629501f8..d562cdc0 100644
> --- a/libxfs/trans.c
> +++ b/libxfs/trans.c
> @@ -826,10 +826,8 @@ inode_item_done(
>  	 * of whether the flush succeed or not. If we fail the flush, make sure
>  	 * we still release the buffer reference we currently hold.
>  	 */
> -	bp->b_log_item = iip;
>  	error = libxfs_iflush_int(ip, bp);
>  	ip->i_transp = NULL;	/* disassociate from transaction */
> -	bp->b_log_item = NULL;	/* remove log item */
>  	bp->b_transp = NULL;	/* remove xact ptr */
>  
>  	if (error) {
> diff --git a/libxfs/util.c b/libxfs/util.c
> index ea75fa20..9fe9a367 100644
> --- a/libxfs/util.c
> +++ b/libxfs/util.c
> @@ -394,7 +394,6 @@ libxfs_iflush_int(xfs_inode_t *ip, xfs_buf_t *bp)
>  	xfs_dinode_t		*dip;
>  	xfs_mount_t		*mp;
>  
> -	ASSERT(bp-b_log_item != NULL);
>  	ASSERT(ip->i_d.di_format != XFS_DINODE_FMT_BTREE ||
>  		ip->i_d.di_nextents > ip->i_df.if_ext_max);
>  	ASSERT(ip->i_d.di_version > 1);
> 

[PATCH 09/10] libxfs: fix buffer log item lifetime weirdness

[Darrick J. Wong]

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

In xfsprogs, the lifetime of xfs_buf log items doesn't match the kernel
because we keep them around after comitting or cancelling transactions.
This is confusing, so change the lifetime to be consistent.  Worse yet,
if an inode cluster buffer gets bjoined to a transaction (e.g. someone
called xfs_trans_read_buf) we'll leak it when flushing an inode core
back to that buffer.

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


diff --git a/libxfs/trans.c b/libxfs/trans.c
index d562cdc0..22926236 100644
--- a/libxfs/trans.c
+++ b/libxfs/trans.c
@@ -545,6 +545,7 @@ libxfs_trans_brelse(
 	xfs_trans_del_item(&bip->bli_item);
 	if (bip->bli_flags & XFS_BLI_HOLD)
 		bip->bli_flags &= ~XFS_BLI_HOLD;
+	xfs_buf_item_put(bip);
 	bp->b_transp = NULL;
 	libxfs_putbuf(bp);
 }
@@ -904,6 +905,7 @@ buf_item_unlock(
 
 	hold = bip->bli_flags & XFS_BLI_HOLD;
 	bip->bli_flags &= ~XFS_BLI_HOLD;
+	xfs_buf_item_put(bip);
 	if (!hold)
 		libxfs_putbuf(bp);
 }

Re: [PATCH 09/10] libxfs: fix buffer log item lifetime weirdness

[Bill O'Donnell]

On Mon, Apr 22, 2019 at 08:45:47AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> In xfsprogs, the lifetime of xfs_buf log items doesn't match the kernel
> because we keep them around after comitting or cancelling transactions.
> This is confusing, so change the lifetime to be consistent.  Worse yet,
> if an inode cluster buffer gets bjoined to a transaction (e.g. someone
> called xfs_trans_read_buf) we'll leak it when flushing an inode core
> back to that buffer.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
>  libxfs/trans.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> 
> diff --git a/libxfs/trans.c b/libxfs/trans.c
> index d562cdc0..22926236 100644
> --- a/libxfs/trans.c
> +++ b/libxfs/trans.c
> @@ -545,6 +545,7 @@ libxfs_trans_brelse(
>  	xfs_trans_del_item(&bip->bli_item);
>  	if (bip->bli_flags & XFS_BLI_HOLD)
>  		bip->bli_flags &= ~XFS_BLI_HOLD;
> +	xfs_buf_item_put(bip);
>  	bp->b_transp = NULL;
>  	libxfs_putbuf(bp);
>  }
> @@ -904,6 +905,7 @@ buf_item_unlock(
>  
>  	hold = bip->bli_flags & XFS_BLI_HOLD;
>  	bip->bli_flags &= ~XFS_BLI_HOLD;
> +	xfs_buf_item_put(bip);
>  	if (!hold)
>  		libxfs_putbuf(bp);
>  }
> 

[PATCH 10/10] libxfs: shorten inode item lifetime

[Darrick J. Wong]

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

Shorten the inode item lifetime so that we only keep them around while
the inode is joined with a transaction.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 libxfs/rdwr.c  |    4 +---
 libxfs/trans.c |   19 ++++++++++++++++---
 2 files changed, 17 insertions(+), 6 deletions(-)


diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
index bc2ed38c..a5efc805 100644
--- a/libxfs/rdwr.c
+++ b/libxfs/rdwr.c
@@ -1428,9 +1428,7 @@ void
 libxfs_irele(
 	struct xfs_inode	*ip)
 {
-	if (ip->i_itemp)
-		kmem_zone_free(xfs_ili_zone, ip->i_itemp);
-	ip->i_itemp = NULL;
+	ASSERT(ip->i_itemp == NULL);
 	libxfs_idestroy(ip);
 	kmem_zone_free(xfs_inode_zone, ip);
 }
diff --git a/libxfs/trans.c b/libxfs/trans.c
index 22926236..1dcdebf3 100644
--- a/libxfs/trans.c
+++ b/libxfs/trans.c
@@ -785,6 +785,16 @@ _("Transaction block reservation exceeded! %u > %u\n"),
 	tp->t_flags |= (XFS_TRANS_SB_DIRTY | XFS_TRANS_DIRTY);
 }
 
+static void
+xfs_inode_item_put(
+	struct xfs_inode_log_item	*iip)
+{
+	struct xfs_inode		*ip = iip->ili_inode;
+
+	ip->i_itemp = NULL;
+	kmem_zone_free(xfs_ili_zone, iip);
+}
+
 
 /*
  * Transaction commital code follows (i.e. write to disk in libxfs)
@@ -809,7 +819,7 @@ inode_item_done(
 	if (!(iip->ili_fields & XFS_ILOG_ALL)) {
 		ip->i_transp = NULL;	/* disassociate from transaction */
 		iip->ili_flags = 0;	/* reset all flags */
-		return;
+		goto free;
 	}
 
 	/*
@@ -819,7 +829,7 @@ inode_item_done(
 	if (error) {
 		fprintf(stderr, _("%s: warning - imap_to_bp failed (%d)\n"),
 			progname, error);
-		return;
+		goto free;
 	}
 
 	/*
@@ -835,7 +845,7 @@ inode_item_done(
 		fprintf(stderr, _("%s: warning - iflush_int failed (%d)\n"),
 			progname, error);
 		libxfs_putbuf(bp);
-		return;
+		goto free;
 	}
 
 	libxfs_writebuf(bp, 0);
@@ -843,6 +853,8 @@ inode_item_done(
 	fprintf(stderr, "flushing dirty inode %llu, buffer %p\n",
 			ip->i_ino, bp);
 #endif
+free:
+	xfs_inode_item_put(iip);
 }
 
 static void
@@ -920,6 +932,7 @@ inode_item_unlock(
 	ip->i_transp = NULL;
 
 	iip->ili_flags = 0;
+	xfs_inode_item_put(iip);
 }
 
 /* Detach and unlock all of the items in a transaction */

Re: [PATCH 10/10] libxfs: shorten inode item lifetime

[Bill O'Donnell]

On Mon, Apr 22, 2019 at 08:45:53AM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Shorten the inode item lifetime so that we only keep them around while
> the inode is joined with a transaction.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
>  libxfs/rdwr.c  |    4 +---
>  libxfs/trans.c |   19 ++++++++++++++++---
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> 
> diff --git a/libxfs/rdwr.c b/libxfs/rdwr.c
> index bc2ed38c..a5efc805 100644
> --- a/libxfs/rdwr.c
> +++ b/libxfs/rdwr.c
> @@ -1428,9 +1428,7 @@ void
>  libxfs_irele(
>  	struct xfs_inode	*ip)
>  {
> -	if (ip->i_itemp)
> -		kmem_zone_free(xfs_ili_zone, ip->i_itemp);
> -	ip->i_itemp = NULL;
> +	ASSERT(ip->i_itemp == NULL);
>  	libxfs_idestroy(ip);
>  	kmem_zone_free(xfs_inode_zone, ip);
>  }
> diff --git a/libxfs/trans.c b/libxfs/trans.c
> index 22926236..1dcdebf3 100644
> --- a/libxfs/trans.c
> +++ b/libxfs/trans.c
> @@ -785,6 +785,16 @@ _("Transaction block reservation exceeded! %u > %u\n"),
>  	tp->t_flags |= (XFS_TRANS_SB_DIRTY | XFS_TRANS_DIRTY);
>  }
>  
> +static void
> +xfs_inode_item_put(
> +	struct xfs_inode_log_item	*iip)
> +{
> +	struct xfs_inode		*ip = iip->ili_inode;
> +
> +	ip->i_itemp = NULL;
> +	kmem_zone_free(xfs_ili_zone, iip);
> +}
> +
>  
>  /*
>   * Transaction commital code follows (i.e. write to disk in libxfs)
> @@ -809,7 +819,7 @@ inode_item_done(
>  	if (!(iip->ili_fields & XFS_ILOG_ALL)) {
>  		ip->i_transp = NULL;	/* disassociate from transaction */
>  		iip->ili_flags = 0;	/* reset all flags */
> -		return;
> +		goto free;
>  	}
>  
>  	/*
> @@ -819,7 +829,7 @@ inode_item_done(
>  	if (error) {
>  		fprintf(stderr, _("%s: warning - imap_to_bp failed (%d)\n"),
>  			progname, error);
> -		return;
> +		goto free;
>  	}
>  
>  	/*
> @@ -835,7 +845,7 @@ inode_item_done(
>  		fprintf(stderr, _("%s: warning - iflush_int failed (%d)\n"),
>  			progname, error);
>  		libxfs_putbuf(bp);
> -		return;
> +		goto free;
>  	}
>  
>  	libxfs_writebuf(bp, 0);
> @@ -843,6 +853,8 @@ inode_item_done(
>  	fprintf(stderr, "flushing dirty inode %llu, buffer %p\n",
>  			ip->i_ino, bp);
>  #endif
> +free:
> +	xfs_inode_item_put(iip);
>  }
>  
>  static void
> @@ -920,6 +932,7 @@ inode_item_unlock(
>  	ip->i_transp = NULL;
>  
>  	iip->ili_flags = 0;
> +	xfs_inode_item_put(iip);
>  }
>  
>  /* Detach and unlock all of the items in a transaction */
> 

[PATCH 11/10] libfrog: fix memory leak in bitmap_free

[Darrick J. Wong]

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

Free the bitmap struct before we null out the caller's pointer.

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

diff --git a/libfrog/bitmap.c b/libfrog/bitmap.c
index aecdba0d..450ebe0a 100644
--- a/libfrog/bitmap.c
+++ b/libfrog/bitmap.c
@@ -104,6 +104,7 @@ bitmap_free(
 		free(ext);
 	}
 	free(bmap->bt_tree);
+	free(bmap);
 	*bmapp = NULL;
 }
 

Re: [PATCH 11/10] libfrog: fix memory leak in bitmap_free

[Bill O'Donnell]

On Tue, Apr 23, 2019 at 02:04:09PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Free the bitmap struct before we null out the caller's pointer.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Bill O'Donnell <billodo@redhat.com>

> ---
>  libfrog/bitmap.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/libfrog/bitmap.c b/libfrog/bitmap.c
> index aecdba0d..450ebe0a 100644
> --- a/libfrog/bitmap.c
> +++ b/libfrog/bitmap.c
> @@ -104,6 +104,7 @@ bitmap_free(
>  		free(ext);
>  	}
>  	free(bmap->bt_tree);
> +	free(bmap);
>  	*bmapp = NULL;
>  }
>