Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCH] block: fix a crash in do_task_dead()
To:     Hugh Dickins <hughd@google.com>, Oleg Nesterov <oleg@redhat.com>
Cc:     Peter Zijlstra <peterz@infradead.org>, Qian Cai <cai@lca.pw>,
        akpm@linux-foundation.org, hch@lst.de, gkohli@codeaurora.org,
        mingo@redhat.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org
References: <1559161526-618-1-git-send-email-cai@lca.pw>
 <20190530080358.GG2623@hirez.programming.kicks-ass.net>
 <82e88482-1b53-9423-baad-484312957e48@kernel.dk>
 <20190603123705.GB3419@hirez.programming.kicks-ass.net>
 <ddf9ee34-cd97-a62b-6e91-6b4511586339@kernel.dk>
 <alpine.LSU.2.11.1906301542410.1105@eggly.anvils>
From:   Jens Axboe <axboe@kernel.dk>
Message-ID: <97d2f5cc-fe98-f28e-86ce-6fbdeb8b67bd@kernel.dk>
Date:   Mon, 1 Jul 2019 08:22:32 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101
 Thunderbird/60.7.1
MIME-Version: 1.0
In-Reply-To: <alpine.LSU.2.11.1906301542410.1105@eggly.anvils>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 6/30/19 5:06 PM, Hugh Dickins wrote:
> On Wed, 5 Jun 2019, Jens Axboe wrote:
>>
>> How about the following plan - if folks are happy with this sched patch,
>> we can queue it up for 5.3. Once that is in, I'll kill the block change
>> that special cases the polled task wakeup. For 5.2, we go with Oleg's
>> patch for the swap case.
> 
> I just hit the do_task_dead() kernel BUG at kernel/sched/core.c:3463!
> while heavy swapping on 5.2-rc7: it looks like Oleg's patch intended
> for 5.2 was not signed off, and got forgotten.
> 
> I did hit the do_task_dead() BUG (but not at all easily) on early -rcs
> before seeing Oleg's patch, then folded it in and and didn't hit the BUG
> again; then just tried again without it, and luckily hit in a few hours.
> 
> So I can give it an enthusiastic
> Acked-by: Hugh Dickins <hughd@google.com>
> because it makes good sense to avoid the get/blk_wake/put overhead on
> the asynch path anyway, even if it didn't work around a bug; but only
> Half-Tested-by: Hugh Dickins <hughd@google.com>
> since I have not been exercising the synchronous path at all.

I'll take the blame for that, went away on vacation for 3 weeks...
But yes, for 5.2, the patch from Oleg looks fine. Once Peter's other
change is in mainline, I'll go through and remove these special cases.

Andrew, can you queue Oleg's patch for 5.2? You can also add my:

Reviewed-by: Jens Axboe <axboe@kernel.dk>

to it.

> 
> Hugh, requoting Oleg:
> 
>>
>> I don't understand this code at all but I am just curious, can we do
>> something like incomplete patch below ?
>>
>> Oleg.
>>
>> --- x/mm/page_io.c
>> +++ x/mm/page_io.c
>> @@ -140,8 +140,10 @@ int swap_readpage(struct page *page, bool synchronous)
>>   	unlock_page(page);
>>   	WRITE_ONCE(bio->bi_private, NULL);
>>   	bio_put(bio);
>> -	blk_wake_io_task(waiter);
>> -	put_task_struct(waiter);
>> +	if (waiter) {
>> +		blk_wake_io_task(waiter);
>> +		put_task_struct(waiter);
>> +	}
>>   }
>>   
>>   int generic_swapfile_activate(struct swap_info_struct *sis,
>> @@ -398,11 +400,12 @@ int swap_readpage(struct page *page, boo
>>   	 * Keep this task valid during swap readpage because the oom killer may
>>   	 * attempt to access it in the page fault retry time check.
>>   	 */
>> -	get_task_struct(current);
>> -	bio->bi_private = current;
>>   	bio_set_op_attrs(bio, REQ_OP_READ, 0);
>> -	if (synchronous)
>> +	if (synchronous) {
>>   		bio->bi_opf |= REQ_HIPRI;
>> +		get_task_struct(current);
>> +		bio->bi_private = current;
>> +	}
>>   	count_vm_event(PSWPIN);
>>   	bio_get(bio);
>>   	qc = submit_bio(bio);


-- 
Jens Axboe