xfs: Skip repetitive warnings about mount options

xfs: Skip repetitive warnings about mount options

[Pavel Reichl]

At least some version of mount will look in /proc/mounts and send in all of the 
options that it finds as part of a remount command. We also /do/ still emit
"attr2" in /proc/mounts (as we probably should), so remount passes that back
in, and we emit a warning, which is not great.

In other words mount passes in "attr2" and the kernel emits a deprecation
warning for attr2, even though the user/admin never explicitly asked for the
option.

So, lets skip the warning if (we are remounting && deprecated option
state is not changing).

I also attached test for xfstests that I used for testing (the test
will be proposed on xfstests-list after/if this patch is merged).


Pavel Reichl (1):
  xfs: Skip repetitive warnings about mount options

 fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

-- 
2.29.2

[PATCH] xfs: Add test for printing deprec. mount options

[Pavel Reichl]

Verify that warnings about deprecated mount options are properly
printed.

Verify that no excessive warnings are printed during remounts.

Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 tests/xfs/528     | 88 +++++++++++++++++++++++++++++++++++++++++++++++
 tests/xfs/528.out |  2 ++
 tests/xfs/group   |  1 +
 3 files changed, 91 insertions(+)
 create mode 100755 tests/xfs/528
 create mode 100644 tests/xfs/528.out

diff --git a/tests/xfs/528 b/tests/xfs/528
new file mode 100755
index 00000000..0fc57cef
--- /dev/null
+++ b/tests/xfs/528
@@ -0,0 +1,88 @@
+#! /bin/bash
+# SPDX-License-Identifier: GPL-2.0
+# Copyright (c) 2020 Red Hat, Inc.. All Rights Reserved.
+#
+# FS QA Test 528
+#
+# Verify that warnings about deprecated mount options are properly printed.
+#  
+# Verify that no excessive warnings are printed during remounts.
+#
+
+seq=`basename $0`
+seqres=$RESULT_DIR/$seq
+echo "QA output created by $seq"
+
+here=`pwd`
+tmp=/tmp/$$
+status=1	# failure is the default!
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+_cleanup()
+{
+	cd /
+	rm -f $tmp.*
+}
+
+# get standard environment, filters and checks
+. ./common/rc
+
+# remove previous $seqres.full before test
+rm -f $seqres.full
+
+_require_check_dmesg
+_supported_fs xfs
+_require_scratch
+
+log_tag()
+{
+	echo "fstests $seqnum [tag]" > /dev/kmsg
+}
+
+dmesg_since_test_tag()
+{
+        dmesg | tac | sed -ne "0,\#fstests $seqnum \[tag\]#p" | \
+                tac
+}
+
+check_dmesg_for_since_tag()
+{
+        dmesg_since_test_tag | egrep -q "$1"
+}
+
+echo "Silence is golden."
+
+log_tag
+
+# Test mount
+for VAR in {attr2,ikeep,noikeep}; do
+	_scratch_mkfs > $seqres.full 2>&1
+	_scratch_mount -o $VAR
+	check_dmesg_for_since_tag "XFS: $VAR mount option is deprecated" || \
+		echo "Could not find deprecation warning for $VAR"
+	umount $SCRATCH_MNT
+done
+
+# Test mount with default options (attr2 and noikeep) and remount with
+# 2 groups of options
+# 1) the defaults (attr2, noikeep)
+# 2) non defaults (noattr2, ikeep)
+_scratch_mount
+for VAR in {attr2,noikeep}; do
+	log_tag
+	mount -o $VAR,remount $SCRATCH_MNT
+	check_dmesg_for_since_tag "XFS: $VAR mount option is deprecated." && \
+		echo "Should not be able to find deprecation warning for $VAR"
+done
+for VAR in {noattr2,ikeep}; do
+	log_tag
+	mount -o $VAR,remount $SCRATCH_MNT
+	check_dmesg_for_since_tag "XFS: $VAR mount option is deprecated" || \
+		echo "Could not find deprecation warning for $VAR"
+done
+umount $SCRATCH_MNT
+
+# success, all done
+status=0
+exit
+
diff --git a/tests/xfs/528.out b/tests/xfs/528.out
new file mode 100644
index 00000000..762dccc0
--- /dev/null
+++ b/tests/xfs/528.out
@@ -0,0 +1,2 @@
+QA output created by 528
+Silence is golden.
diff --git a/tests/xfs/group b/tests/xfs/group
index e861cec9..ad3bd223 100644
--- a/tests/xfs/group
+++ b/tests/xfs/group
@@ -525,3 +525,4 @@
 525 auto quick mkfs
 526 auto quick mkfs
 527 auto quick quota
+528 auto quick mount
-- 
2.29.2

Re: [PATCH] xfs: Add test for printing deprec. mount options

[Darrick J. Wong]

On Sat, Feb 20, 2021 at 11:15:48PM +0100, Pavel Reichl wrote:
> Verify that warnings about deprecated mount options are properly
> printed.
> 
> Verify that no excessive warnings are printed during remounts.
> 
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  tests/xfs/528     | 88 +++++++++++++++++++++++++++++++++++++++++++++++
>  tests/xfs/528.out |  2 ++
>  tests/xfs/group   |  1 +
>  3 files changed, 91 insertions(+)
>  create mode 100755 tests/xfs/528
>  create mode 100644 tests/xfs/528.out
> 
> diff --git a/tests/xfs/528 b/tests/xfs/528
> new file mode 100755
> index 00000000..0fc57cef
> --- /dev/null
> +++ b/tests/xfs/528
> @@ -0,0 +1,88 @@
> +#! /bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +# Copyright (c) 2020 Red Hat, Inc.. All Rights Reserved.
> +#
> +# FS QA Test 528
> +#
> +# Verify that warnings about deprecated mount options are properly printed.
> +#  
> +# Verify that no excessive warnings are printed during remounts.
> +#
> +
> +seq=`basename $0`
> +seqres=$RESULT_DIR/$seq
> +echo "QA output created by $seq"
> +
> +here=`pwd`
> +tmp=/tmp/$$
> +status=1	# failure is the default!
> +trap "_cleanup; exit \$status" 0 1 2 3 15
> +
> +_cleanup()
> +{
> +	cd /
> +	rm -f $tmp.*
> +}
> +
> +# get standard environment, filters and checks
> +. ./common/rc
> +
> +# remove previous $seqres.full before test
> +rm -f $seqres.full
> +
> +_require_check_dmesg
> +_supported_fs xfs
> +_require_scratch
> +
> +log_tag()
> +{
> +	echo "fstests $seqnum [tag]" > /dev/kmsg

_require_check_dmesg?

> +}
> +
> +dmesg_since_test_tag()
> +{
> +        dmesg | tac | sed -ne "0,\#fstests $seqnum \[tag\]#p" | \
> +                tac
> +}
> +
> +check_dmesg_for_since_tag()
> +{
> +        dmesg_since_test_tag | egrep -q "$1"
> +}
> +
> +echo "Silence is golden."
> +
> +log_tag
> +
> +# Test mount
> +for VAR in {attr2,ikeep,noikeep}; do
> +	_scratch_mkfs > $seqres.full 2>&1
> +	_scratch_mount -o $VAR
> +	check_dmesg_for_since_tag "XFS: $VAR mount option is deprecated" || \
> +		echo "Could not find deprecation warning for $VAR"

I think this is going to regress on old stable kernels that don't know
about the mount option deprecation, right?  Shouldn't there be some
logic to skip the test in that case?

--D

> +	umount $SCRATCH_MNT
> +done
> +
> +# Test mount with default options (attr2 and noikeep) and remount with
> +# 2 groups of options
> +# 1) the defaults (attr2, noikeep)
> +# 2) non defaults (noattr2, ikeep)
> +_scratch_mount
> +for VAR in {attr2,noikeep}; do
> +	log_tag
> +	mount -o $VAR,remount $SCRATCH_MNT
> +	check_dmesg_for_since_tag "XFS: $VAR mount option is deprecated." && \
> +		echo "Should not be able to find deprecation warning for $VAR"
> +done
> +for VAR in {noattr2,ikeep}; do
> +	log_tag
> +	mount -o $VAR,remount $SCRATCH_MNT
> +	check_dmesg_for_since_tag "XFS: $VAR mount option is deprecated" || \
> +		echo "Could not find deprecation warning for $VAR"
> +done
> +umount $SCRATCH_MNT
> +
> +# success, all done
> +status=0
> +exit
> +
> diff --git a/tests/xfs/528.out b/tests/xfs/528.out
> new file mode 100644
> index 00000000..762dccc0
> --- /dev/null
> +++ b/tests/xfs/528.out
> @@ -0,0 +1,2 @@
> +QA output created by 528
> +Silence is golden.
> diff --git a/tests/xfs/group b/tests/xfs/group
> index e861cec9..ad3bd223 100644
> --- a/tests/xfs/group
> +++ b/tests/xfs/group
> @@ -525,3 +525,4 @@
>  525 auto quick mkfs
>  526 auto quick mkfs
>  527 auto quick quota
> +528 auto quick mount
> -- 
> 2.29.2
> 

Re: [PATCH] xfs: Add test for printing deprec. mount options

[Pavel Reichl]

On 2/22/21 10:22 PM, Darrick J. Wong wrote:
> On Sat, Feb 20, 2021 at 11:15:48PM +0100, Pavel Reichl wrote:
>> Verify that warnings about deprecated mount options are properly
>> printed.
>>
>> Verify that no excessive warnings are printed during remounts.
>>
>> Signed-off-by: Pavel Reichl <preichl@redhat.com>
>> ---
>>  tests/xfs/528     | 88 +++++++++++++++++++++++++++++++++++++++++++++++
>>  tests/xfs/528.out |  2 ++
>>  tests/xfs/group   |  1 +
>>  3 files changed, 91 insertions(+)
>>  create mode 100755 tests/xfs/528
>>  create mode 100644 tests/xfs/528.out
>>
>> diff --git a/tests/xfs/528 b/tests/xfs/528
>> new file mode 100755
>> index 00000000..0fc57cef
>> --- /dev/null
>> +++ b/tests/xfs/528
>> @@ -0,0 +1,88 @@
>> +#! /bin/bash
>> +# SPDX-License-Identifier: GPL-2.0
>> +# Copyright (c) 2020 Red Hat, Inc.. All Rights Reserved.
>> +#
>> +# FS QA Test 528
>> +#
>> +# Verify that warnings about deprecated mount options are properly printed.
>> +#  
>> +# Verify that no excessive warnings are printed during remounts.
>> +#
>> +
>> +seq=`basename $0`
>> +seqres=$RESULT_DIR/$seq
>> +echo "QA output created by $seq"
>> +
>> +here=`pwd`
>> +tmp=/tmp/$$
>> +status=1	# failure is the default!
>> +trap "_cleanup; exit \$status" 0 1 2 3 15
>> +
>> +_cleanup()
>> +{
>> +	cd /
>> +	rm -f $tmp.*
>> +}
>> +
>> +# get standard environment, filters and checks
>> +. ./common/rc
>> +
>> +# remove previous $seqres.full before test
>> +rm -f $seqres.full
>> +
>> +_require_check_dmesg
>> +_supported_fs xfs
>> +_require_scratch
>> +
>> +log_tag()
>> +{
>> +	echo "fstests $seqnum [tag]" > /dev/kmsg
> 
> _require_check_dmesg?


Yeah? I mean it's right above _supported_fs_xfs and _require_scratch...I guess I should move it right next to _require_scratch, OK?


> 
>> +}
>> +
>> +dmesg_since_test_tag()
>> +{
>> +        dmesg | tac | sed -ne "0,\#fstests $seqnum \[tag\]#p" | \
>> +                tac
>> +}
>> +
>> +check_dmesg_for_since_tag()
>> +{
>> +        dmesg_since_test_tag | egrep -q "$1"
>> +}
>> +
>> +echo "Silence is golden."
>> +
>> +log_tag
>> +
>> +# Test mount
>> +for VAR in {attr2,ikeep,noikeep}; do
>> +	_scratch_mkfs > $seqres.full 2>&1
>> +	_scratch_mount -o $VAR
>> +	check_dmesg_for_since_tag "XFS: $VAR mount option is deprecated" || \
>> +		echo "Could not find deprecation warning for $VAR"
> 
> I think this is going to regress on old stable kernels that don't know
> about the mount option deprecation, right?  Shouldn't there be some
> logic to skip the test in that case?

I think you are right, thanks for the catch.
	
Do you know how to make sure that xfstest is executed only on the kernel that implements the tested feature? I tried to do some grepping but I didn't find anything yet.

Adding check for the output of `uname -r' directly to test seems a bit too crude.

> 
> --D
> 
>> +	umount $SCRATCH_MNT
>> +done
>> +
>> +# Test mount with default options (attr2 and noikeep) and remount with
>> +# 2 groups of options
>> +# 1) the defaults (attr2, noikeep)
>> +# 2) non defaults (noattr2, ikeep)
>> +_scratch_mount
>> +for VAR in {attr2,noikeep}; do
>> +	log_tag
>> +	mount -o $VAR,remount $SCRATCH_MNT
>> +	check_dmesg_for_since_tag "XFS: $VAR mount option is deprecated." && \
>> +		echo "Should not be able to find deprecation warning for $VAR"
>> +done
>> +for VAR in {noattr2,ikeep}; do
>> +	log_tag
>> +	mount -o $VAR,remount $SCRATCH_MNT
>> +	check_dmesg_for_since_tag "XFS: $VAR mount option is deprecated" || \
>> +		echo "Could not find deprecation warning for $VAR"
>> +done
>> +umount $SCRATCH_MNT
>> +
>> +# success, all done
>> +status=0
>> +exit
>> +
>> diff --git a/tests/xfs/528.out b/tests/xfs/528.out
>> new file mode 100644
>> index 00000000..762dccc0
>> --- /dev/null
>> +++ b/tests/xfs/528.out
>> @@ -0,0 +1,2 @@
>> +QA output created by 528
>> +Silence is golden.
>> diff --git a/tests/xfs/group b/tests/xfs/group
>> index e861cec9..ad3bd223 100644
>> --- a/tests/xfs/group
>> +++ b/tests/xfs/group
>> @@ -525,3 +525,4 @@
>>  525 auto quick mkfs
>>  526 auto quick mkfs
>>  527 auto quick quota
>> +528 auto quick mount
>> -- 
>> 2.29.2
>>
> 

Re: [PATCH] xfs: Add test for printing deprec. mount options

[Darrick J. Wong]

On Tue, Feb 23, 2021 at 05:41:04PM +0100, Pavel Reichl wrote:
> 
> 
> On 2/22/21 10:22 PM, Darrick J. Wong wrote:
> > On Sat, Feb 20, 2021 at 11:15:48PM +0100, Pavel Reichl wrote:
> >> Verify that warnings about deprecated mount options are properly
> >> printed.
> >>
> >> Verify that no excessive warnings are printed during remounts.
> >>
> >> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> >> ---
> >>  tests/xfs/528     | 88 +++++++++++++++++++++++++++++++++++++++++++++++
> >>  tests/xfs/528.out |  2 ++
> >>  tests/xfs/group   |  1 +
> >>  3 files changed, 91 insertions(+)
> >>  create mode 100755 tests/xfs/528
> >>  create mode 100644 tests/xfs/528.out
> >>
> >> diff --git a/tests/xfs/528 b/tests/xfs/528
> >> new file mode 100755
> >> index 00000000..0fc57cef
> >> --- /dev/null
> >> +++ b/tests/xfs/528
> >> @@ -0,0 +1,88 @@
> >> +#! /bin/bash
> >> +# SPDX-License-Identifier: GPL-2.0
> >> +# Copyright (c) 2020 Red Hat, Inc.. All Rights Reserved.
> >> +#
> >> +# FS QA Test 528
> >> +#
> >> +# Verify that warnings about deprecated mount options are properly printed.
> >> +#  
> >> +# Verify that no excessive warnings are printed during remounts.
> >> +#
> >> +
> >> +seq=`basename $0`
> >> +seqres=$RESULT_DIR/$seq
> >> +echo "QA output created by $seq"
> >> +
> >> +here=`pwd`
> >> +tmp=/tmp/$$
> >> +status=1	# failure is the default!
> >> +trap "_cleanup; exit \$status" 0 1 2 3 15
> >> +
> >> +_cleanup()
> >> +{
> >> +	cd /
> >> +	rm -f $tmp.*
> >> +}
> >> +
> >> +# get standard environment, filters and checks
> >> +. ./common/rc
> >> +
> >> +# remove previous $seqres.full before test
> >> +rm -f $seqres.full
> >> +
> >> +_require_check_dmesg
> >> +_supported_fs xfs
> >> +_require_scratch
> >> +
> >> +log_tag()
> >> +{
> >> +	echo "fstests $seqnum [tag]" > /dev/kmsg
> > 
> > _require_check_dmesg?
> 
> 
> Yeah? I mean it's right above _supported_fs_xfs and _require_scratch...I guess I should move it right next to _require_scratch, OK?

Doh.  I totally missed that, sorry. :/

> 
> > 
> >> +}
> >> +
> >> +dmesg_since_test_tag()
> >> +{
> >> +        dmesg | tac | sed -ne "0,\#fstests $seqnum \[tag\]#p" | \
> >> +                tac
> >> +}
> >> +
> >> +check_dmesg_for_since_tag()
> >> +{
> >> +        dmesg_since_test_tag | egrep -q "$1"
> >> +}
> >> +
> >> +echo "Silence is golden."
> >> +
> >> +log_tag
> >> +
> >> +# Test mount
> >> +for VAR in {attr2,ikeep,noikeep}; do
> >> +	_scratch_mkfs > $seqres.full 2>&1
> >> +	_scratch_mount -o $VAR
> >> +	check_dmesg_for_since_tag "XFS: $VAR mount option is deprecated" || \
> >> +		echo "Could not find deprecation warning for $VAR"
> > 
> > I think this is going to regress on old stable kernels that don't know
> > about the mount option deprecation, right?  Shouldn't there be some
> > logic to skip the test in that case?
> 
> I think you are right, thanks for the catch.
> 	
> Do you know how to make sure that xfstest is executed only on the kernel that implements the tested feature? I tried to do some grepping but I didn't find anything yet.
> 
> Adding check for the output of `uname -r' directly to test seems a bit too crude.

That might be the only way to do it though...

--D

> > 
> > --D
> > 
> >> +	umount $SCRATCH_MNT
> >> +done
> >> +
> >> +# Test mount with default options (attr2 and noikeep) and remount with
> >> +# 2 groups of options
> >> +# 1) the defaults (attr2, noikeep)
> >> +# 2) non defaults (noattr2, ikeep)
> >> +_scratch_mount
> >> +for VAR in {attr2,noikeep}; do
> >> +	log_tag
> >> +	mount -o $VAR,remount $SCRATCH_MNT
> >> +	check_dmesg_for_since_tag "XFS: $VAR mount option is deprecated." && \
> >> +		echo "Should not be able to find deprecation warning for $VAR"
> >> +done
> >> +for VAR in {noattr2,ikeep}; do
> >> +	log_tag
> >> +	mount -o $VAR,remount $SCRATCH_MNT
> >> +	check_dmesg_for_since_tag "XFS: $VAR mount option is deprecated" || \
> >> +		echo "Could not find deprecation warning for $VAR"
> >> +done
> >> +umount $SCRATCH_MNT
> >> +
> >> +# success, all done
> >> +status=0
> >> +exit
> >> +
> >> diff --git a/tests/xfs/528.out b/tests/xfs/528.out
> >> new file mode 100644
> >> index 00000000..762dccc0
> >> --- /dev/null
> >> +++ b/tests/xfs/528.out
> >> @@ -0,0 +1,2 @@
> >> +QA output created by 528
> >> +Silence is golden.
> >> diff --git a/tests/xfs/group b/tests/xfs/group
> >> index e861cec9..ad3bd223 100644
> >> --- a/tests/xfs/group
> >> +++ b/tests/xfs/group
> >> @@ -525,3 +525,4 @@
> >>  525 auto quick mkfs
> >>  526 auto quick mkfs
> >>  527 auto quick quota
> >> +528 auto quick mount
> >> -- 
> >> 2.29.2
> >>
> > 
> 

[PATCH 1/1] xfs: Skip repetitive warnings about mount options

[Pavel Reichl]

Skip the warnings about mount option being deprecated if we are
remounting and deprecated option state is not changing.

Bug: https://bugzilla.kernel.org/show_bug.cgi?id=211605
Fix-suggested-by: Eric Sandeen <sandeen@redhat.com>
Signed-off-by: Pavel Reichl <preichl@redhat.com>
---
 fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 813be879a5e5..6724a7018d1f 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -1169,6 +1169,13 @@ xfs_fs_parse_param(
 	struct fs_parse_result	result;
 	int			size = 0;
 	int			opt;
+	uint64_t                prev_m_flags = 0; /* Mount flags of prev. mount */
+	bool			remounting = fc->purpose & FS_CONTEXT_FOR_RECONFIGURE;
+
+	/* if reconfiguring then get mount flags of previous flags */
+	if (remounting) {
+		prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
+	}
 
 	opt = fs_parse(fc, xfs_fs_parameters, param, &result);
 	if (opt < 0)
@@ -1294,19 +1301,27 @@ xfs_fs_parse_param(
 #endif
 	/* Following mount options will be removed in September 2025 */
 	case Opt_ikeep:
-		xfs_warn(mp, "%s mount option is deprecated.", param->key);
+		if (!remounting ||  !(prev_m_flags & XFS_MOUNT_IKEEP)) {
+			xfs_warn(mp, "%s mount option is deprecated.", param->key);
+		}
 		mp->m_flags |= XFS_MOUNT_IKEEP;
 		return 0;
 	case Opt_noikeep:
-		xfs_warn(mp, "%s mount option is deprecated.", param->key);
+		if (!remounting || prev_m_flags & XFS_MOUNT_IKEEP) {
+			xfs_warn(mp, "%s mount option is deprecated.", param->key);
+		}
 		mp->m_flags &= ~XFS_MOUNT_IKEEP;
 		return 0;
 	case Opt_attr2:
-		xfs_warn(mp, "%s mount option is deprecated.", param->key);
+		if (!remounting || !(prev_m_flags & XFS_MOUNT_ATTR2)) {
+			xfs_warn(mp, "%s mount option is deprecated.", param->key);
+		}
 		mp->m_flags |= XFS_MOUNT_ATTR2;
 		return 0;
 	case Opt_noattr2:
-		xfs_warn(mp, "%s mount option is deprecated.", param->key);
+		if (!remounting || !(prev_m_flags & XFS_MOUNT_NOATTR2)) {
+			xfs_warn(mp, "%s mount option is deprecated.", param->key);
+		}
 		mp->m_flags &= ~XFS_MOUNT_ATTR2;
 		mp->m_flags |= XFS_MOUNT_NOATTR2;
 		return 0;
-- 
2.29.2

Re: [PATCH 1/1] xfs: Skip repetitive warnings about mount options

[Darrick J. Wong]

On Sat, Feb 20, 2021 at 11:15:49PM +0100, Pavel Reichl wrote:
> Skip the warnings about mount option being deprecated if we are
> remounting and deprecated option state is not changing.
> 
> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=211605
> Fix-suggested-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 813be879a5e5..6724a7018d1f 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1169,6 +1169,13 @@ xfs_fs_parse_param(
>  	struct fs_parse_result	result;
>  	int			size = 0;
>  	int			opt;
> +	uint64_t                prev_m_flags = 0; /* Mount flags of prev. mount */

Nit: spaces here^^^^^^^^^^^^^^^^ should be tabs.

> +	bool			remounting = fc->purpose & FS_CONTEXT_FOR_RECONFIGURE;
> +
> +	/* if reconfiguring then get mount flags of previous flags */
> +	if (remounting) {
> +		prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
> +	}
>  
>  	opt = fs_parse(fc, xfs_fs_parameters, param, &result);
>  	if (opt < 0)
> @@ -1294,19 +1301,27 @@ xfs_fs_parse_param(
>  #endif
>  	/* Following mount options will be removed in September 2025 */
>  	case Opt_ikeep:
> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		if (!remounting ||  !(prev_m_flags & XFS_MOUNT_IKEEP)) {
> +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		}

/me wonders if these could be refactored into a common helper, though I
can't really think of anything less clunky than:

static inline void
xfs_fs_warn_deprecated(
	struct fs_context	*fc,
	struct fs_parameter	*param)
	uint64_t		flag,
	bool			value);
{
	uint64_t		prev_m_flags;

	if (!(fc->purpose & FS_CONTEXT_FOR_RECONFIGURE))
		goto warn;
	prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
	if (!!(prev_m_flags & flag) == value)
		goto warn;
	return;
warn:
	xfs_warn(mp, "%s mount option is deprecated.", param->key);
}
...
	case Opt_ikeep:
		xfs_fs_warn_deprecated(fc, param, XFS_MOUNT_IKEEP, true);
		mp->m_flags |= XFS_MOUNT_IKEEP;
		break;
	case Opt_noikeep:
		xfs_fs_warn_deprecated(fc, param, XFS_MOUNT_IKEEP, false);
		mp->m_flags &= ~XFS_MOUNT_IKEEP;
		break;

Thoughts?

--D

>  		mp->m_flags |= XFS_MOUNT_IKEEP;
>  		return 0;
>  	case Opt_noikeep:
> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		if (!remounting || prev_m_flags & XFS_MOUNT_IKEEP) {
> +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		}
>  		mp->m_flags &= ~XFS_MOUNT_IKEEP;
>  		return 0;
>  	case Opt_attr2:
> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		if (!remounting || !(prev_m_flags & XFS_MOUNT_ATTR2)) {
> +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		}
>  		mp->m_flags |= XFS_MOUNT_ATTR2;
>  		return 0;
>  	case Opt_noattr2:
> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		if (!remounting || !(prev_m_flags & XFS_MOUNT_NOATTR2)) {
> +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		}
>  		mp->m_flags &= ~XFS_MOUNT_ATTR2;
>  		mp->m_flags |= XFS_MOUNT_NOATTR2;
>  		return 0;
> -- 
> 2.29.2
> 

Re: [PATCH 1/1] xfs: Skip repetitive warnings about mount options

[Dave Chinner]

On Mon, Feb 22, 2021 at 01:28:30PM -0800, Darrick J. Wong wrote:
> On Sat, Feb 20, 2021 at 11:15:49PM +0100, Pavel Reichl wrote:
> > Skip the warnings about mount option being deprecated if we are
> > remounting and deprecated option state is not changing.
> > 
> > Bug: https://bugzilla.kernel.org/show_bug.cgi?id=211605
> > Fix-suggested-by: Eric Sandeen <sandeen@redhat.com>
> > Signed-off-by: Pavel Reichl <preichl@redhat.com>
> > ---
> >  fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
> >  1 file changed, 19 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 813be879a5e5..6724a7018d1f 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> > @@ -1169,6 +1169,13 @@ xfs_fs_parse_param(
> >  	struct fs_parse_result	result;
> >  	int			size = 0;
> >  	int			opt;
> > +	uint64_t                prev_m_flags = 0; /* Mount flags of prev. mount */
> 
> Nit: spaces here^^^^^^^^^^^^^^^^ should be tabs.
> 
> > +	bool			remounting = fc->purpose & FS_CONTEXT_FOR_RECONFIGURE;
> > +
> > +	/* if reconfiguring then get mount flags of previous flags */
> > +	if (remounting) {
> > +		prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
> > +	}
> >  
> >  	opt = fs_parse(fc, xfs_fs_parameters, param, &result);
> >  	if (opt < 0)
> > @@ -1294,19 +1301,27 @@ xfs_fs_parse_param(
> >  #endif
> >  	/* Following mount options will be removed in September 2025 */
> >  	case Opt_ikeep:
> > -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> > +		if (!remounting ||  !(prev_m_flags & XFS_MOUNT_IKEEP)) {
> > +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
> > +		}
> 
> /me wonders if these could be refactored into a common helper, though I
> can't really think of anything less clunky than:
> 
> static inline void
> xfs_fs_warn_deprecated(
> 	struct fs_context	*fc,
> 	struct fs_parameter	*param)
> 	uint64_t		flag,
> 	bool			value);
> {
> 	uint64_t		prev_m_flags;
> 
> 	if (!(fc->purpose & FS_CONTEXT_FOR_RECONFIGURE))
> 		goto warn;
> 	prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
> 	if (!!(prev_m_flags & flag) == value)
> 		goto warn;
> 	return;
> warn:
> 	xfs_warn(mp, "%s mount option is deprecated.", param->key);
> }
> ...
> 	case Opt_ikeep:
> 		xfs_fs_warn_deprecated(fc, param, XFS_MOUNT_IKEEP, true);
> 		mp->m_flags |= XFS_MOUNT_IKEEP;
> 		break;
> 	case Opt_noikeep:
> 		xfs_fs_warn_deprecated(fc, param, XFS_MOUNT_IKEEP, false);
> 		mp->m_flags &= ~XFS_MOUNT_IKEEP;
> 		break;

	case Opt_ikeep:
		xfs_fs_warn_deprecated(fc, param,
				(mp->m_flags & XFS_MOUNT_IKEEP), true);
		mp->m_flags |= XFS_MOUNT_IKEEP;
		break;
	case Opt_noikeep:
		xfs_fs_warn_deprecated(fc, param,
				(mp->m_flags & XFS_MOUNT_IKEEP), false);
		mp->m_flags &= ~XFS_MOUNT_IKEEP;
		break;

static inline void
xfs_fs_warn_deprecated(
	struct fs_context	*fc,
	struct fs_parameter	*param)
	bool			old_value,
	bool			new_value);
{
	if ((fc->purpose & FS_CONTEXT_FOR_RECONFIGURE) &&
	    old_value == new_value)
		return;
	xfs_warn(mp, "%s mount option is deprecated.", param->key);
}

-Dave.

> 
> Thoughts?
> 
> --D
> 
> >  		mp->m_flags |= XFS_MOUNT_IKEEP;
> >  		return 0;
> >  	case Opt_noikeep:
> > -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> > +		if (!remounting || prev_m_flags & XFS_MOUNT_IKEEP) {
> > +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
> > +		}
> >  		mp->m_flags &= ~XFS_MOUNT_IKEEP;
> >  		return 0;
> >  	case Opt_attr2:
> > -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> > +		if (!remounting || !(prev_m_flags & XFS_MOUNT_ATTR2)) {
> > +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
> > +		}
> >  		mp->m_flags |= XFS_MOUNT_ATTR2;
> >  		return 0;
> >  	case Opt_noattr2:
> > -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> > +		if (!remounting || !(prev_m_flags & XFS_MOUNT_NOATTR2)) {
> > +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
> > +		}
> >  		mp->m_flags &= ~XFS_MOUNT_ATTR2;
> >  		mp->m_flags |= XFS_MOUNT_NOATTR2;
> >  		return 0;
> > -- 
> > 2.29.2
> > 
> 

-- 
Dave Chinner
david@fromorbit.com

Re: [PATCH 1/1] xfs: Skip repetitive warnings about mount options

[Eric Sandeen]

On 2/20/21 4:15 PM, Pavel Reichl wrote:
> Skip the warnings about mount option being deprecated if we are
> remounting and deprecated option state is not changing.
> 
> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=211605
> Fix-suggested-by: Eric Sandeen <sandeen@redhat.com>
> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> ---
>  fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 813be879a5e5..6724a7018d1f 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -1169,6 +1169,13 @@ xfs_fs_parse_param(
>  	struct fs_parse_result	result;
>  	int			size = 0;
>  	int			opt;
> +	uint64_t                prev_m_flags = 0; /* Mount flags of prev. mount */
> +	bool			remounting = fc->purpose & FS_CONTEXT_FOR_RECONFIGURE;
> +
> +	/* if reconfiguring then get mount flags of previous flags */
> +	if (remounting) {
> +		prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;

I wonder, does mp->m_flags work just as well for this purpose? I do get lost
in how the mount api stashes things. I /think/ that the above is just a
long way of getting to mp->m_flags.

> +	}
>  
>  	opt = fs_parse(fc, xfs_fs_parameters, param, &result);
>  	if (opt < 0)
> @@ -1294,19 +1301,27 @@ xfs_fs_parse_param(
>  #endif
>  	/* Following mount options will be removed in September 2025 */
>  	case Opt_ikeep:
> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		if (!remounting ||  !(prev_m_flags & XFS_MOUNT_IKEEP)) {

while we're nitpicking whitespace, ^^ 2 spaces there

as for the prev_m_flags usage, does:

+		if (!remounting || !(mp->m_flags & XFS_MOUNT_IKEEP)) {

work just as well here or no?

> +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		}
>  		mp->m_flags |= XFS_MOUNT_IKEEP;
>  		return 0;
>  	case Opt_noikeep:
> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		if (!remounting || prev_m_flags & XFS_MOUNT_IKEEP) {

and I dunno, I think I'd like parentheses for clarity here i.e.:

+		if (!remounting || (prev_m_flags & XFS_MOUNT_IKEEP)) {

Thanks,
-Eric


> +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		}
>  		mp->m_flags &= ~XFS_MOUNT_IKEEP;
>  		return 0;
>  	case Opt_attr2:
> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		if (!remounting || !(prev_m_flags & XFS_MOUNT_ATTR2)) {
> +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		}
>  		mp->m_flags |= XFS_MOUNT_ATTR2;
>  		return 0;
>  	case Opt_noattr2:
> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		if (!remounting || !(prev_m_flags & XFS_MOUNT_NOATTR2)) {
> +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
> +		}
>  		mp->m_flags &= ~XFS_MOUNT_ATTR2;
>  		mp->m_flags |= XFS_MOUNT_NOATTR2;
>  		return 0;
> 

Re: [PATCH 1/1] xfs: Skip repetitive warnings about mount options

[Pavel Reichl]

On 2/22/21 11:19 PM, Eric Sandeen wrote:
> 
> On 2/20/21 4:15 PM, Pavel Reichl wrote:
>> Skip the warnings about mount option being deprecated if we are
>> remounting and deprecated option state is not changing.
>>
>> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=211605
>> Fix-suggested-by: Eric Sandeen <sandeen@redhat.com>
>> Signed-off-by: Pavel Reichl <preichl@redhat.com>
>> ---
>>  fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>> index 813be879a5e5..6724a7018d1f 100644
>> --- a/fs/xfs/xfs_super.c
>> +++ b/fs/xfs/xfs_super.c
>> @@ -1169,6 +1169,13 @@ xfs_fs_parse_param(
>>  	struct fs_parse_result	result;
>>  	int			size = 0;
>>  	int			opt;
>> +	uint64_t                prev_m_flags = 0; /* Mount flags of prev. mount */
>> +	bool			remounting = fc->purpose & FS_CONTEXT_FOR_RECONFIGURE;
>> +
>> +	/* if reconfiguring then get mount flags of previous flags */
>> +	if (remounting) {
>> +		prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
> 
> I wonder, does mp->m_flags work just as well for this purpose? I do get lost
> in how the mount api stashes things. I /think/ that the above is just a
> long way of getting to mp->m_flags.

Hi Eric, I'm sorry to disagree, but I think that mp->m_flags is newly allocated for this mount and it's not populated with previous mount's mount options.

static int xfs_init_fs_context(
        struct fs_context       *fc)
{
        struct xfs_mount        *mp;

So here it's allocated and zeroed

        mp = kmem_alloc(sizeof(struct xfs_mount), KM_ZERO);
        if (!mp)
                return -ENOMEM;
                
...

a few flags are set by values from super block, but not nearly all of them

        /*
         * Copy binary VFS mount flags we are interested in.
         */
        if (fc->sb_flags & SB_RDONLY)
                mp->m_flags |= XFS_MOUNT_RDONLY;
        if (fc->sb_flags & SB_DIRSYNC)
                mp->m_flags |= XFS_MOUNT_DIRSYNC;
        if (fc->sb_flags & SB_SYNCHRONOUS)
                mp->m_flags |= XFS_MOUNT_WSYNC;

here it's assigned

        fc->s_fs_info = mp;

...

> 
>> +	}
>>  
>>  	opt = fs_parse(fc, xfs_fs_parameters, param, &result);
>>  	if (opt < 0)
>> @@ -1294,19 +1301,27 @@ xfs_fs_parse_param(
>>  #endif
>>  	/* Following mount options will be removed in September 2025 */
>>  	case Opt_ikeep:
>> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
>> +		if (!remounting ||  !(prev_m_flags & XFS_MOUNT_IKEEP)) {
> 
> while we're nitpicking whitespace, ^^ 2 spaces there
> 
> as for the prev_m_flags usage, does:
> 
> +		if (!remounting || !(mp->m_flags & XFS_MOUNT_IKEEP)) {
> 
> work just as well here or no?

I don't think so - while developing the path I printk-ed  the value and it did not reflect the mount options of previous mount...IIRC we discussed it and you hinted me to use the XFS_M(fc->root->d_sb)->m_flags...which works.

So I'm a bit confused by your comment, but I get confused a lot :-)

> 
>> +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
>> +		}
>>  		mp->m_flags |= XFS_MOUNT_IKEEP;
>>  		return 0;
>>  	case Opt_noikeep:
>> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
>> +		if (!remounting || prev_m_flags & XFS_MOUNT_IKEEP) {
> 
> and I dunno, I think I'd like parentheses for clarity here i.e.:
> 
> +		if (!remounting || (prev_m_flags & XFS_MOUNT_IKEEP)) {
> 
> Thanks,
> -Eric
> 
> 
>> +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
>> +		}
>>  		mp->m_flags &= ~XFS_MOUNT_IKEEP;
>>  		return 0;
>>  	case Opt_attr2:
>> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
>> +		if (!remounting || !(prev_m_flags & XFS_MOUNT_ATTR2)) {
>> +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
>> +		}
>>  		mp->m_flags |= XFS_MOUNT_ATTR2;
>>  		return 0;
>>  	case Opt_noattr2:
>> -		xfs_warn(mp, "%s mount option is deprecated.", param->key);
>> +		if (!remounting || !(prev_m_flags & XFS_MOUNT_NOATTR2)) {
>> +			xfs_warn(mp, "%s mount option is deprecated.", param->key);
>> +		}
>>  		mp->m_flags &= ~XFS_MOUNT_ATTR2;
>>  		mp->m_flags |= XFS_MOUNT_NOATTR2;
>>  		return 0;
>>
> 

Re: [PATCH 1/1] xfs: Skip repetitive warnings about mount options

[Eric Sandeen]

On 2/23/21 11:53 AM, Pavel Reichl wrot
> 
> On 2/22/21 11:19 PM, Eric Sandeen wrote:
>>
>> On 2/20/21 4:15 PM, Pavel Reichl wrote:
>>> Skip the warnings about mount option being deprecated if we are
>>> remounting and deprecated option state is not changing.
>>>
>>> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=211605
>>> Fix-suggested-by: Eric Sandeen <sandeen@redhat.com>
>>> Signed-off-by: Pavel Reichl <preichl@redhat.com>
>>> ---
>>>  fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
>>>  1 file changed, 19 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
>>> index 813be879a5e5..6724a7018d1f 100644
>>> --- a/fs/xfs/xfs_super.c
>>> +++ b/fs/xfs/xfs_super.c
>>> @@ -1169,6 +1169,13 @@ xfs_fs_parse_param(
>>>  	struct fs_parse_result	result;
>>>  	int			size = 0;
>>>  	int			opt;
>>> +	uint64_t                prev_m_flags = 0; /* Mount flags of prev. mount */
>>> +	bool			remounting = fc->purpose & FS_CONTEXT_FOR_RECONFIGURE;
>>> +
>>> +	/* if reconfiguring then get mount flags of previous flags */
>>> +	if (remounting) {
>>> +		prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
>>
>> I wonder, does mp->m_flags work just as well for this purpose? I do get lost
>> in how the mount api stashes things. I /think/ that the above is just a
>> long way of getting to mp->m_flags.
> 
> Hi Eric, I'm sorry to disagree, but I think that mp->m_flags is newly allocated for this mount and it's not populated with previous mount's mount options.

No need to be sorry ;) And in any case, you're corrrect here.

> 
> static int xfs_init_fs_context(
>         struct fs_context       *fc)
> {
>         struct xfs_mount        *mp;
> 
> So here it's allocated and zeroed
> 
>         mp = kmem_alloc(sizeof(struct xfs_mount), KM_ZERO);
>         if (!mp)
>                 return -ENOMEM;
>                 
> ...

and eventually:

	fc->s_fs_info = mp;

Ok, yup, I see.  so I guess we kind of have:

*parsing_mp = fc->s_fs_info;

and 

*current_mp = XFS_M(fc->root->d_sb);

(variable names not actually in the code, just used for example)

Sorry for the noise, my mistake!

-Eric

Re: [PATCH 1/1] xfs: Skip repetitive warnings about mount options

[Darrick J. Wong]

On Tue, Feb 23, 2021 at 12:10:41PM -0600, Eric Sandeen wrote:
> On 2/23/21 11:53 AM, Pavel Reichl wrot
> > 
> > On 2/22/21 11:19 PM, Eric Sandeen wrote:
> >>
> >> On 2/20/21 4:15 PM, Pavel Reichl wrote:
> >>> Skip the warnings about mount option being deprecated if we are
> >>> remounting and deprecated option state is not changing.
> >>>
> >>> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=211605
> >>> Fix-suggested-by: Eric Sandeen <sandeen@redhat.com>
> >>> Signed-off-by: Pavel Reichl <preichl@redhat.com>
> >>> ---
> >>>  fs/xfs/xfs_super.c | 23 +++++++++++++++++++----
> >>>  1 file changed, 19 insertions(+), 4 deletions(-)
> >>>
> >>> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> >>> index 813be879a5e5..6724a7018d1f 100644
> >>> --- a/fs/xfs/xfs_super.c
> >>> +++ b/fs/xfs/xfs_super.c
> >>> @@ -1169,6 +1169,13 @@ xfs_fs_parse_param(
> >>>  	struct fs_parse_result	result;
> >>>  	int			size = 0;
> >>>  	int			opt;
> >>> +	uint64_t                prev_m_flags = 0; /* Mount flags of prev. mount */
> >>> +	bool			remounting = fc->purpose & FS_CONTEXT_FOR_RECONFIGURE;
> >>> +
> >>> +	/* if reconfiguring then get mount flags of previous flags */
> >>> +	if (remounting) {
> >>> +		prev_m_flags  = XFS_M(fc->root->d_sb)->m_flags;
> >>
> >> I wonder, does mp->m_flags work just as well for this purpose? I do get lost
> >> in how the mount api stashes things. I /think/ that the above is just a
> >> long way of getting to mp->m_flags.
> > 
> > Hi Eric, I'm sorry to disagree, but I think that mp->m_flags is
> > newly allocated for this mount and it's not populated with previous
> > mount's mount options.

Yeah, that's one of the (IMHO) ugliest warts of the new fs parsing code.

> No need to be sorry ;) And in any case, you're corrrect here.
> 
> > 
> > static int xfs_init_fs_context(
> >         struct fs_context       *fc)
> > {
> >         struct xfs_mount        *mp;
> > 
> > So here it's allocated and zeroed
> > 
> >         mp = kmem_alloc(sizeof(struct xfs_mount), KM_ZERO);
> >         if (!mp)
> >                 return -ENOMEM;
> >                 
> > ...
> 
> and eventually:
> 
> 	fc->s_fs_info = mp;
> 
> Ok, yup, I see.  so I guess we kind of have:
> 
> *parsing_mp = fc->s_fs_info;
> 
> and 
> 
> *current_mp = XFS_M(fc->root->d_sb);
> 
> (variable names not actually in the code, just used for example)

Maybe they should be. ;)

--D

> Sorry for the noise, my mistake!
> 
> -Eric

Re: [PATCH 1/1] xfs: Skip repetitive warnings about mount options

[Eric Sandeen]

On 2/23/21 12:25 PM, Darrick J. Wong wrote:

>> Ok, yup, I see.  so I guess we kind of have:
>>
>> *parsing_mp = fc->s_fs_info;
>>
>> and 
>>
>> *current_mp = XFS_M(fc->root->d_sb);
>>
>> (variable names not actually in the code, just used for example)
> 
> Maybe they should be. ;)

Yup I almost suggested at least a comment. And since we do care about the
actual/live *mp now, maybe adding self-describing variable names would be a
help.

-Eric

> --D
> 
>> Sorry for the noise, my mistake!
>>
>> -Eric
>