Re: [PATCH v9 2/8] fs/block: Check for IOCB_DIRECT in generic_atomic_write_valid()

["Darrick J. Wong" <djwong@kernel.org>]

On Wed, Oct 16, 2024 at 10:03:19AM +0000, John Garry wrote:
> Currently FMODE_CAN_ATOMIC_WRITE is set if the bdev can atomic write and
> the file is open for direct IO. This does not work if the file is not
> opened for direct IO, yet fcntl(O_DIRECT) is used on the fd later.
> 
> Change to check for direct IO on a per-IO basis in
> generic_atomic_write_valid(). Since we want to report -EOPNOTSUPP for
> non-direct IO for an atomic write, change to return an error code.
> 
> Relocate the block fops atomic write checks to the common write path, as to
> catch non-direct IO.
> 
> Fixes: c34fc6f26ab8 ("fs: Initial atomic write support")
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Signed-off-by: John Garry <john.g.garry@oracle.com>

Looks good to me,
Reviewed-by: Darrick J. Wong <djwong@kernel.org>

--D

> ---
>  block/fops.c       | 18 ++++++++++--------
>  fs/read_write.c    | 13 ++++++++-----
>  include/linux/fs.h |  2 +-
>  3 files changed, 19 insertions(+), 14 deletions(-)
> 
> diff --git a/block/fops.c b/block/fops.c
> index 968b47b615c4..2d01c9007681 100644
> --- a/block/fops.c
> +++ b/block/fops.c
> @@ -36,11 +36,8 @@ static blk_opf_t dio_bio_write_op(struct kiocb *iocb)
>  }
>  
>  static bool blkdev_dio_invalid(struct block_device *bdev, struct kiocb *iocb,
> -				struct iov_iter *iter, bool is_atomic)
> +				struct iov_iter *iter)
>  {
> -	if (is_atomic && !generic_atomic_write_valid(iocb, iter))
> -		return true;
> -
>  	return iocb->ki_pos & (bdev_logical_block_size(bdev) - 1) ||
>  		!bdev_iter_is_aligned(bdev, iter);
>  }
> @@ -368,13 +365,12 @@ static ssize_t __blkdev_direct_IO_async(struct kiocb *iocb,
>  static ssize_t blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>  {
>  	struct block_device *bdev = I_BDEV(iocb->ki_filp->f_mapping->host);
> -	bool is_atomic = iocb->ki_flags & IOCB_ATOMIC;
>  	unsigned int nr_pages;
>  
>  	if (!iov_iter_count(iter))
>  		return 0;
>  
> -	if (blkdev_dio_invalid(bdev, iocb, iter, is_atomic))
> +	if (blkdev_dio_invalid(bdev, iocb, iter))
>  		return -EINVAL;
>  
>  	nr_pages = bio_iov_vecs_to_alloc(iter, BIO_MAX_VECS + 1);
> @@ -383,7 +379,7 @@ static ssize_t blkdev_direct_IO(struct kiocb *iocb, struct iov_iter *iter)
>  			return __blkdev_direct_IO_simple(iocb, iter, bdev,
>  							nr_pages);
>  		return __blkdev_direct_IO_async(iocb, iter, bdev, nr_pages);
> -	} else if (is_atomic) {
> +	} else if (iocb->ki_flags & IOCB_ATOMIC) {
>  		return -EINVAL;
>  	}
>  	return __blkdev_direct_IO(iocb, iter, bdev, bio_max_segs(nr_pages));
> @@ -625,7 +621,7 @@ static int blkdev_open(struct inode *inode, struct file *filp)
>  	if (!bdev)
>  		return -ENXIO;
>  
> -	if (bdev_can_atomic_write(bdev) && filp->f_flags & O_DIRECT)
> +	if (bdev_can_atomic_write(bdev))
>  		filp->f_mode |= FMODE_CAN_ATOMIC_WRITE;
>  
>  	ret = bdev_open(bdev, mode, filp->private_data, NULL, filp);
> @@ -700,6 +696,12 @@ static ssize_t blkdev_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  	if ((iocb->ki_flags & (IOCB_NOWAIT | IOCB_DIRECT)) == IOCB_NOWAIT)
>  		return -EOPNOTSUPP;
>  
> +	if (iocb->ki_flags & IOCB_ATOMIC) {
> +		ret = generic_atomic_write_valid(iocb, from);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	size -= iocb->ki_pos;
>  	if (iov_iter_count(from) > size) {
>  		shorted = iov_iter_count(from) - size;
> diff --git a/fs/read_write.c b/fs/read_write.c
> index 2c3263530828..befec0b5c537 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -1830,18 +1830,21 @@ int generic_file_rw_checks(struct file *file_in, struct file *file_out)
>  	return 0;
>  }
>  
> -bool generic_atomic_write_valid(struct kiocb *iocb, struct iov_iter *iter)
> +int generic_atomic_write_valid(struct kiocb *iocb, struct iov_iter *iter)
>  {
>  	size_t len = iov_iter_count(iter);
>  
>  	if (!iter_is_ubuf(iter))
> -		return false;
> +		return -EINVAL;
>  
>  	if (!is_power_of_2(len))
> -		return false;
> +		return -EINVAL;
>  
>  	if (!IS_ALIGNED(iocb->ki_pos, len))
> -		return false;
> +		return -EINVAL;
>  
> -	return true;
> +	if (!(iocb->ki_flags & IOCB_DIRECT))
> +		return -EOPNOTSUPP;
> +
> +	return 0;
>  }
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index fbfa032d1d90..ba47fb283730 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -3721,6 +3721,6 @@ static inline bool vfs_empty_path(int dfd, const char __user *path)
>  	return !c;
>  }
>  
> -bool generic_atomic_write_valid(struct kiocb *iocb, struct iov_iter *iter);
> +int generic_atomic_write_valid(struct kiocb *iocb, struct iov_iter *iter);
>  
>  #endif /* _LINUX_FS_H */
> -- 
> 2.31.1
> 
>