[-v2] ext4: fake direct I/O mode for data=journal

Currently attempts to open a file with O_DIRECT in data=journal mode
causes the open to fail with -EINVAL.  This makes it very hard to test
data=journal mode.  So we will let the open succeed, but then always
fall back to O_DSYNC buffered writes.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/file.c  |   28 +++++++++++++++++++++++++++-
 fs/ext4/inode.c |    1 +
 2 files changed, 28 insertions(+), 1 deletions(-)

On Tue 16-08-11 15:32:25, Ted Tso wrote:
> Currently attempts to open a file with O_DIRECT in data=journal mode
> causes the open to fail with -EINVAL.  This makes it very hard to test
> data=journal mode.  So we will let the open succeed, but then always
> fall back to O_DSYNC buffered writes.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  fs/ext4/file.c  |   28 +++++++++++++++++++++++++++-
>  fs/ext4/inode.c |    1 +
>  2 files changed, 28 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index e4095e9..566f33f 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -89,6 +89,24 @@ ext4_unaligned_aio(struct inode *inode, const struct iovec *iov,
>  	return 0;
>  }
>  
> +ssize_t
> +ext4_file_read(struct kiocb *iocb, const struct iovec *iov,
> +	       unsigned long nr_segs, loff_t pos)
> +{
> +	struct inode *inode = iocb->ki_filp->f_path.dentry->d_inode;
> +
> +	/*
> +	 * If O_DIRECT is set and we are doing data journalling we
> +	 * don't support O_DIRECT so force it off.
> +	 */
> +	if ((iocb->ki_filp->f_flags & O_DIRECT) &&
> +	    ext4_should_journal_data(inode))
> +		iocb->ki_filp->f_flags &= ~O_DIRECT;
> +
> +	return generic_file_aio_read(iocb, iov, nr_segs, pos);
> +}
> +
> +
  Two empty lines here?

>  static ssize_t
>  ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  		unsigned long nr_segs, loff_t pos)
> @@ -98,6 +116,14 @@ ext4_file_write(struct kiocb *iocb, const struct iovec *iov,
>  	int ret;
>  
>  	/*
> +	 * If O_DIRECT is set and we are doing data journalling we
> +	 * don't support O_DIRECT so force it off.
> +	 */
> +	if ((iocb->ki_filp->f_flags & O_DIRECT) &&
> +	    ext4_should_journal_data(inode))
> +		iocb->ki_filp->f_flags &= ~O_DIRECT;
> +
> +	/*
>  	 * If we have encountered a bitmap-format file, the size limit
>  	 * is smaller than s_maxbytes, which is for extent-mapped files.
>  	 */
> @@ -277,7 +303,7 @@ const struct file_operations ext4_file_operations = {
>  	.llseek		= ext4_llseek,
>  	.read		= do_sync_read,
>  	.write		= do_sync_write,
> -	.aio_read	= generic_file_aio_read,
> +	.aio_read	= ext4_file_read,
>  	.aio_write	= ext4_file_write,
>  	.unlocked_ioctl = ext4_ioctl,
>  #ifdef CONFIG_COMPAT
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 762e803..b7088e2 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -2921,6 +2921,7 @@ static const struct address_space_operations ext4_journalled_aops = {
>  	.bmap			= ext4_bmap,
>  	.invalidatepage		= ext4_invalidatepage,
>  	.releasepage		= ext4_releasepage,
> +	.direct_IO		= ext4_direct_IO,
>  	.is_partially_uptodate  = block_is_partially_uptodate,
>  	.error_remove_page	= generic_error_remove_page,
>  };
  Strictly speaking this is racy wrt. to fcntl() setting O_DIRECT bit.
Fixing it is actually quite simple - just provide ext4_noop_direct_IO
function that will just "return 0" and generic write code will fall
back to buffered IO automatically. We don't want to use the fallback all
the time since it incurs overhead of flushing and invalidating the mapping
so what you did above will catch the common case.

								Honza

On Wed, Aug 17, 2011 at 01:42:52PM +0200, Jan Kara wrote:
>   Strictly speaking this is racy wrt. to fcntl() setting O_DIRECT bit.
> Fixing it is actually quite simple - just provide ext4_noop_direct_IO
> function that will just "return 0" and generic write code will fall
> back to buffered IO automatically. We don't want to use the fallback all
> the time since it incurs overhead of flushing and invalidating the mapping
> so what you did above will catch the common case.

Good point!  Yes, that's a much better way to do it.  Actually
ext4_noop_direct_IO() isn't good enough, since even with data=ordered
or data=journalled, because you can set the 'j' attribute to force the
file to use data journalling.  (This is true in ext3 as well, and I'm
not sure something sane happens if you force a file to be data
journalling and then do direct IO on it when in data=ordered or
data=writeback mode.)

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