[2/2] xfs: deprecate barrier/nobarrier mount option

Commit Message

Dave Chinner Nov. 30, 2016, 10:54 p.m. UTC

From: Dave Chinner <[e-mail]>

We always perform integrity operations now, so these mount options
don't do anything. Deprecate them and mark them for removal in
in a year.

Signed-Off-By: Dave Chinner <[e-mail]>
---
 Documentation/filesystems/xfs.txt | 12 ++++--------
 fs/xfs/xfs_super.c                | 25 ++++++++++++++++---------
 2 files changed, 20 insertions(+), 17 deletions(-)

Comments

Jan Kara Dec. 1, 2016, 9:54 a.m. UTC | #1

On Thu 01-12-16 09:54:44, Dave Chinner wrote:
> From: Dave Chinner <[e-mail]>
> 
> We always perform integrity operations now, so these mount options
> don't do anything. Deprecate them and mark them for removal in
> in a year.
> 
> Signed-Off-By: Dave Chinner <[e-mail]>

We use 'nobarrier' mount option to simulate disks with battery-backed
caches in some of our IO performance testing. I don't say we cannot live
with this mount option but it was convenient...

								Honza

> ---
>  Documentation/filesystems/xfs.txt | 12 ++++--------
>  fs/xfs/xfs_super.c                | 25 ++++++++++++++++---------
>  2 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/Documentation/filesystems/xfs.txt b/Documentation/filesystems/xfs.txt
> index c2d44e6e117b..68a057c8fccf 100644
> --- a/Documentation/filesystems/xfs.txt
> +++ b/Documentation/filesystems/xfs.txt
> @@ -51,13 +51,6 @@ default behaviour.
>  	CRC enabled filesystems always use the attr2 format, and so
>  	will reject the noattr2 mount option if it is set.
>  
> -  barrier (*)
> -  nobarrier
> -	Enables/disables the use of block layer write barriers for
> -	writes into the journal and for data integrity operations.
> -	This allows for drive level write caching to be enabled, for
> -	devices that support write barriers.
> -
>    discard
>    nodiscard (*)
>  	Enable/disable the issuing of commands to let the block
> @@ -228,7 +221,10 @@ default behaviour.
>  Deprecated Mount Options
>  ========================
>  
> -None at present.
> +  Name				Removal Schedule
> +  ----				----------------
> +  barrier			December 2017
> +  nobarrier			December 2017
>  
>  
>  Removed Mount Options
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 563d1d146b8c..eecbaac08eba 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
> @@ -104,9 +104,6 @@ static const match_table_t tokens = {
>  	{Opt_sysvgroups,"sysvgroups"},	/* group-ID from current process */
>  	{Opt_allocsize,	"allocsize=%s"},/* preferred allocation size */
>  	{Opt_norecovery,"norecovery"},	/* don't run XFS recovery */
> -	{Opt_barrier,	"barrier"},	/* use writer barriers for log write and
> -					 * unwritten extent conversion */
> -	{Opt_nobarrier,	"nobarrier"},	/* .. disable */
>  	{Opt_inode64,	"inode64"},	/* inodes can be allocated anywhere */
>  	{Opt_inode32,   "inode32"},	/* inode allocation limited to
>  					 * XFS_MAXINUMBER_32 */
> @@ -134,6 +131,12 @@ static const match_table_t tokens = {
>  	{Opt_nodiscard,	"nodiscard"},	/* Do not discard unused blocks */
>  
>  	{Opt_dax,	"dax"},		/* Enable direct access to bdev pages */
> +
> +	/* Deprecated mount options scheduled for removal */
> +	{Opt_barrier,	"barrier"},	/* use writer barriers for log write and
> +					 * unwritten extent conversion */
> +	{Opt_nobarrier,	"nobarrier"},	/* .. disable */
> +
>  	{Opt_err,	NULL},
>  };
>  
> @@ -301,12 +304,6 @@ xfs_parseargs(
>  		case Opt_nouuid:
>  			mp->m_flags |= XFS_MOUNT_NOUUID;
>  			break;
> -		case Opt_barrier:
> -			mp->m_flags |= XFS_MOUNT_BARRIER;
> -			break;
> -		case Opt_nobarrier:
> -			mp->m_flags &= ~XFS_MOUNT_BARRIER;
> -			break;
>  		case Opt_ikeep:
>  			mp->m_flags |= XFS_MOUNT_IKEEP;
>  			break;
> @@ -374,6 +371,14 @@ xfs_parseargs(
>  			mp->m_flags |= XFS_MOUNT_DAX;
>  			break;
>  #endif
> +		case Opt_barrier:
> +			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
> +			mp->m_flags |= XFS_MOUNT_BARRIER;
> +			break;
> +		case Opt_nobarrier:
> +			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
> +			mp->m_flags &= ~XFS_MOUNT_BARRIER;
> +			break;
>  		default:
>  			xfs_warn(mp, "unknown mount option [%s].", p);
>  			return -EINVAL;
> @@ -1238,9 +1243,11 @@ xfs_fs_remount(
>  		token = match_token(p, tokens, args);
>  		switch (token) {
>  		case Opt_barrier:
> +			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
>  			mp->m_flags |= XFS_MOUNT_BARRIER;
>  			break;
>  		case Opt_nobarrier:
> +			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
>  			mp->m_flags &= ~XFS_MOUNT_BARRIER;
>  			break;
>  		case Opt_inode64:
> -- 
> 2.10.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoph Hellwig Dec. 1, 2016, 9:59 a.m. UTC | #2

On Thu, Dec 01, 2016 at 10:54:39AM +0100, Jan Kara wrote:
> On Thu 01-12-16 09:54:44, Dave Chinner wrote:
> > From: Dave Chinner <[e-mail]>
> > 
> > We always perform integrity operations now, so these mount options
> > don't do anything. Deprecate them and mark them for removal in
> > in a year.
> > 
> > Signed-Off-By: Dave Chinner <[e-mail]>
> 
> We use 'nobarrier' mount option to simulate disks with battery-backed
> caches in some of our IO performance testing. I don't say we cannot live
> with this mount option but it was convenient...

FYI, for scsi (and thus ATA as well) you can override the cache type
using the cache_type sysfs file, which is a much better interface for
this than anything file system related.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Brian Foster Dec. 1, 2016, 12:47 p.m. UTC | #3

On Thu, Dec 01, 2016 at 09:54:44AM +1100, Dave Chinner wrote:
> From: Dave Chinner <[e-mail]>
> 
> We always perform integrity operations now, so these mount options
> don't do anything. Deprecate them and mark them for removal in
> in a year.
> 
> Signed-Off-By: Dave Chinner <[e-mail]>
> ---
>  Documentation/filesystems/xfs.txt | 12 ++++--------
>  fs/xfs/xfs_super.c                | 25 ++++++++++++++++---------
>  2 files changed, 20 insertions(+), 17 deletions(-)
> 
...
> diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> index 563d1d146b8c..eecbaac08eba 100644
> --- a/fs/xfs/xfs_super.c
> +++ b/fs/xfs/xfs_super.c
...
> @@ -374,6 +371,14 @@ xfs_parseargs(
>  			mp->m_flags |= XFS_MOUNT_DAX;
>  			break;
>  #endif
> +		case Opt_barrier:
> +			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
> +			mp->m_flags |= XFS_MOUNT_BARRIER;
> +			break;
> +		case Opt_nobarrier:
> +			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
> +			mp->m_flags &= ~XFS_MOUNT_BARRIER;
> +			break;

So basically XFS_MOUNT_BARRIER exists solely for the purpose of
showargs. Should we just kill it too and do something deterministic on
showargs (i.e., always show 'barrier' or just drop it entirely), or does
precedent suggest otherwise?

That aside, this all seems Ok to me.

Brian

>  		default:
>  			xfs_warn(mp, "unknown mount option [%s].", p);
>  			return -EINVAL;
> @@ -1238,9 +1243,11 @@ xfs_fs_remount(
>  		token = match_token(p, tokens, args);
>  		switch (token) {
>  		case Opt_barrier:
> +			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
>  			mp->m_flags |= XFS_MOUNT_BARRIER;
>  			break;
>  		case Opt_nobarrier:
> +			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
>  			mp->m_flags &= ~XFS_MOUNT_BARRIER;
>  			break;
>  		case Opt_inode64:
> -- 
> 2.10.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to [e-mail]
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dave Chinner Dec. 1, 2016, 8:20 p.m. UTC | #4

On Thu, Dec 01, 2016 at 07:47:46AM -0500, Brian Foster wrote:
> On Thu, Dec 01, 2016 at 09:54:44AM +1100, Dave Chinner wrote:
> > From: Dave Chinner <[e-mail]>
> > 
> > We always perform integrity operations now, so these mount options
> > don't do anything. Deprecate them and mark them for removal in
> > in a year.
> > 
> > Signed-Off-By: Dave Chinner <[e-mail]>
> > ---
> >  Documentation/filesystems/xfs.txt | 12 ++++--------
> >  fs/xfs/xfs_super.c                | 25 ++++++++++++++++---------
> >  2 files changed, 20 insertions(+), 17 deletions(-)
> > 
> ...
> > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > index 563d1d146b8c..eecbaac08eba 100644
> > --- a/fs/xfs/xfs_super.c
> > +++ b/fs/xfs/xfs_super.c
> ...
> > @@ -374,6 +371,14 @@ xfs_parseargs(
> >  			mp->m_flags |= XFS_MOUNT_DAX;
> >  			break;
> >  #endif
> > +		case Opt_barrier:
> > +			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
> > +			mp->m_flags |= XFS_MOUNT_BARRIER;
> > +			break;
> > +		case Opt_nobarrier:
> > +			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
> > +			mp->m_flags &= ~XFS_MOUNT_BARRIER;
> > +			break;
> 
> So basically XFS_MOUNT_BARRIER exists solely for the purpose of
> showargs. Should we just kill it too and do something deterministic on
> showargs (i.e., always show 'barrier' or just drop it entirely), or does
> precedent suggest otherwise?

Well, they are deprecated and ignored, but we can't remove them
straight away so I thought we should still report them appropriately
in showargs.

I don't really care either way - I can kill it completely if
you think that's better.

Cheers,

Dave.

Brian Foster Dec. 1, 2016, 9:31 p.m. UTC | #5

On Fri, Dec 02, 2016 at 07:20:06AM +1100, Dave Chinner wrote:
> On Thu, Dec 01, 2016 at 07:47:46AM -0500, Brian Foster wrote:
> > On Thu, Dec 01, 2016 at 09:54:44AM +1100, Dave Chinner wrote:
> > > From: Dave Chinner <[e-mail]>
> > > 
> > > We always perform integrity operations now, so these mount options
> > > don't do anything. Deprecate them and mark them for removal in
> > > in a year.
> > > 
> > > Signed-Off-By: Dave Chinner <[e-mail]>
> > > ---
> > >  Documentation/filesystems/xfs.txt | 12 ++++--------
> > >  fs/xfs/xfs_super.c                | 25 ++++++++++++++++---------
> > >  2 files changed, 20 insertions(+), 17 deletions(-)
> > > 
> > ...
> > > diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
> > > index 563d1d146b8c..eecbaac08eba 100644
> > > --- a/fs/xfs/xfs_super.c
> > > +++ b/fs/xfs/xfs_super.c
> > ...
> > > @@ -374,6 +371,14 @@ xfs_parseargs(
> > >  			mp->m_flags |= XFS_MOUNT_DAX;
> > >  			break;
> > >  #endif
> > > +		case Opt_barrier:
> > > +			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
> > > +			mp->m_flags |= XFS_MOUNT_BARRIER;
> > > +			break;
> > > +		case Opt_nobarrier:
> > > +			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
> > > +			mp->m_flags &= ~XFS_MOUNT_BARRIER;
> > > +			break;
> > 
> > So basically XFS_MOUNT_BARRIER exists solely for the purpose of
> > showargs. Should we just kill it too and do something deterministic on
> > showargs (i.e., always show 'barrier' or just drop it entirely), or does
> > precedent suggest otherwise?
> 
> Well, they are deprecated and ignored, but we can't remove them
> straight away so I thought we should still report them appropriately
> in showargs.
> 
> I don't really care either way - I can kill it completely if
> you think that's better.
> 

Ok. No major preference.. my first instinct was just to see it all
killed off to explicitly show that the mount option has no effect.
Either way is fine, thanks.

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> [e-mail]
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Christoph Hellwig Dec. 5, 2016, 4:23 p.m. UTC | #6

On Thu, Dec 01, 2016 at 09:54:44AM +1100, Dave Chinner wrote:
> From: Dave Chinner <[e-mail]>
> 
> We always perform integrity operations now, so these mount options
> don't do anything. Deprecate them and mark them for removal in
> in a year.

This might be a bit of a too aggressive schedule, but we'll see..

Looks fine otherwise:

Reviewed-by: Christoph Hellwig <[e-mail]>
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/Documentation/filesystems/xfs.txt b/Documentation/filesystems/xfs.txt
index c2d44e6e117b..68a057c8fccf 100644
--- a/Documentation/filesystems/xfs.txt
+++ b/Documentation/filesystems/xfs.txt
@@ -51,13 +51,6 @@  default behaviour.
 	CRC enabled filesystems always use the attr2 format, and so
 	will reject the noattr2 mount option if it is set.
 
-  barrier (*)
-  nobarrier
-	Enables/disables the use of block layer write barriers for
-	writes into the journal and for data integrity operations.
-	This allows for drive level write caching to be enabled, for
-	devices that support write barriers.
-
   discard
   nodiscard (*)
 	Enable/disable the issuing of commands to let the block
@@ -228,7 +221,10 @@  default behaviour.
 Deprecated Mount Options
 ========================
 
-None at present.
+  Name				Removal Schedule
+  ----				----------------
+  barrier			December 2017
+  nobarrier			December 2017
 
 
 Removed Mount Options
diff --git a/fs/xfs/xfs_super.c b/fs/xfs/xfs_super.c
index 563d1d146b8c..eecbaac08eba 100644
--- a/fs/xfs/xfs_super.c
+++ b/fs/xfs/xfs_super.c
@@ -104,9 +104,6 @@  static const match_table_t tokens = {
 	{Opt_sysvgroups,"sysvgroups"},	/* group-ID from current process */
 	{Opt_allocsize,	"allocsize=%s"},/* preferred allocation size */
 	{Opt_norecovery,"norecovery"},	/* don't run XFS recovery */
-	{Opt_barrier,	"barrier"},	/* use writer barriers for log write and
-					 * unwritten extent conversion */
-	{Opt_nobarrier,	"nobarrier"},	/* .. disable */
 	{Opt_inode64,	"inode64"},	/* inodes can be allocated anywhere */
 	{Opt_inode32,   "inode32"},	/* inode allocation limited to
 					 * XFS_MAXINUMBER_32 */
@@ -134,6 +131,12 @@  static const match_table_t tokens = {
 	{Opt_nodiscard,	"nodiscard"},	/* Do not discard unused blocks */
 
 	{Opt_dax,	"dax"},		/* Enable direct access to bdev pages */
+
+	/* Deprecated mount options scheduled for removal */
+	{Opt_barrier,	"barrier"},	/* use writer barriers for log write and
+					 * unwritten extent conversion */
+	{Opt_nobarrier,	"nobarrier"},	/* .. disable */
+
 	{Opt_err,	NULL},
 };
 
@@ -301,12 +304,6 @@  xfs_parseargs(
 		case Opt_nouuid:
 			mp->m_flags |= XFS_MOUNT_NOUUID;
 			break;
-		case Opt_barrier:
-			mp->m_flags |= XFS_MOUNT_BARRIER;
-			break;
-		case Opt_nobarrier:
-			mp->m_flags &= ~XFS_MOUNT_BARRIER;
-			break;
 		case Opt_ikeep:
 			mp->m_flags |= XFS_MOUNT_IKEEP;
 			break;
@@ -374,6 +371,14 @@  xfs_parseargs(
 			mp->m_flags |= XFS_MOUNT_DAX;
 			break;
 #endif
+		case Opt_barrier:
+			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
+			mp->m_flags |= XFS_MOUNT_BARRIER;
+			break;
+		case Opt_nobarrier:
+			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
+			mp->m_flags &= ~XFS_MOUNT_BARRIER;
+			break;
 		default:
 			xfs_warn(mp, "unknown mount option [%s].", p);
 			return -EINVAL;
@@ -1238,9 +1243,11 @@  xfs_fs_remount(
 		token = match_token(p, tokens, args);
 		switch (token) {
 		case Opt_barrier:
+			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
 			mp->m_flags |= XFS_MOUNT_BARRIER;
 			break;
 		case Opt_nobarrier:
+			xfs_warn(mp, "%s option is deprecated, ignoring.", p);
 			mp->m_flags &= ~XFS_MOUNT_BARRIER;
 			break;
 		case Opt_inode64: