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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: 310ee0902b8d ("ext4: allow concurrent unaligned dio overwrites")
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
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@xxxxxxx>
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@xxxxxxxx>
SUSE Labs, CR
