[PATCH] btrfs: handle errors from async submission

[PATCH] btrfs: handle errors from async submission

[Johannes Thumshirn]

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(-)

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)
-- 
2.26.2

Re: [PATCH] btrfs: handle errors from async submission

[Josef Bacik]

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

Re: [PATCH] btrfs: handle errors from async submission

[David Sterba]

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.

Re: [PATCH] btrfs: handle errors from async submission

[Johannes Thumshirn]

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

Re: [PATCH] btrfs: handle errors from async submission

[David Sterba]

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.