All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] xfs_admin: revert online label setting ability
@ 2020-03-01 17:50 Eric Sandeen
  2020-03-01 20:55 ` Dave Chinner
  2020-03-02 14:35 ` [PATCH V2] " Eric Sandeen
  0 siblings, 2 replies; 6+ messages in thread
From: Eric Sandeen @ 2020-03-01 17:50 UTC (permalink / raw)
  To: linux-xfs, Darrick J. Wong

The changes to xfs_admin which allowed online label setting via
ioctl had some unintended consequences in terms of changing command
order and processing.  It's going to be somewhat tricky to fix, so
back it out for now.

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

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

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH] xfs_admin: revert online label setting ability
  2020-03-01 17:50 [PATCH] xfs_admin: revert online label setting ability Eric Sandeen
@ 2020-03-01 20:55 ` Dave Chinner
  2020-03-01 21:13   ` Eric Sandeen
  2020-03-02 14:35 ` [PATCH V2] " Eric Sandeen
  1 sibling, 1 reply; 6+ messages in thread
From: Dave Chinner @ 2020-03-01 20:55 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: linux-xfs, Darrick J. Wong

On Sun, Mar 01, 2020 at 09:50:03AM -0800, Eric Sandeen wrote:
> The changes to xfs_admin which allowed online label setting via
> ioctl had some unintended consequences in terms of changing command
> order and processing.  It's going to be somewhat tricky to fix, so
> back it out for now.

What are the symptoms and behaviour of these "unintended
consequences"? And why are they tricky to fix?

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] xfs_admin: revert online label setting ability
  2020-03-01 20:55 ` Dave Chinner
@ 2020-03-01 21:13   ` Eric Sandeen
  2020-03-02  3:10     ` Dave Chinner
  0 siblings, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2020-03-01 21:13 UTC (permalink / raw)
  To: Dave Chinner, Eric Sandeen; +Cc: linux-xfs, Darrick J. Wong

On 3/1/20 12:55 PM, Dave Chinner wrote:
> On Sun, Mar 01, 2020 at 09:50:03AM -0800, Eric Sandeen wrote:
>> The changes to xfs_admin which allowed online label setting via
>> ioctl had some unintended consequences in terms of changing command
>> order and processing.  It's going to be somewhat tricky to fix, so
>> back it out for now.
> 
> What are the symptoms and behaviour of these "unintended
> consequences"? And why are they tricky to fix?

Yeah, I should have probably said more in the commit log.

https://bugzilla.kernel.org/show_bug.cgi?id=206429

was the first clue,

"xfs_admin can't print both label and UUID for mounted filesystems"

The main problem is that if /any/ options that trigger xfs_io get specified,
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 the non-io / db opts don't get run at all.

So sure, we could then move on to the db commands, but we actually built them
all up along the way as well:

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

so we'd need to keep those separate, and not re-run them in db.

And another thing that I struggled with was preserving the order; you'd
kind of expect that if you specify commands in a certain order 
they'd be executed in that order, and that used to be true.  Now it's not,
even if we don't exit in the "if IO_OPTS" case above.

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 just got worse
and worse, TBH.

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

-Eric

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [PATCH] xfs_admin: revert online label setting ability
  2020-03-01 21:13   ` Eric Sandeen
@ 2020-03-02  3:10     ` Dave Chinner
  0 siblings, 0 replies; 6+ messages in thread
From: Dave Chinner @ 2020-03-02  3:10 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: Eric Sandeen, linux-xfs, Darrick J. Wong

On Sun, Mar 01, 2020 at 03:13:07PM -0600, Eric Sandeen wrote:
> On 3/1/20 12:55 PM, Dave Chinner wrote:
> > On Sun, Mar 01, 2020 at 09:50:03AM -0800, Eric Sandeen wrote:
> >> The changes to xfs_admin which allowed online label setting via
> >> ioctl had some unintended consequences in terms of changing command
> >> order and processing.  It's going to be somewhat tricky to fix, so
> >> back it out for now.
> > 
> > What are the symptoms and behaviour of these "unintended
> > consequences"? And why are they tricky to fix?
> 
> Yeah, I should have probably said more in the commit log.
> 
> https://bugzilla.kernel.org/show_bug.cgi?id=206429
> 
> was the first clue,
> 
> "xfs_admin can't print both label and UUID for mounted filesystems"
> 
> The main problem is that if /any/ options that trigger xfs_io get specified,
> 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 the non-io / db opts don't get run at all.
> 
> So sure, we could then move on to the db commands, but we actually built them
> all up along the way as well:
> 
>         l)      DB_OPTS=$DB_OPTS" -r -c label"
>                 IO_OPTS=$IO_OPTS" -r -c label"
>                 ;;
> 
> so we'd need to keep those separate, and not re-run them in db.
> 
> And another thing that I struggled with was preserving the order; you'd
> kind of expect that if you specify commands in a certain order 
> they'd be executed in that order, and that used to be true.  Now it's not,
> even if we don't exit in the "if IO_OPTS" case above.
> 
> 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 just got worse
> and worse, TBH.

And there's your new commit message. :)

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH V2] xfs_admin: revert online label setting ability
  2020-03-01 17:50 [PATCH] xfs_admin: revert online label setting ability Eric Sandeen
  2020-03-01 20:55 ` Dave Chinner
@ 2020-03-02 14:35 ` Eric Sandeen
  2020-03-03  1:18   ` Dave Chinner
  1 sibling, 1 reply; 6+ messages in thread
From: Eric Sandeen @ 2020-03-02 14:35 UTC (permalink / raw)
  To: linux-xfs, Darrick J. Wong

"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>
---

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



^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH V2] xfs_admin: revert online label setting ability
  2020-03-02 14:35 ` [PATCH V2] " Eric Sandeen
@ 2020-03-03  1:18   ` Dave Chinner
  0 siblings, 0 replies; 6+ messages in thread
From: Dave Chinner @ 2020-03-03  1:18 UTC (permalink / raw)
  To: Eric Sandeen; +Cc: linux-xfs, Darrick J. Wong

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>
-- 
Dave Chinner
david@fromorbit.com

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-03-03  1:18 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-01 17:50 [PATCH] xfs_admin: revert online label setting ability Eric Sandeen
2020-03-01 20:55 ` Dave Chinner
2020-03-01 21:13   ` Eric Sandeen
2020-03-02  3:10     ` Dave Chinner
2020-03-02 14:35 ` [PATCH V2] " Eric Sandeen
2020-03-03  1:18   ` Dave Chinner

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.