| Message ID | 20200924170747.65876-2-preichl@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | xfs: remove deprecated mount and sysctl options | expand |
On Thu, Sep 24, 2020 at 07:07:46PM +0200, Pavel Reichl wrote: > ikeep/noikeep was a workaround for old DMAPI code which is no longer > relevant. > > attr2/noattr2 - is for controlling upgrade behaviour from fixed attribute > fork sizes in the inode (attr1) and dynamic attribute fork sizes (attr2). > mkfs has defaulted to setting attr2 since 2007, hence just about every > XFS filesystem out there in production right now uses attr2. > > Signed-off-by: Pavel Reichl <preichl@redhat.com> > --- > Documentation/admin-guide/xfs.rst | 2 ++ > fs/xfs/xfs_super.c | 30 +++++++++++++++++------------- > 2 files changed, 19 insertions(+), 13 deletions(-) > > diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst > index f461d6c33534..413f68efccc0 100644 > --- a/Documentation/admin-guide/xfs.rst > +++ b/Documentation/admin-guide/xfs.rst > @@ -217,6 +217,8 @@ Deprecated Mount Options > =========================== ================ > Name Removal Schedule > =========================== ================ > + ikeep/noikeep TBD > + attr2/noattr2 TBD Er... what date did you have in mind? > =========================== ================ > > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > index 71ac6c1cdc36..4c26b283b7d8 100644 > --- a/fs/xfs/xfs_super.c > +++ b/fs/xfs/xfs_super.c > @@ -1234,25 +1234,12 @@ xfs_fc_parse_param( > case Opt_nouuid: > mp->m_flags |= XFS_MOUNT_NOUUID; > return 0; > - case Opt_ikeep: > - mp->m_flags |= XFS_MOUNT_IKEEP; > - return 0; > - case Opt_noikeep: > - mp->m_flags &= ~XFS_MOUNT_IKEEP; > - return 0; > case Opt_largeio: > mp->m_flags |= XFS_MOUNT_LARGEIO; > return 0; > case Opt_nolargeio: > mp->m_flags &= ~XFS_MOUNT_LARGEIO; > return 0; > - case Opt_attr2: > - mp->m_flags |= XFS_MOUNT_ATTR2; > - return 0; > - case Opt_noattr2: > - mp->m_flags &= ~XFS_MOUNT_ATTR2; > - mp->m_flags |= XFS_MOUNT_NOATTR2; > - return 0; > case Opt_filestreams: > mp->m_flags |= XFS_MOUNT_FILESTREAMS; > return 0; > @@ -1304,6 +1291,23 @@ xfs_fc_parse_param( > xfs_mount_set_dax_mode(mp, result.uint_32); > return 0; > #endif > + case Opt_ikeep: > + xfs_warn(mp, "%s mount option is deprecated.", param->key); > + mp->m_flags |= XFS_MOUNT_IKEEP; It's a little odd that you didn't then remove these XFS_MOUNT_ flags. It's strange to declare a mount option deprecated but still have it change behavior. In this case, I guess we should keep ikeep/noikeep in the mount options table so that scripts won't fail, but then we remove XFS_MOUNT_IKEEP and change the codebase to always take the IKEEP behavior and delete the code that handled the !IKEEP behavior. > + return 0; > + case Opt_noikeep: > + 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); > + mp->m_flags |= XFS_MOUNT_ATTR2; If the kernel /does/ encounter an attr1 filesystem, what will it do now? IIRC the default (if there is no attr2/noattr2 mount option) is to auto-upgrade the fs, right? So will we stop doing that, or are we making the upgrade mandatory now? > + return 0; > + case Opt_noattr2: > + xfs_warn(mp, "%s mount option is deprecated.", param->key); > + mp->m_flags &= ~XFS_MOUNT_ATTR2; > + mp->m_flags |= XFS_MOUNT_NOATTR2; Also, uh, why move these code hunks? --D > + return 0; > default: > xfs_warn(mp, "unknown mount option [%s].", param->key); > return -EINVAL; > -- > 2.26.2 >
On 9/24/20 12:26 PM, Darrick J. Wong wrote: > On Thu, Sep 24, 2020 at 07:07:46PM +0200, Pavel Reichl wrote: >> ikeep/noikeep was a workaround for old DMAPI code which is no longer >> relevant. >> >> attr2/noattr2 - is for controlling upgrade behaviour from fixed attribute >> fork sizes in the inode (attr1) and dynamic attribute fork sizes (attr2). >> mkfs has defaulted to setting attr2 since 2007, hence just about every >> XFS filesystem out there in production right now uses attr2. >> >> Signed-off-by: Pavel Reichl <preichl@redhat.com> >> --- >> Documentation/admin-guide/xfs.rst | 2 ++ >> fs/xfs/xfs_super.c | 30 +++++++++++++++++------------- >> 2 files changed, 19 insertions(+), 13 deletions(-) >> >> diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst >> index f461d6c33534..413f68efccc0 100644 >> --- a/Documentation/admin-guide/xfs.rst >> +++ b/Documentation/admin-guide/xfs.rst >> @@ -217,6 +217,8 @@ Deprecated Mount Options >> =========================== ================ >> Name Removal Schedule >> =========================== ================ >> + ikeep/noikeep TBD >> + attr2/noattr2 TBD > > Er... what date did you have in mind? > >> =========================== ================ >> >> >> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c >> index 71ac6c1cdc36..4c26b283b7d8 100644 >> --- a/fs/xfs/xfs_super.c >> +++ b/fs/xfs/xfs_super.c >> @@ -1234,25 +1234,12 @@ xfs_fc_parse_param( >> case Opt_nouuid: >> mp->m_flags |= XFS_MOUNT_NOUUID; >> return 0; >> - case Opt_ikeep: >> - mp->m_flags |= XFS_MOUNT_IKEEP; >> - return 0; >> - case Opt_noikeep: >> - mp->m_flags &= ~XFS_MOUNT_IKEEP; >> - return 0; >> case Opt_largeio: >> mp->m_flags |= XFS_MOUNT_LARGEIO; >> return 0; >> case Opt_nolargeio: >> mp->m_flags &= ~XFS_MOUNT_LARGEIO; >> return 0; >> - case Opt_attr2: >> - mp->m_flags |= XFS_MOUNT_ATTR2; >> - return 0; >> - case Opt_noattr2: >> - mp->m_flags &= ~XFS_MOUNT_ATTR2; >> - mp->m_flags |= XFS_MOUNT_NOATTR2; >> - return 0; >> case Opt_filestreams: >> mp->m_flags |= XFS_MOUNT_FILESTREAMS; >> return 0; >> @@ -1304,6 +1291,23 @@ xfs_fc_parse_param( >> xfs_mount_set_dax_mode(mp, result.uint_32); >> return 0; >> #endif >> + case Opt_ikeep: >> + xfs_warn(mp, "%s mount option is deprecated.", param->key); >> + mp->m_flags |= XFS_MOUNT_IKEEP; > > It's a little odd that you didn't then remove these XFS_MOUNT_ flags. > It's strange to declare a mount option deprecated but still have it > change behavior. but ... this doesn't change behavior, right? The flag is still set. I think it makes sense to announce deprecation, with a date set for future removal, but keep all other behavior the same. That gives people who still need it (if any exist) time to complain, right? > In this case, I guess we should keep ikeep/noikeep in the mount options > table so that scripts won't fail, but then we remove XFS_MOUNT_IKEEP and > change the codebase to always take the IKEEP behavior and delete the > code that handled the !IKEEP behavior. > >> + return 0; >> + case Opt_noikeep: >> + 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); >> + mp->m_flags |= XFS_MOUNT_ATTR2; > > If the kernel /does/ encounter an attr1 filesystem, what will it do now? The same as it did yesterday; the flag is still set for now. > IIRC the default (if there is no attr2/noattr2 mount option) is to > auto-upgrade the fs, right? So will we stop doing that, or are we > making the upgrade mandatory now? > >> + return 0; >> + case Opt_noattr2: >> + xfs_warn(mp, "%s mount option is deprecated.", param->key); >> + mp->m_flags &= ~XFS_MOUNT_ATTR2; >> + mp->m_flags |= XFS_MOUNT_NOATTR2; > > Also, uh, why move these code hunks? That's my fault, I had suggested moving all the deprecated options to the end. Maybe with a comment, /* REMOVE ME 2089 */ or whatever we pick? -Eric
On Thu, Sep 24, 2020 at 12:39:07PM -0500, Eric Sandeen wrote: > On 9/24/20 12:26 PM, Darrick J. Wong wrote: > > On Thu, Sep 24, 2020 at 07:07:46PM +0200, Pavel Reichl wrote: > >> ikeep/noikeep was a workaround for old DMAPI code which is no longer > >> relevant. > >> > >> attr2/noattr2 - is for controlling upgrade behaviour from fixed attribute > >> fork sizes in the inode (attr1) and dynamic attribute fork sizes (attr2). > >> mkfs has defaulted to setting attr2 since 2007, hence just about every > >> XFS filesystem out there in production right now uses attr2. > >> > >> Signed-off-by: Pavel Reichl <preichl@redhat.com> > >> --- > >> Documentation/admin-guide/xfs.rst | 2 ++ > >> fs/xfs/xfs_super.c | 30 +++++++++++++++++------------- > >> 2 files changed, 19 insertions(+), 13 deletions(-) > >> > >> diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst > >> index f461d6c33534..413f68efccc0 100644 > >> --- a/Documentation/admin-guide/xfs.rst > >> +++ b/Documentation/admin-guide/xfs.rst > >> @@ -217,6 +217,8 @@ Deprecated Mount Options > >> =========================== ================ > >> Name Removal Schedule > >> =========================== ================ > >> + ikeep/noikeep TBD > >> + attr2/noattr2 TBD > > > > Er... what date did you have in mind? June 65th, 2089 it is, then. ;) > >> =========================== ================ > >> > >> > >> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c > >> index 71ac6c1cdc36..4c26b283b7d8 100644 > >> --- a/fs/xfs/xfs_super.c > >> +++ b/fs/xfs/xfs_super.c > >> @@ -1234,25 +1234,12 @@ xfs_fc_parse_param( > >> case Opt_nouuid: > >> mp->m_flags |= XFS_MOUNT_NOUUID; > >> return 0; > >> - case Opt_ikeep: > >> - mp->m_flags |= XFS_MOUNT_IKEEP; > >> - return 0; > >> - case Opt_noikeep: > >> - mp->m_flags &= ~XFS_MOUNT_IKEEP; > >> - return 0; > >> case Opt_largeio: > >> mp->m_flags |= XFS_MOUNT_LARGEIO; > >> return 0; > >> case Opt_nolargeio: > >> mp->m_flags &= ~XFS_MOUNT_LARGEIO; > >> return 0; > >> - case Opt_attr2: > >> - mp->m_flags |= XFS_MOUNT_ATTR2; > >> - return 0; > >> - case Opt_noattr2: > >> - mp->m_flags &= ~XFS_MOUNT_ATTR2; > >> - mp->m_flags |= XFS_MOUNT_NOATTR2; > >> - return 0; > >> case Opt_filestreams: > >> mp->m_flags |= XFS_MOUNT_FILESTREAMS; > >> return 0; > >> @@ -1304,6 +1291,23 @@ xfs_fc_parse_param( > >> xfs_mount_set_dax_mode(mp, result.uint_32); > >> return 0; > >> #endif > >> + case Opt_ikeep: > >> + xfs_warn(mp, "%s mount option is deprecated.", param->key); > >> + mp->m_flags |= XFS_MOUNT_IKEEP; > > > > It's a little odd that you didn't then remove these XFS_MOUNT_ flags. > > It's strange to declare a mount option deprecated but still have it > > change behavior. > > but ... this doesn't change behavior, right? The flag is still set. > > I think it makes sense to announce deprecation, with a date set for future > removal, but keep all other behavior the same. That gives people who still > need it (if any exist) time to complain, right? Ok. I'm fine with these knobs continuing to affect xfs behavior right up to the deprecation date. > > In this case, I guess we should keep ikeep/noikeep in the mount options > > table so that scripts won't fail, but then we remove XFS_MOUNT_IKEEP and > > change the codebase to always take the IKEEP behavior and delete the > > code that handled the !IKEEP behavior. > > > >> + return 0; > >> + case Opt_noikeep: > >> + 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); > >> + mp->m_flags |= XFS_MOUNT_ATTR2; Side note: shouldn't this clause be clearing XFS_MOUNT_NOATTR2? > > > > If the kernel /does/ encounter an attr1 filesystem, what will it do now? > > The same as it did yesterday; the flag is still set for now. > > > IIRC the default (if there is no attr2/noattr2 mount option) is to > > auto-upgrade the fs, right? So will we stop doing that, or are we > > making the upgrade mandatory now? > > > >> + return 0; > >> + case Opt_noattr2: > >> + xfs_warn(mp, "%s mount option is deprecated.", param->key); > >> + mp->m_flags &= ~XFS_MOUNT_ATTR2; > >> + mp->m_flags |= XFS_MOUNT_NOATTR2; > > > > Also, uh, why move these code hunks? > > That's my fault, I had suggested moving all the deprecated options to the end. > > Maybe with a comment, /* REMOVE ME 2089 */ or whatever we pick? Ah. --D > -Eric
that handled the !IKEEP behavior. >>> >>>> + return 0; >>>> + case Opt_noikeep: >>>> + 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); >>>> + mp->m_flags |= XFS_MOUNT_ATTR2; > > Side note: shouldn't this clause be clearing XFS_MOUNT_NOATTR2? > I don't know but since nobody complained so far and we are actually starting work to remove it...I think it's safe to ignore that...?
Thanks for discussion, if I get it right, the only thing to change is to add the date when mount options will me removed (September 2025)?
On 9/25/20 8:40 AM, Pavel Reichl wrote:
> Thanks for discussion, if I get it right, the only thing to change is to add the date when mount options will me removed (September 2025)?
Please also add a comment above the moved mount options indicating that
all options below the comment are slated for deprecation.
Not sure if Darrick had anything else. Are we happy w/ the kernel logging?
On Fri, Sep 25, 2020 at 09:50:45AM -0500, Eric Sandeen wrote: > On 9/25/20 8:40 AM, Pavel Reichl wrote: > > Thanks for discussion, if I get it right, the only thing to change is to add the date when mount options will me removed (September 2025)? > > Please also add a comment above the moved mount options indicating that > all options below the comment are slated for deprecation. > > Not sure if Darrick had anything else. Are we happy w/ the kernel logging? Yeah, I'm fine with it. :) --D
diff --git a/Documentation/admin-guide/xfs.rst b/Documentation/admin-guide/xfs.rst index f461d6c33534..413f68efccc0 100644 --- a/Documentation/admin-guide/xfs.rst +++ b/Documentation/admin-guide/xfs.rst @@ -217,6 +217,8 @@ Deprecated Mount Options =========================== ================ Name Removal Schedule =========================== ================ + ikeep/noikeep TBD + attr2/noattr2 TBD =========================== ================ diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c index 71ac6c1cdc36..4c26b283b7d8 100644 --- a/fs/xfs/xfs_super.c +++ b/fs/xfs/xfs_super.c @@ -1234,25 +1234,12 @@ xfs_fc_parse_param( case Opt_nouuid: mp->m_flags |= XFS_MOUNT_NOUUID; return 0; - case Opt_ikeep: - mp->m_flags |= XFS_MOUNT_IKEEP; - return 0; - case Opt_noikeep: - mp->m_flags &= ~XFS_MOUNT_IKEEP; - return 0; case Opt_largeio: mp->m_flags |= XFS_MOUNT_LARGEIO; return 0; case Opt_nolargeio: mp->m_flags &= ~XFS_MOUNT_LARGEIO; return 0; - case Opt_attr2: - mp->m_flags |= XFS_MOUNT_ATTR2; - return 0; - case Opt_noattr2: - mp->m_flags &= ~XFS_MOUNT_ATTR2; - mp->m_flags |= XFS_MOUNT_NOATTR2; - return 0; case Opt_filestreams: mp->m_flags |= XFS_MOUNT_FILESTREAMS; return 0; @@ -1304,6 +1291,23 @@ xfs_fc_parse_param( xfs_mount_set_dax_mode(mp, result.uint_32); return 0; #endif + case Opt_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); + mp->m_flags &= ~XFS_MOUNT_IKEEP; + return 0; + case Opt_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); + mp->m_flags &= ~XFS_MOUNT_ATTR2; + mp->m_flags |= XFS_MOUNT_NOATTR2; + return 0; default: xfs_warn(mp, "unknown mount option [%s].", param->key); return -EINVAL;
ikeep/noikeep was a workaround for old DMAPI code which is no longer relevant. attr2/noattr2 - is for controlling upgrade behaviour from fixed attribute fork sizes in the inode (attr1) and dynamic attribute fork sizes (attr2). mkfs has defaulted to setting attr2 since 2007, hence just about every XFS filesystem out there in production right now uses attr2. Signed-off-by: Pavel Reichl <preichl@redhat.com> --- Documentation/admin-guide/xfs.rst | 2 ++ fs/xfs/xfs_super.c | 30 +++++++++++++++++------------- 2 files changed, 19 insertions(+), 13 deletions(-)