Re: 5.14.9 aarch64 OOPS Workqueue: btrfs-delalloc btrfs_work_helper

[Nikolay Borisov <nborisov@suse.com>]

On 26.10.21 г. 15:51, Chris Murphy wrote:
> On Tue, Oct 26, 2021 at 3:14 AM Nikolay Borisov <nborisov@suse.com> wrote:
>>
>> I think I identified a race that could cause the crash, can you apply the
>> following diff and re-run the tests and leave them for a couple of days.
>> Preferably apply it on 5.4.10 so that there is the highest chance to reproduce:
>>
>> diff --git a/fs/btrfs/async-thread.c b/fs/btrfs/async-thread.c
>> index 309516e6a968..a3d788dcbd34 100644
>> --- a/fs/btrfs/async-thread.c
>> +++ b/fs/btrfs/async-thread.c
>> @@ -234,6 +234,11 @@ static void run_ordered_work(struct __btrfs_workqueue *wq,
>>                                   ordered_list);
>>                 if (!test_bit(WORK_DONE_BIT, &work->flags))
>>                         break;
>> +               /*
>> +                * Orders all subsequent loads after WORK_DONE_BIT, paired with
>> +                * the smp_mb__before_atomic in btrfs_work_helper
>> +                */
>> +               smp_rmb();
>>
>>                 /*
>>                  * we are going to call the ordered done function, but
>> @@ -317,6 +322,12 @@ static void btrfs_work_helper(struct work_struct *normal_work)
>>         thresh_exec_hook(wq);
>>         work->func(work);
>>         if (need_order) {
>> +               /*
>> +                * Ensures all вритес done in ->func are ordered before
>> +                * setting the WORK_DONE_BIT making them visible to ordered
>> +                * func
>> +                */
>> +               smp_mb__before_atomic();
>>                 set_bit(WORK_DONE_BIT, &work->flags);
>>                 run_ordered_work(wq, work);
>>         } else {
>>
> 
> Couple typos: 'вритес' looks like keyboard layout hiccup and should be
> 'writes'; and 5.4.10 should be 5.14.10 (I'm betting all the tea in
> China that upstream isn't asking me to test a patch on a two year old
> kernel).

Correct in both cases :)

> 
> Unfortunately the test we have is non-automated, it's "install this
> package set" and wait. It always hangs, usually recovers without an
> oops, but sometimes there's an oops. So it's pretty tedious to test it
> with the "testcase" we currently have. I'd like a better one that
> triggers this faster, but more importantly would be a reliable one.
> We'll do our best though. Thanks!

I thought the hang and the crash one are two different issues. What the
above diff is supposed to solve is the case in which
submit_compressed_extent is called with async_chunk->inode is null.

For the lockup issue it might or might not be related to this. But it
will be best if a crashdump is provided when the hang has occurred.
Looking at the task call trace in
https://bugzilla-attachments.redhat.com/attachment.cgi?id=1836995
doesn't point at a hang. Just a bunch of threads waiting on IO in the
metadata reclaim path.

>