[PATCH 0/7] xfsprogs-4.17: last of the fixes

[PATCH 0/7] xfsprogs-4.17: last of the fixes

[Darrick J. Wong]

Hi all,

The first patch fixes a Coverity bug that was introduced in the latest
push of for-next.

Then there are three patches to 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.

The fifth patch removes crc32 support (not crc32c support) which is not
used anywhere in xfs.  This reduces text segment size by 8K in all
programs.

--D

[PATCH 1/7] xfs_repair: fix integer handling issues

[Darrick J. Wong]

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

When we shift sb_logblocks to the right we need to ensure that we have
enough storage space to shift correctly.  Cast logblocks to a 64-bit
type so that we don't screw up the check.

Coverity-id: 1435810
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 repair/sb.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


diff --git a/repair/sb.c b/repair/sb.c
index ef44e39c..543200f7 100644
--- a/repair/sb.c
+++ b/repair/sb.c
@@ -313,7 +313,8 @@ verify_sb_loginfo(
 	if (xfs_sb_version_hascrc(sb) &&
 	    (sb->sb_logblocks == 0 ||
 	     sb->sb_logblocks > XFS_MAX_LOG_BLOCKS ||
-	     (sb->sb_logblocks << sb->sb_blocklog) > XFS_MAX_LOG_BYTES))
+	     ((unsigned long long)sb->sb_logblocks << sb->sb_blocklog) >
+	     XFS_MAX_LOG_BYTES))
 		return false;
 
 	if (sb->sb_logsunit > 1 && sb->sb_logsunit % sb->sb_blocksize)

Re: [PATCH 1/7] xfs_repair: fix integer handling issues

[Allison Henderson]

Looks ok to me.
Reviewed by: Allison Henderson <allison.henderson@oracle.com>

On 05/25/2018 03:11 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we shift sb_logblocks to the right we need to ensure that we have
> enough storage space to shift correctly.  Cast logblocks to a 64-bit
> type so that we don't screw up the check.
> 
> Coverity-id: 1435810
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   repair/sb.c |    3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/repair/sb.c b/repair/sb.c
> index ef44e39c..543200f7 100644
> --- a/repair/sb.c
> +++ b/repair/sb.c
> @@ -313,7 +313,8 @@ verify_sb_loginfo(
>   	if (xfs_sb_version_hascrc(sb) &&
>   	    (sb->sb_logblocks == 0 ||
>   	     sb->sb_logblocks > XFS_MAX_LOG_BLOCKS ||
> -	     (sb->sb_logblocks << sb->sb_blocklog) > XFS_MAX_LOG_BYTES))
> +	     ((unsigned long long)sb->sb_logblocks << sb->sb_blocklog) >
> +	     XFS_MAX_LOG_BYTES))
>   		return false;
>   
>   	if (sb->sb_logsunit > 1 && sb->sb_logsunit % sb->sb_blocksize)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=VfFiG-M8c2_2RI75jbfn7wWPm8FSu5Sdve4CVE9qM28&s=KCxhdbGZuRnZDLgbz2McNDDVl_f6-6DFW-Qj_vqgDKk&e=
> 

Re: [PATCH 1/7] xfs_repair: fix integer handling issues

[Carlos Maiolino]

On Fri, May 25, 2018 at 03:11:43PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> When we shift sb_logblocks to the right we need to ensure that we have
Unless I'm missing something, it should be "to the left"?

Otherwise:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>

> enough storage space to shift correctly.  Cast logblocks to a 64-bit
> type so that we don't screw up the check.

> 
> Coverity-id: 1435810
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  repair/sb.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> 
> diff --git a/repair/sb.c b/repair/sb.c
> index ef44e39c..543200f7 100644
> --- a/repair/sb.c
> +++ b/repair/sb.c
> @@ -313,7 +313,8 @@ verify_sb_loginfo(
>  	if (xfs_sb_version_hascrc(sb) &&
>  	    (sb->sb_logblocks == 0 ||
>  	     sb->sb_logblocks > XFS_MAX_LOG_BLOCKS ||
> -	     (sb->sb_logblocks << sb->sb_blocklog) > XFS_MAX_LOG_BYTES))
> +	     ((unsigned long long)sb->sb_logblocks << sb->sb_blocklog) >
> +	     XFS_MAX_LOG_BYTES))
>  		return false;
>  
>  	if (sb->sb_logsunit > 1 && sb->sb_logsunit % sb->sb_blocksize)
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos

Re: [PATCH 1/7] xfs_repair: fix integer handling issues

[Darrick J. Wong]

On Mon, May 28, 2018 at 03:56:08PM +0200, Carlos Maiolino wrote:
> On Fri, May 25, 2018 at 03:11:43PM -0700, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > When we shift sb_logblocks to the right we need to ensure that we have
> Unless I'm missing something, it should be "to the left"?

Oops, yes, s/right/left/.

--D

> Otherwise:
> 
> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> 
> > enough storage space to shift correctly.  Cast logblocks to a 64-bit
> > type so that we don't screw up the check.
> 
> > 
> > Coverity-id: 1435810
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  repair/sb.c |    3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > 
> > diff --git a/repair/sb.c b/repair/sb.c
> > index ef44e39c..543200f7 100644
> > --- a/repair/sb.c
> > +++ b/repair/sb.c
> > @@ -313,7 +313,8 @@ verify_sb_loginfo(
> >  	if (xfs_sb_version_hascrc(sb) &&
> >  	    (sb->sb_logblocks == 0 ||
> >  	     sb->sb_logblocks > XFS_MAX_LOG_BLOCKS ||
> > -	     (sb->sb_logblocks << sb->sb_blocklog) > XFS_MAX_LOG_BYTES))
> > +	     ((unsigned long long)sb->sb_logblocks << sb->sb_blocklog) >
> > +	     XFS_MAX_LOG_BYTES))
> >  		return false;
> >  
> >  	if (sb->sb_logsunit > 1 && sb->sb_logsunit % sb->sb_blocksize)
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Carlos
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 1/7] xfs_repair: fix integer handling issues

[Darrick J. Wong]

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

When we shift sb_logblocks to the left we need to ensure that we have
enough storage space to shift correctly.  Cast logblocks to a 64-bit
type so that we don't screw up the check.

Coverity-id: 1435810
Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
Reviewed-by: Allison Henderson <allison.henderson@oracle.com>
Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
---
v2: fix commit message
---
 repair/sb.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/repair/sb.c b/repair/sb.c
index ef44e39c..543200f7 100644
--- a/repair/sb.c
+++ b/repair/sb.c
@@ -313,7 +313,8 @@ verify_sb_loginfo(
 	if (xfs_sb_version_hascrc(sb) &&
 	    (sb->sb_logblocks == 0 ||
 	     sb->sb_logblocks > XFS_MAX_LOG_BLOCKS ||
-	     (sb->sb_logblocks << sb->sb_blocklog) > XFS_MAX_LOG_BYTES))
+	     ((unsigned long long)sb->sb_logblocks << sb->sb_blocklog) >
+	     XFS_MAX_LOG_BYTES))
 		return false;
 
 	if (sb->sb_logsunit > 1 && sb->sb_logsunit % sb->sb_blocksize)

[PATCH 2/7] 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:

Re: [PATCH 2/7] xfs_buflock: ignore if buffer already locked

[Allison Henderson]

Looks ok:
Reviewed by: Allison Henderson <allison.henderson@oracle.com>

On 05/25/2018 03:11 PM, Darrick J. Wong wrote:
> 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:
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=8vuusUIByYR12ltQmXnO0mMuejRO5YBnWt7UmuNa7xI&s=0Vksri2Xw7b7Ozg6v-k-gqsXg6ESgtqgviNx2HQgSuE&e=
> 

[PATCH 3/7] 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' % \

Re: [PATCH 3/7] xfs_buflock: record line number of trace where we locked the buffer

[Allison Henderson]

Looks ok:
Reviewed by: Allison Henderson <allison.henderson@oracle.com>

On 05/25/2018 03:11 PM, Darrick J. Wong wrote:
> 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' % \
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=A4tW0F_UCH_YAKW12KzczzEcpxJELFgLQG594Ox2rhM&s=6yKJqQwtIKgwri-M_jFoU5ujldLBzdDVf_JJ3QxNi3o&e=
> 

[PATCH 4/7] 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:

Re: [PATCH 4/7] xfs_buflock: record buffer initialization

[Allison Henderson]

Looks ok:
Reviewed by: Allison Henderson <allison.henderson@oracle.com>

On 05/25/2018 03:12 PM, Darrick J. Wong wrote:
> 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:
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=ZceT9igTX4NtDB5x4zRNsz3TmN4D9IUS-PIL2AEkKxI&s=VCNmW1CRDq3QALM6L6wQiTmHHrEeGSZVf4H7GUBQG2w&e=
> 

[PATCH 5/7] 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 5/7] fsck: fix more bashisms

[Allison Henderson]

Alrighty, you can add my review
Reviewed by: Allison Henderson <allison.henderson@oracle.com>

On 05/25/2018 03:12 PM, 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>
> ---
>   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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=esB0ntIPOKHhY_lkqtduLsdDYUrE4GycWhxY5h4WJLo&s=rMUHX1OjhARCGXqcRie_xKoEdOlq1hooycaZJzLEqX0&e=
> 

Re: [PATCH 5/7] fsck: fix more bashisms

[Carlos Maiolino]

On Fri, May 25, 2018 at 03:12:07PM -0700, 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>

Reviewed-by: Carlos Maiolino <cmaiolino@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
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos

[PATCH 6/7] 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;
 	}
 

Re: [PATCH 6/7] xfs_scrub: actually check for errors coming from close()

[Allison Henderson]

Looks ok:
Reviewed by: Allison Henderson <allison.henderson@oracle.com>

On 05/25/2018 03:12 PM, Darrick J. Wong wrote:
> 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;
>   	}
>   
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=QEHg-4gJOhupzORWTApyuKuBTYX6Sum0GqFWPnC_mCo&s=QA446VHazWxqoc7nECcFGE-upy-amurTz6LUSuZURGg&e=
> 

Re: [PATCH 6/7] xfs_scrub: actually check for errors coming from close()

[Carlos Maiolino]

On Fri, May 25, 2018 at 03:12:14PM -0700, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Report errors reported by close().
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.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;
>  	}
>  
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos

[PATCH 7/7] libxfs: remove crc32 functions

[Darrick J. Wong]

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

XFS uses crc32c, not crc32.  Remove the unnecessary crc32 code, which
decreases binary size by the 8K crc32 table.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 include/libxfs.h        |    3 ---
 libxfs/crc32.c          |   16 ++++++++++------
 libxfs/gen_crc32table.c |    8 +++++++-
 libxfs/libxfs_priv.h    |    3 ---
 4 files changed, 17 insertions(+), 13 deletions(-)


diff --git a/include/libxfs.h b/include/libxfs.h
index fbaae089..109866de 100644
--- a/include/libxfs.h
+++ b/include/libxfs.h
@@ -43,10 +43,7 @@
 
 
 /* CRC stuff, buffer API dependent on it */
-extern uint32_t crc32_le(uint32_t crc, unsigned char const *p, size_t len);
 extern uint32_t crc32c_le(uint32_t crc, unsigned char const *p, size_t len);
-
-#define crc32(c,p,l)	crc32_le((c),(unsigned char const *)(p),(l))
 #define crc32c(c,p,l)	crc32c_le((c),(unsigned char const *)(p),(l))
 
 #include "xfs_cksum.h"
diff --git a/libxfs/crc32.c b/libxfs/crc32.c
index 783d62e9..8fe5c42c 100644
--- a/libxfs/crc32.c
+++ b/libxfs/crc32.c
@@ -176,20 +176,26 @@ static inline u32 __pure crc32_le_generic(u32 crc, unsigned char const *p,
 }
 
 #if CRC_LE_BITS == 1
+/*
+ * not used by xfs.
 u32 __pure crc32_le(u32 crc, unsigned char const *p, size_t len)
 {
 	return crc32_le_generic(crc, p, len, NULL, CRCPOLY_LE);
 }
+ */
 u32 __pure crc32c_le(u32 crc, unsigned char const *p, size_t len)
 {
 	return crc32_le_generic(crc, p, len, NULL, CRC32C_POLY_LE);
 }
 #else
+/*
+ * not used by xfs.
 u32 __pure crc32_le(u32 crc, unsigned char const *p, size_t len)
 {
 	return crc32_le_generic(crc, p, len,
 			(const u32 (*)[256])crc32table_le, CRCPOLY_LE);
 }
+ */
 u32 __pure crc32c_le(u32 crc, unsigned char const *p, size_t len)
 {
 	return crc32_le_generic(crc, p, len,
@@ -970,6 +976,7 @@ static int crc32c_test(void)
 	return errors;
 }
 
+#if 0	/* not used by xfs */
 static int crc32_test(void)
 {
 	int i;
@@ -989,10 +996,8 @@ static int crc32_test(void)
 		crc ^= crc32_le(test[i].crc, test_buf +
 		    test[i].start, test[i].length);
 
-#if 0 /* not used */
 		crc ^= crc32_be(test[i].crc, test_buf +
 		    test[i].start, test[i].length);
-#endif
 	}
 
 	gettimeofday(&start, NULL);
@@ -1001,11 +1006,9 @@ static int crc32_test(void)
 		    test[i].start, test[i].length))
 			errors++;
 
-#if 0 /* not used */
 		if (test[i].crc_be != crc32_be(test[i].crc, test_buf +
 		    test[i].start, test[i].length))
 			errors++;
-#endif
 	}
 	gettimeofday(&stop, NULL);
 
@@ -1021,6 +1024,8 @@ static int crc32_test(void)
 
 	return errors;
 }
+#endif
+
 /*
  * make sure we always return 0 for a successful test run, and non-zero for a
  * failed run. The build infrastructure is looking for this information to
@@ -1032,8 +1037,7 @@ int main(int argc, char **argv)
 
 	printf("CRC_LE_BITS = %d\n", CRC_LE_BITS);
 
-	errors = crc32_test();
-	errors += crc32c_test();
+	errors = crc32c_test();
 
 	return errors != 0;
 }
diff --git a/libxfs/gen_crc32table.c b/libxfs/gen_crc32table.c
index 574a2d1a..d81164b7 100644
--- a/libxfs/gen_crc32table.c
+++ b/libxfs/gen_crc32table.c
@@ -20,7 +20,10 @@
 # define BE_TABLE_SIZE (1 << CRC_BE_BITS)
 #endif
 
+/*
+ * crc32 not used by xfs.
 static uint32_t crc32table_le[LE_TABLE_ROWS][256];
+ */
 static uint32_t crc32ctable_le[LE_TABLE_ROWS][256];
 
 /*
@@ -57,10 +60,13 @@ static void crc32init_le_generic(const uint32_t polynomial,
 	}
 }
 
+/*
+ * crc32 not used by xfs.
 static void crc32init_le(void)
 {
 	crc32init_le_generic(CRCPOLY_LE, crc32table_le);
 }
+ */
 
 static void crc32cinit_le(void)
 {
@@ -112,6 +118,7 @@ int main(int argc, char** argv)
 {
 	printf("/* this file is generated - do not edit */\n\n");
 
+#if 0	/* not used by xfsprogs */
 	if (CRC_LE_BITS > 1) {
 		crc32init_le();
 		printf("static u32 crc32table_le[%d][%d] = {",
@@ -121,7 +128,6 @@ int main(int argc, char** argv)
 		printf("};\n");
 	}
 
-#if 0	/* not used by xfsprogs */
 	if (CRC_BE_BITS > 1) {
 		crc32init_be();
 		printf("static u32 crc32table_be[%d][%d] = {",
diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
index e5eb3de1..fae32411 100644
--- a/libxfs/libxfs_priv.h
+++ b/libxfs/libxfs_priv.h
@@ -67,10 +67,7 @@
 #include "xfs_fs.h"
 
 /* CRC stuff, buffer API dependent on it */
-extern uint32_t crc32_le(uint32_t crc, unsigned char const *p, size_t len);
 extern uint32_t crc32c_le(uint32_t crc, unsigned char const *p, size_t len);
-
-#define crc32(c,p,l)	crc32_le((c),(unsigned char const *)(p),(l))
 #define crc32c(c,p,l)	crc32c_le((c),(unsigned char const *)(p),(l))
 
 #include "xfs_cksum.h"

Re: [PATCH 7/7] libxfs: remove crc32 functions

[Allison Henderson]

Seems like it would be ok.  But if these functions are not really used,
is there a reason as to why we are just commenting them out instead of 
removing them entirely?  Thx!

Allison

On 05/25/2018 03:12 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> XFS uses crc32c, not crc32.  Remove the unnecessary crc32 code, which
> decreases binary size by the 8K crc32 table.
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>   include/libxfs.h        |    3 ---
>   libxfs/crc32.c          |   16 ++++++++++------
>   libxfs/gen_crc32table.c |    8 +++++++-
>   libxfs/libxfs_priv.h    |    3 ---
>   4 files changed, 17 insertions(+), 13 deletions(-)
> 
> 
> diff --git a/include/libxfs.h b/include/libxfs.h
> index fbaae089..109866de 100644
> --- a/include/libxfs.h
> +++ b/include/libxfs.h
> @@ -43,10 +43,7 @@
>   
>   
>   /* CRC stuff, buffer API dependent on it */
> -extern uint32_t crc32_le(uint32_t crc, unsigned char const *p, size_t len);
>   extern uint32_t crc32c_le(uint32_t crc, unsigned char const *p, size_t len);
> -
> -#define crc32(c,p,l)	crc32_le((c),(unsigned char const *)(p),(l))
>   #define crc32c(c,p,l)	crc32c_le((c),(unsigned char const *)(p),(l))
>   
>   #include "xfs_cksum.h"
> diff --git a/libxfs/crc32.c b/libxfs/crc32.c
> index 783d62e9..8fe5c42c 100644
> --- a/libxfs/crc32.c
> +++ b/libxfs/crc32.c
> @@ -176,20 +176,26 @@ static inline u32 __pure crc32_le_generic(u32 crc, unsigned char const *p,
>   }
>   
>   #if CRC_LE_BITS == 1
> +/*
> + * not used by xfs.
>   u32 __pure crc32_le(u32 crc, unsigned char const *p, size_t len)
>   {
>   	return crc32_le_generic(crc, p, len, NULL, CRCPOLY_LE);
>   }
> + */
>   u32 __pure crc32c_le(u32 crc, unsigned char const *p, size_t len)
>   {
>   	return crc32_le_generic(crc, p, len, NULL, CRC32C_POLY_LE);
>   }
>   #else
> +/*
> + * not used by xfs.
>   u32 __pure crc32_le(u32 crc, unsigned char const *p, size_t len)
>   {
>   	return crc32_le_generic(crc, p, len,
>   			(const u32 (*)[256])crc32table_le, CRCPOLY_LE);
>   }
> + */
>   u32 __pure crc32c_le(u32 crc, unsigned char const *p, size_t len)
>   {
>   	return crc32_le_generic(crc, p, len,
> @@ -970,6 +976,7 @@ static int crc32c_test(void)
>   	return errors;
>   }
>   
> +#if 0	/* not used by xfs */
>   static int crc32_test(void)
>   {
>   	int i;
> @@ -989,10 +996,8 @@ static int crc32_test(void)
>   		crc ^= crc32_le(test[i].crc, test_buf +
>   		    test[i].start, test[i].length);
>   
> -#if 0 /* not used */
>   		crc ^= crc32_be(test[i].crc, test_buf +
>   		    test[i].start, test[i].length);
> -#endif
>   	}
>   
>   	gettimeofday(&start, NULL);
> @@ -1001,11 +1006,9 @@ static int crc32_test(void)
>   		    test[i].start, test[i].length))
>   			errors++;
>   
> -#if 0 /* not used */
>   		if (test[i].crc_be != crc32_be(test[i].crc, test_buf +
>   		    test[i].start, test[i].length))
>   			errors++;
> -#endif
>   	}
>   	gettimeofday(&stop, NULL);
>   
> @@ -1021,6 +1024,8 @@ static int crc32_test(void)
>   
>   	return errors;
>   }
> +#endif
> +
>   /*
>    * make sure we always return 0 for a successful test run, and non-zero for a
>    * failed run. The build infrastructure is looking for this information to
> @@ -1032,8 +1037,7 @@ int main(int argc, char **argv)
>   
>   	printf("CRC_LE_BITS = %d\n", CRC_LE_BITS);
>   
> -	errors = crc32_test();
> -	errors += crc32c_test();
> +	errors = crc32c_test();
>   
>   	return errors != 0;
>   }
> diff --git a/libxfs/gen_crc32table.c b/libxfs/gen_crc32table.c
> index 574a2d1a..d81164b7 100644
> --- a/libxfs/gen_crc32table.c
> +++ b/libxfs/gen_crc32table.c
> @@ -20,7 +20,10 @@
>   # define BE_TABLE_SIZE (1 << CRC_BE_BITS)
>   #endif
>   
> +/*
> + * crc32 not used by xfs.
>   static uint32_t crc32table_le[LE_TABLE_ROWS][256];
> + */
>   static uint32_t crc32ctable_le[LE_TABLE_ROWS][256];
>   
>   /*
> @@ -57,10 +60,13 @@ static void crc32init_le_generic(const uint32_t polynomial,
>   	}
>   }
>   
> +/*
> + * crc32 not used by xfs.
>   static void crc32init_le(void)
>   {
>   	crc32init_le_generic(CRCPOLY_LE, crc32table_le);
>   }
> + */
>   
>   static void crc32cinit_le(void)
>   {
> @@ -112,6 +118,7 @@ int main(int argc, char** argv)
>   {
>   	printf("/* this file is generated - do not edit */\n\n");
>   
> +#if 0	/* not used by xfsprogs */
>   	if (CRC_LE_BITS > 1) {
>   		crc32init_le();
>   		printf("static u32 crc32table_le[%d][%d] = {",
> @@ -121,7 +128,6 @@ int main(int argc, char** argv)
>   		printf("};\n");
>   	}
>   
> -#if 0	/* not used by xfsprogs */
>   	if (CRC_BE_BITS > 1) {
>   		crc32init_be();
>   		printf("static u32 crc32table_be[%d][%d] = {",
> diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
> index e5eb3de1..fae32411 100644
> --- a/libxfs/libxfs_priv.h
> +++ b/libxfs/libxfs_priv.h
> @@ -67,10 +67,7 @@
>   #include "xfs_fs.h"
>   
>   /* CRC stuff, buffer API dependent on it */
> -extern uint32_t crc32_le(uint32_t crc, unsigned char const *p, size_t len);
>   extern uint32_t crc32c_le(uint32_t crc, unsigned char const *p, size_t len);
> -
> -#define crc32(c,p,l)	crc32_le((c),(unsigned char const *)(p),(l))
>   #define crc32c(c,p,l)	crc32c_le((c),(unsigned char const *)(p),(l))
>   
>   #include "xfs_cksum.h"
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=UTANtlZSRetfnz4BmXOr1fXJ6ePKjcWFnaB8-d9hRFk&s=SKcuMvUHSCQzGlRx1lPp-Hym6boKRBUhcRY7YNsWRQo&e=
> 

Re: [PATCH 7/7] libxfs: remove crc32 functions

[Carlos Maiolino]

On Sun, May 27, 2018 at 08:53:04PM -0700, Allison Henderson wrote:
> Seems like it would be ok.  But if these functions are not really used,
> is there a reason as to why we are just commenting them out instead of
> removing them entirely?  Thx!

I've the same question, I'd say you expect to keep them there for historical
reasons, but I think at least a comment in the patch description would be fine,
but either if you keep the commented code, or remove it completely:

Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>


> 
> Allison
> 
> On 05/25/2018 03:12 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > XFS uses crc32c, not crc32.  Remove the unnecessary crc32 code, which
> > decreases binary size by the 8K crc32 table.
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >   include/libxfs.h        |    3 ---
> >   libxfs/crc32.c          |   16 ++++++++++------
> >   libxfs/gen_crc32table.c |    8 +++++++-
> >   libxfs/libxfs_priv.h    |    3 ---
> >   4 files changed, 17 insertions(+), 13 deletions(-)
> > 
> > 
> > diff --git a/include/libxfs.h b/include/libxfs.h
> > index fbaae089..109866de 100644
> > --- a/include/libxfs.h
> > +++ b/include/libxfs.h
> > @@ -43,10 +43,7 @@
> >   /* CRC stuff, buffer API dependent on it */
> > -extern uint32_t crc32_le(uint32_t crc, unsigned char const *p, size_t len);
> >   extern uint32_t crc32c_le(uint32_t crc, unsigned char const *p, size_t len);
> > -
> > -#define crc32(c,p,l)	crc32_le((c),(unsigned char const *)(p),(l))
> >   #define crc32c(c,p,l)	crc32c_le((c),(unsigned char const *)(p),(l))
> >   #include "xfs_cksum.h"
> > diff --git a/libxfs/crc32.c b/libxfs/crc32.c
> > index 783d62e9..8fe5c42c 100644
> > --- a/libxfs/crc32.c
> > +++ b/libxfs/crc32.c
> > @@ -176,20 +176,26 @@ static inline u32 __pure crc32_le_generic(u32 crc, unsigned char const *p,
> >   }
> >   #if CRC_LE_BITS == 1
> > +/*
> > + * not used by xfs.
> >   u32 __pure crc32_le(u32 crc, unsigned char const *p, size_t len)
> >   {
> >   	return crc32_le_generic(crc, p, len, NULL, CRCPOLY_LE);
> >   }
> > + */
> >   u32 __pure crc32c_le(u32 crc, unsigned char const *p, size_t len)
> >   {
> >   	return crc32_le_generic(crc, p, len, NULL, CRC32C_POLY_LE);
> >   }
> >   #else
> > +/*
> > + * not used by xfs.
> >   u32 __pure crc32_le(u32 crc, unsigned char const *p, size_t len)
> >   {
> >   	return crc32_le_generic(crc, p, len,
> >   			(const u32 (*)[256])crc32table_le, CRCPOLY_LE);
> >   }
> > + */
> >   u32 __pure crc32c_le(u32 crc, unsigned char const *p, size_t len)
> >   {
> >   	return crc32_le_generic(crc, p, len,
> > @@ -970,6 +976,7 @@ static int crc32c_test(void)
> >   	return errors;
> >   }
> > +#if 0	/* not used by xfs */
> >   static int crc32_test(void)
> >   {
> >   	int i;
> > @@ -989,10 +996,8 @@ static int crc32_test(void)
> >   		crc ^= crc32_le(test[i].crc, test_buf +
> >   		    test[i].start, test[i].length);
> > -#if 0 /* not used */
> >   		crc ^= crc32_be(test[i].crc, test_buf +
> >   		    test[i].start, test[i].length);
> > -#endif
> >   	}
> >   	gettimeofday(&start, NULL);
> > @@ -1001,11 +1006,9 @@ static int crc32_test(void)
> >   		    test[i].start, test[i].length))
> >   			errors++;
> > -#if 0 /* not used */
> >   		if (test[i].crc_be != crc32_be(test[i].crc, test_buf +
> >   		    test[i].start, test[i].length))
> >   			errors++;
> > -#endif
> >   	}
> >   	gettimeofday(&stop, NULL);
> > @@ -1021,6 +1024,8 @@ static int crc32_test(void)
> >   	return errors;
> >   }
> > +#endif
> > +
> >   /*
> >    * make sure we always return 0 for a successful test run, and non-zero for a
> >    * failed run. The build infrastructure is looking for this information to
> > @@ -1032,8 +1037,7 @@ int main(int argc, char **argv)
> >   	printf("CRC_LE_BITS = %d\n", CRC_LE_BITS);
> > -	errors = crc32_test();
> > -	errors += crc32c_test();
> > +	errors = crc32c_test();
> >   	return errors != 0;
> >   }
> > diff --git a/libxfs/gen_crc32table.c b/libxfs/gen_crc32table.c
> > index 574a2d1a..d81164b7 100644
> > --- a/libxfs/gen_crc32table.c
> > +++ b/libxfs/gen_crc32table.c
> > @@ -20,7 +20,10 @@
> >   # define BE_TABLE_SIZE (1 << CRC_BE_BITS)
> >   #endif
> > +/*
> > + * crc32 not used by xfs.
> >   static uint32_t crc32table_le[LE_TABLE_ROWS][256];
> > + */
> >   static uint32_t crc32ctable_le[LE_TABLE_ROWS][256];
> >   /*
> > @@ -57,10 +60,13 @@ static void crc32init_le_generic(const uint32_t polynomial,
> >   	}
> >   }
> > +/*
> > + * crc32 not used by xfs.
> >   static void crc32init_le(void)
> >   {
> >   	crc32init_le_generic(CRCPOLY_LE, crc32table_le);
> >   }
> > + */
> >   static void crc32cinit_le(void)
> >   {
> > @@ -112,6 +118,7 @@ int main(int argc, char** argv)
> >   {
> >   	printf("/* this file is generated - do not edit */\n\n");
> > +#if 0	/* not used by xfsprogs */
> >   	if (CRC_LE_BITS > 1) {
> >   		crc32init_le();
> >   		printf("static u32 crc32table_le[%d][%d] = {",
> > @@ -121,7 +128,6 @@ int main(int argc, char** argv)
> >   		printf("};\n");
> >   	}
> > -#if 0	/* not used by xfsprogs */
> >   	if (CRC_BE_BITS > 1) {
> >   		crc32init_be();
> >   		printf("static u32 crc32table_be[%d][%d] = {",
> > diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
> > index e5eb3de1..fae32411 100644
> > --- a/libxfs/libxfs_priv.h
> > +++ b/libxfs/libxfs_priv.h
> > @@ -67,10 +67,7 @@
> >   #include "xfs_fs.h"
> >   /* CRC stuff, buffer API dependent on it */
> > -extern uint32_t crc32_le(uint32_t crc, unsigned char const *p, size_t len);
> >   extern uint32_t crc32c_le(uint32_t crc, unsigned char const *p, size_t len);
> > -
> > -#define crc32(c,p,l)	crc32_le((c),(unsigned char const *)(p),(l))
> >   #define crc32c(c,p,l)	crc32c_le((c),(unsigned char const *)(p),(l))
> >   #include "xfs_cksum.h"
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=UTANtlZSRetfnz4BmXOr1fXJ6ePKjcWFnaB8-d9hRFk&s=SKcuMvUHSCQzGlRx1lPp-Hym6boKRBUhcRY7YNsWRQo&e=
> > 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Carlos

Re: [PATCH 7/7] libxfs: remove crc32 functions

[Darrick J. Wong]

On Mon, May 28, 2018 at 04:11:36PM +0200, Carlos Maiolino wrote:
> On Sun, May 27, 2018 at 08:53:04PM -0700, Allison Henderson wrote:
> > Seems like it would be ok.  But if these functions are not really used,
> > is there a reason as to why we are just commenting them out instead of
> > removing them entirely?  Thx!
> 
> I've the same question, I'd say you expect to keep them there for historical
> reasons, but I think at least a comment in the patch description would be fine,
> but either if you keep the commented code, or remove it completely:

It's hard to say -- xfsprogs ported the slice-by-8 code from the kernel,
then #if'd out some of the irrelevant bits.  If we ever need to resync
w/ the kernel it'd be much easier to do so if the file is mostly intact
save for some preprocessor gunk to debloat the object files.

OTOH this file is ugly.  Let's see if the maintainer accepts this or
yells 'rewrite!' :)

--D

> Reviewed-by: Carlos Maiolino <cmaiolino@redhat.com>
> 
> 
> > 
> > Allison
> > 
> > On 05/25/2018 03:12 PM, Darrick J. Wong wrote:
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > 
> > > XFS uses crc32c, not crc32.  Remove the unnecessary crc32 code, which
> > > decreases binary size by the 8K crc32 table.
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >   include/libxfs.h        |    3 ---
> > >   libxfs/crc32.c          |   16 ++++++++++------
> > >   libxfs/gen_crc32table.c |    8 +++++++-
> > >   libxfs/libxfs_priv.h    |    3 ---
> > >   4 files changed, 17 insertions(+), 13 deletions(-)
> > > 
> > > 
> > > diff --git a/include/libxfs.h b/include/libxfs.h
> > > index fbaae089..109866de 100644
> > > --- a/include/libxfs.h
> > > +++ b/include/libxfs.h
> > > @@ -43,10 +43,7 @@
> > >   /* CRC stuff, buffer API dependent on it */
> > > -extern uint32_t crc32_le(uint32_t crc, unsigned char const *p, size_t len);
> > >   extern uint32_t crc32c_le(uint32_t crc, unsigned char const *p, size_t len);
> > > -
> > > -#define crc32(c,p,l)	crc32_le((c),(unsigned char const *)(p),(l))
> > >   #define crc32c(c,p,l)	crc32c_le((c),(unsigned char const *)(p),(l))
> > >   #include "xfs_cksum.h"
> > > diff --git a/libxfs/crc32.c b/libxfs/crc32.c
> > > index 783d62e9..8fe5c42c 100644
> > > --- a/libxfs/crc32.c
> > > +++ b/libxfs/crc32.c
> > > @@ -176,20 +176,26 @@ static inline u32 __pure crc32_le_generic(u32 crc, unsigned char const *p,
> > >   }
> > >   #if CRC_LE_BITS == 1
> > > +/*
> > > + * not used by xfs.
> > >   u32 __pure crc32_le(u32 crc, unsigned char const *p, size_t len)
> > >   {
> > >   	return crc32_le_generic(crc, p, len, NULL, CRCPOLY_LE);
> > >   }
> > > + */
> > >   u32 __pure crc32c_le(u32 crc, unsigned char const *p, size_t len)
> > >   {
> > >   	return crc32_le_generic(crc, p, len, NULL, CRC32C_POLY_LE);
> > >   }
> > >   #else
> > > +/*
> > > + * not used by xfs.
> > >   u32 __pure crc32_le(u32 crc, unsigned char const *p, size_t len)
> > >   {
> > >   	return crc32_le_generic(crc, p, len,
> > >   			(const u32 (*)[256])crc32table_le, CRCPOLY_LE);
> > >   }
> > > + */
> > >   u32 __pure crc32c_le(u32 crc, unsigned char const *p, size_t len)
> > >   {
> > >   	return crc32_le_generic(crc, p, len,
> > > @@ -970,6 +976,7 @@ static int crc32c_test(void)
> > >   	return errors;
> > >   }
> > > +#if 0	/* not used by xfs */
> > >   static int crc32_test(void)
> > >   {
> > >   	int i;
> > > @@ -989,10 +996,8 @@ static int crc32_test(void)
> > >   		crc ^= crc32_le(test[i].crc, test_buf +
> > >   		    test[i].start, test[i].length);
> > > -#if 0 /* not used */
> > >   		crc ^= crc32_be(test[i].crc, test_buf +
> > >   		    test[i].start, test[i].length);
> > > -#endif
> > >   	}
> > >   	gettimeofday(&start, NULL);
> > > @@ -1001,11 +1006,9 @@ static int crc32_test(void)
> > >   		    test[i].start, test[i].length))
> > >   			errors++;
> > > -#if 0 /* not used */
> > >   		if (test[i].crc_be != crc32_be(test[i].crc, test_buf +
> > >   		    test[i].start, test[i].length))
> > >   			errors++;
> > > -#endif
> > >   	}
> > >   	gettimeofday(&stop, NULL);
> > > @@ -1021,6 +1024,8 @@ static int crc32_test(void)
> > >   	return errors;
> > >   }
> > > +#endif
> > > +
> > >   /*
> > >    * make sure we always return 0 for a successful test run, and non-zero for a
> > >    * failed run. The build infrastructure is looking for this information to
> > > @@ -1032,8 +1037,7 @@ int main(int argc, char **argv)
> > >   	printf("CRC_LE_BITS = %d\n", CRC_LE_BITS);
> > > -	errors = crc32_test();
> > > -	errors += crc32c_test();
> > > +	errors = crc32c_test();
> > >   	return errors != 0;
> > >   }
> > > diff --git a/libxfs/gen_crc32table.c b/libxfs/gen_crc32table.c
> > > index 574a2d1a..d81164b7 100644
> > > --- a/libxfs/gen_crc32table.c
> > > +++ b/libxfs/gen_crc32table.c
> > > @@ -20,7 +20,10 @@
> > >   # define BE_TABLE_SIZE (1 << CRC_BE_BITS)
> > >   #endif
> > > +/*
> > > + * crc32 not used by xfs.
> > >   static uint32_t crc32table_le[LE_TABLE_ROWS][256];
> > > + */
> > >   static uint32_t crc32ctable_le[LE_TABLE_ROWS][256];
> > >   /*
> > > @@ -57,10 +60,13 @@ static void crc32init_le_generic(const uint32_t polynomial,
> > >   	}
> > >   }
> > > +/*
> > > + * crc32 not used by xfs.
> > >   static void crc32init_le(void)
> > >   {
> > >   	crc32init_le_generic(CRCPOLY_LE, crc32table_le);
> > >   }
> > > + */
> > >   static void crc32cinit_le(void)
> > >   {
> > > @@ -112,6 +118,7 @@ int main(int argc, char** argv)
> > >   {
> > >   	printf("/* this file is generated - do not edit */\n\n");
> > > +#if 0	/* not used by xfsprogs */
> > >   	if (CRC_LE_BITS > 1) {
> > >   		crc32init_le();
> > >   		printf("static u32 crc32table_le[%d][%d] = {",
> > > @@ -121,7 +128,6 @@ int main(int argc, char** argv)
> > >   		printf("};\n");
> > >   	}
> > > -#if 0	/* not used by xfsprogs */
> > >   	if (CRC_BE_BITS > 1) {
> > >   		crc32init_be();
> > >   		printf("static u32 crc32table_be[%d][%d] = {",
> > > diff --git a/libxfs/libxfs_priv.h b/libxfs/libxfs_priv.h
> > > index e5eb3de1..fae32411 100644
> > > --- a/libxfs/libxfs_priv.h
> > > +++ b/libxfs/libxfs_priv.h
> > > @@ -67,10 +67,7 @@
> > >   #include "xfs_fs.h"
> > >   /* CRC stuff, buffer API dependent on it */
> > > -extern uint32_t crc32_le(uint32_t crc, unsigned char const *p, size_t len);
> > >   extern uint32_t crc32c_le(uint32_t crc, unsigned char const *p, size_t len);
> > > -
> > > -#define crc32(c,p,l)	crc32_le((c),(unsigned char const *)(p),(l))
> > >   #define crc32c(c,p,l)	crc32c_le((c),(unsigned char const *)(p),(l))
> > >   #include "xfs_cksum.h"
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  https://urldefense.proofpoint.com/v2/url?u=http-3A__vger.kernel.org_majordomo-2Dinfo.html&d=DwICaQ&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=LHZQ8fHvy6wDKXGTWcm97burZH5sQKHRDMaY1UthQxc&m=UTANtlZSRetfnz4BmXOr1fXJ6ePKjcWFnaB8-d9hRFk&s=SKcuMvUHSCQzGlRx1lPp-Hym6boKRBUhcRY7YNsWRQo&e=
> > > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> -- 
> Carlos
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/7] libxfs: remove crc32 functions

[Eric Sandeen]

On 5/29/18 9:56 AM, Darrick J. Wong wrote:
> On Mon, May 28, 2018 at 04:11:36PM +0200, Carlos Maiolino wrote:
>> On Sun, May 27, 2018 at 08:53:04PM -0700, Allison Henderson wrote:
>>> Seems like it would be ok.  But if these functions are not really used,
>>> is there a reason as to why we are just commenting them out instead of
>>> removing them entirely?  Thx!
>>
>> I've the same question, I'd say you expect to keep them there for historical
>> reasons, but I think at least a comment in the patch description would be fine,
>> but either if you keep the commented code, or remove it completely:
> 
> It's hard to say -- xfsprogs ported the slice-by-8 code from the kernel,
> then #if'd out some of the irrelevant bits.  If we ever need to resync
> w/ the kernel it'd be much easier to do so if the file is mostly intact
> save for some preprocessor gunk to debloat the object files.
> 
> OTOH this file is ugly.  Let's see if the maintainer accepts this or
> yells 'rewrite!' :)

oh I had missed this.

.... 'rewrite!'

Let's just remove the unused stuff.  We don't need it for libxfs-sync-ability
or libxfs-diff-happiness, so I'd say just nuke it.  git history is there for
a reason.

V2 or another patch for later is fine though.

-Eric

Re: [PATCH 7/7] libxfs: remove crc32 functions

[Eric Sandeen]

On 5/25/18 3:12 PM, Darrick J. Wong wrote:
> From: Darrick J. Wong <darrick.wong@oracle.com>
> 
> XFS uses crc32c, not crc32.  Remove the unnecessary crc32 code, which
> decreases binary size by the 8K crc32 table.

Getting back to this ...

The files have diverged a bit since the initial lift from the kernel.
Commenting out to keep things diffable is a bit pointless now, due
to that divergence (for example, the crc32test stuff has been moved
out to its own file, a big swath of code.)

So, I think we should make a decision about whether we want to try
to keep this in sync with the kernel, or just let it drift.

If the former, I'd go with an #if 0 but only after syncing things
up again.

If the latter, just nuke the unused code, if we ever need the crc32c
variants we can re-lift them from the kernel.  I'd still take a brief
look at whether there's anything that needs to be synced in an ad-hoc
fashion.

I guess right now I'm feeling inclined to go with the former, and
even add the crc32 file(s) to the libxfs-diff and/or libxfs-apply
scripts, if we plan to keep them more in sync.

I'd accept either plan, really - but there's no reason to comment
out or #ifdef code to keep it around if it doesn't even necessarily
match kernelspace anymore.

-Eric

> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  include/libxfs.h        |    3 ---
>  libxfs/crc32.c          |   16 ++++++++++------
>  libxfs/gen_crc32table.c |    8 +++++++-
>  libxfs/libxfs_priv.h    |    3 ---
>  4 files changed, 17 insertions(+), 13 deletions(-)
> 
> 
> diff --git a/include/libxfs.h b/include/libxfs.h
> index fbaae089..109866de 100644
> --- a/include/libxfs.h
> +++ b/include/libxfs.h
> @@ -43,10 +43,7 @@
>  
>  
>  /* CRC stuff, buffer API dependent on it */
> -extern uint32_t crc32_le(uint32_t crc, unsigned char const *p, size_t len);
>  extern uint32_t crc32c_le(uint32_t crc, unsigned char const *p, size_t len);
> -
> -#define crc32(c,p,l)	crc32_le((c),(unsigned char const *)(p),(l))
>  #define crc32c(c,p,l)	crc32c_le((c),(unsigned char const *)(p),(l))
>  
>  #include "xfs_cksum.h"
> diff --git a/libxfs/crc32.c b/libxfs/crc32.c
> index 783d62e9..8fe5c42c 100644
> --- a/libxfs/crc32.c
> +++ b/libxfs/crc32.c
> @@ -176,20 +176,26 @@ static inline u32 __pure crc32_le_generic(u32 crc, unsigned char const *p,
>  }
>  
>  #if CRC_LE_BITS == 1
> +/*
> + * not used by xfs.
>  u32 __pure crc32_le(u32 crc, unsigned char const *p, size_t len)
>  {
>  	return crc32_le_generic(crc, p, len, NULL, CRCPOLY_LE);
>  }
> + */

...

Re: [PATCH 7/7] libxfs: remove crc32 functions

[Darrick J. Wong]

On Mon, Jul 23, 2018 at 03:55:46PM -0700, Eric Sandeen wrote:
> On 5/25/18 3:12 PM, Darrick J. Wong wrote:
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > 
> > XFS uses crc32c, not crc32.  Remove the unnecessary crc32 code, which
> > decreases binary size by the 8K crc32 table.
> 
> Getting back to this ...
> 
> The files have diverged a bit since the initial lift from the kernel.

They've diverged a lot -- in the kernel there's code for some sort of
software parallelized computation, and the self test code has been moved
to a separate file.  There's enough difference in the preprocessor code
that I think we might be better off just removing all the cruft except
for the bare minimum code we need + test code.

The giant ugly table of self test code was created to check the fast
crc32* computations against the slow bit-by-bit versions, so as long as
we maintain the same input and output tables in userspace I don't think
we have much to worry about wrt breakage.

TBH this discussion has also convinced me that perhaps it would be
useful to add a crc test command to xfs_db so that we can have xfstests
run the crc32selftest code on any xfstests target instead of just the
distro package build machines and developers' workstations.

--D

> Commenting out to keep things diffable is a bit pointless now, due
> to that divergence (for example, the crc32test stuff has been moved
> out to its own file, a big swath of code.)
> 
> So, I think we should make a decision about whether we want to try
> to keep this in sync with the kernel, or just let it drift.
> 
> If the former, I'd go with an #if 0 but only after syncing things
> up again.
> 
> If the latter, just nuke the unused code, if we ever need the crc32c
> variants we can re-lift them from the kernel.  I'd still take a brief
> look at whether there's anything that needs to be synced in an ad-hoc
> fashion.
> 
> I guess right now I'm feeling inclined to go with the former, and
> even add the crc32 file(s) to the libxfs-diff and/or libxfs-apply
> scripts, if we plan to keep them more in sync.
> 
> I'd accept either plan, really - but there's no reason to comment
> out or #ifdef code to keep it around if it doesn't even necessarily
> match kernelspace anymore.
> 
> -Eric
> 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  include/libxfs.h        |    3 ---
> >  libxfs/crc32.c          |   16 ++++++++++------
> >  libxfs/gen_crc32table.c |    8 +++++++-
> >  libxfs/libxfs_priv.h    |    3 ---
> >  4 files changed, 17 insertions(+), 13 deletions(-)
> > 
> > 
> > diff --git a/include/libxfs.h b/include/libxfs.h
> > index fbaae089..109866de 100644
> > --- a/include/libxfs.h
> > +++ b/include/libxfs.h
> > @@ -43,10 +43,7 @@
> >  
> >  
> >  /* CRC stuff, buffer API dependent on it */
> > -extern uint32_t crc32_le(uint32_t crc, unsigned char const *p, size_t len);
> >  extern uint32_t crc32c_le(uint32_t crc, unsigned char const *p, size_t len);
> > -
> > -#define crc32(c,p,l)	crc32_le((c),(unsigned char const *)(p),(l))
> >  #define crc32c(c,p,l)	crc32c_le((c),(unsigned char const *)(p),(l))
> >  
> >  #include "xfs_cksum.h"
> > diff --git a/libxfs/crc32.c b/libxfs/crc32.c
> > index 783d62e9..8fe5c42c 100644
> > --- a/libxfs/crc32.c
> > +++ b/libxfs/crc32.c
> > @@ -176,20 +176,26 @@ static inline u32 __pure crc32_le_generic(u32 crc, unsigned char const *p,
> >  }
> >  
> >  #if CRC_LE_BITS == 1
> > +/*
> > + * not used by xfs.
> >  u32 __pure crc32_le(u32 crc, unsigned char const *p, size_t len)
> >  {
> >  	return crc32_le_generic(crc, p, len, NULL, CRCPOLY_LE);
> >  }
> > + */
> 
> ...
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/7] xfsprogs-4.17: last of the fixes

[Eric Sandeen]

On 5/25/18 5:11 PM, Darrick J. Wong wrote:
> Hi all,

Thanks, I will pull these all in today.

And thank for the reviews, that helps.

Alison, can you be sure to add a "-" in your "Reviewed-by:" tags
in the future, please?

Thanks,
-Eric
  
> The first patch fixes a Coverity bug that was introduced in the latest
> push of for-next.
> 
> Then there are three patches to 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.
> 
> The fifth patch removes crc32 support (not crc32c support) which is not
> used anywhere in xfs.  This reduces text segment size by 8K in all
> programs.
> 
> --D
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>