ext4: drop dio overwrite only flag and associated warning
PATCH 1/1 ext4: drop dio overwrite only flag and associated warning
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.
---
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(-)
fs/ext4/file.c +10 -15
@@ -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 ;
Signature · 1 line
2.41.0
Quoted 25 lines · 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_ONLYflag and added a warning on unexpected-EAGAINreturns 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
-EAGAINis expected from the storage layer without necessarily havingIOCB_NOWAITset on the iocb. For example,io_uringcan setIOCB_HIPRI, which the iomap/dio layer turns into REQ_POLLED|REQ_NOWAIT on the bio, which then can result in an-EAGAINresult 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-EAGAINin 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
-EAGAINreturns 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.
Quoted 3 lines
Reported-by [email protected]Fixes 310ee0902b8d ("ext4: allow concurrent unaligned dio overwrites")Signed-off-by Brian Foster <[email protected]>
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:
Honza
Quoted 73 lines
---
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(-)fs/ext4/file.c +10
-15@@ -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 ;Signature · 1 line
2.41.0
Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR
Quoted 30 lines · Jan Kara
Quoted 25 lines · 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_ONLYflag and added a warning on unexpected-EAGAINreturns 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
-EAGAINis expected from the storage layer without necessarily havingIOCB_NOWAITset on the iocb. For example,io_uringcan setIOCB_HIPRI, which the iomap/dio layer turns into REQ_POLLED|REQ_NOWAIT on the bio, which then can result in an-EAGAINresult 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-EAGAINin 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
-EAGAINreturns 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.Quoted 3 lines
Reported-by [email protected]Fixes 310ee0902b8d ("ext4: allow concurrent unaligned dio overwrites")Signed-off-by Brian Foster <[email protected]>
Quoted 6 lines
So if I understand right, you're trying to say that if
iomap_dio_rw()returns-EAGAIN, the caller ofext4_file_write_iter()and notext4_file_write_iter()itself is expected to deal with it (like withIOCB_NOWAITor 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/[email protected]/
[2] https://lore.kernel.org/linux-ext4/ZNTxfQ0StiqKbvWj@bfoster/
Quoted 80 lines
Reviewed-by Jan Kara <[email protected]>Honza
Quoted 73 lines
---
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(-)fs/ext4/file.c +10
-15@@ -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 ;Signature · 1 line
2.41.0Signature · 2 lines
Jan Kara <[email protected]> SUSE Labs, CR