[PATCH 0/2] block: fail short atomic bounce bios in bio_iov_iter_bounce_write()

[PATCH 0/2] block: fail short atomic bounce bios in bio_iov_iter_bounce_write()

[Tal Zussman]

bio_iov_iter_get_pages() fails a short REQ_ATOMIC bio. In contrast,
bio_iov_iter_bounce_write() doesn't, so a bounced atomic write that runs
into a folio allocation failure triggers a WARN_ON_ONCE() in
iomap_dio_bio_iter_one().

The first patch makes bio_iov_iter_bounce_write() also check that a
REQ_ATOMIC bio covers the whole iterator, and clean up and return
-EINVAL if it doesn't. With both paths covered the iomap check can't
trigger, so the second patch removes it, as suggested by John Garry.

---
Tal Zussman (2):
      block: fail a short atomic bounce in bio_iov_iter_bounce_write()
      iomap: remove the atomic write length check in iomap_dio_bio_iter_one()

 block/bio.c          | 10 ++++++++++
 fs/iomap/direct-io.c | 14 --------------
 2 files changed, 10 insertions(+), 14 deletions(-)
---
base-commit: 839203759f536e6ef1267c80309c3532a7d588cc
change-id: 20261014-blkdev-atomic-bounce-fc9c583083d7

Best regards,
--  
Tal Zussman <tz2294@columbia.edu>

[PATCH 1/2] block: fail a short atomic bounce in bio_iov_iter_bounce_write()

[Tal Zussman]

bio_iov_iter_get_pages() fails a REQ_ATOMIC bio that doesn't cover the
whole iterator, since an atomic write is submitted as a single bio, and
submitting only part of it would tear the write. In contrast,
bio_iov_iter_bounce_write() doesn't. If a folio allocation fails partway
through, it returns a short bio, which for an atomic write triggers a
WARN_ON_ONCE() in iomap_dio_bio_iter_one().

Make bio_iov_iter_bounce_write() also check that a REQ_ATOMIC bio covers
the whole iterator, and clean up and return -EINVAL if it doesn't.

Signed-off-by: Tal Zussman <tz2294@columbia.edu>
---
 block/bio.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/block/bio.c b/block/bio.c
index b0fcc69de653..61a3812520a2 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -1523,6 +1523,16 @@ int bio_iov_iter_bounce_write(struct bio *bio, struct iov_iter *iter,
 		}
 	}
 
+	/*
+	 * An atomic write is submitted as a single bio, so it has to cover
+	 * the whole iterator or it would be torn.
+	 */
+	if ((bio->bi_opf & REQ_ATOMIC) && iov_iter_count(iter)) {
+		iov_iter_revert(iter, total_copied);
+		bio_free_folios(bio);
+		return -EINVAL;
+	}
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(bio_iov_iter_bounce_write);

-- 
2.39.5

Re: [PATCH 1/2] block: fail a short atomic bounce in bio_iov_iter_bounce_write()

[John Garry]

On 11/8/26 04:24, Tal Zussman wrote:
> bio_iov_iter_get_pages() fails a REQ_ATOMIC bio that doesn't cover the
> whole iterator, since an atomic write is submitted as a single bio, and
> submitting only part of it would tear the write. In contrast,
> bio_iov_iter_bounce_write() doesn't. If a folio allocation fails partway
> through, it returns a short bio, which for an atomic write triggers a
> WARN_ON_ONCE() in iomap_dio_bio_iter_one().
> 
> Make bio_iov_iter_bounce_write() also check that a REQ_ATOMIC bio covers
> the whole iterator, and clean up and return -EINVAL if it doesn't.
> 
> Signed-off-by: Tal Zussman <tz2294@columbia.edu>

This looks reasonable, as atomic stable writes are implied to be 
supported from 3373503df025a ("xfs: use bounce buffering direct I/O when 
the device requires stable pages"). I am wondering how to test and prove 
this (change is good and maintained)?

> ---
>   block/bio.c | 10 ++++++++++
>   1 file changed, 10 insertions(+)
> 
> diff --git a/block/bio.c b/block/bio.c
> index b0fcc69de653..61a3812520a2 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -1523,6 +1523,16 @@ int bio_iov_iter_bounce_write(struct bio *bio, struct iov_iter *iter,
>   		}
>   	}
>   
> +	/*
> +	 * An atomic write is submitted as a single bio, so it has to cover
> +	 * the whole iterator or it would be torn.
> +	 */
> +	if ((bio->bi_opf & REQ_ATOMIC) && iov_iter_count(iter)) {
> +		iov_iter_revert(iter, total_copied);
> +		bio_free_folios(bio);
> +		return -EINVAL;
> +	}
> +
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(bio_iov_iter_bounce_write);
> 

[PATCH 2/2] iomap: remove the atomic write length check in iomap_dio_bio_iter_one()

[Tal Zussman]

bio_iov_iter_get_pages() and bio_iov_iter_bounce_write() now fail a
REQ_ATOMIC bio that doesn't cover the whole iterator, so the bio here
always covers iomap_length() and the check can't trigger. Remove the
check.

Suggested-by: John Garry <john.garry@linux.dev>
Link: https://lore.kernel.org/all/82978d5c-3e08-48e9-bf0a-7913e1cd0507@linux.dev/
Signed-off-by: Tal Zussman <tz2294@columbia.edu>
---
 fs/iomap/direct-io.c | 14 --------------
 1 file changed, 14 deletions(-)

diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
index ad041ac3252a..aca7cbaea4af 100644
--- a/fs/iomap/direct-io.c
+++ b/fs/iomap/direct-io.c
@@ -370,15 +370,6 @@ static ssize_t iomap_dio_bio_iter_one(struct iomap_iter *iter,
 		goto out_put_bio;
 	ret = bio->bi_iter.bi_size;
 
-	/*
-	 * An atomic write bio must cover the complete length.  If it doesn't,
-	 * error out.
-	 */
-	if ((op & REQ_ATOMIC) && WARN_ON_ONCE(ret != iomap_length(iter))) {
-		ret = -EINVAL;
-		goto out_bio_release_pages;
-	}
-
 	if (dio->flags & IOMAP_DIO_WRITE)
 		task_io_account_write(ret);
 	else if ((dio->flags & IOMAP_DIO_USER_BACKED) &&
@@ -393,11 +384,6 @@ static ssize_t iomap_dio_bio_iter_one(struct iomap_iter *iter,
 	iomap_dio_submit_bio(iter, dio, bio, pos);
 	return ret;
 
-out_bio_release_pages:
-	if (dio->flags & IOMAP_DIO_BOUNCE)
-		bio_free_folios(bio);
-	else
-		bio_release_pages(bio, false);
 out_put_bio:
 	bio_put(bio);
 	return ret;

-- 
2.39.5

Re: [PATCH 2/2] iomap: remove the atomic write length check in iomap_dio_bio_iter_one()

[John Garry]

On 11/8/26 04:24, Tal Zussman wrote:
> bio_iov_iter_get_pages() and bio_iov_iter_bounce_write() now fail a
> REQ_ATOMIC bio that doesn't cover the whole iterator, so the bio here
> always covers iomap_length() and the check can't trigger. Remove the
> check.
> 
> Suggested-by: John Garry <john.garry@linux.dev>
> Link: https://lore.kernel.org/all/82978d5c-3e08-48e9-bf0a-7913e1cd0507@linux.dev/
> Signed-off-by: Tal Zussman <tz2294@columbia.edu>

Reviewed-by: John Garry <john.garry@linux.dev>

This looks ok. But I am just thinking that a CoW-based atomic write does 
not have the single bio restriction, so we don't need to fail the atomic 
in this case. However, we should have consistent rules for iovec's 
whether it's a block device or regular file for ext4 or xfs, so this 
makes sense.

thanks

BTW, you could be a real hero and add an xfstests case for this also.

> ---
>   fs/iomap/direct-io.c | 14 --------------
>   1 file changed, 14 deletions(-)
> 
> diff --git a/fs/iomap/direct-io.c b/fs/iomap/direct-io.c
> index ad041ac3252a..aca7cbaea4af 100644
> --- a/fs/iomap/direct-io.c
> +++ b/fs/iomap/direct-io.c
> @@ -370,15 +370,6 @@ static ssize_t iomap_dio_bio_iter_one(struct iomap_iter *iter,
>   		goto out_put_bio;
>   	ret = bio->bi_iter.bi_size;
>   
> -	/*
> -	 * An atomic write bio must cover the complete length.  If it doesn't,
> -	 * error out.
> -	 */
> -	if ((op & REQ_ATOMIC) && WARN_ON_ONCE(ret != iomap_length(iter))) {
> -		ret = -EINVAL;
> -		goto out_bio_release_pages;
> -	}
> -
>   	if (dio->flags & IOMAP_DIO_WRITE)
>   		task_io_account_write(ret);
>   	else if ((dio->flags & IOMAP_DIO_USER_BACKED) &&
> @@ -393,11 +384,6 @@ static ssize_t iomap_dio_bio_iter_one(struct iomap_iter *iter,
>   	iomap_dio_submit_bio(iter, dio, bio, pos);
>   	return ret;
>   
> -out_bio_release_pages:
> -	if (dio->flags & IOMAP_DIO_BOUNCE)
> -		bio_free_folios(bio);
> -	else
> -		bio_release_pages(bio, false);
>   out_put_bio:
>   	bio_put(bio);
>   	return ret;
>