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

[Andreas Rohner <andreas.rohner-hi6Y0CQ0nG0@public.gmane.org>]

On 2014-09-09 23:17, 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      | 10 +++++++++-
>  fs/nilfs2/ioctl.c     | 10 +++++++++-
>  fs/nilfs2/segment.c   |  4 ++++
>  fs/nilfs2/super.c     | 17 +++++++++++++++++
>  fs/nilfs2/the_nilfs.h |  2 ++
>  5 files changed, 41 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/nilfs2/file.c b/fs/nilfs2/file.c
> index 2497815..16375c2 100644
> --- a/fs/nilfs2/file.c
> +++ b/fs/nilfs2/file.c
> @@ -56,7 +56,15 @@ 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) &&
> +	    !nilfs->ns_flushed_device) {
> +		nilfs->ns_flushed_device = 1;
> +		/*
> +		 * the store to ns_flushed_device must not be reordered after
> +		 * blkdev_issue_flush
> +		 */
> +		smp_wmb();
> +
>  		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..9444d5d 100644
> --- a/fs/nilfs2/ioctl.c
> +++ b/fs/nilfs2/ioctl.c
> @@ -1022,7 +1022,15 @@ 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) &&
> +	    !nilfs->ns_flushed_device) {
> +		nilfs->ns_flushed_device = 1;
> +		/*
> +		 * the store to ns_flushed_device must not be reordered after
> +		 * blkdev_issue_flush
> +		 */
> +		smp_wmb();
> +
>  		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..379da1b 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)
> +				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..33aafbd 100644
> --- a/fs/nilfs2/super.c
> +++ b/fs/nilfs2/super.c
> @@ -310,6 +310,9 @@ int nilfs_commit_super(struct super_block *sb, int flag)
>  					    nilfs->ns_sbsize));
>  	}
>  	clear_nilfs_sb_dirty(nilfs);
> +	nilfs->ns_flushed_device = 1;
> +	/* make sure store to ns_flushed_device cannot be reordered */
> +	smp_wmb();
>  	return nilfs_sync_super(sb, flag);
>  }
>  
> @@ -514,6 +517,20 @@ static int nilfs_sync_fs(struct super_block *sb, int wait)
>  	}
>  	up_write(&nilfs->ns_sem);
>  
> +	if (wait && !err && nilfs_test_opt(nilfs, BARRIER) &&
> +	    !nilfs->ns_flushed_device) {
> +		nilfs->ns_flushed_device = 1;
> +		/*
> +		 * the store to ns_flushed_device must not be reordered after
> +		 * blkdev_issue_flush
> +		 */
> +		smp_wmb();

I am not at all sure if this memory barrier is enough. Memory barriers
only guarantee the order in which memory operations hit the CPU cache.
They do not guarantee that all CPUSs see the previous memory operations.
They cannot be used to provide unconditional ordering. I am not even
sure if this can be done without proper locks, but I am not an expert in
lock-free algorithms.

Maybe the safest way would be to use nilfs->ns_sem around the whole if
statement.

> +		err = blkdev_issue_flush(sb->s_bdev, GFP_KERNEL, NULL);
> +		if (err != -EIO)
> +			err = 0;
> +	}
> +
>  	return err;
>  }
>  
> diff --git a/fs/nilfs2/the_nilfs.h b/fs/nilfs2/the_nilfs.h
> index d01ead1..dabb02c 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;
> +	int			ns_flushed_device;
>  
>  	struct block_device    *ns_bdev;
>  	struct rw_semaphore	ns_sem;
> 

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