On Mon, Aug 07, 2023 at 12:59:06PM +0200, Jan Kara wrote:
> 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:
>
Yeah.. the TLDR is basically that there were other paths that could set
REQ_NOWAIT on the bio (i.e. bio_set_polled()) that were unrelated to
IOCB_NOWAIT, hence the warning was spurious.
I recently noticed this patch [1], however, that seems to untangle some
of this logic. This patch looks like it wants to enforce IOCB_NOWAIT ->
REQ_NOWAIT for the particular case described in the commit log (i.e.
REQ_POLLED), which I also think would avoid the warning.
I was also finally able to reproduce the syzbot variant of the warning
and confirmed that it is different from the io_uring/HIPRI variant, but
still spurious wrt to IOCB_NOWAIT [2]. That one relates to GUP and doing
a killable wait on the mmap lock. I think I'll post a v2 of this patch
just to update the commit log with some of these details and I'll add
your R-b as well. Thanks for the review!
Brian
[1] https://lore.kernel.org/linux-block/b655aa3a-17f6-d25a-38b1-4a02e87e2c98@xxxxxxxxx/
[2] https://lore.kernel.org/linux-ext4/ZNTxfQ0StiqKbvWj@bfoster/
> 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
>
