Re: [PATCH 07/13] btrfs: basic direct read operation

[RITESH HARJANI <riteshh@linux.ibm.com>]

On 8/3/19 3:30 AM, Goldwyn Rodrigues wrote:
> From: Goldwyn Rodrigues <rgoldwyn@suse.com>
>
> Add btrfs_dio_iomap_ops for iomap.begin() function. In order to
> accomodate dio reads, add a new function btrfs_file_read_iter()
> which would call btrfs_dio_iomap_read() for DIO reads and
> fallback to generic_file_read_iter otherwise.
>
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn@suse.com>
> ---
>   fs/btrfs/ctree.h |  2 ++
>   fs/btrfs/file.c  | 10 +++++++++-
>   fs/btrfs/iomap.c | 20 ++++++++++++++++++++
>   3 files changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index 7a4ff524dc77..9eca2d576dd1 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -3247,7 +3247,9 @@ int btrfs_fdatawrite_range(struct inode *inode, loff_t start, loff_t end);
>   loff_t btrfs_remap_file_range(struct file *file_in, loff_t pos_in,
>   			      struct file *file_out, loff_t pos_out,
>   			      loff_t len, unsigned int remap_flags);
> +/* iomap.c */
>   size_t btrfs_buffered_iomap_write(struct kiocb *iocb, struct iov_iter *from);
> +ssize_t btrfs_dio_iomap_read(struct kiocb *iocb, struct iov_iter *to);
>   
>   /* tree-defrag.c */
>   int btrfs_defrag_leaves(struct btrfs_trans_handle *trans,
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index f7087e28ac08..997eb152a35a 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -2839,9 +2839,17 @@ static int btrfs_file_open(struct inode *inode, struct file *filp)
>   	return generic_file_open(inode, filp);
>   }
>   
> +static ssize_t btrfs_file_read_iter(struct kiocb *iocb, struct iov_iter *to)
> +{
> +	if (iocb->ki_flags & IOCB_DIRECT)
> +		return btrfs_dio_iomap_read(iocb, to);

No provision to fallback to bufferedIO read? Not sure from btrfs 
perspective,
but earlier generic_file_read_iter may fall through to bufferedIO read 
say in case where directIO could not be completed (returned 0 or less 
than the requested read bytes).
Is it not required anymore in case of btrfs when we move to iomap 
infrastructure, to still fall back to bufferedIO read?
Correct me if I am missing anything here.

> +
> +	return generic_file_read_iter(iocb, to);
> +}
> +
>   const struct file_operations btrfs_file_operations = {
>   	.llseek		= btrfs_file_llseek,
> -	.read_iter      = generic_file_read_iter,
> +	.read_iter      = btrfs_file_read_iter,
>   	.splice_read	= generic_file_splice_read,
>   	.write_iter	= btrfs_file_write_iter,
>   	.mmap		= btrfs_file_mmap,
> diff --git a/fs/btrfs/iomap.c b/fs/btrfs/iomap.c
> index 879038e2f1a0..36df606fc028 100644
> --- a/fs/btrfs/iomap.c
> +++ b/fs/btrfs/iomap.c
> @@ -420,3 +420,23 @@ size_t btrfs_buffered_iomap_write(struct kiocb *iocb, struct iov_iter *from)
>   	return written;
>   }
>   
> +static int btrfs_dio_iomap_begin(struct inode *inode, loff_t pos,
> +		loff_t length, unsigned flags, struct iomap *iomap,
> +		struct iomap *srcmap)
> +{
> +	return get_iomap(inode, pos, length, iomap);
> +}
> +
> +static const struct iomap_ops btrfs_dio_iomap_ops = {
> +	.iomap_begin            = btrfs_dio_iomap_begin,
> +};
> +
> +ssize_t btrfs_dio_iomap_read(struct kiocb *iocb, struct iov_iter *to)
> +{
> +	struct inode *inode = file_inode(iocb->ki_filp);
> +	ssize_t ret;
> +	inode_lock_shared(inode);
> +	ret = iomap_dio_rw(iocb, to, &btrfs_dio_iomap_ops, NULL);
> +	inode_unlock_shared(inode);
> +	return ret;
> +}