[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <874j4rzlzl.fsf@gmail.com>
Date: Fri, 01 Nov 2024 08:41:42 +0530
From: Ritesh Harjani (IBM) <ritesh.list@...il.com>
To: "Darrick J. Wong" <djwong@...nel.org>
Cc: linux-ext4@...r.kernel.org, Theodore Ts'o <tytso@....edu>, Jan Kara <jack@...e.cz>, Christoph Hellwig <hch@...radead.org>, John Garry <john.g.garry@...cle.com>, Ojaswin Mujoo <ojaswin@...ux.ibm.com>, Dave Chinner <david@...morbit.com>, linux-kernel@...r.kernel.org, linux-xfs@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v3 4/4] ext4: Do not fallback to buffered-io for DIO atomic write
"Darrick J. Wong" <djwong@...nel.org> writes:
> On Wed, Oct 30, 2024 at 09:27:41PM +0530, Ritesh Harjani (IBM) wrote:
>> atomic writes is currently only supported for single fsblock and only
>> for direct-io. We should not return -ENOTBLK for atomic writes since we
>> want the atomic write request to either complete fully or fail
>> otherwise. We should not fallback to buffered-io in case of DIO atomic
>> write requests.
>> Let's also catch if this ever happens by adding some WARN_ON_ONCE before
>> buffered-io handling for direct-io atomic writes.
>>
>> More details of the discussion [1].
>>
>> [1]: https://lore.kernel.org/linux-xfs/cover.1729825985.git.ritesh.list@gmail.com/T/#m9dbecc11bed713ed0d7a486432c56b105b555f04
>>
>> Signed-off-by: Ritesh Harjani (IBM) <ritesh.list@...il.com>
>> ---
>> fs/ext4/file.c | 7 +++++++
>> fs/ext4/inode.c | 14 +++++++++-----
>> 2 files changed, 16 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
>> index 8116bd78910b..61787a37e9d4 100644
>> --- a/fs/ext4/file.c
>> +++ b/fs/ext4/file.c
>> @@ -599,6 +599,13 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
>> ssize_t err;
>> loff_t endbyte;
>>
>> + /*
>> + * There is no support for atomic writes on buffered-io yet,
>> + * we should never fallback to buffered-io for DIO atomic
>> + * writes.
>> + */
>> + WARN_ON_ONCE(iocb->ki_flags & IOCB_ATOMIC);
>> +
>> offset = iocb->ki_pos;
>> err = ext4_buffered_write_iter(iocb, from);
>> if (err < 0)
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index fcdee27b9aa2..26b3c84d7f64 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -3449,12 +3449,16 @@ static int ext4_iomap_end(struct inode *inode, loff_t offset, loff_t length,
>> {
>> /*
>> * Check to see whether an error occurred while writing out the data to
>> - * the allocated blocks. If so, return the magic error code so that we
>> - * fallback to buffered I/O and attempt to complete the remainder of
>> - * the I/O. Any blocks that may have been allocated in preparation for
>> - * the direct I/O will be reused during buffered I/O.
>> + * the allocated blocks. If so, return the magic error code for
>> + * non-atomic write so that we fallback to buffered I/O and attempt to
>> + * complete the remainder of the I/O.
>> + * For atomic writes we will simply fail the I/O request if we coudn't
>> + * write anything. For non-atomic writes, any blocks that may have been
>> + * allocated in preparation for the direct I/O will be reused during
>> + * buffered I/O.
>> */
>> - if (flags & (IOMAP_WRITE | IOMAP_DIRECT) && written == 0)
>> + if (!(flags & IOMAP_ATOMIC) && (flags & (IOMAP_WRITE | IOMAP_DIRECT))
>
> Huh. The WRITE|DIRECT check doesn't look right to me, because the
> expression returns true for any write or any directio. I think that's
> currently "ok" because ext4_iomap_end is only called for directio
> writes, but this bugs me anyway. For a directio write fallback, that
> comparison really should be:
>
> (flags & (WRITE|DIRECT)) == (WRITE|DIRECT)
>
yes. You are right. It is working since ext4 only supports iomap
for DIRECTIO. But I agree it's better be fixed to avoid problem in future.
> static inline bool
> ext4_want_directio_fallback(unsigned flags, ssize_t written)
> {
> /* must be a directio to fall back to buffered */
> if (flags & (IOMAP_WRITE | IOMAP_DIRECT)) !=
> (IOMAP_WRITE | IOMAP_DIRECT)
> return false;
>
> /* atomic writes are all-or-nothing */
> if (flags & IOMAP_ATOMIC)
> return false;
>
> /* can only try again if we wrote nothing */
> return written == 0;
> }
>
> if (ext4_want_directio_fallback(flags, written))
> return -ENOTBLK;
>
I like the above helper. Thanks for doing that.
I will incorporate this in v4.
>> + && written == 0)
>
> Nit: put the '&&' operator on the previous line when there's a multiline
> expression.
>
I guess we don't need this if we do it with your above inline helper.
But sure, next time will keep in mind for any such changes.
> --D
>
Thanks for the review!
-ritesh
>> return -ENOTBLK;
>>
>> return 0;
>> --
>> 2.46.0
>>
>>
Powered by blists - more mailing lists