On Tue, 2014-05-27 at 00:52 -0700, hch@xxxxxxxxxxxxx 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
