[PATCH] shared/298: run xfs_db against the loop device instead of the image file

[PATCH] shared/298: run xfs_db against the loop device instead of the image file

[Christoph Hellwig]

xfs_db fails to properly detect the device sector size and thus segfaults
when run again an image file with 4k sector size.  While that's something
we should fix in xfs_db it will require a fair amount of refactoring of
the libxfs init code.  For now just change shared/298 to run xfs_db
against the loop device created on the image file that is used for I/O,
which feels like the right thing to do anyway to avoid cache coherency
issues.

Signed-off-by: Christoph Hellwig <hch@lst.de>
---
 tests/shared/298 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/shared/298 b/tests/shared/298
index 071c03dee..f657578c7 100755
--- a/tests/shared/298
+++ b/tests/shared/298
@@ -69,7 +69,7 @@ get_free_sectors()
 	agsize=`$XFS_INFO_PROG $loop_mnt | $SED_PROG -n 's/.*agsize=\(.*\) blks.*/\1/p'`
 	# Convert free space (agno, block, length) to (start sector, end sector)
 	_umount $loop_mnt
-	$XFS_DB_PROG -r -c "freesp -d" $img_file | $SED_PROG '/^.*from/,$d'| \
+	$XFS_DB_PROG -r -c "freesp -d" $loop_dev | $SED_PROG '/^.*from/,$d'| \
 		 $AWK_PROG -v spb=$sectors_per_block -v agsize=$agsize \
 		'{ print spb * ($1 * agsize + $2), spb * ($1 * agsize + $2 + $3) - 1 }'
 	;;
-- 
2.39.2

Re: [PATCH] shared/298: run xfs_db against the loop device instead of the image file

[Darrick J. Wong]

On Fri, Mar 01, 2024 at 08:28:20AM -0700, Christoph Hellwig wrote:
> xfs_db fails to properly detect the device sector size and thus segfaults
> when run again an image file with 4k sector size.  While that's something
> we should fix in xfs_db it will require a fair amount of refactoring of
> the libxfs init code.  For now just change shared/298 to run xfs_db
> against the loop device created on the image file that is used for I/O,
> which feels like the right thing to do anyway to avoid cache coherency
> issues.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  tests/shared/298 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/shared/298 b/tests/shared/298
> index 071c03dee..f657578c7 100755
> --- a/tests/shared/298
> +++ b/tests/shared/298
> @@ -69,7 +69,7 @@ get_free_sectors()
>  	agsize=`$XFS_INFO_PROG $loop_mnt | $SED_PROG -n 's/.*agsize=\(.*\) blks.*/\1/p'`
>  	# Convert free space (agno, block, length) to (start sector, end sector)
>  	_umount $loop_mnt
> -	$XFS_DB_PROG -r -c "freesp -d" $img_file | $SED_PROG '/^.*from/,$d'| \
> +	$XFS_DB_PROG -r -c "freesp -d" $loop_dev | $SED_PROG '/^.*from/,$d'| \

Might want to leave a comment here about why xfs uses $loop_dev unlike
the other clauses that use $img_file

# Use $loop_dev to work around sector size misdetection bugs in xfs_db
$XFS_DB_PROG...

With that changed,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

>  		 $AWK_PROG -v spb=$sectors_per_block -v agsize=$agsize \
>  		'{ print spb * ($1 * agsize + $2), spb * ($1 * agsize + $2 + $3) - 1 }'
>  	;;
> -- 
> 2.39.2
> 
> 

Re: [PATCH] shared/298: run xfs_db against the loop device instead of the image file

[Christoph Hellwig]

On Fri, Mar 01, 2024 at 09:47:56AM -0800, Darrick J. Wong wrote:
> Might want to leave a comment here about why xfs uses $loop_dev unlike
> the other clauses that use $img_file

Well, as I tried to explain in my commit message running it against
the file always seemed weird.  The loop device is the canonical place
to run fs tools against.  I can throw in a cleanup patch to also do
this for the other file systems.

Re: [PATCH] shared/298: run xfs_db against the loop device instead of the image file

[Darrick J. Wong]

On Sat, Mar 02, 2024 at 03:01:41PM +0100, Christoph Hellwig wrote:
> On Fri, Mar 01, 2024 at 09:47:56AM -0800, Darrick J. Wong wrote:
> > Might want to leave a comment here about why xfs uses $loop_dev unlike
> > the other clauses that use $img_file
> 
> Well, as I tried to explain in my commit message running it against
> the file always seemed weird.  The loop device is the canonical place
> to run fs tools against.  I can throw in a cleanup patch to also do
> this for the other file systems.

That would also work.

--D

Re: [PATCH] shared/298: run xfs_db against the loop device instead of the image file

[Zorro Lang]

On Fri, Mar 01, 2024 at 08:28:20AM -0700, Christoph Hellwig wrote:
> xfs_db fails to properly detect the device sector size and thus segfaults
> when run again an image file with 4k sector size.  While that's something
> we should fix in xfs_db it will require a fair amount of refactoring of
> the libxfs init code.  For now just change shared/298 to run xfs_db
> against the loop device created on the image file that is used for I/O,
> which feels like the right thing to do anyway to avoid cache coherency
> issues.
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
>  tests/shared/298 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/shared/298 b/tests/shared/298
> index 071c03dee..f657578c7 100755
> --- a/tests/shared/298
> +++ b/tests/shared/298
> @@ -69,7 +69,7 @@ get_free_sectors()
>  	agsize=`$XFS_INFO_PROG $loop_mnt | $SED_PROG -n 's/.*agsize=\(.*\) blks.*/\1/p'`
>  	# Convert free space (agno, block, length) to (start sector, end sector)
>  	_umount $loop_mnt
        ^^^^^^^
Above line causes a conflict, due to it doesn't match the current shared/298 code. It's
"$UMOUNT_PROG $loop_mnt" in current fstests. So you might have another patch to do this
change.

> -	$XFS_DB_PROG -r -c "freesp -d" $img_file | $SED_PROG '/^.*from/,$d'| \
> +	$XFS_DB_PROG -r -c "freesp -d" $loop_dev | $SED_PROG '/^.*from/,$d'| \

As this patch focus on this change, so I'll only make this change. But if above
"_umount $loop_mnt" is needed too, please tell me.

Thanks,
Zorro

>  		 $AWK_PROG -v spb=$sectors_per_block -v agsize=$agsize \
>  		'{ print spb * ($1 * agsize + $2), spb * ($1 * agsize + $2 + $3) - 1 }'
>  	;;
> -- 
> 2.39.2
> 
> 

Re: [PATCH] shared/298: run xfs_db against the loop device instead of the image file

[Christoph Hellwig]

On Sun, Mar 03, 2024 at 09:10:48PM +0800, Zorro Lang wrote:
> >  	# Convert free space (agno, block, length) to (start sector, end sector)
> >  	_umount $loop_mnt
>         ^^^^^^^
> Above line causes a conflict, due to it doesn't match the current shared/298 code. It's
> "$UMOUNT_PROG $loop_mnt" in current fstests. So you might have another patch to do this
> change.

That line actually is from a patch in Darrick's patch queue that I'm
working ontop of right now for some feture development.  Sorry for not
remembering to rebase against current for-next first.

Re: [PATCH] shared/298: run xfs_db against the loop device instead of the image file

[Zorro Lang]

On Sun, Mar 03, 2024 at 03:15:26PM +0100, Christoph Hellwig wrote:
> On Sun, Mar 03, 2024 at 09:10:48PM +0800, Zorro Lang wrote:
> > >  	# Convert free space (agno, block, length) to (start sector, end sector)
> > >  	_umount $loop_mnt
> >         ^^^^^^^
> > Above line causes a conflict, due to it doesn't match the current shared/298 code. It's
> > "$UMOUNT_PROG $loop_mnt" in current fstests. So you might have another patch to do this
> > change.
> 
> That line actually is from a patch in Darrick's patch queue that I'm
> working ontop of right now for some feture development.  Sorry for not
> remembering to rebase against current for-next first.

Never mind:) As that "_umount $loop_mnt" isn't needed, I'll change the
single line only. This patch and the other patch "[PATCH] common:
dm-error now supports zoned devices" are in fstests' "patches-in-queue"
branch, they'll be in next release.

Thanks,
Zorro

> 
>