Re: [PATCH vfs.tmpfs v2 4/5] tmpfs: trivial support for direct IO

[Jan Kara <jack@suse.cz>]

On Thu 10-08-23 23:27:07, Hugh Dickins wrote:
> Depending upon your philosophical viewpoint, either tmpfs always does
> direct IO, or it cannot ever do direct IO; but whichever, if tmpfs is to
> stand in for a more sophisticated filesystem, it can be helpful for tmpfs
> to support O_DIRECT.  So, give tmpfs a shmem_file_open() method, to set
> the FMODE_CAN_ODIRECT flag: then unchanged shmem_file_read_iter() and new
> shmem_file_write_iter() do the work (without any shmem_direct_IO() stub).
> 
> Perhaps later, once the direct_IO method has been eliminated from all
> filesystems, generic_file_write_iter() will be such that tmpfs can again
> use it, even for O_DIRECT.
> 
> xfstests auto generic which were not run on tmpfs before but now pass:
> 036 091 113 125 130 133 135 198 207 208 209 210 211 212 214 226 239 263
> 323 355 391 406 412 422 427 446 451 465 551 586 591 609 615 647 708 729
> with no new failures.
> 
> LTP dio tests which were not run on tmpfs before but now pass:
> dio01 through dio30, except for dio04 and dio10, which fail because
> tmpfs dio read and write allow odd count: tmpfs could be made stricter,
> but would that be an improvement?
> 
> Signed-off-by: Hugh Dickins <hughd@google.com>
> ---
> Thanks for your earlier review, Jan: I've certainly not copied that
> into this entirely different version.  I prefer the v1, but fine if
> people prefer this v2.

Yeah, this solution is also fine with me so feel free to add:

Reviewed-by: Jan Kara <jack@suse.cz>

I agree the previous version has less code duplication but once .direct_IO
is gone shmem_file_write_iter() will be actually how some generic helper
will look like so we can deduplicate the code then.

								Honza
 
>  mm/shmem.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index ca43fb256b8e..b782edeb69aa 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -2388,6 +2388,12 @@ static int shmem_mmap(struct file *file, struct vm_area_struct *vma)
>  	return 0;
>  }
>  
> +static int shmem_file_open(struct inode *inode, struct file *file)
> +{
> +	file->f_mode |= FMODE_CAN_ODIRECT;
> +	return generic_file_open(inode, file);
> +}
> +
>  #ifdef CONFIG_TMPFS_XATTR
>  static int shmem_initxattrs(struct inode *, const struct xattr *, void *);
>  
> @@ -2839,6 +2845,28 @@ static ssize_t shmem_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
>  	return retval ? retval : error;
>  }
>  
> +static ssize_t shmem_file_write_iter(struct kiocb *iocb, struct iov_iter *from)
> +{
> +	struct file *file = iocb->ki_filp;
> +	struct inode *inode = file->f_mapping->host;
> +	ssize_t ret;
> +
> +	inode_lock(inode);
> +	ret = generic_write_checks(iocb, from);
> +	if (ret <= 0)
> +		goto unlock;
> +	ret = file_remove_privs(file);
> +	if (ret)
> +		goto unlock;
> +	ret = file_update_time(file);
> +	if (ret)
> +		goto unlock;
> +	ret = generic_perform_write(iocb, from);
> +unlock:
> +	inode_unlock(inode);
> +	return ret;
> +}
> +
>  static bool zero_pipe_buf_get(struct pipe_inode_info *pipe,
>  			      struct pipe_buffer *buf)
>  {
> @@ -4434,12 +4462,12 @@ EXPORT_SYMBOL(shmem_aops);
>  
>  static const struct file_operations shmem_file_operations = {
>  	.mmap		= shmem_mmap,
> -	.open		= generic_file_open,
> +	.open		= shmem_file_open,
>  	.get_unmapped_area = shmem_get_unmapped_area,
>  #ifdef CONFIG_TMPFS
>  	.llseek		= shmem_file_llseek,
>  	.read_iter	= shmem_file_read_iter,
> -	.write_iter	= generic_file_write_iter,
> +	.write_iter	= shmem_file_write_iter,
>  	.fsync		= noop_fsync,
>  	.splice_read	= shmem_file_splice_read,
>  	.splice_write	= iter_file_splice_write,
> -- 
> 2.35.3
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR