[PATCH 0/2] btrfs: fix the generic/475 crash for subpage case

[PATCH 0/2] btrfs: fix the generic/475 crash for subpage case

[Qu Wenruo]

Test case generic/475 expose random crash (1/20~1/5) trigged by the
BUG_ON() inside btrfs_csum_one_bio().

The direct cause is we're trying to submit a write bio, even after we hit
some error and already have ordered extent cleaned up.

The first patch will try to fix all those call sites, then the 2nd patch
will add an extra safe net to prevent such case to be escalated into a
crash.

Qu Wenruo (2):
  btrfs: don't try to flush data write bio if we hit error preparing it
  btrfs: replace BUG_ON() in btrfs_csum_one_bio() with proper error
    handling

 fs/btrfs/extent_io.c | 17 ++++++++++++-----
 fs/btrfs/file-item.c | 14 +++++++++++++-
 2 files changed, 25 insertions(+), 6 deletions(-)

-- 
2.32.0

[PATCH 1/2] btrfs: don't try to flush data write bio if we hit error preparing it

[Qu Wenruo]

[BUG]
When running with 64K page size and 4K sectorsize (aka, subpage), there
is a chance that generic/475 would crash with the following BUG_ON() in
btrfs_csum_one_bio() triggered:

		if (!ordered) {
			ordered = btrfs_lookup_ordered_extent(inode, offset);
			BUG_ON(!ordered); /* Logic error */
		}

[CAUSE]
Test case generic/475 is doing dm-error testing, while for subpage case
we could hit error caused by dm-errors.

In that case, we will proper call end_extent_writepage() with @err = 1
to do the cleanup, including finishing the ordered extent.

In that case, the assembled bio still needs to be finished, by
end_write_bio() function.

But there are call sites that doesn't properly call end_write_bio(), but
go flush_write_bio() to submit the assembled bio.

In that case, we will call btrfs_csum_one_bio() even the ordered extent
is already cleaned up, and trigger the BUG_ON().

[FIX]
There are two call sites where we still try to call flush_write_bio()
even if we hit error:

- extent_write_cache_pages()
- extent_write_locked_range()

Fix both of them by calling end_write_bio() if we hit error.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/extent_io.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/fs/btrfs/extent_io.c b/fs/btrfs/extent_io.c
index e82328bcb281..49f7dbbb2d73 100644
--- a/fs/btrfs/extent_io.c
+++ b/fs/btrfs/extent_io.c
@@ -5035,9 +5035,13 @@ static int extent_write_cache_pages(struct address_space *mapping,
 		 * page in our current bio, and thus deadlock, so flush the
 		 * write bio here.
 		 */
-		ret = flush_write_bio(epd);
-		if (!ret)
-			goto retry;
+		if (ret < 0) {
+			end_write_bio(epd, ret);
+		} else {
+			ret = flush_write_bio(epd);
+			if (!ret)
+				goto retry;
+		}
 	}
 
 	if (wbc->range_cyclic || (wbc->nr_to_write > 0 && range_whole))
@@ -5095,9 +5099,11 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end,
 	wbc_attach_fdatawrite_inode(&wbc_writepages, inode);
 	while (start <= end) {
 		page = find_get_page(mapping, start >> PAGE_SHIFT);
-		if (clear_page_dirty_for_io(page))
+		if (clear_page_dirty_for_io(page)) {
 			ret = __extent_writepage(page, &wbc_writepages, &epd);
-		else {
+			if (ret < 0)
+				goto out;
+		} else {
 			btrfs_writepage_endio_finish_ordered(BTRFS_I(inode),
 					page, start, start + PAGE_SIZE - 1, true);
 			unlock_page(page);
@@ -5106,6 +5112,7 @@ int extent_write_locked_range(struct inode *inode, u64 start, u64 end,
 		start += PAGE_SIZE;
 	}
 
+out:
 	ASSERT(ret <= 0);
 	if (ret == 0)
 		ret = flush_write_bio(&epd);
-- 
2.32.0

[PATCH 2/2] btrfs: replace BUG_ON() in btrfs_csum_one_bio() with proper error handling

[Qu Wenruo]

The BUG_ON() in btrfs_csum_one_bio() means we're trying to submit a bio
while we don't have ordered extent for it at all.

Normally this won't happen and is indeed a code logical error.

But previous fix has already shown another possibility that, some call
sites don't handle error properly and submit the write bio after its
ordered extent has already been cleaned up.

This patch will add an extra safe net by replacing the BUG_ON() to
proper error handling.

And even if some day we hit a regression that we're submitting bio
without an ordered extent, we will return error and the pages will be
marked Error, and being caught properly.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/file-item.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
index 2673c6ba7a4e..25205b9dad69 100644
--- a/fs/btrfs/file-item.c
+++ b/fs/btrfs/file-item.c
@@ -665,7 +665,19 @@ blk_status_t btrfs_csum_one_bio(struct btrfs_inode *inode, struct bio *bio,
 
 		if (!ordered) {
 			ordered = btrfs_lookup_ordered_extent(inode, offset);
-			BUG_ON(!ordered); /* Logic error */
+			/*
+			 * No ordered extent mostly means the OE has been
+			 * removed (mostly for error handling). Normally for
+			 * such case we should not flush_write_bio(), but
+			 * end_write_bio().
+			 *
+			 * But an extra safe net will never hurt. Just error
+			 * out.
+			 */
+			if (unlikely(!ordered)) {
+				kvfree(sums);
+				return BLK_STS_IOERR;
+			}
 		}
 
 		nr_sectors = BTRFS_BYTES_TO_BLKS(fs_info,
-- 
2.32.0

Re: [PATCH 2/2] btrfs: replace BUG_ON() in btrfs_csum_one_bio() with proper error handling

[Nikolay Borisov]

On 2.08.21 г. 9:54, Qu Wenruo wrote:
> The BUG_ON() in btrfs_csum_one_bio() means we're trying to submit a bio
> while we don't have ordered extent for it at all.
> 
> Normally this won't happen and is indeed a code logical error.
> 
> But previous fix has already shown another possibility that, some call
> sites don't handle error properly and submit the write bio after its
> ordered extent has already been cleaned up.
> 
> This patch will add an extra safe net by replacing the BUG_ON() to
> proper error handling.
> 
> And even if some day we hit a regression that we're submitting bio
> without an ordered extent, we will return error and the pages will be
> marked Error, and being caught properly.

Would this hamper debugability? I.e it will result in some writes
failing with an error, right?

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/file-item.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
> index 2673c6ba7a4e..25205b9dad69 100644
> --- a/fs/btrfs/file-item.c
> +++ b/fs/btrfs/file-item.c
> @@ -665,7 +665,19 @@ blk_status_t btrfs_csum_one_bio(struct btrfs_inode *inode, struct bio *bio,
>  
>  		if (!ordered) {
>  			ordered = btrfs_lookup_ordered_extent(inode, offset);
> -			BUG_ON(!ordered); /* Logic error */
> +			/*
> +			 * No ordered extent mostly means the OE has been
> +			 * removed (mostly for error handling). Normally for
> +			 * such case we should not flush_write_bio(), but
> +			 * end_write_bio().
> +			 *
> +			 * But an extra safe net will never hurt. Just error
> +			 * out.
> +			 */
> +			if (unlikely(!ordered)) {
> +				kvfree(sums);
> +				return BLK_STS_IOERR;
> +			}
>  		}
>  
>  		nr_sectors = BTRFS_BYTES_TO_BLKS(fs_info,
> 

Re: [PATCH 2/2] btrfs: replace BUG_ON() in btrfs_csum_one_bio() with proper error handling

[Qu Wenruo]

On 2021/8/2 下午3:53, Nikolay Borisov wrote:
>
>
> On 2.08.21 г. 9:54, Qu Wenruo wrote:
>> The BUG_ON() in btrfs_csum_one_bio() means we're trying to submit a bio
>> while we don't have ordered extent for it at all.
>>
>> Normally this won't happen and is indeed a code logical error.
>>
>> But previous fix has already shown another possibility that, some call
>> sites don't handle error properly and submit the write bio after its
>> ordered extent has already been cleaned up.
>>
>> This patch will add an extra safe net by replacing the BUG_ON() to
>> proper error handling.
>>
>> And even if some day we hit a regression that we're submitting bio
>> without an ordered extent, we will return error and the pages will be
>> marked Error, and being caught properly.
>
> Would this hamper debugability? I.e it will result in some writes
> failing with an error, right?

Yes, it will make such corner case way more silent than before.

But IMHO the existing BUG_ON() is also overkilled.

Maybe converting it to WARN_ON() would be a good middle land?

Thanks,
Qu

>
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>> ---
>>   fs/btrfs/file-item.c | 14 +++++++++++++-
>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
>> index 2673c6ba7a4e..25205b9dad69 100644
>> --- a/fs/btrfs/file-item.c
>> +++ b/fs/btrfs/file-item.c
>> @@ -665,7 +665,19 @@ blk_status_t btrfs_csum_one_bio(struct btrfs_inode *inode, struct bio *bio,
>>
>>   		if (!ordered) {
>>   			ordered = btrfs_lookup_ordered_extent(inode, offset);
>> -			BUG_ON(!ordered); /* Logic error */
>> +			/*
>> +			 * No ordered extent mostly means the OE has been
>> +			 * removed (mostly for error handling). Normally for
>> +			 * such case we should not flush_write_bio(), but
>> +			 * end_write_bio().
>> +			 *
>> +			 * But an extra safe net will never hurt. Just error
>> +			 * out.
>> +			 */
>> +			if (unlikely(!ordered)) {
>> +				kvfree(sums);
>> +				return BLK_STS_IOERR;
>> +			}
>>   		}
>>
>>   		nr_sectors = BTRFS_BYTES_TO_BLKS(fs_info,
>>

Re: [PATCH 2/2] btrfs: replace BUG_ON() in btrfs_csum_one_bio() with proper error handling

[Nikolay Borisov]

On 2.08.21 г. 11:03, Qu Wenruo wrote:
> 
> 
> On 2021/8/2 下午3:53, Nikolay Borisov wrote:
>>
>>
>> On 2.08.21 г. 9:54, Qu Wenruo wrote:
>>> The BUG_ON() in btrfs_csum_one_bio() means we're trying to submit a bio
>>> while we don't have ordered extent for it at all.
>>>
>>> Normally this won't happen and is indeed a code logical error.
>>>
>>> But previous fix has already shown another possibility that, some call
>>> sites don't handle error properly and submit the write bio after its
>>> ordered extent has already been cleaned up.
>>>
>>> This patch will add an extra safe net by replacing the BUG_ON() to
>>> proper error handling.
>>>
>>> And even if some day we hit a regression that we're submitting bio
>>> without an ordered extent, we will return error and the pages will be
>>> marked Error, and being caught properly.
>>
>> Would this hamper debugability? I.e it will result in some writes
>> failing with an error, right?
> 
> Yes, it will make such corner case way more silent than before.
> 
> But IMHO the existing BUG_ON() is also overkilled.
> 
> Maybe converting it to WARN_ON() would be a good middle land?

If this can occur only due to code bugs I'd prefer to leave it as a
BUG_ON. Ideally this should only trigger on developer machines when
testing code changes.

> 
> Thanks,
> Qu
> 
>>
>>>
>>> Signed-off-by: Qu Wenruo <wqu@suse.com>
>>> ---
>>>   fs/btrfs/file-item.c | 14 +++++++++++++-
>>>   1 file changed, 13 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/fs/btrfs/file-item.c b/fs/btrfs/file-item.c
>>> index 2673c6ba7a4e..25205b9dad69 100644
>>> --- a/fs/btrfs/file-item.c
>>> +++ b/fs/btrfs/file-item.c
>>> @@ -665,7 +665,19 @@ blk_status_t btrfs_csum_one_bio(struct
>>> btrfs_inode *inode, struct bio *bio,
>>>
>>>           if (!ordered) {
>>>               ordered = btrfs_lookup_ordered_extent(inode, offset);
>>> -            BUG_ON(!ordered); /* Logic error */
>>> +            /*
>>> +             * No ordered extent mostly means the OE has been
>>> +             * removed (mostly for error handling). Normally for
>>> +             * such case we should not flush_write_bio(), but
>>> +             * end_write_bio().
>>> +             *
>>> +             * But an extra safe net will never hurt. Just error
>>> +             * out.
>>> +             */
>>> +            if (unlikely(!ordered)) {
>>> +                kvfree(sums);
>>> +                return BLK_STS_IOERR;
>>> +            }
>>>           }
>>>
>>>           nr_sectors = BTRFS_BYTES_TO_BLKS(fs_info,
>>>
> 

Re: [PATCH 0/2] btrfs: fix the generic/475 crash for subpage case

[Qu Wenruo]

On 2021/8/2 下午2:54, Qu Wenruo wrote:
> Test case generic/475 expose random crash (1/20~1/5) trigged by the
> BUG_ON() inside btrfs_csum_one_bio().
> 
> The direct cause is we're trying to submit a write bio, even after we hit
> some error and already have ordered extent cleaned up.
> 
> The first patch will try to fix all those call sites, then the 2nd patch
> will add an extra safe net to prevent such case to be escalated into a
> crash.

Please discard this patchset.

The first patch is not the direct cause of the generic/475 crash, and 
it's the 2nd patch masking the problem.

The real problem is the end_extent_writepage() call in 
__extent_writepage() when the page is marked Error.

Thanks,
Qu

> 
> Qu Wenruo (2):
>    btrfs: don't try to flush data write bio if we hit error preparing it
>    btrfs: replace BUG_ON() in btrfs_csum_one_bio() with proper error
>      handling
> 
>   fs/btrfs/extent_io.c | 17 ++++++++++++-----
>   fs/btrfs/file-item.c | 14 +++++++++++++-
>   2 files changed, 25 insertions(+), 6 deletions(-)
> 

Re: [PATCH 2/2] btrfs: replace BUG_ON() in btrfs_csum_one_bio() with proper error handling

[David Sterba]

On Mon, Aug 02, 2021 at 11:49:38AM +0300, Nikolay Borisov wrote:
> 
> 
> On 2.08.21 г. 11:03, Qu Wenruo wrote:
> > 
> > 
> > On 2021/8/2 下午3:53, Nikolay Borisov wrote:
> >>
> >>
> >> On 2.08.21 г. 9:54, Qu Wenruo wrote:
> >>> The BUG_ON() in btrfs_csum_one_bio() means we're trying to submit a bio
> >>> while we don't have ordered extent for it at all.
> >>>
> >>> Normally this won't happen and is indeed a code logical error.
> >>>
> >>> But previous fix has already shown another possibility that, some call
> >>> sites don't handle error properly and submit the write bio after its
> >>> ordered extent has already been cleaned up.
> >>>
> >>> This patch will add an extra safe net by replacing the BUG_ON() to
> >>> proper error handling.
> >>>
> >>> And even if some day we hit a regression that we're submitting bio
> >>> without an ordered extent, we will return error and the pages will be
> >>> marked Error, and being caught properly.
> >>
> >> Would this hamper debugability? I.e it will result in some writes
> >> failing with an error, right?
> > 
> > Yes, it will make such corner case way more silent than before.
> > 
> > But IMHO the existing BUG_ON() is also overkilled.
> > 
> > Maybe converting it to WARN_ON() would be a good middle land?
> 
> If this can occur only due to code bugs I'd prefer to leave it as a
> BUG_ON. Ideally this should only trigger on developer machines when
> testing code changes.

I'd rather see a WARN_ON + the error handling code, a BUG_ON will shoot
down the whole machine. In this case it's probably serious enough but
in the long run we want to get rid of BUG_ONs that can be reasonably
handled.

Re: [PATCH 2/2] btrfs: replace BUG_ON() in btrfs_csum_one_bio() with proper error handling

[Qu Wenruo]

On 2021/8/16 下午10:21, David Sterba wrote:
> On Mon, Aug 02, 2021 at 11:49:38AM +0300, Nikolay Borisov wrote:
>>
>>
>> On 2.08.21 г. 11:03, Qu Wenruo wrote:
>>>
>>>
>>> On 2021/8/2 下午3:53, Nikolay Borisov wrote:
>>>>
>>>>
>>>> On 2.08.21 г. 9:54, Qu Wenruo wrote:
>>>>> The BUG_ON() in btrfs_csum_one_bio() means we're trying to submit a bio
>>>>> while we don't have ordered extent for it at all.
>>>>>
>>>>> Normally this won't happen and is indeed a code logical error.
>>>>>
>>>>> But previous fix has already shown another possibility that, some call
>>>>> sites don't handle error properly and submit the write bio after its
>>>>> ordered extent has already been cleaned up.
>>>>>
>>>>> This patch will add an extra safe net by replacing the BUG_ON() to
>>>>> proper error handling.
>>>>>
>>>>> And even if some day we hit a regression that we're submitting bio
>>>>> without an ordered extent, we will return error and the pages will be
>>>>> marked Error, and being caught properly.
>>>>
>>>> Would this hamper debugability? I.e it will result in some writes
>>>> failing with an error, right?
>>>
>>> Yes, it will make such corner case way more silent than before.
>>>
>>> But IMHO the existing BUG_ON() is also overkilled.
>>>
>>> Maybe converting it to WARN_ON() would be a good middle land?
>>
>> If this can occur only due to code bugs I'd prefer to leave it as a
>> BUG_ON. Ideally this should only trigger on developer machines when
>> testing code changes.
> 
> I'd rather see a WARN_ON + the error handling code, a BUG_ON will shoot
> down the whole machine. In this case it's probably serious enough but
> in the long run we want to get rid of BUG_ONs that can be reasonably
> handled.
> 
OK, I'll re-send the patch with WARN_ON() and error handling.

THanks,
Qu