diff mbox series

[V2] xfs_admin: revert online label setting ability

Message ID b4d2d6cf-dbae-2a2e-7580-3b6fd13aad9a@redhat.com (mailing list archive)
State Accepted, archived
Headers show
Series [V2] xfs_admin: revert online label setting ability | expand

Commit Message

Eric Sandeen March 2, 2020, 2:35 p.m. UTC
"xfs_admin can't print both label and UUID for mounted filesystems"
https://bugzilla.kernel.org/show_bug.cgi?id=206429

alerted us to the problem that if /any/ options that use xfs_io get
specified to xfs_admin, they are the /only/ ones that get run:

                # Try making the changes online, if supported
                if [ -n "$IO_OPTS" ] && mntpt="$(find_mntpt_for_arg "$1")"
                then
                        eval xfs_io -x -p xfs_admin $IO_OPTS "$mntpt"
                        test "$?" -eq 0 && exit 0
                fi

and thanks to the exit, the xfs_db operations don't get run at all.

We could move on to the xfs_db commands after executing the xfs_io
commands, but we build them all up in parallel at this time:

        l)      DB_OPTS=$DB_OPTS" -r -c label"
                IO_OPTS=$IO_OPTS" -r -c label"
                ;;

so we'd need to keep track of these, and not re-run them in xfs_db.

Another issue is that prior to this commit, we'd run commands in
command line order.

So I experimented with building up an array of commands, invoking xfs_db
or xfs_io one command at a time as needed for each, and ... it got overly
complicated.

It's broken now, and so far a clean solution isn't evident, and I hate to
leave it broken across another release.  So revert it for now.

Reverts: 3f153e051a ("xfs_admin: enable online label getting and setting")

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

Comments

Dave Chinner March 3, 2020, 1:18 a.m. UTC | #1
On Mon, Mar 02, 2020 at 08:35:02AM -0600, Eric Sandeen wrote:
> "xfs_admin can't print both label and UUID for mounted filesystems"
> https://bugzilla.kernel.org/show_bug.cgi?id=206429
> 
> alerted us to the problem that if /any/ options that use xfs_io get
> specified to xfs_admin, they are the /only/ ones that get run:
> 
>                 # Try making the changes online, if supported
>                 if [ -n "$IO_OPTS" ] && mntpt="$(find_mntpt_for_arg "$1")"
>                 then
>                         eval xfs_io -x -p xfs_admin $IO_OPTS "$mntpt"
>                         test "$?" -eq 0 && exit 0
>                 fi
> 
> and thanks to the exit, the xfs_db operations don't get run at all.
> 
> We could move on to the xfs_db commands after executing the xfs_io
> commands, but we build them all up in parallel at this time:
> 
>         l)      DB_OPTS=$DB_OPTS" -r -c label"
>                 IO_OPTS=$IO_OPTS" -r -c label"
>                 ;;
> 
> so we'd need to keep track of these, and not re-run them in xfs_db.
> 
> Another issue is that prior to this commit, we'd run commands in
> command line order.
> 
> So I experimented with building up an array of commands, invoking xfs_db
> or xfs_io one command at a time as needed for each, and ... it got overly
> complicated.
> 
> It's broken now, and so far a clean solution isn't evident, and I hate to
> leave it broken across another release.  So revert it for now.
> 
> Reverts: 3f153e051a ("xfs_admin: enable online label getting and setting")
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---

Looks fine, I understand why now :)

Reviewed-by: Dave Chinner <dchinner@redhat.com>
diff mbox series

Patch

diff --git a/db/xfs_admin.sh b/db/xfs_admin.sh
index d18959bf..bd325da2 100755
--- a/db/xfs_admin.sh
+++ b/db/xfs_admin.sh
@@ -7,30 +7,8 @@ 
 status=0
 DB_OPTS=""
 REPAIR_OPTS=""
-IO_OPTS=""
 USAGE="Usage: xfs_admin [-efjlpuV] [-c 0|1] [-L label] [-U uuid] device [logdev]"
 
-# 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"
-}
-
-# See if we can find a mount point for the argument.
-find_mntpt_for_arg() {
-	local arg="$1"
-
-	# See if we can map the arg to a loop device
-	local 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.
-	findmnt -t xfs -f -n -o TARGET "${arg}" 2> /dev/null
-}
-
 while getopts "efjlpuc:L:U:V" c
 do
 	case $c in
@@ -38,16 +16,8 @@  do
 	e)	DB_OPTS=$DB_OPTS" -c 'version extflg'";;
 	f)	DB_OPTS=$DB_OPTS" -f";;
 	j)	DB_OPTS=$DB_OPTS" -c 'version log2'";;
-	l)	DB_OPTS=$DB_OPTS" -r -c label"
-		IO_OPTS=$IO_OPTS" -r -c label"
-		;;
-	L)	DB_OPTS=$DB_OPTS" -c 'label "$OPTARG"'"
-		if [ "$OPTARG" = "--" ]; then
-			IO_OPTS=$IO_OPTS" -c 'label -c'"
-		else
-			IO_OPTS=$IO_OPTS" -c 'label -s "$OPTARG"'"
-		fi
-		;;
+	l)	DB_OPTS=$DB_OPTS" -r -c label";;
+	L)	DB_OPTS=$DB_OPTS" -c 'label "$OPTARG"'";;
 	p)	DB_OPTS=$DB_OPTS" -c 'version projid32bit'";;
 	u)	DB_OPTS=$DB_OPTS" -r -c uuid";;
 	U)	DB_OPTS=$DB_OPTS" -c 'uuid "$OPTARG"'";;
@@ -71,14 +41,6 @@  case $# in
 				REPAIR_OPTS=$REPAIR_OPTS" -l '$2'"
 		fi
 
-		# Try making the changes online, if supported
-		if [ -n "$IO_OPTS" ] && mntpt="$(find_mntpt_for_arg "$1")"
-		then
-			eval xfs_io -x -p xfs_admin $IO_OPTS "$mntpt"
-			test "$?" -eq 0 && exit 0
-		fi
-
-		# Otherwise try offline changing
 		if [ -n "$DB_OPTS" ]
 		then
 			eval xfs_db -x -p xfs_admin $DB_OPTS $1
diff --git a/man/man8/xfs_admin.8 b/man/man8/xfs_admin.8
index 220dd803..8afc873f 100644
--- a/man/man8/xfs_admin.8
+++ b/man/man8/xfs_admin.8
@@ -26,7 +26,7 @@  uses the
 .BR xfs_db (8)
 command to modify various parameters of a filesystem.
 .PP
-Devices that are mounted cannot be modified, except as noted below.
+Devices that are mounted cannot be modified.
 Administrators must unmount filesystems before
 .BR xfs_admin " or " xfs_db (8)
 can convert parameters.
@@ -67,7 +67,6 @@  log buffers).
 .TP
 .B \-l
 Print the current filesystem label.
-This command can be run if the filesystem is mounted.
 .TP
 .B \-p
 Enable 32bit project identifier support (PROJID32BIT feature).
@@ -103,7 +102,6 @@  The filesystem label can be cleared using the special "\c
 .B \-\-\c
 " value for
 .IR label .
-This command can be run if the filesystem is mounted.
 .TP
 .BI \-U " uuid"
 Set the UUID of the filesystem to