Re: SCSI eats error from flush failure during hot plug

James Bottomley
2014-06-19T18:06:02Z

On Wed, 2014-06-11 at 06:37 -0700, Christoph Hellwig wrote:
> On Mon, Jun 09, 2014 at 10:29:06AM -0700, James Bottomley wrote:
> > I'll do it as a bug fix, but I do need Jens to make sure nothing else
> > breaks first. Best I can tell, the state model for compound commands
> > like flush doesn't expect us to change the request type ... nothing puts
> > it back to REQ_TYPE_FS. In your case, the flush is the last command
> > sent, so there's no problem ... I just worry we will get an obscure
> > problem later on from something that does a BLOCK_PC prepared first
> > command.
>
> Yes, I don't think resetting cmd_type is a good idea. I'd much rather
> see a special case for rq->cmd_flags & REQ_FLUSH in the completion
> handler - we already treat it special during setup anyway.

That's not really a good idea either ... I did think of it. We'll end
up with a cmd_type of REQ_TYPE_FS which because of REQ_FLUSH (or REQ_FUA
or REQ_DISCARD or any number of other things) we have to treat as though
it were REQ_TYPE_BLOCK_PC. It's much better to tune handling
expectations according to req->cmd_type because that's what we already
do. These commands are actually set up by our handlers, so it's up to
us to mark the request type correctly.

This, I think, does everything we need

James

---

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index f7e3163..d1458ec 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1103,6 +1103,11 @@ int scsi_setup_blk_pc_cmnd(struct scsi_device *sdev, struct request *req)

cmd->transfersize = blk_rq_bytes(req);
cmd->allowed = req->retries;
+ /*
+ * Thanks to flush and other PC prepared commands, we may
+ * not be a REQ_TYPE_BLOCK_PC; make sure we are
+ */
+ req->cmd_type = REQ_TYPE_BLOCK_PC;
return BLKPREP_OK;
}
EXPORT_SYMBOL(scsi_setup_blk_pc_cmnd);
@@ -1129,6 +1134,11 @@ int scsi_setup_fs_cmnd(struct scsi_device *sdev, struct request *req)
BUG_ON(!req->nr_phys_segments);

memset(cmd->cmnd, 0, BLK_MAX_CDB);
+ /*
+ * Thanks to flush and other PC prepared commands, we may
+ * not be a REQ_TYPE_FS; make sure we are
+ */
+ req->cmd_type = REQ_TYPE_FS;
return scsi_init_io(cmd, GFP_ATOMIC);
}
EXPORT_SYMBOL(scsi_setup_fs_cmnd);