[PATCH 0/4] xfsprogs-4.17: tooling fixes

[PATCH 0/4] xfsprogs-4.17: tooling fixes

[Darrick J. Wong]

Hi all,

The first three patches of this series fix some deficiencies I found
when I tried to use xfsbuflock.py to diagnose deadlocks in generic/388.
The script didn't know that buffers are created locked, it didn't tell
you the line number of when a buffer got locked, and it got a little
confused about trylock.  So fix all three of those.

The fourth patch fixes another bashism in fsck.xfs.

This probably won't eat your data, and the branch[2] should apply
against for-next.

--D

[1] https://git.kernel.org/cgit/linux/kernel/git/djwong/xfsprogs-dev.git/log/?h=djwong-devel

[PATCH 1/4] xfs_buflock: ignore if buffer already locked

[Darrick J. Wong]

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

If the trace data says we ran trylock but we were already locked, don't
record another lock.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tools/xfsbuflock.py |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/tools/xfsbuflock.py b/tools/xfsbuflock.py
index 82b6e01f..cc15f582 100755
--- a/tools/xfsbuflock.py
+++ b/tools/xfsbuflock.py
@@ -87,7 +87,8 @@ class Buffer:
 		self.waiters = set()
 
 	def trylock(self, process, time):
-		self.lockdone(process, time)
+		if not self.locked:
+			self.lockdone(process, time)
 
 	def lockdone(self, process, time):
 		if self.locked:

[PATCH 2/4] xfs_buflock: record line number of trace where we locked the buffer

[Darrick J. Wong]

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

Enhance the debug output by reporting at which line in the trace output
we locked a particular buffer.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tools/xfsbuflock.py |    8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)


diff --git a/tools/xfsbuflock.py b/tools/xfsbuflock.py
index cc15f582..954f0954 100755
--- a/tools/xfsbuflock.py
+++ b/tools/xfsbuflock.py
@@ -85,6 +85,7 @@ class Buffer:
 		self.locktime = None
 		self.owner = None
 		self.waiters = set()
+		self.lockline = 0
 
 	def trylock(self, process, time):
 		if not self.locked:
@@ -92,7 +93,8 @@ class Buffer:
 
 	def lockdone(self, process, time):
 		if self.locked:
-			print('Buffer already locked on line %d?!' % nr)
+			print('Buffer 0x%x already locked at line %d? (line %d)' % \
+					(self.bno, self.lockline, nr))
 		#	process.dump()
 		#	self.dump()
 		#	assert False
@@ -101,6 +103,7 @@ class Buffer:
 		self.locked = True
 		self.owner = process
 		self.locktime = time
+		self.lockline = nr
 		process.locked_bufs.add(self)
 		process.bufs.add(self)
 		locked_buffers.add(self)
@@ -118,7 +121,8 @@ class Buffer:
 
 	def dump(self):
 		if self.owner is not None:
-			pid = '%s@%f' % (self.owner.pid, self.locktime)
+			pid = '%s@%f (line %d)' % \
+				(self.owner.pid, self.locktime, self.lockline)
 		else:
 			pid = ''
 		print('dev %s bno 0x%x nblks 0x%x lock %d owner %s' % \

[PATCH 3/4] xfs_buflock: record buffer initialization

[Darrick J. Wong]

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

Buffers are created locked, so we have to factor that into the buffer
state machine that the script utilizes.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 tools/xfsbuflock.py |   11 +++++++++++
 1 file changed, 11 insertions(+)


diff --git a/tools/xfsbuflock.py b/tools/xfsbuflock.py
index 954f0954..8f38f9f0 100755
--- a/tools/xfsbuflock.py
+++ b/tools/xfsbuflock.py
@@ -91,6 +91,13 @@ class Buffer:
 		if not self.locked:
 			self.lockdone(process, time)
 
+	def init(self, process, time):
+		# Buffers are initialized locked, but we could be allocating
+		# a surplus buffer while trying to grab a buffer that may or
+		# may not already exist.
+		if not self.locked:
+			self.lockdone(process, time)
+
 	def lockdone(self, process, time):
 		if self.locked:
 			print('Buffer 0x%x already locked at line %d? (line %d)' % \
@@ -183,6 +190,10 @@ for line in fileinput.input():
 		buf = getbuf(toks)
 		if buf is not None:
 			buf.trylock(proc, time)
+	elif fn == 'xfs_buf_init':
+		buf = getbuf(toks)
+		if buf is not None:
+			buf.init(proc, time)
 	elif fn == 'xfs_buf_item_unlock':
 		pass
 	else:

[PATCH 4/4] fsck: fix more bashisms

[Darrick J. Wong]

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

command -v is a bashism, so we need to get rid of it.  The shell returns
an error code of 127 if it couldn't invoke xfs_repair, so teach
repair2fsck_code to deal with this.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 fsck/xfs_fsck.sh |   12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)


diff --git a/fsck/xfs_fsck.sh b/fsck/xfs_fsck.sh
index 1916c07e..6af0f224 100755
--- a/fsck/xfs_fsck.sh
+++ b/fsck/xfs_fsck.sh
@@ -20,6 +20,10 @@ repair2fsck_code() {
 		;;
 	4)  return 1 # The fs has been fixed
 		;;
+	127)
+		echo "$NAME error: xfs_repair was not found!" 1>&2
+		return 4
+		;;
 	*)  echo "$NAME error: An unknown return code from xfs_repair '$1'" 1>&2
 		return 4 # something went wrong with xfs_repair
 	esac
@@ -59,13 +63,7 @@ if [ -n "$PS1" -o -t 0 ]; then
 fi
 
 if $FORCE; then
-	XFS_REPAIR=`command -v xfs_repair`
-	if [ ! -x "$XFS_REPAIR" ] ; then
-		echo "$NAME error: xfs_repair was not found!" 1>&2
-		exit 4
-	fi
-
-	$XFS_REPAIR -e $DEV
+	xfs_repair -e $DEV
 	repair2fsck_code $?
 	exit $?
 fi

Re: [PATCH 4/4] fsck: fix more bashisms

[Eric Sandeen]

On 5/8/18 11:37 AM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> command -v is a bashism, so we need to get rid of it.  The shell returns
> an error code of 127 if it couldn't invoke xfs_repair, so teach
> repair2fsck_code to deal with this.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

seems straightforward, thanks.

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

> ---
>  fsck/xfs_fsck.sh |   12 +++++-------
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> 
> diff --git a/fsck/xfs_fsck.sh b/fsck/xfs_fsck.sh
> index 1916c07e..6af0f224 100755
> --- a/fsck/xfs_fsck.sh
> +++ b/fsck/xfs_fsck.sh
> @@ -20,6 +20,10 @@ repair2fsck_code() {
>  		;;
>  	4)  return 1 # The fs has been fixed
>  		;;
> +	127)
> +		echo "$NAME error: xfs_repair was not found!" 1>&2
> +		return 4
> +		;;
>  	*)  echo "$NAME error: An unknown return code from xfs_repair '$1'" 1>&2
>  		return 4 # something went wrong with xfs_repair
>  	esac
> @@ -59,13 +63,7 @@ if [ -n "$PS1" -o -t 0 ]; then
>  fi
>  
>  if $FORCE; then
> -	XFS_REPAIR=`command -v xfs_repair`
> -	if [ ! -x "$XFS_REPAIR" ] ; then
> -		echo "$NAME error: xfs_repair was not found!" 1>&2
> -		exit 4
> -	fi
> -
> -	$XFS_REPAIR -e $DEV
> +	xfs_repair -e $DEV
>  	repair2fsck_code $?
>  	exit $?
>  fi
> 

[PATCH 5/4] xfs_scrub: actually check for errors coming from close()

[Darrick J. Wong]

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

Report errors reported by close().

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 scrub/phase1.c |    6 +++++-
 scrub/phase3.c |   27 +++++++++++++++++++++++++--
 scrub/phase5.c |    8 ++++++--
 scrub/phase6.c |   10 +++++++---
 scrub/vfs.c    |    4 +++-
 5 files changed, 46 insertions(+), 9 deletions(-)

diff --git a/scrub/phase1.c b/scrub/phase1.c
index c2b9067a..87847259 100644
--- a/scrub/phase1.c
+++ b/scrub/phase1.c
@@ -60,6 +60,8 @@ bool
 xfs_cleanup_fs(
 	struct scrub_ctx	*ctx)
 {
+	int			error;
+
 	if (ctx->fshandle)
 		free_handle(ctx->fshandle, ctx->fshandle_len);
 	if (ctx->rtdev)
@@ -69,7 +71,9 @@ xfs_cleanup_fs(
 	if (ctx->datadev)
 		disk_close(ctx->datadev);
 	fshandle_destroy();
-	close(ctx->mnt_fd);
+	error = close(ctx->mnt_fd);
+	if (error)
+		str_errno(ctx, _("closing mountpoint fd"));
 	fs_table_destroy();
 
 	return true;
diff --git a/scrub/phase3.c b/scrub/phase3.c
index 68c95e67..cbaa80ba 100644
--- a/scrub/phase3.c
+++ b/scrub/phase3.c
@@ -53,6 +53,25 @@ struct scrub_inode_ctx {
 	bool			moveon;
 };
 
+/* Report a filesystem error that the vfs fed us on close. */
+static void
+xfs_scrub_inode_vfs_error(
+	struct scrub_ctx	*ctx,
+	struct xfs_bstat	*bstat)
+{
+	char			descr[DESCR_BUFSZ];
+	xfs_agnumber_t		agno;
+	xfs_agino_t		agino;
+	int			old_errno = errno;
+
+	agno = bstat->bs_ino / (1ULL << (ctx->inopblog + ctx->agblklog));
+	agino = bstat->bs_ino % (1ULL << (ctx->inopblog + ctx->agblklog));
+	snprintf(descr, DESCR_BUFSZ, _("inode %"PRIu64" (%u/%u)"),
+			(uint64_t)bstat->bs_ino, agno, agino);
+	errno = old_errno;
+	str_errno(ctx, descr);
+}
+
 /* Verify the contents, xattrs, and extent maps of an inode. */
 static int
 xfs_scrub_inode(
@@ -65,6 +84,7 @@ xfs_scrub_inode(
 	struct ptcounter	*icount = ictx->icount;
 	bool			moveon = true;
 	int			fd = -1;
+	int			error;
 
 	background_sleep();
 
@@ -116,8 +136,11 @@ xfs_scrub_inode(
 out:
 	ptcounter_add(icount, 1);
 	progress_add(1);
-	if (fd >= 0)
-		close(fd);
+	if (fd >= 0) {
+		error = close(fd);
+		if (error)
+			xfs_scrub_inode_vfs_error(ctx, bstat);
+	}
 	if (!moveon)
 		ictx->moveon = false;
 	return ictx->moveon ? 0 : XFS_ITERATE_INODES_ABORT;
diff --git a/scrub/phase5.c b/scrub/phase5.c
index 01038f77..456f38e2 100644
--- a/scrub/phase5.c
+++ b/scrub/phase5.c
@@ -250,6 +250,7 @@ xfs_scrub_connections(
 	xfs_agnumber_t		agno;
 	xfs_agino_t		agino;
 	int			fd = -1;
+	int			error;
 
 	agno = bstat->bs_ino / (1ULL << (ctx->inopblog + ctx->agblklog));
 	agino = bstat->bs_ino % (1ULL << (ctx->inopblog + ctx->agblklog));
@@ -285,8 +286,11 @@ xfs_scrub_connections(
 
 out:
 	progress_add(1);
-	if (fd >= 0)
-		close(fd);
+	if (fd >= 0) {
+		error = close(fd);
+		if (error)
+			str_errno(ctx, descr);
+	}
 	if (!moveon)
 		*pmoveon = false;
 	return *pmoveon ? 0 : XFS_ITERATE_INODES_ABORT;
diff --git a/scrub/phase6.c b/scrub/phase6.c
index b533cbbd..26540155 100644
--- a/scrub/phase6.c
+++ b/scrub/phase6.c
@@ -212,7 +212,9 @@ _("Disappeared during read error reporting."));
 
 	/* Go find the badness. */
 	moveon = xfs_report_verify_fd(ctx, descr, fd, arg);
-	close(fd);
+	error = close(fd);
+	if (error)
+		str_errno(ctx, descr);
 
 	return moveon ? 0 : XFS_ITERATE_INODES_ABORT;
 }
@@ -243,6 +245,7 @@ xfs_report_verify_dirent(
 {
 	bool			moveon;
 	int			fd;
+	int			error;
 
 	/* Ignore things we can't open. */
 	if (!S_ISREG(sb->st_mode) && !S_ISDIR(sb->st_mode))
@@ -268,8 +271,9 @@ xfs_report_verify_dirent(
 		goto out;
 
 out:
-	close(fd);
-
+	error = close(fd);
+	if (error)
+		str_errno(ctx, path);
 	return moveon;
 }
 
diff --git a/scrub/vfs.c b/scrub/vfs.c
index cfb58782..77df2874 100644
--- a/scrub/vfs.c
+++ b/scrub/vfs.c
@@ -86,7 +86,9 @@ scan_fs_dir(
 	/* Caller-specific directory checks. */
 	if (!sft->dir_fn(ctx, sftd->path, dir_fd, sft->arg)) {
 		sft->moveon = false;
-		close(dir_fd);
+		error = close(dir_fd);
+		if (error)
+			str_errno(ctx, sftd->path);
 		goto out;
 	}