From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933835AbbBCLdz (ORCPT ); Tue, 3 Feb 2015 06:33:55 -0500 Received: from bombadil.infradead.org ([198.137.202.9]:54982 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932250AbbBCLdx (ORCPT ); Tue, 3 Feb 2015 06:33:53 -0500 Date: Tue, 3 Feb 2015 12:33:48 +0100 From: Peter Zijlstra To: Linus Torvalds Cc: Benjamin LaHaise , linux-aio@kvack.org, Linux Kernel Subject: Re: [GIT PULL] aio: fix sleeping while TASK_INTERRUPTIBLE Message-ID: <20150203113348.GH24151@twins.programming.kicks-ass.net> References: <20150201144058.GM2974@kvack.org> <20150201221458.GN2974@kvack.org> <20150202001628.GO2974@kvack.org> <20150203112733.GM26304@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150203112733.GM26304@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.21 (2012-12-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Feb 03, 2015 at 12:27:33PM +0100, Peter Zijlstra wrote: > On Sun, Feb 01, 2015 at 05:18:17PM -0800, Linus Torvalds wrote: > > Ahh. That would be a bug, yes, but it wouldn't be one in the aio code. > > > > If somebody just does a "schedule()" and thinks that their own private > > events are the only thing that can wake it up, and doesn't use one of > > the millions of "wait_event_xyz()" variations to actually wait for the > > real completion, that is just buggy. Always. Always has been. > > > > So I wouldn't worry too much about it. It has never been correct to do > > that, and it's not one of the standard patterns for sleeping anyway. > > Which is not to say that it might not exist in some dank corner of the > > kernel, of course, but you shouldn't write code trying to make buggy > > code like that happy. If we ever find code like that, let's just fix > > it where the bug exists, not try to write odd code in places where it > > isn't. > > > > And I'd actually be a bit surprised to see that kind of really broken > > code. You really almost have to work at it. All our normal "sleep > > until X" patterns are much more obvious, and it's just _simpler_ to do > > the right thing with "wait_event()" than to mis-code an explicit "set > > task state and then just schedule without actually checking the thing > > you are waiting for". > > block/bsg.c- prepare_to_wait(&bd->wq_done, &wait, TASK_UNINTERRUPTIBLE); > block/bsg.c- spin_unlock_irq(&bd->lock); > block/bsg.c: io_schedule(); > block/bsg.c- finish_wait(&bd->wq_done, &wait); > > Which is double buggy because: > 1) it doesn't loop > 2) it sets TASK_UNINTERRUPTIBLE _after_ testing for the sleep event. OK, actually had a look at this one; it might be ok. The spinlock might fully serialize the state so no fails, and the entire function is called in a loop. Still seriously obtuse code.