linux-kernel - Re: [PATCH v10 5/8] fs: iomap: Atomic write support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87sesrgp5v.fsf@gmail.com>
Date: Sun, 20 Oct 2024 13:51:48 +0530
From: Ritesh Harjani (IBM) <ritesh.list@...il.com>
To: John Garry <john.g.garry@...cle.com>, axboe@...nel.dk, brauner@...nel.org, djwong@...nel.org, viro@...iv.linux.org.uk, jack@...e.cz, dchinner@...hat.com, hch@....de, cem@...nel.org
Cc: linux-block@...r.kernel.org, linux-kernel@...r.kernel.org, linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org, hare@...e.de, martin.petersen@...cle.com, catherine.hoang@...cle.com, mcgrof@...nel.org, ojaswin@...ux.ibm.com, John Garry <john.g.garry@...cle.com>
Subject: Re: [PATCH v10 5/8] fs: iomap: Atomic write support

John Garry <john.g.garry@...cle.com> writes:

> Support direct I/O atomic writes by producing a single bio with REQ_ATOMIC
> flag set.
>
> Initially FSes (XFS) should only support writing a single FS block
> atomically.
>
> As with any atomic write, we should produce a single bio which covers the
> complete write length.
>
> Reviewed-by: Christoph Hellwig <hch@....de>
> Reviewed-by: "Darrick J. Wong" <djwong@...nel.org>
> Signed-off-by: John Garry <john.g.garry@...cle.com>
> ---
>  .../filesystems/iomap/operations.rst          | 12 ++++++
>  fs/iomap/direct-io.c                          | 38 +++++++++++++++++--
>  fs/iomap/trace.h                              |  3 +-
>  include/linux/iomap.h                         |  1 +
>  4 files changed, 49 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/filesystems/iomap/operations.rst b/Documentation/filesystems/iomap/operations.rst
> index b93115ab8748..529f81dd3d2c 100644
> --- a/Documentation/filesystems/iomap/operations.rst
> +++ b/Documentation/filesystems/iomap/operations.rst
> @@ -513,6 +513,18 @@ IOMAP_WRITE`` with any combination of the following enhancements:
>     if the mapping is unwritten and the filesystem cannot handle zeroing
>     the unaligned regions without exposing stale contents.
>  
> + * ``IOMAP_ATOMIC``: This write is being issued with torn-write
> +   protection.
> +   Only a single bio can be created for the write, and the write must
> +   not be split into multiple I/O requests, i.e. flag REQ_ATOMIC must be
> +   set.
> +   The file range to write must be aligned to satisfy the requirements
> +   of both the filesystem and the underlying block device's atomic
> +   commit capabilities.
> +   If filesystem metadata updates are required (e.g. unwritten extent
> +   conversion or copy on write), all updates for the entire file range
> +   must be committed atomically as well.
> +
>  Callers commonly hold ``i_rwsem`` in shared or exclusive mode before
>  calling this function.
>  
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index f637aa0706a3..ed4764e3b8f0 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -271,7 +271,7 @@ static int iomap_dio_zero(const struct iomap_iter *iter, struct iomap_dio *dio,
>   * clearing the WRITE_THROUGH flag in the dio request.
>   */
>  static inline blk_opf_t iomap_dio_bio_opflags(struct iomap_dio *dio,
> -		const struct iomap *iomap, bool use_fua)
> +		const struct iomap *iomap, bool use_fua, bool atomic)
>  {
>  	blk_opf_t opflags = REQ_SYNC | REQ_IDLE;
>  
> @@ -283,6 +283,8 @@ static inline blk_opf_t iomap_dio_bio_opflags(struct iomap_dio *dio,
>  		opflags |= REQ_FUA;
>  	else
>  		dio->flags &= ~IOMAP_DIO_WRITE_THROUGH;
> +	if (atomic)
> +		opflags |= REQ_ATOMIC;
>  
>  	return opflags;
>  }
> @@ -293,7 +295,8 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>  	const struct iomap *iomap = &iter->iomap;
>  	struct inode *inode = iter->inode;
>  	unsigned int fs_block_size = i_blocksize(inode), pad;
> -	loff_t length = iomap_length(iter);
> +	const loff_t length = iomap_length(iter);
> +	bool atomic = iter->flags & IOMAP_ATOMIC;
>  	loff_t pos = iter->pos;
>  	blk_opf_t bio_opf;
>  	struct bio *bio;
> @@ -303,6 +306,9 @@ static loff_t iomap_dio_bio_iter(const struct iomap_iter *iter,
>  	size_t copied = 0;
>  	size_t orig_count;
>  
> +	if (atomic && length != fs_block_size)
> +		return -EINVAL;

We anyway mandate iov_iter_count() write should be same as sb_blocksize
in xfs_file_write_iter() for atomic writes.
This comparison here is not required. I believe we do plan to lift this
restriction maybe when we are going to add forcealign support right? 

And similarly this needs to be lifted when ext4 adds support for atomic
write even with bigalloc. I hope we can do so when we add such support, right?

(I guess, that is also the reason we haven't mentioned this restriction
in description of "IOMAP_ATOMIC" in Documentation.)

-ritesh