[PATCH] ext4: drop dio overwrite only flag and associated warning

[PATCH] ext4: drop dio overwrite only flag and associated warning

[Brian Foster]

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@syzkaller.appspotmail.com
Fixes: 310ee0902b8d ("ext4: allow concurrent unaligned dio overwrites")
Signed-off-by: Brian Foster <bfoster@redhat.com>
---

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

Re: [PATCH] ext4: drop dio overwrite only flag and associated warning

[Jan Kara]

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@syzkaller.appspotmail.com
> Fixes: 310ee0902b8d ("ext4: allow concurrent unaligned dio overwrites")
> Signed-off-by: Brian Foster <bfoster@redhat.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@suse.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@suse.com>
SUSE Labs, CR

Re: [PATCH] ext4: drop dio overwrite only flag and associated warning

[Brian Foster]

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@syzkaller.appspotmail.com
> > Fixes: 310ee0902b8d ("ext4: allow concurrent unaligned dio overwrites")
> > Signed-off-by: Brian Foster <bfoster@redhat.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:
> 

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@kernel.dk/
[2] https://lore.kernel.org/linux-ext4/ZNTxfQ0StiqKbvWj@bfoster/

> Reviewed-by: Jan Kara <jack@suse.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@suse.com>
> SUSE Labs, CR
>