* [RFC PATCH 0/2] xfs: deprecate barrier/nobarrier
@ 2016-11-30 22:54 Dave Chinner
2016-11-30 22:54 ` [PATCH 1/2] xfs: Always flush caches when integrity is required Dave Chinner
2016-11-30 22:54 ` [PATCH 2/2] xfs: deprecate barrier/nobarrier mount option Dave Chinner
0 siblings, 2 replies; 11+ messages in thread
From: Dave Chinner @ 2016-11-30 22:54 UTC (permalink / raw)
To: linux-xfs
Hi folks,
In response to the recent conversation here:
https://www.spinics.net/lists/linux-xfs/msg02501.html
I've come to the conclusion that barrier/nobarrier mount options no
longer make sense to maintain. There is no performance penalty for
the vast majority of users to enabling barriersand it's been the
default behaviour for a long time. And given the misunderstanding
that users have over what barriers are and when it is safe to turn
them off, it seems to me like the best solution is to simply
remove the option altogther.
Comments, thoughts, flames all welcome.
-Dave.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 1/2] xfs: Always flush caches when integrity is required
2016-11-30 22:54 [RFC PATCH 0/2] xfs: deprecate barrier/nobarrier Dave Chinner
@ 2016-11-30 22:54 ` Dave Chinner
2016-12-01 12:47 ` Brian Foster
2016-12-05 16:22 ` Christoph Hellwig
2016-11-30 22:54 ` [PATCH 2/2] xfs: deprecate barrier/nobarrier mount option Dave Chinner
1 sibling, 2 replies; 11+ messages in thread
From: Dave Chinner @ 2016-11-30 22:54 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
There is no reason anymore for not issuing device integrity
operations when teh filesystem requires ordering or data integrity
guarantees. We should always issue cache flushes and FUA writes
where necessary and let the underlying storage optimise them as
necessary for correct integrity operation.
Signed-Off-By: Dave Chinner <dchinner@redhat.com>
---
fs/xfs/xfs_buf.c | 3 +--
fs/xfs/xfs_file.c | 29 ++++++++++++-----------------
fs/xfs/xfs_log.c | 36 +++++++++++++++---------------------
3 files changed, 28 insertions(+), 40 deletions(-)
diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
index a2f0648743db..1264908ef8f2 100644
--- a/fs/xfs/xfs_buf.c
+++ b/fs/xfs/xfs_buf.c
@@ -1738,8 +1738,7 @@ xfs_free_buftarg(
percpu_counter_destroy(&btp->bt_io_count);
list_lru_destroy(&btp->bt_lru);
- if (mp->m_flags & XFS_MOUNT_BARRIER)
- xfs_blkdev_issue_flush(btp);
+ xfs_blkdev_issue_flush(btp);
kmem_free(btp);
}
diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
index f5effa68e037..2951c483b24b 100644
--- a/fs/xfs/xfs_file.c
+++ b/fs/xfs/xfs_file.c
@@ -149,19 +149,16 @@ xfs_file_fsync(
xfs_iflags_clear(ip, XFS_ITRUNCATED);
- if (mp->m_flags & XFS_MOUNT_BARRIER) {
- /*
- * If we have an RT and/or log subvolume we need to make sure
- * to flush the write cache the device used for file data
- * first. This is to ensure newly written file data make
- * it to disk before logging the new inode size in case of
- * an extending write.
- */
- if (XFS_IS_REALTIME_INODE(ip))
- xfs_blkdev_issue_flush(mp->m_rtdev_targp);
- else if (mp->m_logdev_targp != mp->m_ddev_targp)
- xfs_blkdev_issue_flush(mp->m_ddev_targp);
- }
+ /*
+ * If we have an RT and/or log subvolume we need to make sure to flush
+ * the write cache the device used for file data first. This is to
+ * ensure newly written file data make it to disk before logging the new
+ * inode size in case of an extending write.
+ */
+ if (XFS_IS_REALTIME_INODE(ip))
+ xfs_blkdev_issue_flush(mp->m_rtdev_targp);
+ else if (mp->m_logdev_targp != mp->m_ddev_targp)
+ xfs_blkdev_issue_flush(mp->m_ddev_targp);
/*
* All metadata updates are logged, which means that we just have to
@@ -196,10 +193,8 @@ xfs_file_fsync(
* an already allocated file and thus do not have any metadata to
* commit.
*/
- if ((mp->m_flags & XFS_MOUNT_BARRIER) &&
- mp->m_logdev_targp == mp->m_ddev_targp &&
- !XFS_IS_REALTIME_INODE(ip) &&
- !log_flushed)
+ if (!log_flushed && !XFS_IS_REALTIME_INODE(ip) &&
+ mp->m_logdev_targp == mp->m_ddev_targp)
xfs_blkdev_issue_flush(mp->m_ddev_targp);
return error;
diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
index 3ebe444eb60f..573d0841851d 100644
--- a/fs/xfs/xfs_log.c
+++ b/fs/xfs/xfs_log.c
@@ -1863,25 +1863,21 @@ xlog_sync(
bp->b_io_length = BTOBB(count);
bp->b_fspriv = iclog;
bp->b_flags &= ~(XBF_FUA | XBF_FLUSH);
- bp->b_flags |= (XBF_ASYNC | XBF_SYNCIO | XBF_WRITE);
+ bp->b_flags |= (XBF_ASYNC | XBF_SYNCIO | XBF_WRITE | XBF_FUA);
- if (log->l_mp->m_flags & XFS_MOUNT_BARRIER) {
- bp->b_flags |= XBF_FUA;
-
- /*
- * Flush the data device before flushing the log to make
- * sure all meta data written back from the AIL actually made
- * it to disk before stamping the new log tail LSN into the
- * log buffer. For an external log we need to issue the
- * flush explicitly, and unfortunately synchronously here;
- * for an internal log we can simply use the block layer
- * state machine for preflushes.
- */
- if (log->l_mp->m_logdev_targp != log->l_mp->m_ddev_targp)
- xfs_blkdev_issue_flush(log->l_mp->m_ddev_targp);
- else
- bp->b_flags |= XBF_FLUSH;
- }
+ /*
+ * Flush the data device before flushing the log to make
+ * sure all meta data written back from the AIL actually made
+ * it to disk before stamping the new log tail LSN into the
+ * log buffer. For an external log we need to issue the
+ * flush explicitly, and unfortunately synchronously here;
+ * for an internal log we can simply use the block layer
+ * state machine for preflushes.
+ */
+ if (log->l_mp->m_logdev_targp != log->l_mp->m_ddev_targp)
+ xfs_blkdev_issue_flush(log->l_mp->m_ddev_targp);
+ else
+ bp->b_flags |= XBF_FLUSH;
ASSERT(XFS_BUF_ADDR(bp) <= log->l_logBBsize-1);
ASSERT(XFS_BUF_ADDR(bp) + BTOBB(count) <= log->l_logBBsize);
@@ -1907,9 +1903,7 @@ xlog_sync(
(char *)&iclog->ic_header + count, split);
bp->b_fspriv = iclog;
bp->b_flags &= ~(XBF_FUA | XBF_FLUSH);
- bp->b_flags |= (XBF_ASYNC | XBF_SYNCIO | XBF_WRITE);
- if (log->l_mp->m_flags & XFS_MOUNT_BARRIER)
- bp->b_flags |= XBF_FUA;
+ bp->b_flags |= (XBF_ASYNC | XBF_SYNCIO | XBF_WRITE | XBF_FUA);
ASSERT(XFS_BUF_ADDR(bp) <= log->l_logBBsize-1);
ASSERT(XFS_BUF_ADDR(bp) + BTOBB(count) <= log->l_logBBsize);
--
2.10.2
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH 2/2] xfs: deprecate barrier/nobarrier mount option
2016-11-30 22:54 [RFC PATCH 0/2] xfs: deprecate barrier/nobarrier Dave Chinner
2016-11-30 22:54 ` [PATCH 1/2] xfs: Always flush caches when integrity is required Dave Chinner
@ 2016-11-30 22:54 ` Dave Chinner
2016-12-01 9:54 ` Jan Kara
` (2 more replies)
1 sibling, 3 replies; 11+ messages in thread
From: Dave Chinner @ 2016-11-30 22:54 UTC (permalink / raw)
To: linux-xfs
From: Dave Chinner <dchinner@redhat.com>
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 <dchinner@redhat.com>
---
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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: deprecate barrier/nobarrier mount option
2016-11-30 22:54 ` [PATCH 2/2] xfs: deprecate barrier/nobarrier mount option Dave Chinner
@ 2016-12-01 9:54 ` Jan Kara
2016-12-01 9:59 ` Christoph Hellwig
2016-12-01 12:47 ` Brian Foster
2016-12-05 16:23 ` Christoph Hellwig
2 siblings, 1 reply; 11+ messages in thread
From: Jan Kara @ 2016-12-01 9:54 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Thu 01-12-16 09:54:44, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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 <dchinner@redhat.com>
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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Jan Kara <jack@suse.com>
SUSE Labs, CR
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: deprecate barrier/nobarrier mount option
2016-12-01 9:54 ` Jan Kara
@ 2016-12-01 9:59 ` Christoph Hellwig
0 siblings, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2016-12-01 9:59 UTC (permalink / raw)
To: Jan Kara; +Cc: Dave Chinner, linux-xfs
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 <dchinner@redhat.com>
> >
> > 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 <dchinner@redhat.com>
>
> 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] xfs: Always flush caches when integrity is required
2016-11-30 22:54 ` [PATCH 1/2] xfs: Always flush caches when integrity is required Dave Chinner
@ 2016-12-01 12:47 ` Brian Foster
2016-12-05 16:22 ` Christoph Hellwig
1 sibling, 0 replies; 11+ messages in thread
From: Brian Foster @ 2016-12-01 12:47 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Thu, Dec 01, 2016 at 09:54:43AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> There is no reason anymore for not issuing device integrity
> operations when teh filesystem requires ordering or data integrity
> guarantees. We should always issue cache flushes and FUA writes
> where necessary and let the underlying storage optimise them as
> necessary for correct integrity operation.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
> ---
Seems reasonable to me. Just a few nits below..
> fs/xfs/xfs_buf.c | 3 +--
> fs/xfs/xfs_file.c | 29 ++++++++++++-----------------
> fs/xfs/xfs_log.c | 36 +++++++++++++++---------------------
> 3 files changed, 28 insertions(+), 40 deletions(-)
>
> diff --git a/fs/xfs/xfs_buf.c b/fs/xfs/xfs_buf.c
> index a2f0648743db..1264908ef8f2 100644
> --- a/fs/xfs/xfs_buf.c
> +++ b/fs/xfs/xfs_buf.c
> @@ -1738,8 +1738,7 @@ xfs_free_buftarg(
> percpu_counter_destroy(&btp->bt_io_count);
> list_lru_destroy(&btp->bt_lru);
>
> - if (mp->m_flags & XFS_MOUNT_BARRIER)
> - xfs_blkdev_issue_flush(btp);
> + xfs_blkdev_issue_flush(btp);
>
> kmem_free(btp);
> }
> diff --git a/fs/xfs/xfs_file.c b/fs/xfs/xfs_file.c
> index f5effa68e037..2951c483b24b 100644
> --- a/fs/xfs/xfs_file.c
> +++ b/fs/xfs/xfs_file.c
> @@ -149,19 +149,16 @@ xfs_file_fsync(
>
> xfs_iflags_clear(ip, XFS_ITRUNCATED);
>
> - if (mp->m_flags & XFS_MOUNT_BARRIER) {
> - /*
> - * If we have an RT and/or log subvolume we need to make sure
> - * to flush the write cache the device used for file data
> - * first. This is to ensure newly written file data make
> - * it to disk before logging the new inode size in case of
> - * an extending write.
> - */
> - if (XFS_IS_REALTIME_INODE(ip))
> - xfs_blkdev_issue_flush(mp->m_rtdev_targp);
> - else if (mp->m_logdev_targp != mp->m_ddev_targp)
> - xfs_blkdev_issue_flush(mp->m_ddev_targp);
> - }
> + /*
> + * If we have an RT and/or log subvolume we need to make sure to flush
> + * the write cache the device used for file data first. This is to
> + * ensure newly written file data make it to disk before logging the new
> + * inode size in case of an extending write.
> + */
> + if (XFS_IS_REALTIME_INODE(ip))
> + xfs_blkdev_issue_flush(mp->m_rtdev_targp);
> + else if (mp->m_logdev_targp != mp->m_ddev_targp)
> + xfs_blkdev_issue_flush(mp->m_ddev_targp);
>
> /*
> * All metadata updates are logged, which means that we just have to
> @@ -196,10 +193,8 @@ xfs_file_fsync(
> * an already allocated file and thus do not have any metadata to
> * commit.
> */
> - if ((mp->m_flags & XFS_MOUNT_BARRIER) &&
> - mp->m_logdev_targp == mp->m_ddev_targp &&
> - !XFS_IS_REALTIME_INODE(ip) &&
> - !log_flushed)
> + if (!log_flushed && !XFS_IS_REALTIME_INODE(ip) &&
> + mp->m_logdev_targp == mp->m_ddev_targp)
> xfs_blkdev_issue_flush(mp->m_ddev_targp);
>
> return error;
> diff --git a/fs/xfs/xfs_log.c b/fs/xfs/xfs_log.c
> index 3ebe444eb60f..573d0841851d 100644
> --- a/fs/xfs/xfs_log.c
> +++ b/fs/xfs/xfs_log.c
> @@ -1863,25 +1863,21 @@ xlog_sync(
> bp->b_io_length = BTOBB(count);
> bp->b_fspriv = iclog;
> bp->b_flags &= ~(XBF_FUA | XBF_FLUSH);
> - bp->b_flags |= (XBF_ASYNC | XBF_SYNCIO | XBF_WRITE);
> + bp->b_flags |= (XBF_ASYNC | XBF_SYNCIO | XBF_WRITE | XBF_FUA);
There's no need to clear XBF_FUA on the line above any longer.
>
> - if (log->l_mp->m_flags & XFS_MOUNT_BARRIER) {
> - bp->b_flags |= XBF_FUA;
> -
> - /*
> - * Flush the data device before flushing the log to make
> - * sure all meta data written back from the AIL actually made
> - * it to disk before stamping the new log tail LSN into the
> - * log buffer. For an external log we need to issue the
> - * flush explicitly, and unfortunately synchronously here;
> - * for an internal log we can simply use the block layer
> - * state machine for preflushes.
> - */
> - if (log->l_mp->m_logdev_targp != log->l_mp->m_ddev_targp)
> - xfs_blkdev_issue_flush(log->l_mp->m_ddev_targp);
> - else
> - bp->b_flags |= XBF_FLUSH;
> - }
> + /*
> + * Flush the data device before flushing the log to make
> + * sure all meta data written back from the AIL actually made
> + * it to disk before stamping the new log tail LSN into the
> + * log buffer. For an external log we need to issue the
> + * flush explicitly, and unfortunately synchronously here;
> + * for an internal log we can simply use the block layer
> + * state machine for preflushes.
> + */
Comment can be rewrapped.
> + if (log->l_mp->m_logdev_targp != log->l_mp->m_ddev_targp)
> + xfs_blkdev_issue_flush(log->l_mp->m_ddev_targp);
> + else
> + bp->b_flags |= XBF_FLUSH;
>
> ASSERT(XFS_BUF_ADDR(bp) <= log->l_logBBsize-1);
> ASSERT(XFS_BUF_ADDR(bp) + BTOBB(count) <= log->l_logBBsize);
> @@ -1907,9 +1903,7 @@ xlog_sync(
> (char *)&iclog->ic_header + count, split);
> bp->b_fspriv = iclog;
> bp->b_flags &= ~(XBF_FUA | XBF_FLUSH);
No need to clear XBF_FUA here as well.
Brian
> - bp->b_flags |= (XBF_ASYNC | XBF_SYNCIO | XBF_WRITE);
> - if (log->l_mp->m_flags & XFS_MOUNT_BARRIER)
> - bp->b_flags |= XBF_FUA;
> + bp->b_flags |= (XBF_ASYNC | XBF_SYNCIO | XBF_WRITE | XBF_FUA);
>
> ASSERT(XFS_BUF_ADDR(bp) <= log->l_logBBsize-1);
> ASSERT(XFS_BUF_ADDR(bp) + BTOBB(count) <= log->l_logBBsize);
> --
> 2.10.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: deprecate barrier/nobarrier mount option
2016-11-30 22:54 ` [PATCH 2/2] xfs: deprecate barrier/nobarrier mount option Dave Chinner
2016-12-01 9:54 ` Jan Kara
@ 2016-12-01 12:47 ` Brian Foster
2016-12-01 20:20 ` Dave Chinner
2016-12-05 16:23 ` Christoph Hellwig
2 siblings, 1 reply; 11+ messages in thread
From: Brian Foster @ 2016-12-01 12:47 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Thu, Dec 01, 2016 at 09:54:44AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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 <dchinner@redhat.com>
> ---
> 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 majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: deprecate barrier/nobarrier mount option
2016-12-01 12:47 ` Brian Foster
@ 2016-12-01 20:20 ` Dave Chinner
2016-12-01 21:31 ` Brian Foster
0 siblings, 1 reply; 11+ messages in thread
From: Dave Chinner @ 2016-12-01 20:20 UTC (permalink / raw)
To: Brian Foster; +Cc: linux-xfs
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 <dchinner@redhat.com>
> >
> > 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 <dchinner@redhat.com>
> > ---
> > 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.
--
Dave Chinner
david@fromorbit.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: deprecate barrier/nobarrier mount option
2016-12-01 20:20 ` Dave Chinner
@ 2016-12-01 21:31 ` Brian Foster
0 siblings, 0 replies; 11+ messages in thread
From: Brian Foster @ 2016-12-01 21:31 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
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 <dchinner@redhat.com>
> > >
> > > 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 <dchinner@redhat.com>
> > > ---
> > > 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
> david@fromorbit.com
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 1/2] xfs: Always flush caches when integrity is required
2016-11-30 22:54 ` [PATCH 1/2] xfs: Always flush caches when integrity is required Dave Chinner
2016-12-01 12:47 ` Brian Foster
@ 2016-12-05 16:22 ` Christoph Hellwig
1 sibling, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2016-12-05 16:22 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Thu, Dec 01, 2016 at 09:54:43AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> There is no reason anymore for not issuing device integrity
> operations when teh filesystem requires ordering or data integrity
> guarantees. We should always issue cache flushes and FUA writes
> where necessary and let the underlying storage optimise them as
> necessary for correct integrity operation.
>
> Signed-Off-By: Dave Chinner <dchinner@redhat.com>
Looks fine,
Reviewed-by: Christoph Hellwig <hch@lst.de>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH 2/2] xfs: deprecate barrier/nobarrier mount option
2016-11-30 22:54 ` [PATCH 2/2] xfs: deprecate barrier/nobarrier mount option Dave Chinner
2016-12-01 9:54 ` Jan Kara
2016-12-01 12:47 ` Brian Foster
@ 2016-12-05 16:23 ` Christoph Hellwig
2 siblings, 0 replies; 11+ messages in thread
From: Christoph Hellwig @ 2016-12-05 16:23 UTC (permalink / raw)
To: Dave Chinner; +Cc: linux-xfs
On Thu, Dec 01, 2016 at 09:54:44AM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@redhat.com>
>
> 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 <hch@lst.de>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2016-12-05 16:23 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-30 22:54 [RFC PATCH 0/2] xfs: deprecate barrier/nobarrier Dave Chinner
2016-11-30 22:54 ` [PATCH 1/2] xfs: Always flush caches when integrity is required Dave Chinner
2016-12-01 12:47 ` Brian Foster
2016-12-05 16:22 ` Christoph Hellwig
2016-11-30 22:54 ` [PATCH 2/2] xfs: deprecate barrier/nobarrier mount option Dave Chinner
2016-12-01 9:54 ` Jan Kara
2016-12-01 9:59 ` Christoph Hellwig
2016-12-01 12:47 ` Brian Foster
2016-12-01 20:20 ` Dave Chinner
2016-12-01 21:31 ` Brian Foster
2016-12-05 16:23 ` Christoph Hellwig
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).