In a few places, target->bi_status is set to source->bi_status only if
source->bi_status is not 0 and target->bi_status is (still) 0.  Here,
checking the value of target->bi_status before setting it is an
unnecessary micro optimization because we are already on an error path.

In addition, we can be racing with other execution contexts in the case
of chained bios, so there is no guarantee that target->bi_status won't
be set concurrently.  We don't require atomic test-and-set semantics
here.

Created with Coccinelle using the following semantic patch:

@@
struct bio *source;
struct bio *target;
@@
- if (source->bi_status && !target->bi_status)
-       target->bi_status = source->bi_status;
+ bio_set_status(target, source->bi_status);

@@
struct bio *source;
struct bio target;
@@
- if (source->bi_status && !target.bi_status)
-       target.bi_status = source->bi_status;
+ bio_set_status(&target, source->bi_status);

Signed-off-by: Andreas Gruenbacher <agruenba@redhat.com>
---
 block/bio.c     | 3 +--
 block/fops.c    | 3 +--
 drivers/md/md.c | 6 ++----
 3 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/block/bio.c b/block/bio.c
index 3f408e1ba13f..5389321872f0 100644
--- a/block/bio.c
+++ b/block/bio.c
@@ -314,8 +314,7 @@ static struct bio *__bio_chain_endio(struct bio *bio)
 {
 	struct bio *parent = bio->bi_private;
 
-	if (bio->bi_status && !parent->bi_status)
-		parent->bi_status = bio->bi_status;
+	bio_set_status(parent, bio->bi_status);
 	bio_put(bio);
 	return parent;
 }
diff --git a/block/fops.c b/block/fops.c
index b4f911273289..a4a6972cbfbf 100644
--- a/block/fops.c
+++ b/block/fops.c
@@ -135,8 +135,7 @@ static void blkdev_bio_end_io(struct bio *bio)
 	bool should_dirty = dio->flags & DIO_SHOULD_DIRTY;
 	bool is_sync = dio->flags & DIO_IS_SYNC;
 
-	if (bio->bi_status && !dio->bio.bi_status)
-		dio->bio.bi_status = bio->bi_status;
+	bio_set_status(&dio->bio, bio->bi_status);
 
 	if (bio_integrity(bio))
 		bio_integrity_unmap_user(bio);
diff --git a/drivers/md/md.c b/drivers/md/md.c
index 41c476b40c7a..f6f1aab18a8b 100644
--- a/drivers/md/md.c
+++ b/drivers/md/md.c
@@ -9084,8 +9084,7 @@ static void md_end_clone_io(struct bio *bio)
 	if (bio_data_dir(orig_bio) == WRITE && md_bitmap_enabled(mddev, false))
 		md_bitmap_end(mddev, md_io_clone);
 
-	if (bio->bi_status && !orig_bio->bi_status)
-		orig_bio->bi_status = bio->bi_status;
+	bio_set_status(orig_bio, bio->bi_status);
 
 	if (md_io_clone->start_time)
 		bio_end_io_acct(orig_bio, md_io_clone->start_time);
@@ -9136,8 +9135,7 @@ void md_free_cloned_bio(struct bio *bio)
 	if (bio_data_dir(orig_bio) == WRITE && md_bitmap_enabled(mddev, false))
 		md_bitmap_end(mddev, md_io_clone);
 
-	if (bio->bi_status && !orig_bio->bi_status)
-		orig_bio->bi_status = bio->bi_status;
+	bio_set_status(orig_bio, bio->bi_status);
 
 	if (md_io_clone->start_time)
 		bio_end_io_acct(orig_bio, md_io_clone->start_time);
-- 
2.51.0

On Mon, Dec 08, 2025 at 12:10:13PM +0000, Andreas Gruenbacher wrote:
> In a few places, target->bi_status is set to source->bi_status only if
> source->bi_status is not 0 and target->bi_status is (still) 0.  Here,
> checking the value of target->bi_status before setting it is an
> unnecessary micro optimization because we are already on an error path.

What is source and target here?  I have a hard time trying to follow
what this is trying to do.

On Tue, Dec 16, 2025 at 8:59 AM Christoph Hellwig <hch@infradead.org> wrote:
> On Mon, Dec 08, 2025 at 12:10:13PM +0000, Andreas Gruenbacher wrote:
> > In a few places, target->bi_status is set to source->bi_status only if
> > source->bi_status is not 0 and target->bi_status is (still) 0.  Here,
> > checking the value of target->bi_status before setting it is an
> > unnecessary micro optimization because we are already on an error path.
>
> What is source and target here?  I have a hard time trying to follow
> what this is trying to do.

Not sure, what would you suggest instead?

Thanks,
Andreas

On Tue, Dec 16, 2025 at 09:41:49AM +0100, Andreas Gruenbacher wrote:
> On Tue, Dec 16, 2025 at 8:59 AM Christoph Hellwig <hch@infradead.org> wrote:
> > On Mon, Dec 08, 2025 at 12:10:13PM +0000, Andreas Gruenbacher wrote:
> > > In a few places, target->bi_status is set to source->bi_status only if
> > > source->bi_status is not 0 and target->bi_status is (still) 0.  Here,
> > > checking the value of target->bi_status before setting it is an
> > > unnecessary micro optimization because we are already on an error path.
> >
> > What is source and target here?  I have a hard time trying to follow
> > what this is trying to do.
> 
> Not sure, what would you suggest instead?

I still don't understand what you're saying here at all, or what this is
trying to fix or optimize.

On Tue, Dec 16, 2025 at 11:44 AM Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Dec 16, 2025 at 09:41:49AM +0100, Andreas Gruenbacher wrote:
> > On Tue, Dec 16, 2025 at 8:59 AM Christoph Hellwig <hch@infradead.org> wrote:
> > > On Mon, Dec 08, 2025 at 12:10:13PM +0000, Andreas Gruenbacher wrote:
> > > > In a few places, target->bi_status is set to source->bi_status only if
> > > > source->bi_status is not 0 and target->bi_status is (still) 0.  Here,
> > > > checking the value of target->bi_status before setting it is an
> > > > unnecessary micro optimization because we are already on an error path.
> > >
> > > What is source and target here?  I have a hard time trying to follow
> > > what this is trying to do.
> >
> > Not sure, what would you suggest instead?
>
> I still don't understand what you're saying here at all, or what this is
> trying to fix or optimize.

When we have this construct in the code and we know that status is not 0:

  if (!bio->bi_status)
    bio->bi_status = status;

we can just do this instead:

  bio>bi_status = status;

Andreas

On Tue, Dec 16, 2025 at 12:20:07PM +0100, Andreas Gruenbacher wrote:
> > I still don't understand what you're saying here at all, or what this is
> > trying to fix or optimize.
> 
> When we have this construct in the code and we know that status is not 0:
> 
>   if (!bio->bi_status)
>     bio->bi_status = status;
> 
> we can just do this instead:
> 
>   bio>bi_status = status;

But this now overrides the previous status instead of preserving the
first error?

On Thu, Dec 18, 2025 at 10:08 AM Christoph Hellwig <hch@infradead.org> wrote:
> On Tue, Dec 16, 2025 at 12:20:07PM +0100, Andreas Gruenbacher wrote:
> > > I still don't understand what you're saying here at all, or what this is
> > > trying to fix or optimize.
> >
> > When we have this construct in the code and we know that status is not 0:
> >
> >   if (!bio->bi_status)
> >     bio->bi_status = status;
> >
> > we can just do this instead:
> >
> >   bio>bi_status = status;
>
> But this now overrides the previous status instead of preserving the
> first error?

This is exactly my point: we already don't preserve the first error,
it only looks like we do. Here are the possible cases:

(1) A single bio A: there are no competing completions. A->bi_status
is set before calling bio_endio(), and it can be set to any value
including BLK_STS_OK (0) with a simple assignment.

(2) An A -> B chain: there are two competing completions, and
B->bi_status is the resulting status of the bio chain. Both
completions will immediately update B->bi_status. When B->bi_status is
updated, it must not be set to BLK_STS_OK (0) or else a previous
non-zero status code could be wiped out. But for such a non-zero
status code, a construct like 'if (B->status != BLK_STS_OK) B->status
= status' is no better than a simple 'B->status = status' because the
if is not atomic. But we only care about preserving an error code, not
the first error code that occurred, so that's fine.

(3) An A -> B -> C chain: There are three competing completions, and
C->bi_status is the resulting status of the bio chain. Not all
completions will immediately update C->bi_status, but if at least one
of the completions fails, we know that we will always end up with a
non-zero error code in C->bi_status eventually.

And again, switching to cmpxchg() would not always preserve the first
error, either: for example, in case (3), if the bios complete in the
order A, C, B and they all fail, C->bi_status would end up with the
error code of C instead of A even though A completed before C.

This could be fixed by chaining all consecutive bios to bio A (the
first one) and reversing the pointer direction so that all cmpxchg()
operations will target A->bi_status. But again, I don't think
preserving the first error is actually needed. And it certainly isn't
being done today.

Thanks,
Andreas