* [PATCH 01/10] scrub: fix Makefile targets which depend on builddefs
2019-04-22 15:44 [PATCH v3 00/10] xfsprogs-5.0: fix various problems Darrick J. Wong
@ 2019-04-22 15:44 ` Darrick J. Wong
2019-04-22 18:27 ` Eric Sandeen
2019-04-22 18:28 ` Bill O'Donnell
2019-04-22 15:45 ` [PATCH 02/10] xfs_info: use findmnt to handle mounted block devices Darrick J. Wong
` (9 subsequent siblings)
10 siblings, 2 replies; 38+ messages in thread
From: Darrick J. Wong @ 2019-04-22 15:44 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
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" < $< > $@
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 01/10] scrub: fix Makefile targets which depend on builddefs
2019-04-22 15:44 ` [PATCH 01/10] scrub: fix Makefile targets which depend on builddefs Darrick J. Wong
@ 2019-04-22 18:27 ` Eric Sandeen
2019-04-22 18:28 ` Bill O'Donnell
1 sibling, 0 replies; 38+ messages in thread
From: Eric Sandeen @ 2019-04-22 18:27 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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" < $< > $@
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 01/10] scrub: fix Makefile targets which depend on builddefs
2019-04-22 15:44 ` [PATCH 01/10] scrub: fix Makefile targets which depend on builddefs Darrick J. Wong
2019-04-22 18:27 ` Eric Sandeen
@ 2019-04-22 18:28 ` Bill O'Donnell
1 sibling, 0 replies; 38+ messages in thread
From: Bill O'Donnell @ 2019-04-22 18:28 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
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" < $< > $@
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 02/10] xfs_info: use findmnt to handle mounted block devices
2019-04-22 15:44 [PATCH v3 00/10] xfsprogs-5.0: fix various problems Darrick J. Wong
2019-04-22 15:44 ` [PATCH 01/10] scrub: fix Makefile targets which depend on builddefs Darrick J. Wong
@ 2019-04-22 15:45 ` Darrick J. Wong
2019-04-22 18:35 ` Eric Sandeen
2019-04-22 19:27 ` Bill O'Donnell
2019-04-22 15:45 ` [PATCH 03/10] xfs_repair: correctly account for free space btree shrinks when fixing freelist Darrick J. Wong
` (8 subsequent siblings)
10 siblings, 2 replies; 38+ messages in thread
From: Darrick J. Wong @ 2019-04-22 15:45 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
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
;;
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 02/10] xfs_info: use findmnt to handle mounted block devices
2019-04-22 15:45 ` [PATCH 02/10] xfs_info: use findmnt to handle mounted block devices Darrick J. Wong
@ 2019-04-22 18:35 ` Eric Sandeen
2019-04-22 19:27 ` Bill O'Donnell
1 sibling, 0 replies; 38+ messages in thread
From: Eric Sandeen @ 2019-04-22 18:35 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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
> ;;
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 02/10] xfs_info: use findmnt to handle mounted block devices
2019-04-22 15:45 ` [PATCH 02/10] xfs_info: use findmnt to handle mounted block devices Darrick J. Wong
2019-04-22 18:35 ` Eric Sandeen
@ 2019-04-22 19:27 ` Bill O'Donnell
1 sibling, 0 replies; 38+ messages in thread
From: Bill O'Donnell @ 2019-04-22 19:27 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
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
> ;;
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 03/10] xfs_repair: correctly account for free space btree shrinks when fixing freelist
2019-04-22 15:44 [PATCH v3 00/10] xfsprogs-5.0: fix various problems Darrick J. Wong
2019-04-22 15:44 ` [PATCH 01/10] scrub: fix Makefile targets which depend on builddefs Darrick J. Wong
2019-04-22 15:45 ` [PATCH 02/10] xfs_info: use findmnt to handle mounted block devices Darrick J. Wong
@ 2019-04-22 15:45 ` Darrick J. Wong
2019-04-22 19:24 ` Eric Sandeen
2019-04-22 19:36 ` Bill O'Donnell
2019-04-22 15:45 ` [PATCH 04/10] libxfs: retain ifork_ops when flushing inode Darrick J. Wong
` (7 subsequent siblings)
10 siblings, 2 replies; 38+ messages in thread
From: Darrick J. Wong @ 2019-04-22 15:45 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
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;
}
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 03/10] xfs_repair: correctly account for free space btree shrinks when fixing freelist
2019-04-22 15:45 ` [PATCH 03/10] xfs_repair: correctly account for free space btree shrinks when fixing freelist Darrick J. Wong
@ 2019-04-22 19:24 ` Eric Sandeen
2019-04-22 19:36 ` Bill O'Donnell
1 sibling, 0 replies; 38+ messages in thread
From: Eric Sandeen @ 2019-04-22 19:24 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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;
> }
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 03/10] xfs_repair: correctly account for free space btree shrinks when fixing freelist
2019-04-22 15:45 ` [PATCH 03/10] xfs_repair: correctly account for free space btree shrinks when fixing freelist Darrick J. Wong
2019-04-22 19:24 ` Eric Sandeen
@ 2019-04-22 19:36 ` Bill O'Donnell
1 sibling, 0 replies; 38+ messages in thread
From: Bill O'Donnell @ 2019-04-22 19:36 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
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;
> }
>
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 04/10] libxfs: retain ifork_ops when flushing inode
2019-04-22 15:44 [PATCH v3 00/10] xfsprogs-5.0: fix various problems Darrick J. Wong
` (2 preceding siblings ...)
2019-04-22 15:45 ` [PATCH 03/10] xfs_repair: correctly account for free space btree shrinks when fixing freelist Darrick J. Wong
@ 2019-04-22 15:45 ` Darrick J. Wong
2019-04-22 19:40 ` Bill O'Donnell
` (2 more replies)
2019-04-22 15:45 ` [PATCH 05/10] libxfs: drop the ifork_ops parameter from _inode_verify_forks Darrick J. Wong
` (6 subsequent siblings)
10 siblings, 3 replies; 38+ messages in thread
From: Darrick J. Wong @ 2019-04-22 15:45 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs, Arkadiusz Miskiewicz
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;
/*
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 04/10] libxfs: retain ifork_ops when flushing inode
2019-04-22 15:45 ` [PATCH 04/10] libxfs: retain ifork_ops when flushing inode Darrick J. Wong
@ 2019-04-22 19:40 ` Bill O'Donnell
2019-04-22 19:45 ` Eric Sandeen
2019-10-02 6:00 ` Arkadiusz Miśkiewicz
2 siblings, 0 replies; 38+ messages in thread
From: Bill O'Donnell @ 2019-04-22 19:40 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs, Arkadiusz Miskiewicz
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;
>
> /*
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 04/10] libxfs: retain ifork_ops when flushing inode
2019-04-22 15:45 ` [PATCH 04/10] libxfs: retain ifork_ops when flushing inode Darrick J. Wong
2019-04-22 19:40 ` Bill O'Donnell
@ 2019-04-22 19:45 ` Eric Sandeen
2019-10-02 6:00 ` Arkadiusz Miśkiewicz
2 siblings, 0 replies; 38+ messages in thread
From: Eric Sandeen @ 2019-04-22 19:45 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs, Arkadiusz Miskiewicz
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;
>
> /*
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 04/10] libxfs: retain ifork_ops when flushing inode
2019-04-22 15:45 ` [PATCH 04/10] libxfs: retain ifork_ops when flushing inode Darrick J. Wong
2019-04-22 19:40 ` Bill O'Donnell
2019-04-22 19:45 ` Eric Sandeen
@ 2019-10-02 6:00 ` Arkadiusz Miśkiewicz
2 siblings, 0 replies; 38+ messages in thread
From: Arkadiusz Miśkiewicz @ 2019-10-02 6:00 UTC (permalink / raw)
To: Darrick J. Wong, sandeen; +Cc: linux-xfs
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 )
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 05/10] libxfs: drop the ifork_ops parameter from _inode_verify_forks
2019-04-22 15:44 [PATCH v3 00/10] xfsprogs-5.0: fix various problems Darrick J. Wong
` (3 preceding siblings ...)
2019-04-22 15:45 ` [PATCH 04/10] libxfs: retain ifork_ops when flushing inode Darrick J. Wong
@ 2019-04-22 15:45 ` Darrick J. Wong
2019-04-22 19:43 ` Bill O'Donnell
2019-04-22 20:49 ` Eric Sandeen
2019-04-22 15:45 ` [PATCH 06/10] misc: fix strncpy length complaints Darrick J. Wong
` (5 subsequent siblings)
10 siblings, 2 replies; 38+ messages in thread
From: Darrick J. Wong @ 2019-04-22 15:45 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
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;
/*
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 05/10] libxfs: drop the ifork_ops parameter from _inode_verify_forks
2019-04-22 15:45 ` [PATCH 05/10] libxfs: drop the ifork_ops parameter from _inode_verify_forks Darrick J. Wong
@ 2019-04-22 19:43 ` Bill O'Donnell
2019-04-22 20:49 ` Eric Sandeen
1 sibling, 0 replies; 38+ messages in thread
From: Bill O'Donnell @ 2019-04-22 19:43 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
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;
>
> /*
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 05/10] libxfs: drop the ifork_ops parameter from _inode_verify_forks
2019-04-22 15:45 ` [PATCH 05/10] libxfs: drop the ifork_ops parameter from _inode_verify_forks Darrick J. Wong
2019-04-22 19:43 ` Bill O'Donnell
@ 2019-04-22 20:49 ` Eric Sandeen
1 sibling, 0 replies; 38+ messages in thread
From: Eric Sandeen @ 2019-04-22 20:49 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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;
>
> /*
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 06/10] misc: fix strncpy length complaints
2019-04-22 15:44 [PATCH v3 00/10] xfsprogs-5.0: fix various problems Darrick J. Wong
` (4 preceding siblings ...)
2019-04-22 15:45 ` [PATCH 05/10] libxfs: drop the ifork_ops parameter from _inode_verify_forks Darrick J. Wong
@ 2019-04-22 15:45 ` Darrick J. Wong
2019-04-22 20:48 ` Eric Sandeen
` (2 more replies)
2019-04-22 15:45 ` [PATCH 07/10] libxfs: refactor buffer item release code Darrick J. Wong
` (4 subsequent siblings)
10 siblings, 3 replies; 38+ messages in thread
From: Darrick J. Wong @ 2019-04-22 15:45 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
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;
}
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 06/10] misc: fix strncpy length complaints
2019-04-22 15:45 ` [PATCH 06/10] misc: fix strncpy length complaints Darrick J. Wong
@ 2019-04-22 20:48 ` Eric Sandeen
2019-04-22 20:57 ` Darrick J. Wong
2019-04-22 21:07 ` Eric Sandeen
2019-04-23 15:07 ` Bill O'Donnell
2 siblings, 1 reply; 38+ messages in thread
From: Eric Sandeen @ 2019-04-22 20:48 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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;
> }
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 06/10] misc: fix strncpy length complaints
2019-04-22 20:48 ` Eric Sandeen
@ 2019-04-22 20:57 ` Darrick J. Wong
2019-04-22 21:04 ` Eric Sandeen
0 siblings, 1 reply; 38+ messages in thread
From: Darrick J. Wong @ 2019-04-22 20:57 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
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;
> > }
> >
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 06/10] misc: fix strncpy length complaints
2019-04-22 20:57 ` Darrick J. Wong
@ 2019-04-22 21:04 ` Eric Sandeen
0 siblings, 0 replies; 38+ messages in thread
From: Eric Sandeen @ 2019-04-22 21:04 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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;
>>> }
>>>
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 06/10] misc: fix strncpy length complaints
2019-04-22 15:45 ` [PATCH 06/10] misc: fix strncpy length complaints Darrick J. Wong
2019-04-22 20:48 ` Eric Sandeen
@ 2019-04-22 21:07 ` Eric Sandeen
2019-04-23 15:07 ` Bill O'Donnell
2 siblings, 0 replies; 38+ messages in thread
From: Eric Sandeen @ 2019-04-22 21:07 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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;
> }
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 06/10] misc: fix strncpy length complaints
2019-04-22 15:45 ` [PATCH 06/10] misc: fix strncpy length complaints Darrick J. Wong
2019-04-22 20:48 ` Eric Sandeen
2019-04-22 21:07 ` Eric Sandeen
@ 2019-04-23 15:07 ` Bill O'Donnell
2 siblings, 0 replies; 38+ messages in thread
From: Bill O'Donnell @ 2019-04-23 15:07 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
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;
> }
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 07/10] libxfs: refactor buffer item release code
2019-04-22 15:44 [PATCH v3 00/10] xfsprogs-5.0: fix various problems Darrick J. Wong
` (5 preceding siblings ...)
2019-04-22 15:45 ` [PATCH 06/10] misc: fix strncpy length complaints Darrick J. Wong
@ 2019-04-22 15:45 ` Darrick J. Wong
2019-04-22 21:26 ` Eric Sandeen
2019-04-23 20:51 ` [PATCH v2 " Darrick J. Wong
2019-04-22 15:45 ` [PATCH 08/10] libxfs: don't touch buffer log item pointer when flushing inode log item Darrick J. Wong
` (3 subsequent siblings)
10 siblings, 2 replies; 38+ messages in thread
From: Darrick J. Wong @ 2019-04-22 15:45 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
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
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 07/10] libxfs: refactor buffer item release code
2019-04-22 15:45 ` [PATCH 07/10] libxfs: refactor buffer item release code Darrick J. Wong
@ 2019-04-22 21:26 ` Eric Sandeen
2019-04-22 21:35 ` Darrick J. Wong
2019-04-23 20:51 ` [PATCH v2 " Darrick J. Wong
1 sibling, 1 reply; 38+ messages in thread
From: Eric Sandeen @ 2019-04-22 21:26 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 07/10] libxfs: refactor buffer item release code
2019-04-22 21:26 ` Eric Sandeen
@ 2019-04-22 21:35 ` Darrick J. Wong
2019-04-22 21:40 ` Eric Sandeen
0 siblings, 1 reply; 38+ messages in thread
From: Darrick J. Wong @ 2019-04-22 21:35 UTC (permalink / raw)
To: Eric Sandeen; +Cc: linux-xfs
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
> >
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 07/10] libxfs: refactor buffer item release code
2019-04-22 21:35 ` Darrick J. Wong
@ 2019-04-22 21:40 ` Eric Sandeen
0 siblings, 0 replies; 38+ messages in thread
From: Eric Sandeen @ 2019-04-22 21:40 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH v2 07/10] libxfs: refactor buffer item release code
2019-04-22 15:45 ` [PATCH 07/10] libxfs: refactor buffer item release code Darrick J. Wong
2019-04-22 21:26 ` Eric Sandeen
@ 2019-04-23 20:51 ` Darrick J. Wong
2019-04-23 20:56 ` Bill O'Donnell
1 sibling, 1 reply; 38+ messages in thread
From: Darrick J. Wong @ 2019-04-23 20:51 UTC (permalink / raw)
To: sandeen; +Cc: linux-xfs
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
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH v2 07/10] libxfs: refactor buffer item release code
2019-04-23 20:51 ` [PATCH v2 " Darrick J. Wong
@ 2019-04-23 20:56 ` Bill O'Donnell
0 siblings, 0 replies; 38+ messages in thread
From: Bill O'Donnell @ 2019-04-23 20:56 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
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
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 08/10] libxfs: don't touch buffer log item pointer when flushing inode log item
2019-04-22 15:44 [PATCH v3 00/10] xfsprogs-5.0: fix various problems Darrick J. Wong
` (6 preceding siblings ...)
2019-04-22 15:45 ` [PATCH 07/10] libxfs: refactor buffer item release code Darrick J. Wong
@ 2019-04-22 15:45 ` Darrick J. Wong
2019-04-23 17:56 ` Eric Sandeen
2019-04-23 20:52 ` Bill O'Donnell
2019-04-22 15:45 ` [PATCH 09/10] libxfs: fix buffer log item lifetime weirdness Darrick J. Wong
` (2 subsequent siblings)
10 siblings, 2 replies; 38+ messages in thread
From: Darrick J. Wong @ 2019-04-22 15:45 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
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);
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 08/10] libxfs: don't touch buffer log item pointer when flushing inode log item
2019-04-22 15:45 ` [PATCH 08/10] libxfs: don't touch buffer log item pointer when flushing inode log item Darrick J. Wong
@ 2019-04-23 17:56 ` Eric Sandeen
2019-04-23 20:52 ` Bill O'Donnell
1 sibling, 0 replies; 38+ messages in thread
From: Eric Sandeen @ 2019-04-23 17:56 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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);
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* Re: [PATCH 08/10] libxfs: don't touch buffer log item pointer when flushing inode log item
2019-04-22 15:45 ` [PATCH 08/10] libxfs: don't touch buffer log item pointer when flushing inode log item Darrick J. Wong
2019-04-23 17:56 ` Eric Sandeen
@ 2019-04-23 20:52 ` Bill O'Donnell
1 sibling, 0 replies; 38+ messages in thread
From: Bill O'Donnell @ 2019-04-23 20:52 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
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);
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 09/10] libxfs: fix buffer log item lifetime weirdness
2019-04-22 15:44 [PATCH v3 00/10] xfsprogs-5.0: fix various problems Darrick J. Wong
` (7 preceding siblings ...)
2019-04-22 15:45 ` [PATCH 08/10] libxfs: don't touch buffer log item pointer when flushing inode log item Darrick J. Wong
@ 2019-04-22 15:45 ` Darrick J. Wong
2019-04-23 21:15 ` Bill O'Donnell
2019-04-22 15:45 ` [PATCH 10/10] libxfs: shorten inode item lifetime Darrick J. Wong
2019-04-23 21:04 ` [PATCH 11/10] libfrog: fix memory leak in bitmap_free Darrick J. Wong
10 siblings, 1 reply; 38+ messages in thread
From: Darrick J. Wong @ 2019-04-22 15:45 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
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);
}
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 09/10] libxfs: fix buffer log item lifetime weirdness
2019-04-22 15:45 ` [PATCH 09/10] libxfs: fix buffer log item lifetime weirdness Darrick J. Wong
@ 2019-04-23 21:15 ` Bill O'Donnell
0 siblings, 0 replies; 38+ messages in thread
From: Bill O'Donnell @ 2019-04-23 21:15 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
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);
> }
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 10/10] libxfs: shorten inode item lifetime
2019-04-22 15:44 [PATCH v3 00/10] xfsprogs-5.0: fix various problems Darrick J. Wong
` (8 preceding siblings ...)
2019-04-22 15:45 ` [PATCH 09/10] libxfs: fix buffer log item lifetime weirdness Darrick J. Wong
@ 2019-04-22 15:45 ` Darrick J. Wong
2019-04-23 21:22 ` Bill O'Donnell
2019-04-23 21:04 ` [PATCH 11/10] libfrog: fix memory leak in bitmap_free Darrick J. Wong
10 siblings, 1 reply; 38+ messages in thread
From: Darrick J. Wong @ 2019-04-22 15:45 UTC (permalink / raw)
To: sandeen, darrick.wong; +Cc: linux-xfs
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 */
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 10/10] libxfs: shorten inode item lifetime
2019-04-22 15:45 ` [PATCH 10/10] libxfs: shorten inode item lifetime Darrick J. Wong
@ 2019-04-23 21:22 ` Bill O'Donnell
0 siblings, 0 replies; 38+ messages in thread
From: Bill O'Donnell @ 2019-04-23 21:22 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
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 */
>
^ permalink raw reply [flat|nested] 38+ messages in thread
* [PATCH 11/10] libfrog: fix memory leak in bitmap_free
2019-04-22 15:44 [PATCH v3 00/10] xfsprogs-5.0: fix various problems Darrick J. Wong
` (9 preceding siblings ...)
2019-04-22 15:45 ` [PATCH 10/10] libxfs: shorten inode item lifetime Darrick J. Wong
@ 2019-04-23 21:04 ` Darrick J. Wong
2019-04-23 21:23 ` Bill O'Donnell
10 siblings, 1 reply; 38+ messages in thread
From: Darrick J. Wong @ 2019-04-23 21:04 UTC (permalink / raw)
To: sandeen; +Cc: linux-xfs
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;
}
^ permalink raw reply related [flat|nested] 38+ messages in thread
* Re: [PATCH 11/10] libfrog: fix memory leak in bitmap_free
2019-04-23 21:04 ` [PATCH 11/10] libfrog: fix memory leak in bitmap_free Darrick J. Wong
@ 2019-04-23 21:23 ` Bill O'Donnell
0 siblings, 0 replies; 38+ messages in thread
From: Bill O'Donnell @ 2019-04-23 21:23 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: sandeen, linux-xfs
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;
> }
>
^ permalink raw reply [flat|nested] 38+ messages in thread