Date: Tue, 3 Feb 2015 12:33:48 +0100
From: Peter Zijlstra <peterz@infradead.org>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Benjamin LaHaise <bcrl@kvack.org>, linux-aio@kvack.org,
        Linux Kernel <linux-kernel@vger.kernel.org>
Subject: Re: [GIT PULL] aio: fix sleeping while TASK_INTERRUPTIBLE
Message-ID: <20150203113348.GH24151@twins.programming.kicks-ass.net>
References: <20150201144058.GM2974@kvack.org>
 <CA+55aFx+HVnythC+BpmDMa0Oeq1MxnXXOvs771DXpJ=PBjy1Nw@mail.gmail.com>
 <20150201221458.GN2974@kvack.org>
 <CA+55aFzAPms_orX7TY6zGCg=G-yXCrr5z7LMme7xcrXCAk6JAQ@mail.gmail.com>
 <20150202001628.GO2974@kvack.org>
 <CA+55aFy2URGTL_XKQhEO_4dTc3KNJDm9Z0O_1Gqs4G3TvW7APg@mail.gmail.com>
 <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
Content-Length: 2131
Lines: 42

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/