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@xxxxxxxxxx> >> Signed-off-by: Pavel Reichl <preichl@xxxxxxxxxx> >> --- >> 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; >> >
