* xfs: Skip repetitive warnings about mount options
@ 2021-02-20 22:15 Pavel Reichl
2021-02-20 22:15 ` [PATCH] xfs: Add test for printing deprec. " Pavel Reichl
2021-02-20 22:15 ` [PATCH 1/1] xfs: Skip repetitive warnings about " Pavel Reichl
0 siblings, 2 replies; 13+ messages in thread
From: Pavel Reichl @ 2021-02-20 22:15 UTC (permalink / raw)
To: linux-xfs
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH] xfs: Add test for printing deprec. mount options
2021-02-20 22:15 xfs: Skip repetitive warnings about mount options Pavel Reichl
@ 2021-02-20 22:15 ` Pavel Reichl
2021-02-22 21:22 ` Darrick J. Wong
2021-02-20 22:15 ` [PATCH 1/1] xfs: Skip repetitive warnings about " Pavel Reichl
1 sibling, 1 reply; 13+ messages in thread
From: Pavel Reichl @ 2021-02-20 22:15 UTC (permalink / raw)
To: linux-xfs
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [PATCH 1/1] xfs: Skip repetitive warnings about mount options
2021-02-20 22:15 xfs: Skip repetitive warnings about mount options Pavel Reichl
2021-02-20 22:15 ` [PATCH] xfs: Add test for printing deprec. " Pavel Reichl
@ 2021-02-20 22:15 ` Pavel Reichl
2021-02-22 21:28 ` Darrick J. Wong
2021-02-22 22:19 ` Eric Sandeen
1 sibling, 2 replies; 13+ messages in thread
From: Pavel Reichl @ 2021-02-20 22:15 UTC (permalink / raw)
To: linux-xfs
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
^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: [PATCH] xfs: Add test for printing deprec. mount options
2021-02-20 22:15 ` [PATCH] xfs: Add test for printing deprec. " Pavel Reichl
@ 2021-02-22 21:22 ` Darrick J. Wong
2021-02-23 16:41 ` Pavel Reichl
0 siblings, 1 reply; 13+ messages in thread
From: Darrick J. Wong @ 2021-02-22 21:22 UTC (permalink / raw)
To: Pavel Reichl; +Cc: linux-xfs
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] xfs: Skip repetitive warnings about mount options
2021-02-20 22:15 ` [PATCH 1/1] xfs: Skip repetitive warnings about " Pavel Reichl
@ 2021-02-22 21:28 ` Darrick J. Wong
2021-02-23 4:32 ` Dave Chinner
2021-02-22 22:19 ` Eric Sandeen
1 sibling, 1 reply; 13+ messages in thread
From: Darrick J. Wong @ 2021-02-22 21:28 UTC (permalink / raw)
To: Pavel Reichl; +Cc: linux-xfs
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] xfs: Skip repetitive warnings about mount options
2021-02-20 22:15 ` [PATCH 1/1] xfs: Skip repetitive warnings about " Pavel Reichl
2021-02-22 21:28 ` Darrick J. Wong
@ 2021-02-22 22:19 ` Eric Sandeen
2021-02-23 17:53 ` Pavel Reichl
1 sibling, 1 reply; 13+ messages in thread
From: Eric Sandeen @ 2021-02-22 22:19 UTC (permalink / raw)
To: Pavel Reichl, linux-xfs
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;
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] xfs: Skip repetitive warnings about mount options
2021-02-22 21:28 ` Darrick J. Wong
@ 2021-02-23 4:32 ` Dave Chinner
0 siblings, 0 replies; 13+ messages in thread
From: Dave Chinner @ 2021-02-23 4:32 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Pavel Reichl, linux-xfs
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfs: Add test for printing deprec. mount options
2021-02-22 21:22 ` Darrick J. Wong
@ 2021-02-23 16:41 ` Pavel Reichl
2021-02-23 16:54 ` Darrick J. Wong
0 siblings, 1 reply; 13+ messages in thread
From: Pavel Reichl @ 2021-02-23 16:41 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: linux-xfs
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
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH] xfs: Add test for printing deprec. mount options
2021-02-23 16:41 ` Pavel Reichl
@ 2021-02-23 16:54 ` Darrick J. Wong
0 siblings, 0 replies; 13+ messages in thread
From: Darrick J. Wong @ 2021-02-23 16:54 UTC (permalink / raw)
To: Pavel Reichl; +Cc: linux-xfs
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
> >>
> >
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] xfs: Skip repetitive warnings about mount options
2021-02-22 22:19 ` Eric Sandeen
@ 2021-02-23 17:53 ` Pavel Reichl
2021-02-23 18:10 ` Eric Sandeen
0 siblings, 1 reply; 13+ messages in thread
From: Pavel Reichl @ 2021-02-23 17:53 UTC (permalink / raw)
To: Eric Sandeen, linux-xfs
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;
>>
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] xfs: Skip repetitive warnings about mount options
2021-02-23 17:53 ` Pavel Reichl
@ 2021-02-23 18:10 ` Eric Sandeen
2021-02-23 18:25 ` Darrick J. Wong
0 siblings, 1 reply; 13+ messages in thread
From: Eric Sandeen @ 2021-02-23 18:10 UTC (permalink / raw)
To: Pavel Reichl, linux-xfs
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] xfs: Skip repetitive warnings about mount options
2021-02-23 18:10 ` Eric Sandeen
@ 2021-02-23 18:25 ` Darrick J. Wong
2021-02-23 21:05 ` Eric Sandeen
0 siblings, 1 reply; 13+ messages in thread
From: Darrick J. Wong @ 2021-02-23 18:25 UTC (permalink / raw)
To: Eric Sandeen; +Cc: Pavel Reichl, linux-xfs
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
^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: [PATCH 1/1] xfs: Skip repetitive warnings about mount options
2021-02-23 18:25 ` Darrick J. Wong
@ 2021-02-23 21:05 ` Eric Sandeen
0 siblings, 0 replies; 13+ messages in thread
From: Eric Sandeen @ 2021-02-23 21:05 UTC (permalink / raw)
To: Darrick J. Wong; +Cc: Pavel Reichl, linux-xfs
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
>
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2021-02-23 21:06 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-20 22:15 xfs: Skip repetitive warnings about mount options Pavel Reichl
2021-02-20 22:15 ` [PATCH] xfs: Add test for printing deprec. " Pavel Reichl
2021-02-22 21:22 ` Darrick J. Wong
2021-02-23 16:41 ` Pavel Reichl
2021-02-23 16:54 ` Darrick J. Wong
2021-02-20 22:15 ` [PATCH 1/1] xfs: Skip repetitive warnings about " Pavel Reichl
2021-02-22 21:28 ` Darrick J. Wong
2021-02-23 4:32 ` Dave Chinner
2021-02-22 22:19 ` Eric Sandeen
2021-02-23 17:53 ` Pavel Reichl
2021-02-23 18:10 ` Eric Sandeen
2021-02-23 18:25 ` Darrick J. Wong
2021-02-23 21:05 ` Eric Sandeen
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).