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

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

[Andreas Rohner]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[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.

Signed-off-by: Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@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;
-- 
2.1.0

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

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

[Ryusuke Konishi]

On Tue,  9 Sep 2014 18:35:40 +0200, Andreas Rohner wrote:
> 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-hi6Y0CQ0nG0@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;

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.

>  	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
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andreas Rohner]

On 2014-09-09 21:18, Ryusuke Konishi wrote:
> On Tue,  9 Sep 2014 18:35:40 +0200, Andreas Rohner wrote:
>> 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-hi6Y0CQ0nG0@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;
> 
> 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.

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.

> 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.
> 
>>  	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
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> 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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Andreas Rohner]

Hi,

I do not know, if this patch is really necessary or not. I am not an 
expert in how the BARRIER flag should be interpreted. So this patch is 
more like a question, than a fix for any real bug.

Looking over the code I noticed, that nilfs_sync_file() gets called by 
the fsync() syscall and it basically constructs a new partial segment 
and calls blkdev_issue_flush().

nilfs_ioctl_sync(), which is used by the cleaner, also essentially works 
the same way. It writes all the dirty files to disk and calls 
blkdev_issue_flush().

nilfs_sync_fs() also writes out all the dirty files, but there is no 
blkdev_issue_flush() at the end. At first I thought, that 
nilfs_commit_super() may flush the block device anyway and therefore no 
additional flush is necessary, but nilfs_sb_dirty(nilfs) is only set to 
true if a new segment is started. So in the following scenario data 
could be lost despite a call to sync():

1. Write out less data than a full segment
2. Call sync()
3. nilfs_sb_dirty() is false and nilfs_commit_super() is NOT called
4. Cut power to the device
5. Data loss

As I stated above, I am not sure if this is really necessary. Maybe I 
have overlooked something obvious.

br,
Andreas Rohner


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

 fs/nilfs2/super.c | 6 ++++++
 1 file changed, 6 insertions(+)

-- 
2.1.0

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

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

[Andreas Rohner]

This patch adds a call to blkdev_issue_flush() to nilfs_sync_fs(), which
is the nilfs implementation of the sync() syscall. If the BARRIER
mount option is set, both the nilfs implementation of fsync() and nilfs'
custom ioctl version of sync() used by the cleaner, use
blkdev_issue_flush() to guarantee that the data is written to the
underlying device. To get the same behaviour and guarantees for the
sync() syscall, blkdev_issue_flush() should also be called in
nilfs_sync_fs().

Signed-off-by: Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>
---
 fs/nilfs2/super.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/fs/nilfs2/super.c b/fs/nilfs2/super.c
index 228f5bd..1f21e81 100644
--- a/fs/nilfs2/super.c
+++ b/fs/nilfs2/super.c
@@ -514,6 +514,12 @@ static int nilfs_sync_fs(struct super_block *sb, int wait)
 	}
 	up_write(&nilfs->ns_sem);
 
+	if (wait && !err && nilfs_test_opt(nilfs, BARRIER)) {
+		err = blkdev_issue_flush(sb->s_bdev, GFP_KERNEL, NULL);
+		if (err != -EIO)
+			err = 0;
+	}
+
 	return err;
 }
 
-- 
2.1.0

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