| Message ID | 20200728112541.3401-1-johannes.thumshirn@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | btrfs: handle errors from async submission | expand |
On 7/28/20 7:25 AM, Johannes Thumshirn wrote: > Btrfs' async submit mechanism is able to handle errors in the submission > path and the meta-data async submit function correctly passes the error > code to the caller. > > In btrfs_submit_bio_start() and btrfs_submit_bio_start_direct_io() we're > not handling the errors returned by btrfs_csum_one_bio() correctly though > and simply call BUG_ON(). This is unnecessary as the caller of these two > functions - run_one_async_start - correctly checks for the return values > and sets the status of the async_submit_bio. The actual bio submission > will be handled later on by run_one_async_done only if > async_submit_bio::status is 0, so the data won't be written if we > encountered an error in the checksum process. > > Simply return the error from btrfs_csum_one_bio() to the async submitters, > like it's done in btree_submit_bio_start(). > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Reviewed-by: Josef Bacik <josef@toxicpanda.com> Thanks, Josef
On Tue, Jul 28, 2020 at 08:25:41PM +0900, Johannes Thumshirn wrote: > Btrfs' async submit mechanism is able to handle errors in the submission > path and the meta-data async submit function correctly passes the error > code to the caller. > > In btrfs_submit_bio_start() and btrfs_submit_bio_start_direct_io() we're > not handling the errors returned by btrfs_csum_one_bio() correctly though > and simply call BUG_ON(). This is unnecessary as the caller of these two > functions - run_one_async_start - correctly checks for the return values > and sets the status of the async_submit_bio. The actual bio submission > will be handled later on by run_one_async_done only if > async_submit_bio::status is 0, so the data won't be written if we > encountered an error in the checksum process. > > Simply return the error from btrfs_csum_one_bio() to the async submitters, > like it's done in btree_submit_bio_start(). > > Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> Added to misc-next, thanks. > @@ -2154,11 +2154,8 @@ static blk_status_t btrfs_submit_bio_start(void *private_data, struct bio *bio, > u64 bio_offset) > { > struct inode *inode = private_data; > - blk_status_t ret = 0; > > - ret = btrfs_csum_one_bio(BTRFS_I(inode), bio, 0, 0); > - BUG_ON(ret); /* -ENOMEM */ > - return 0; > + return btrfs_csum_one_bio(BTRFS_I(inode), bio, 0, 0); The submit bio hooks have become a trivial indirect call to two functions, so we might get rid of the indirection eventually.
On 30/07/2020 18:47, David Sterba wrote: [...] > The submit bio hooks have become a trivial indirect call to two > functions, so we might get rid of the indirection eventually. Yes that was my thought as well, but then I got distracted by more urgent things again. I'll add it to my backlog
On Fri, Jul 31, 2020 at 07:39:06AM +0000, Johannes Thumshirn wrote: > On 30/07/2020 18:47, David Sterba wrote: > [...] > > The submit bio hooks have become a trivial indirect call to two > > functions, so we might get rid of the indirection eventually. > > Yes that was my thought as well, but then I got distracted by more urgent > things again. Sure, that's just a drive-by comment. I had a quick look at the io submit call chains for some easy cleanups but haven't spotted anything. So this will need some deeper reorganization, we can live with the indirection until then.
diff --git a/fs/btrfs/inode.c b/fs/btrfs/inode.c index 611b3412fbfd..edb468e8db3d 100644 --- a/fs/btrfs/inode.c +++ b/fs/btrfs/inode.c @@ -2154,11 +2154,8 @@ static blk_status_t btrfs_submit_bio_start(void *private_data, struct bio *bio, u64 bio_offset) { struct inode *inode = private_data; - blk_status_t ret = 0; - ret = btrfs_csum_one_bio(BTRFS_I(inode), bio, 0, 0); - BUG_ON(ret); /* -ENOMEM */ - return 0; + return btrfs_csum_one_bio(BTRFS_I(inode), bio, 0, 0); } /* @@ -7612,10 +7609,8 @@ static blk_status_t btrfs_submit_bio_start_direct_io(void *private_data, struct bio *bio, u64 offset) { struct inode *inode = private_data; - blk_status_t ret; - ret = btrfs_csum_one_bio(BTRFS_I(inode), bio, offset, 1); - BUG_ON(ret); /* -ENOMEM */ - return 0; + + return btrfs_csum_one_bio(BTRFS_I(inode), bio, offset, 1); } static void btrfs_end_dio_bio(struct bio *bio)
Btrfs' async submit mechanism is able to handle errors in the submission path and the meta-data async submit function correctly passes the error code to the caller. In btrfs_submit_bio_start() and btrfs_submit_bio_start_direct_io() we're not handling the errors returned by btrfs_csum_one_bio() correctly though and simply call BUG_ON(). This is unnecessary as the caller of these two functions - run_one_async_start - correctly checks for the return values and sets the status of the async_submit_bio. The actual bio submission will be handled later on by run_one_async_done only if async_submit_bio::status is 0, so the data won't be written if we encountered an error in the checksum process. Simply return the error from btrfs_csum_one_bio() to the async submitters, like it's done in btree_submit_bio_start(). Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com> --- fs/btrfs/inode.c | 11 +++-------- 1 file changed, 3 insertions(+), 8 deletions(-)