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?