[PATCH 0/1] nilfs2: add missing blkdev_issue_flush() to nilfs_sync

Discussion:

[PATCH 0/1] nilfs2: add missing blkdev_issue_flush() to nilfs_sync_fs()

Andreas Rohner

2014-09-09 16:35:39 UTC

Hi,

I have looked a bit more into the semantics of the various flags
concerning block device caching behaviour. According to
"Documentation/block/writeback_cache_control.txt" a call to
blkdev_issue_flush() is equivalent to an empty bio with the
REQ_FLUSH flag set. So there is no need to call blkdev_issue_flush()
after a call to nilfs_commit_super(). But if there is no need to write
the super block an additional call to blkdev_issue_flush() is necessary.

To avoid an overhead I introduced the nilfs->ns_flushed_device flag,
which is set to 0 whenever new logs are written and set to 1 whenever
the block device is flushed. If the super block was written during
segment construction or in nilfs_sync_fs(), then blkdev_issue_flush() is not
called.

On most modern architectures loads and stores of single word integers
are atomic. I still used atomic_t for ns_flushed_device for
documentation purposes. I only use atomic_read() and atomic_set(). Both
are inline functions, which compile down to simple loads and stores on
modern architectures, so there is no performance benefit in using a
simple int instead.

br,
Andreas Rohner

v2->v3 (based on review of Ryusuke Konishi)
* Use separate atomic flag for ns_flushed_device instead of a bit flag
in ns_flags
* Use smp_mb__after_atomic() after setting ns_flushed_device

v1->v2
* Add new flag THE_NILFS_FLUSHED

Andreas Rohner (1):
nilfs2: add missing blkdev_issue_flush() to nilfs_sync_fs()

fs/nilfs2/file.c | 6 +++++-
fs/nilfs2/ioctl.c | 6 +++++-
fs/nilfs2/segment.c | 4 ++++
fs/nilfs2/super.c | 12 ++++++++++++
fs/nilfs2/the_nilfs.c | 1 +
fs/nilfs2/the_nilfs.h | 2 ++
6 files changed, 29 insertions(+), 2 deletions(-)

--
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Andreas Rohner

2014-09-09 16:35:40 UTC

Permalink

Under normal circumstances nilfs_sync_fs() writes out the super block,
which causes a flush of the underlying block device. But this depends on
the THE_NILFS_SB_DIRTY flag, which is only set if the pointer to the
last segment crosses a segment boundary. So if only a small amount of
data is written before the call to nilfs_sync_fs(), no flush of the
block device occurs.

In the above case an additional call to blkdev_issue_flush() is needed.
To prevent unnecessary overhead, the new flag nilfs->ns_flushed_device
is introduced, which is cleared whenever new logs are written and set
whenever the block device is flushed.

Signed-off-by: Andreas Rohner <andreas.rohner-***@public.gmane.org>
---
fs/nilfs2/file.c | 6 +++++-
fs/nilfs2/ioctl.c | 6 +++++-
fs/nilfs2/segment.c | 4 ++++
fs/nilfs2/super.c | 12 ++++++++++++
fs/nilfs2/the_nilfs.c | 1 +
fs/nilfs2/the_nilfs.h | 2 ++
6 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/fs/nilfs2/file.c b/fs/nilfs2/file.c
index 2497815..8a3e702 100644
--- a/fs/nilfs2/file.c
+++ b/fs/nilfs2/file.c
@@ -56,7 +56,11 @@ int nilfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
mutex_unlock(&inode->i_mutex);

nilfs = inode->i_sb->s_fs_info;
- if (!err && nilfs_test_opt(nilfs, BARRIER)) {
+ if (!err && nilfs_test_opt(nilfs, BARRIER) &&
+ !atomic_read(&nilfs->ns_flushed_device)) {
+ atomic_set(&nilfs->ns_flushed_device, 1);
+ smp_mb__after_atomic();
+
err = blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
if (err != -EIO)
err = 0;
diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
index 422fb54..47fe7cf 100644
--- a/fs/nilfs2/ioctl.c
+++ b/fs/nilfs2/ioctl.c
@@ -1022,7 +1022,11 @@ static int nilfs_ioctl_sync(struct inode *inode, struct file *filp,
return ret;

nilfs = inode->i_sb->s_fs_info;
- if (nilfs_test_opt(nilfs, BARRIER)) {
+ if (nilfs_test_opt(nilfs, BARRIER) &&
+ !atomic_read(&nilfs->ns_flushed_device)) {
+ atomic_set(&nilfs->ns_flushed_device, 1);
+ smp_mb__after_atomic();
+
ret = blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
if (ret == -EIO)
return ret;
diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
index a1a1916..3119b64 100644
--- a/fs/nilfs2/segment.c
+++ b/fs/nilfs2/segment.c
@@ -1997,6 +1997,10 @@ static int nilfs_segctor_do_construct(struct nilfs_sc_info *sci, int mode)
err = nilfs_segctor_wait(sci);
if (err)
goto failed_to_write;
+
+ if (test_bit(NILFS_SC_SUPER_ROOT, &sci->sc_flags) ||
+ mode == SC_LSEG_DSYNC)
+ atomic_set(&nilfs->ns_flushed_device, 0);
}
} while (sci->sc_stage.scnt != NILFS_ST_DONE);

diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 228f5bd..74a9930 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -310,6 +310,8 @@ int nilfs_commit_super(struct super_block *sb, int flag)
nilfs->ns_sbsize));
}
clear_nilfs_sb_dirty(nilfs);
+ atomic_set(&nilfs->ns_flushed_device, 1);
+ smp_mb__after_atomic();
return nilfs_sync_super(sb, flag);
}

@@ -514,6 +516,16 @@ static int nilfs_sync_fs(struct super_block *sb, int wait)
}
up_write(&nilfs->ns_sem);

+ if (wait && !err && nilfs_test_opt(nilfs, BARRIER) &&
+ !atomic_read(&nilfs->ns_flushed_device)) {
+ atomic_set(&nilfs->ns_flushed_device, 1);
+ smp_mb__after_atomic();
+
+ err = blkdev_issue_flush(sb->s_bdev, GFP_KERNEL, NULL);
+ if (err != -EIO)
+ err = 0;
+ }
+
return err;
}

diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
index 9da25fe..d37c50b 100644
--- a/fs/nilfs2/the_nilfs.c
+++ b/fs/nilfs2/the_nilfs.c
@@ -74,6 +74,7 @@ struct the_nilfs *alloc_nilfs(struct block_device *bdev)
return NULL;

nilfs->ns_bdev = bdev;
+ atomic_set(&nilfs->ns_flushed_device, 0);
atomic_set(&nilfs->ns_ndirtyblks, 0);
init_rwsem(&nilfs->ns_sem);
mutex_init(&nilfs->ns_snapshot_mount_mutex);
diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
index d01ead1..ec53958 100644
--- a/fs/nilfs2/the_nilfs.h
+++ b/fs/nilfs2/the_nilfs.h
@@ -45,6 +45,7 @@ enum {

/**
* struct the_nilfs - struct to supervise multiple nilfs mount points
+ * @ns_flushed_device: flag indicating if all volatile data was flushed
* @ns_flags: flags
* @ns_bdev: block device
* @ns_sem: semaphore for shared states
@@ -103,6 +104,7 @@ enum {
*/
struct the_nilfs {
unsigned long ns_flags;
+ atomic_t ns_flushed_device;

struct block_device *ns_bdev;
struct rw_semaphore ns_sem;

Ryusuke Konishi

2014-09-09 19:18:40 UTC

Permalink

Post by Andreas Rohner
Under normal circumstances nilfs_sync_fs() writes out the super block,
which causes a flush of the underlying block device. But this depends on
the THE_NILFS_SB_DIRTY flag, which is only set if the pointer to the
last segment crosses a segment boundary. So if only a small amount of
data is written before the call to nilfs_sync_fs(), no flush of the
block device occurs.
In the above case an additional call to blkdev_issue_flush() is needed.
To prevent unnecessary overhead, the new flag nilfs->ns_flushed_device
is introduced, which is cleared whenever new logs are written and set
whenever the block device is flushed.
---
fs/nilfs2/file.c | 6 +++++-
fs/nilfs2/ioctl.c | 6 +++++-
fs/nilfs2/segment.c | 4 ++++
fs/nilfs2/super.c | 12 ++++++++++++
fs/nilfs2/the_nilfs.c | 1 +
fs/nilfs2/the_nilfs.h | 2 ++
6 files changed, 29 insertions(+), 2 deletions(-)
diff --git a/fs/nilfs2/file.c b/fs/nilfs2/file.c
index 2497815..8a3e702 100644
--- a/fs/nilfs2/file.c
+++ b/fs/nilfs2/file.c
@@ -56,7 +56,11 @@ int nilfs_sync_file(struct file *file, loff_t start, loff_t end, int datasync)
mutex_unlock(&inode->i_mutex);
nilfs = inode->i_sb->s_fs_info;
- if (!err && nilfs_test_opt(nilfs, BARRIER)) {
+ if (!err && nilfs_test_opt(nilfs, BARRIER) &&
+ !atomic_read(&nilfs->ns_flushed_device)) {
+ atomic_set(&nilfs->ns_flushed_device, 1);
+ smp_mb__after_atomic();
+
err = blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
if (err != -EIO)
err = 0;
diff --git a/fs/nilfs2/ioctl.c b/fs/nilfs2/ioctl.c
index 422fb54..47fe7cf 100644
--- a/fs/nilfs2/ioctl.c
+++ b/fs/nilfs2/ioctl.c
@@ -1022,7 +1022,11 @@ static int nilfs_ioctl_sync(struct inode *inode, struct file *filp,
return ret;
nilfs = inode->i_sb->s_fs_info;
- if (nilfs_test_opt(nilfs, BARRIER)) {
+ if (nilfs_test_opt(nilfs, BARRIER) &&
+ !atomic_read(&nilfs->ns_flushed_device)) {
+ atomic_set(&nilfs->ns_flushed_device, 1);
+ smp_mb__after_atomic();
+
ret = blkdev_issue_flush(inode->i_sb->s_bdev, GFP_KERNEL, NULL);
if (ret == -EIO)
return ret;
diff --git a/fs/nilfs2/segment.c b/fs/nilfs2/segment.c
index a1a1916..3119b64 100644
--- a/fs/nilfs2/segment.c
+++ b/fs/nilfs2/segment.c
@@ -1997,6 +1997,10 @@ static int nilfs_segctor_do_construct(struct nilfs_sc_info *sci, int mode)
err = nilfs_segctor_wait(sci);
if (err)
goto failed_to_write;
+
+ if (test_bit(NILFS_SC_SUPER_ROOT, &sci->sc_flags) ||
+ mode == SC_LSEG_DSYNC)
+ atomic_set(&nilfs->ns_flushed_device, 0);
}
} while (sci->sc_stage.scnt != NILFS_ST_DONE);
diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 228f5bd..74a9930 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -310,6 +310,8 @@ int nilfs_commit_super(struct super_block *sb, int flag)
nilfs->ns_sbsize));
}
clear_nilfs_sb_dirty(nilfs);
+ atomic_set(&nilfs->ns_flushed_device, 1);
+ smp_mb__after_atomic();
return nilfs_sync_super(sb, flag);
}
@@ -514,6 +516,16 @@ static int nilfs_sync_fs(struct super_block *sb, int wait)
}
up_write(&nilfs->ns_sem);
+ if (wait && !err && nilfs_test_opt(nilfs, BARRIER) &&
+ !atomic_read(&nilfs->ns_flushed_device)) {
+ atomic_set(&nilfs->ns_flushed_device, 1);
+ smp_mb__after_atomic();
+
+ err = blkdev_issue_flush(sb->s_bdev, GFP_KERNEL, NULL);
+ if (err != -EIO)
+ err = 0;
+ }
+
return err;
}
diff --git a/fs/nilfs2/the_nilfs.c b/fs/nilfs2/the_nilfs.c
index 9da25fe..d37c50b 100644
--- a/fs/nilfs2/the_nilfs.c
+++ b/fs/nilfs2/the_nilfs.c
@@ -74,6 +74,7 @@ struct the_nilfs *alloc_nilfs(struct block_device *bdev)
return NULL;
nilfs->ns_bdev = bdev;
+ atomic_set(&nilfs->ns_flushed_device, 0);
atomic_set(&nilfs->ns_ndirtyblks, 0);
init_rwsem(&nilfs->ns_sem);
mutex_init(&nilfs->ns_snapshot_mount_mutex);
diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
index d01ead1..ec53958 100644
--- a/fs/nilfs2/the_nilfs.h
+++ b/fs/nilfs2/the_nilfs.h
@@ -45,6 +45,7 @@ enum {
/**
* struct the_nilfs - struct to supervise multiple nilfs mount points
@@ -103,6 +104,7 @@ enum {
*/
struct the_nilfs {
unsigned long ns_flags;
+ atomic_t ns_flushed_device;

Andreas, can you implement this with just an integer variable like below ?

int ns_flushed_device

I want to limit the use of atomic_t type just for counters such as
ref-counts, statistics information, and so forth.

The load/store operations for this value is done atomically because
the_nilfs structure is natually aligned without "packed" attribute
though it may need that smp barriers are inserted properly.

Post by Andreas Rohner
struct block_device *ns_bdev;
struct rw_semaphore ns_sem;
--
2.1.0
--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html

Andreas Rohner

2014-09-09 19:58:27 UTC

Permalink

Post by Ryusuke Konishi

Yes sure. I thought it is a good idea to use atomic_t, because
atomic_read() and atomic_set() are implemented with simple inline functions:

static inline int atomic_read(const atomic_t *v)
{
return (*(volatile int *)&(v)->counter);
}

static inline void atomic_set(atomic_t *v, int i)
{
v->counter = i;
}

So the performance would be essentially the same as with int. No extra
locks or memory barriers at least on architectures where an int is
always stored atomically. I thought it is good practice to use the
inline functions in this case.

Post by Ryusuke Konishi
The load/store operations for this value is done atomically because
the_nilfs structure is natually aligned without "packed" attribute
though it may need that smp barriers are inserted properly.

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
More majordomo info at http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-nilfs" in
the body of a message to majordomo-***@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html