Message-ID: <20230807105906.teovthvnwrpbmx7n@quack3>
Date:   Mon, 7 Aug 2023 12:59:06 +0200
From:   Jan Kara <jack@...e.cz>
To:     Brian Foster <bfoster@...hat.com>
Cc:     linux-ext4@...r.kernel.org, tytso@....edu,
        Ritesh Harjani <ritesh.list@...il.com>, Jan Kara <jack@...e.cz>
Subject: Re: [PATCH] ext4: drop dio overwrite only flag and associated warning

On Fri 04-08-23 14:29:52, Brian Foster wrote:
> The commit referenced below opened up concurrent unaligned dio under
> shared locking for pure overwrites. In doing so, it enabled use of
> the IOMAP_DIO_OVERWRITE_ONLY flag and added a warning on unexpected
> -EAGAIN returns as an extra precaution, since ext4 does not retry
> writes in such cases. The flag itself is advisory in this case since
> ext4 checks for unaligned I/Os and uses appropriate locking up
> front, rather than on a retry in response to -EAGAIN.
> 
> As it turns out, the warning check is susceptible to false positives
> because there are scenarios where -EAGAIN is expected from the
> storage layer without necessarily having IOCB_NOWAIT set on the
> iocb. For example, io_uring can set IOCB_HIPRI, which the iomap/dio
> layer turns into REQ_POLLED|REQ_NOWAIT on the bio, which then can
> result in an -EAGAIN result if the block layer is unable to allocate
> a request, etc. syzbot has also reported an instance of this warning
> and while the source of the -EAGAIN in that case is not currently
> known, it is confirmed that the iomap dio overwrite flag is also not
> set.
> 
> Since this flag is precautionary, avoid the false positive warning
> and future whack-a-mole games with -EAGAIN returns by removing it
> and the associated warning. Update the comments to document when
> concurrent unaligned dio writes are allowed and why the associated
> flag is not used.
> 
> Reported-by: syzbot+5050ad0fb47527b1808a@...kaller.appspotmail.com
> Fixes: 310ee0902b8d ("ext4: allow concurrent unaligned dio overwrites")
> Signed-off-by: Brian Foster <bfoster@...hat.com>

So if I understand right, you're trying to say that if iomap_dio_rw()
returns -EAGAIN, the caller of ext4_file_write_iter() and not
ext4_file_write_iter() itself is expected to deal with it (like with
IOCB_NOWAIT or other ways that can trigger similar behavior). That sounds
good to me and the patch looks also fine. Feel free to add:

Reviewed-by: Jan Kara <jack@...e.cz>

								Honza
> ---
> 
> Hi all,
> 
> This addresses some false positives associated with the warning for the
> recently merged patch. I considered leaving the flag and more tightly
> associating the warning to it (instead of IOCB_NOWAIT), but ISTM that is
> still flakey and I'd rather not play whack-a-mole when the assumption is
> shown to be wrong.
> 
> I'm still waiting on a syzbot test of this patch, but local tests look
> Ok and I'm away for a few days after today so wanted to get this on the
> list. Thoughts, reviews, flames appreciated.
> 
> Brian
> 
>  fs/ext4/file.c | 25 ++++++++++---------------
>  1 file changed, 10 insertions(+), 15 deletions(-)
> 
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index c457c8517f0f..73a4b711be02 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -476,6 +476,11 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
>  	 * required to change security info in file_modified(), for extending
>  	 * I/O, any form of non-overwrite I/O, and unaligned I/O to unwritten
>  	 * extents (as partial block zeroing may be required).
> +	 *
> +	 * Note that unaligned writes are allowed under shared lock so long as
> +	 * they are pure overwrites. Otherwise, concurrent unaligned writes risk
> +	 * data corruption due to partial block zeroing in the dio layer, and so
> +	 * the I/O must occur exclusively.
>  	 */
>  	if (*ilock_shared &&
>  	    ((!IS_NOSEC(inode) || *extend || !overwrite ||
> @@ -492,21 +497,12 @@ static ssize_t ext4_dio_write_checks(struct kiocb *iocb, struct iov_iter *from,
>  
>  	/*
>  	 * Now that locking is settled, determine dio flags and exclusivity
> -	 * requirements. Unaligned writes are allowed under shared lock so long
> -	 * as they are pure overwrites. Set the iomap overwrite only flag as an
> -	 * added precaution in this case. Even though this is unnecessary, we
> -	 * can detect and warn on unexpected -EAGAIN if an unsafe unaligned
> -	 * write is ever submitted.
> -	 *
> -	 * Otherwise, concurrent unaligned writes risk data corruption due to
> -	 * partial block zeroing in the dio layer, and so the I/O must occur
> -	 * exclusively. The inode lock is already held exclusive if the write is
> -	 * non-overwrite or extending, so drain all outstanding dio and set the
> -	 * force wait dio flag.
> +	 * requirements. We don't use DIO_OVERWRITE_ONLY because we enforce
> +	 * behavior already. The inode lock is already held exclusive if the
> +	 * write is non-overwrite or extending, so drain all outstanding dio and
> +	 * set the force wait dio flag.
>  	 */
> -	if (*ilock_shared && unaligned_io) {
> -		*dio_flags = IOMAP_DIO_OVERWRITE_ONLY;
> -	} else if (!*ilock_shared && (unaligned_io || *extend)) {
> +	if (!*ilock_shared && (unaligned_io || *extend)) {
>  		if (iocb->ki_flags & IOCB_NOWAIT) {
>  			ret = -EAGAIN;
>  			goto out;
> @@ -608,7 +604,6 @@ static ssize_t ext4_dio_write_iter(struct kiocb *iocb, struct iov_iter *from)
>  		iomap_ops = &ext4_iomap_overwrite_ops;
>  	ret = iomap_dio_rw(iocb, from, iomap_ops, &ext4_dio_write_ops,
>  			   dio_flags, NULL, 0);
> -	WARN_ON_ONCE(ret == -EAGAIN && !(iocb->ki_flags & IOCB_NOWAIT));
>  	if (ret == -ENOTBLK)
>  		ret = 0;
>  
> -- 
> 2.41.0
> 
-- 
Jan Kara <jack@...e.com>
SUSE Labs, CR

Powered by blists - more mailing lists