Re: [PATCH 2/3] block: Introduce blk_rq_completed()

[James Bottomley <jbottomley@parallels.com>]

On Tue, 2014-05-27 at 00:52 -0700, hch@infradead.org wrote:
> On Tue, May 27, 2014 at 09:49:48AM +0200, Bart Van Assche wrote:
> > > I don't see the value of patches 2,3 they're checking for an impossible
> > > condition ... why might it be possible?
> > 
> > When reading the source code in scsi_error.c it's easy to overlook that
> > scmd_eh_abort_handler(), scsi_abort_command() and scsi_eh_scmd_add() are
> > all invoked for requests in which the REQ_ATOM_COMPLETE bit has been
> > set. Although it is possible to mention this as a comment above these
> > functions, such comments are not checked at runtime. It would require
> > additional work from the reader to verify whether or not such source
> > code comments are up to date. However, the condition inside a
> > WARN_ON_ONCE() statement is checked every time the code is executed.
> > Hence my preference for a WARN_ON_ONCE() statement instead of writing
> > down somewhere that these three functions operate on requests in which
> > the REQ_ATOM_COMPLETE bit has been set.
> 
> 
> I really do like the REQ_ATOM_COMPLETE asserts - as experience tells
> these kinds of assumptions are best checked, otherwise they will
> unintentionally be violated.
> 
> I'm less excited about the list walk I have to say, as the overhead is
> getting fairly large for a simple assertation.

OK, my two objections are unconditional export of state from block that
we're trying to confine there.  Experience shows we'll grow unwanted
users of blk_rq_completed.  Probably export the assert from block not
blk_rq_completed().

The second is the WARN_ON_ONCE.  If this assert fails, we're doing error
handling on an uncompleted command and that will cause double
completion, leading to a massive cockup but one which might not
necessarily bring the machine down, so this should be BUG_ON

James