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@xxxxxx>
> Signed-off-by: John Garry <john.g.garry@xxxxxxxxxx>
Looks good to me,
Reviewed-by: Darrick J. Wong <djwong@xxxxxxxxxx>
--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
>
>
