Re: [PATCH] btrfs: fix error propagation of split bios

[David Sterba <dsterba@suse.cz>]

On Thu, Oct 10, 2024 at 05:51:50AM +0000, Naohiro Aota wrote:
> > > -		if (bbio->bio.bi_status)
> > > -			btrfs_bbio_propagate_error(bbio, orig_bbio);
> > > +		/* Save the last error. */
> > > +		if (bbio->bio.bi_status != BLK_STS_OK)
> > > +			atomic_set(&orig_bbio->status, bbio->bio.bi_status);
> > >  		btrfs_cleanup_bio(bbio);
> > >  		bbio = orig_bbio;
> > >  	}
> > >  
> > > -	if (atomic_dec_and_test(&bbio->pending_ios))
> > > +	if (atomic_dec_and_test(&bbio->pending_ios)) {
> > > +		/* Load split bio's error which might be set above. */
> > > +		if (status == BLK_STS_OK)
> > > +			bbio->bio.bi_status = atomic_read(&bbio->status);
> > >  		__btrfs_bio_end_io(bbio);
> > > +	}
> > >  }
> > >  
> > >  static int next_repair_mirror(struct btrfs_failed_bio *fbio, int cur_mirror)
> > > diff --git a/fs/btrfs/bio.h b/fs/btrfs/bio.h
> > > index e48612340745..b8f7f6071bc2 100644
> > > --- a/fs/btrfs/bio.h
> > > +++ b/fs/btrfs/bio.h
> > > @@ -79,6 +79,9 @@ struct btrfs_bio {
> > >  	/* File system that this I/O operates on. */
> > >  	struct btrfs_fs_info *fs_info;
> > >  
> > > +	/* Set the error status of split bio. */
> > > +	atomic_t status;
> > 
> > To repeat my comments from slack here. This should not be atomic when
> > it's using only set and read, a plain int or blk_sts_t.
> 
> Yes, I'll change this to blk_status_t for better understanding.
> 
> > 
> > The logic of storing the last error in btrfs_bio makes sense, I don't
> > see other ways to do it. If there are multiple errors we can store the
> > first one or the last one, we'd always lose some information. When it's
> > the first one it could be set by cmpxchg.
> 
> Sure. I'll use cmpxchg to save the first one.

The type blk_status_t is a u8, this works with cmpxchg, at least on
x86_64 and the common architectures.