Skip to contentSkip to conversation

ub: use __blk_end_request_all()

PATCH In review Tejun Heo 7 participants 29 messages May 15, 2009 – May 19, 2009

Subsystems (6)
VFS SCSI Block layer libata +2 more subsystems
lore
Share & tools
lore

Tejun Heo

author
#0 lore raw

PATCH 1/3 ub: use __blk_end_request_all()

ub_end_rq() always tries to complete full request. The @cmd_len parameter was there because rq->data_len used to be overwritten with residue count. Drop @cmd_len and use __blk_end_request_all().

Signed-off-by Tejun Heo <[email protected]>
Cc Pete Zaitcev <[email protected]>
---
 drivers/block/ub.c |   24 +++++++-----------------
 1 file changed, 7 insertions(+), 17 deletions(-)
drivers/block/ub.c +7 -17
Index: block/drivers/block/ub.c
===================================================================
@@ -360,8 +360,7 @@ static void ub_cmd_build_block(struct ub
 static void ub_cmd_build_packet(struct ub_dev *sc, struct ub_lun *lun,
     struct ub_scsi_cmd *cmd, struct ub_request *urq);
 static void ub_rw_cmd_done(struct ub_dev *sc, struct ub_scsi_cmd *cmd);
-static void ub_end_rq(struct request *rq, unsigned int status,
-    unsigned int cmd_len);
+static void ub_end_rq(struct request *rq, unsigned int status);
 static int ub_rw_cmd_retry(struct ub_dev *sc, struct ub_lun *lun,
     struct ub_request *urq, struct ub_scsi_cmd *cmd);
 static int ub_submit_scsi(struct ub_dev *sc, struct ub_scsi_cmd *cmd);

@@ -644,13 +643,13 @@ static int ub_request_fn_1(struct ub_lun
 
 	if (atomic_read(&sc->poison)) {
 		blk_start_request(rq);
-		ub_end_rq(rq, DID_NO_CONNECT << 16, blk_rq_bytes(rq));
+		ub_end_rq(rq, DID_NO_CONNECT << 16);
 		return 0;
 	}
 
 	if (lun->changed && !blk_pc_request(rq)) {
 		blk_start_request(rq);
-		ub_end_rq(rq, SAM_STAT_CHECK_CONDITION, blk_rq_bytes(rq));
+		ub_end_rq(rq, SAM_STAT_CHECK_CONDITION);
 		return 0;
 	}
 

@@ -702,7 +701,7 @@ static int ub_request_fn_1(struct ub_lun
 
 drop:
 	ub_put_cmd(lun, cmd);
-	ub_end_rq(rq, DID_ERROR << 16, blk_rq_bytes(rq));
+	ub_end_rq(rq, DID_ERROR << 16);
 	return 0;
 }
 

@@ -777,7 +776,6 @@ static void ub_rw_cmd_done(struct ub_dev
 	struct ub_request *urq = cmd->back;
 	struct request *rq;
 	unsigned int scsi_status;
-	unsigned int cmd_len;
 
 	rq = urq->rq;
 

@@ -816,17 +814,14 @@ static void ub_rw_cmd_done(struct ub_dev
 
 	urq->rq = NULL;
 
-	cmd_len = cmd->len;
 	ub_put_cmd(lun, cmd);
-	ub_end_rq(rq, scsi_status, cmd_len);
+	ub_end_rq(rq, scsi_status);
 	blk_start_queue(lun->disk->queue);
 }
 
-static void ub_end_rq(struct request *rq, unsigned int scsi_status,
-    unsigned int cmd_len)
+static void ub_end_rq(struct request *rq, unsigned int scsi_status)
 {
 	int error;
-	long rqlen;
 
 	if (scsi_status == 0) {
 		error = 0;

@@ -834,12 +829,7 @@ static void ub_end_rq(struct request *rq
 		error = -EIO;
 		rq->errors = scsi_status;
 	}
-	rqlen = blk_rq_bytes(rq);    /* Oddly enough, this is the residue. */
-	if (__blk_end_request(rq, error, cmd_len)) {
-		printk(KERN_WARNING DRV_NAME
-		    ": __blk_end_request blew, %s-cmd total %u rqlen %ld\n",
-		    blk_pc_request(rq)? "pc": "fs", cmd_len, rqlen);
-	}
+	__blk_end_request_all(rq, error);
 }
 
 static int ub_rw_cmd_retry(struct ub_dev *sc, struct ub_lun *lun,
#1 lore raw

PATCH 2/3 block: set rq->resid_len to blk_rq_bytes() on issue

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing rq->resid_len, the default value of residue count was changed from full count to zero. The conversion was done under the assumption that when a request fails residue count wasn't defined. However, Boaz and James pointed out that this wasn't true and the residue count should be preserved for failed requests too.

This patchset restores the original behavior by setting rq->resid_len to blk_rq_bytes(rq) on issue and restoring explicit clearing in affected drivers. While at it, take advantage of the fact that rq->resid_len is set to full count where applicable.

* ide-cd: rq->resid_len cleared on pc success

* mptsas: req->resid_len cleared on success

* sas_expander: rsp/req->resid_len cleared on success

* mpt2sas_transport: req->resid_len cleared on success

* ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take advantage of initial full count to simplify code

Signed-off-by Tejun Heo <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Boaz Harrosh <[email protected]>
Cc James Bottomley <[email protected]>
Cc Pete Zaitcev <[email protected]>
Cc Bartlomiej Zolnierkiewicz <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc Eric Moore <[email protected]>
Cc Darrick J. Wong <[email protected]>
---
 block/blk-core.c                         |    1 +
 drivers/block/ub.c                       |    3 +--
 drivers/ide/ide-cd.c                     |    4 ++--
 drivers/ide/ide-tape.c                   |    2 +-
 drivers/message/fusion/mptsas.c          |    3 ++-
 drivers/scsi/libsas/sas_expander.c       |    4 ++++
 drivers/scsi/libsas/sas_host_smp.c       |    3 ---
 drivers/scsi/mpt2sas/mpt2sas_transport.c |    4 ++--
 8 files changed, 13 insertions(+), 11 deletions(-)
block/blk-core.c +1 -0
Index: block/block/blk-core.c
===================================================================
@@ -2198,6 +2198,7 @@ void blk_rq_bio_prep(struct request_queu
 		rq->buffer = bio_data(bio);
 	}
 	rq->__data_len = bio->bi_size;
+	rq->resid_len = bio->bi_size;
 	rq->bio = rq->biotail = bio;
 
 	if (bio->bi_bdev)
drivers/ide/ide-cd.c +2 -2
Index: block/drivers/ide/ide-cd.c
===================================================================
@@ -699,6 +699,7 @@ static ide_startstop_t cdrom_newpc_intr(
 
 out_end:
 	if (blk_pc_request(rq) && rc == 0) {
+		rq->resid_len = 0;
 		blk_end_request_all(rq, 0);
 		hwif->rq = NULL;
 	} else {

@@ -718,8 +719,7 @@ out_end:
 
 		/* make sure it's fully ended */
 		if (blk_fs_request(rq) == 0) {
-			rq->resid_len = blk_rq_bytes(rq) -
-				(cmd->nbytes - cmd->nleft);
+			rq->resid_len -= cmd->nbytes - cmd->nleft;
 			if (uptodate == 0 && (cmd->tf_flags & IDE_TFLAG_WRITE))
 				rq->resid_len += cmd->last_xfer_len;
 		}
drivers/ide/ide-tape.c +1 -1
Index: block/drivers/ide/ide-tape.c
===================================================================
@@ -380,7 +380,7 @@ static int ide_tape_callback(ide_drive_t
 		}
 
 		tape->first_frame += blocks;
-		rq->resid_len = blk_rq_bytes(rq) - blocks * tape->blk_size;
+		rq->resid_len -= blocks * tape->blk_size;
 
 		if (pc->error) {
 			uptodate = 0;
drivers/message/fusion/mptsas.c +2 -1
Index: block/drivers/message/fusion/mptsas.c
===================================================================
@@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
 		smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
 		memcpy(req->sense, smprep, sizeof(*smprep));
 		req->sense_len = sizeof(*smprep);
-		rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= smprep->ResponseDataLength;
 	} else {
 		printk(MYIOC_s_ERR_FMT "%s: smp passthru reply failed to be returned\n",
 		    ioc->name, __func__);
drivers/scsi/libsas/sas_expander.c +4 -0
Index: block/drivers/scsi/libsas/sas_expander.c
===================================================================
@@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
 	if (ret > 0) {
 		/* positive number is the untransferred residual */
 		rsp->resid_len = ret;
+		req->resid_len = 0;
 		ret = 0;
+	} else if (ret == 0) {
+		rsp->resid_len = 0;
+		req->resid_len = 0;
 	}
 
 	return ret;
drivers/scsi/libsas/sas_host_smp.c +0 -3
Index: block/drivers/scsi/libsas/sas_host_smp.c
===================================================================
@@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
 	resp_data[1] = req_data[1];
 	resp_data[2] = SMP_RESP_FUNC_UNK;
 
-	req->resid_len = blk_rq_bytes(req);
-	rsp->resid_len = blk_rq_bytes(rsp);
-
 	switch (req_data[1]) {
 	case SMP_REPORT_GENERAL:
 		req->resid_len -= 8;
drivers/scsi/mpt2sas/mpt2sas_transport.c +2 -2
Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
===================================================================
@@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
 
 		memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
 		req->sense_len = sizeof(*mpi_reply);
-		rsp->resid_len = blk_rq_bytes(rsp) -
-				 mpi_reply->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= mpi_reply->ResponseDataLength;
 	} else {
 		dtransportprintk(ioc, printk(MPT2SAS_DEBUG_FMT
 		    "%s - no reply\n", ioc->name, __func__));
drivers/block/ub.c +1 -2
Index: block/drivers/block/ub.c
===================================================================
@@ -781,8 +781,7 @@ static void ub_rw_cmd_done(struct ub_dev
 
 	if (cmd->error == 0) {
 		if (blk_pc_request(rq)) {
-			if (cmd->act_len < blk_rq_bytes(rq))
-				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
+			rq->resid_len -= min(cmd->act_len, rq->resid_len);
 			scsi_status = 0;
 		} else {
 			if (cmd->act_len != cmd->len) {
#2 lore raw

PATCH 2/3 block: set rq->resid_len to blk_rq_bytes() on issue

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing rq->resid_len, the default value of residue count was changed from full count to zero. The conversion was done under the assumption that when a request fails residue count wasn't defined. However, Boaz and James pointed out that this wasn't true and the residue count should be preserved for failed requests too.

This patchset restores the original behavior by setting rq->resid_len to blk_rq_bytes(rq) on issue and restoring explicit clearing in affected drivers. While at it, take advantage of the fact that rq->resid_len is set to full count where applicable.

* ide-cd: rq->resid_len cleared on pc success

* mptsas: req->resid_len cleared on success

* sas_expander: rsp/req->resid_len cleared on success

* mpt2sas_transport: req->resid_len cleared on success

* ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take advantage of initial full count to simplify code

Signed-off-by Tejun Heo <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Boaz Harrosh <[email protected]>
Cc James Bottomley <[email protected]>
Cc Pete Zaitcev <[email protected]>
Cc Bartlomiej Zolnierkiewicz <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc Eric Moore <[email protected]>
Cc Darrick J. Wong <[email protected]>

---
ub updated per Pete's comment.

Thanks.

 block/blk-core.c                         |    1 +
 drivers/block/ub.c                       |    6 ++++--
 drivers/ide/ide-cd.c                     |    4 ++--
 drivers/ide/ide-tape.c                   |    2 +-
 drivers/message/fusion/mptsas.c          |    3 ++-
 drivers/scsi/libsas/sas_expander.c       |    4 ++++
 drivers/scsi/libsas/sas_host_smp.c       |    3 ---
 drivers/scsi/mpt2sas/mpt2sas_transport.c |    4 ++--
 8 files changed, 16 insertions(+), 11 deletions(-)
block/blk-core.c +1 -0
Index: block/block/blk-core.c
===================================================================
@@ -2198,6 +2198,7 @@ void blk_rq_bio_prep(struct request_queu
 		rq->buffer = bio_data(bio);
 	}
 	rq->__data_len = bio->bi_size;
+	rq->resid_len = bio->bi_size;
 	rq->bio = rq->biotail = bio;
 
 	if (bio->bi_bdev)
drivers/ide/ide-cd.c +2 -2
Index: block/drivers/ide/ide-cd.c
===================================================================
@@ -699,6 +699,7 @@ static ide_startstop_t cdrom_newpc_intr(
 
 out_end:
 	if (blk_pc_request(rq) && rc == 0) {
+		rq->resid_len = 0;
 		blk_end_request_all(rq, 0);
 		hwif->rq = NULL;
 	} else {

@@ -718,8 +719,7 @@ out_end:
 
 		/* make sure it's fully ended */
 		if (blk_fs_request(rq) == 0) {
-			rq->resid_len = blk_rq_bytes(rq) -
-				(cmd->nbytes - cmd->nleft);
+			rq->resid_len -= cmd->nbytes - cmd->nleft;
 			if (uptodate == 0 && (cmd->tf_flags & IDE_TFLAG_WRITE))
 				rq->resid_len += cmd->last_xfer_len;
 		}
drivers/ide/ide-tape.c +1 -1
Index: block/drivers/ide/ide-tape.c
===================================================================
@@ -380,7 +380,7 @@ static int ide_tape_callback(ide_drive_t
 		}
 
 		tape->first_frame += blocks;
-		rq->resid_len = blk_rq_bytes(rq) - blocks * tape->blk_size;
+		rq->resid_len -= blocks * tape->blk_size;
 
 		if (pc->error) {
 			uptodate = 0;
drivers/message/fusion/mptsas.c +2 -1
Index: block/drivers/message/fusion/mptsas.c
===================================================================
@@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
 		smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
 		memcpy(req->sense, smprep, sizeof(*smprep));
 		req->sense_len = sizeof(*smprep);
-		rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= smprep->ResponseDataLength;
 	} else {
 		printk(MYIOC_s_ERR_FMT "%s: smp passthru reply failed to be returned\n",
 		    ioc->name, __func__);
drivers/scsi/libsas/sas_expander.c +4 -0
Index: block/drivers/scsi/libsas/sas_expander.c
===================================================================
@@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
 	if (ret > 0) {
 		/* positive number is the untransferred residual */
 		rsp->resid_len = ret;
+		req->resid_len = 0;
 		ret = 0;
+	} else if (ret == 0) {
+		rsp->resid_len = 0;
+		req->resid_len = 0;
 	}
 
 	return ret;
drivers/scsi/libsas/sas_host_smp.c +0 -3
Index: block/drivers/scsi/libsas/sas_host_smp.c
===================================================================
@@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
 	resp_data[1] = req_data[1];
 	resp_data[2] = SMP_RESP_FUNC_UNK;
 
-	req->resid_len = blk_rq_bytes(req);
-	rsp->resid_len = blk_rq_bytes(rsp);
-
 	switch (req_data[1]) {
 	case SMP_REPORT_GENERAL:
 		req->resid_len -= 8;
drivers/scsi/mpt2sas/mpt2sas_transport.c +2 -2
Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
===================================================================
@@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
 
 		memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
 		req->sense_len = sizeof(*mpi_reply);
-		rsp->resid_len = blk_rq_bytes(rsp) -
-				 mpi_reply->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= mpi_reply->ResponseDataLength;
 	} else {
 		dtransportprintk(ioc, printk(MPT2SAS_DEBUG_FMT
 		    "%s - no reply\n", ioc->name, __func__));
drivers/block/ub.c +4 -2
Index: block/drivers/block/ub.c
===================================================================
@@ -781,8 +781,10 @@ static void ub_rw_cmd_done(struct ub_dev
 
 	if (cmd->error == 0) {
 		if (blk_pc_request(rq)) {
-			if (cmd->act_len < blk_rq_bytes(rq))
-				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
+			if (cmd->act_len >= rq->resid_len)
+				rq->resid_len = 0;
+			else
+				rq->resid_len -= cmd->act_len;
 			scsi_status = 0;
 		} else {
 			if (cmd->act_len != cmd->len) {
#3 lore raw

Re: [PATCH UPDATED block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

Quoted 53 lines · Tejun Heo

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing rq->resid_len, the default value of residue count was changed from full count to zero. The conversion was done under the assumption that when a request fails residue count wasn't defined. However, Boaz and James pointed out that this wasn't true and the residue count should be preserved for failed requests too.

This patchset restores the original behavior by setting rq->resid_len to blk_rq_bytes(rq) on issue and restoring explicit clearing in affected drivers. While at it, take advantage of the fact that rq->resid_len is set to full count where applicable.

* ide-cd: rq->resid_len cleared on pc success

* mptsas: req->resid_len cleared on success

* sas_expander: rsp/req->resid_len cleared on success

* mpt2sas_transport: req->resid_len cleared on success

* ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take advantage of initial full count to simplify code

Signed-off-by Tejun Heo <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Boaz Harrosh <[email protected]>
Cc James Bottomley <[email protected]>
Cc Pete Zaitcev <[email protected]>
Cc Bartlomiej Zolnierkiewicz <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc Eric Moore <[email protected]>
Cc Darrick J. Wong <[email protected]>

---
ub updated per Pete's comment.

Thanks.

 block/blk-core.c                         |    1 +
 drivers/block/ub.c                       |    6 ++++--
 drivers/ide/ide-cd.c                     |    4 ++--
 drivers/ide/ide-tape.c                   |    2 +-
 drivers/message/fusion/mptsas.c          |    3 ++-
 drivers/scsi/libsas/sas_expander.c       |    4 ++++
 drivers/scsi/libsas/sas_host_smp.c       |    3 ---
 drivers/scsi/mpt2sas/mpt2sas_transport.c |    4 ++--
 8 files changed, 16 insertions(+), 11 deletions(-)

Index: block/block/blk-core.c
===================================================================

block/blk-core.c void blk_rq_bio_prep(struct request_queu · line 2203

Quoted 6 lines
block/blk-core.c +1 -0
@@ -2198,6 +2198,7 @@ void blk_rq_bio_prep(struct request_queu
 		rq->buffer = bio_data(bio);
 	}
 	rq->__data_len = bio->bi_size;
+	rq->resid_len = bio->bi_size;
 	rq->bio = rq->biotail = bio;

Hi Tejun.
Surly this is not enough. This is not the only place rq->__data_len gets a value. Specifically it can be added to in blk_rq_append_bio. You must then update all places that set rq->__data_len to also set rq->resid_len.

Alternatively you could do a single:
+ rq->resid_len = blk_rq_bytes(rq);

inside blk_fetch_request(q) or before the call to prep_fn, or even at __elv_add_request().

Currently this is a bug with chained bios

Thanks
Boaz

Quoted 112 lines
block/blk-core.c +0 -0
 	if (bio->bi_bdev)

Index: block/drivers/ide/ide-cd.c
===================================================================

drivers/ide/ide-cd.c +2 -2
@@ -699,6 +699,7 @@ static ide_startstop_t cdrom_newpc_intr(
 
 out_end:
 	if (blk_pc_request(rq) && rc == 0) {
+		rq->resid_len = 0;
 		blk_end_request_all(rq, 0);
 		hwif->rq = NULL;
 	} else {

@@ -718,8 +719,7 @@ out_end:
 
 		/* make sure it's fully ended */
 		if (blk_fs_request(rq) == 0) {
-			rq->resid_len = blk_rq_bytes(rq) -
-				(cmd->nbytes - cmd->nleft);
+			rq->resid_len -= cmd->nbytes - cmd->nleft;
 			if (uptodate == 0 && (cmd->tf_flags & IDE_TFLAG_WRITE))
 				rq->resid_len += cmd->last_xfer_len;
 		}

Index: block/drivers/ide/ide-tape.c
===================================================================

drivers/ide/ide-tape.c +1 -1
@@ -380,7 +380,7 @@ static int ide_tape_callback(ide_drive_t
 		}
 
 		tape->first_frame += blocks;
-		rq->resid_len = blk_rq_bytes(rq) - blocks * tape->blk_size;
+		rq->resid_len -= blocks * tape->blk_size;
 
 		if (pc->error) {
 			uptodate = 0;

Index: block/drivers/message/fusion/mptsas.c
===================================================================

drivers/message/fusion/mptsas.c +2 -1
@@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
 		smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
 		memcpy(req->sense, smprep, sizeof(*smprep));
 		req->sense_len = sizeof(*smprep);
-		rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= smprep->ResponseDataLength;
 	} else {
 		printk(MYIOC_s_ERR_FMT "%s: smp passthru reply failed to be returned\n",
 		    ioc->name, __func__);

Index: block/drivers/scsi/libsas/sas_expander.c
===================================================================

drivers/scsi/libsas/sas_expander.c +4 -0
@@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
 	if (ret > 0) {
 		/* positive number is the untransferred residual */
 		rsp->resid_len = ret;
+		req->resid_len = 0;
 		ret = 0;
+	} else if (ret == 0) {
+		rsp->resid_len = 0;
+		req->resid_len = 0;
 	}
 
 	return ret;

Index: block/drivers/scsi/libsas/sas_host_smp.c
===================================================================

drivers/scsi/libsas/sas_host_smp.c +0 -3
@@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
 	resp_data[1] = req_data[1];
 	resp_data[2] = SMP_RESP_FUNC_UNK;
 
-	req->resid_len = blk_rq_bytes(req);
-	rsp->resid_len = blk_rq_bytes(rsp);
-
 	switch (req_data[1]) {
 	case SMP_REPORT_GENERAL:
 		req->resid_len -= 8;

Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
===================================================================

drivers/scsi/mpt2sas/mpt2sas_transport.c +2 -2
@@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
 
 		memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
 		req->sense_len = sizeof(*mpi_reply);
-		rsp->resid_len = blk_rq_bytes(rsp) -
-				 mpi_reply->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= mpi_reply->ResponseDataLength;
 	} else {
 		dtransportprintk(ioc, printk(MPT2SAS_DEBUG_FMT
 		    "%s - no reply\n", ioc->name, __func__));

Index: block/drivers/block/ub.c
===================================================================

drivers/block/ub.c +4 -2
@@ -781,8 +781,10 @@ static void ub_rw_cmd_done(struct ub_dev
 
 	if (cmd->error == 0) {
 		if (blk_pc_request(rq)) {
-			if (cmd->act_len < blk_rq_bytes(rq))
-				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
+			if (cmd->act_len >= rq->resid_len)
+				rq->resid_len = 0;
+			else
+				rq->resid_len -= cmd->act_len;
 			scsi_status = 0;
 		} else {
 			if (cmd->act_len != cmd->len) {
#4 lore raw

Re: [PATCH UPDATED block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

Hello, Boaz.

Quoted 4 lines · Boaz Harrosh

Surly this is not enough. This is not the only place rq->__data_len gets a value. Specifically it can be added to in blk_rq_append_bio. You must then update all places that set rq->__data_len to also set rq->resid_len.

Right, nice spotting, but I think those are the only two places. You now see why I don't like that append thing. :-)

Quoted 5 lines

Alternatively you could do a single:

(quoted diff) +1 -0
+	rq->resid_len = blk_rq_bytes(rq);

inside blk_fetch_request(q) or before the call to prep_fn, or even at __elv_add_request().

I think I'll just add resid_len adjustment there right after __data_len manipulation. That code path should really go away eventually.

Thanks.

Signature · 1 line
tejun
#5 lore raw

Re: [PATCH UPDATED block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

Quoted 3 lines · Tejun Heo

I think I'll just add resid_len adjustment there right after __data_len manipulation. That code path should really go away eventually.

Aieee... crap, I also forgot the merging path. I'll change as you suggested. Thanks.

Signature · 1 line
tejun
#6 lore raw

PATCH 2/3 block: set rq->resid_len to blk_rq_bytes() on issue

In reply to Tejun Heo

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing rq->resid_len, the default value of residue count was changed from full count to zero. The conversion was done under the assumption that when a request fails residue count wasn't defined. However, Boaz and James pointed out that this wasn't true and the residue count should be preserved for failed requests too.

This patchset restores the original behavior by setting rq->resid_len to blk_rq_bytes(rq) on request start and restoring explicit clearing in affected drivers. While at it, take advantage of the fact that rq->resid_len is set to full count where applicable.

* ide-cd: rq->resid_len cleared on pc success

* mptsas: req->resid_len cleared on success

* sas_expander: rsp/req->resid_len cleared on success

* mpt2sas_transport: req->resid_len cleared on success

* ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take advantage of initial full count to simplify code

Boaz Harrosh spotted bug in resid_len initialization. Fixed as suggested.

Signed-off-by Tejun Heo <[email protected]>
Acked-by Borislav Petkov <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Boaz Harrosh <[email protected]>
Cc James Bottomley <[email protected]>
Cc Pete Zaitcev <[email protected]>
Cc Bartlomiej Zolnierkiewicz <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc Eric Moore <[email protected]>
Cc Darrick J. Wong <[email protected]>

---
Fixed as Boaz suggested. Thanks Boaz.

 block/blk-core.c                         |    5 +++--
 drivers/block/ub.c                       |    6 ++++--
 drivers/ide/ide-cd.c                     |    4 ++--
 drivers/ide/ide-tape.c                   |    2 +-
 drivers/message/fusion/mptsas.c          |    3 ++-
 drivers/scsi/libsas/sas_expander.c       |    4 ++++
 drivers/scsi/libsas/sas_host_smp.c       |    3 ---
 drivers/scsi/mpt2sas/mpt2sas_transport.c |    4 ++--
 8 files changed, 18 insertions(+), 13 deletions(-)
block/blk-core.c +3 -2
Index: block/block/blk-core.c
===================================================================
@@ -1783,9 +1783,10 @@ void blk_start_request(struct request *r
 	blk_dequeue_request(req);
 
 	/*
-	 * We are now handing the request to the hardware, add the
-	 * timeout handler.
+	 * We are now handing the request to the hardware, initialize
+	 * resid_len to full count and add the timeout handler.
 	 */
+	req->resid_len = blk_rq_bytes(req);
 	blk_add_timer(req);
 }
 EXPORT_SYMBOL(blk_start_request);
drivers/ide/ide-cd.c +2 -2
Index: block/drivers/ide/ide-cd.c
===================================================================
@@ -699,6 +699,7 @@ static ide_startstop_t cdrom_newpc_intr(
 
 out_end:
 	if (blk_pc_request(rq) && rc == 0) {
+		rq->resid_len = 0;
 		blk_end_request_all(rq, 0);
 		hwif->rq = NULL;
 	} else {

@@ -718,8 +719,7 @@ out_end:
 
 		/* make sure it's fully ended */
 		if (blk_fs_request(rq) == 0) {
-			rq->resid_len = blk_rq_bytes(rq) -
-				(cmd->nbytes - cmd->nleft);
+			rq->resid_len -= cmd->nbytes - cmd->nleft;
 			if (uptodate == 0 && (cmd->tf_flags & IDE_TFLAG_WRITE))
 				rq->resid_len += cmd->last_xfer_len;
 		}
drivers/ide/ide-tape.c +1 -1
Index: block/drivers/ide/ide-tape.c
===================================================================
@@ -380,7 +380,7 @@ static int ide_tape_callback(ide_drive_t
 		}
 
 		tape->first_frame += blocks;
-		rq->resid_len = blk_rq_bytes(rq) - blocks * tape->blk_size;
+		rq->resid_len -= blocks * tape->blk_size;
 
 		if (pc->error) {
 			uptodate = 0;
drivers/message/fusion/mptsas.c +2 -1
Index: block/drivers/message/fusion/mptsas.c
===================================================================
@@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
 		smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
 		memcpy(req->sense, smprep, sizeof(*smprep));
 		req->sense_len = sizeof(*smprep);
-		rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= smprep->ResponseDataLength;
 	} else {
 		printk(MYIOC_s_ERR_FMT "%s: smp passthru reply failed to be returned\n",
 		    ioc->name, __func__);
drivers/scsi/libsas/sas_expander.c +4 -0
Index: block/drivers/scsi/libsas/sas_expander.c
===================================================================
@@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
 	if (ret > 0) {
 		/* positive number is the untransferred residual */
 		rsp->resid_len = ret;
+		req->resid_len = 0;
 		ret = 0;
+	} else if (ret == 0) {
+		rsp->resid_len = 0;
+		req->resid_len = 0;
 	}
 
 	return ret;
drivers/scsi/libsas/sas_host_smp.c +0 -3
Index: block/drivers/scsi/libsas/sas_host_smp.c
===================================================================
@@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
 	resp_data[1] = req_data[1];
 	resp_data[2] = SMP_RESP_FUNC_UNK;
 
-	req->resid_len = blk_rq_bytes(req);
-	rsp->resid_len = blk_rq_bytes(rsp);
-
 	switch (req_data[1]) {
 	case SMP_REPORT_GENERAL:
 		req->resid_len -= 8;
drivers/scsi/mpt2sas/mpt2sas_transport.c +2 -2
Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
===================================================================
@@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
 
 		memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
 		req->sense_len = sizeof(*mpi_reply);
-		rsp->resid_len = blk_rq_bytes(rsp) -
-				 mpi_reply->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= mpi_reply->ResponseDataLength;
 	} else {
 		dtransportprintk(ioc, printk(MPT2SAS_DEBUG_FMT
 		    "%s - no reply\n", ioc->name, __func__));
drivers/block/ub.c +4 -2
Index: block/drivers/block/ub.c
===================================================================
@@ -781,8 +781,10 @@ static void ub_rw_cmd_done(struct ub_dev
 
 	if (cmd->error == 0) {
 		if (blk_pc_request(rq)) {
-			if (cmd->act_len < blk_rq_bytes(rq))
-				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
+			if (cmd->act_len >= rq->resid_len)
+				rq->resid_len = 0;
+			else
+				rq->resid_len -= cmd->act_len;
 			scsi_status = 0;
 		} else {
 			if (cmd->act_len != cmd->len) {
#7 lore raw

Re: [PATCH UPDATED2 block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

Quoted 14 lines · Tejun Heo

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing rq->resid_len, the default value of residue count was changed from full count to zero. The conversion was done under the assumption that when a request fails residue count wasn't defined. However, Boaz and James pointed out that this wasn't true and the residue count should be preserved for failed requests too.

This patchset restores the original behavior by setting rq->resid_len to blk_rq_bytes(rq) on request start and restoring explicit clearing in affected drivers. While at it, take advantage of the fact that rq->resid_len is set to full count where applicable.

* ide-cd: rq->resid_len cleared on pc success

Quoted 8 lines

* mptsas: req->resid_len cleared on success

* sas_expander: rsp/req->resid_len cleared on success

* mpt2sas_transport: req->resid_len cleared on success

* ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take advantage of initial full count to simplify code

Tejun, you have to stop replying with updated patches in an existing thread. It's virtually impossible to sort the resulting mess into patches worth applying.

If you update patches in a series, or post a new patch, do it in a new thread. And use explicit naming (like "foo patchset version bar") so it's easy to grasp the chronology of the patches. Otherwise I have to spend lots of time extracting and comparing patches, which is just a waste of time.

I haven't applied anything in this thread. Please repost the pending patches so they can be reviewed and applied! Thanks.

Quoted 155 lines

Boaz Harrosh spotted bug in resid_len initialization. Fixed as suggested.

Signed-off-by Tejun Heo <[email protected]>
Acked-by Borislav Petkov <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Boaz Harrosh <[email protected]>
Cc James Bottomley <[email protected]>
Cc Pete Zaitcev <[email protected]>
Cc Bartlomiej Zolnierkiewicz <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc Eric Moore <[email protected]>
Cc Darrick J. Wong <[email protected]>

---
Fixed as Boaz suggested. Thanks Boaz.

 block/blk-core.c                         |    5 +++--
 drivers/block/ub.c                       |    6 ++++--
 drivers/ide/ide-cd.c                     |    4 ++--
 drivers/ide/ide-tape.c                   |    2 +-
 drivers/message/fusion/mptsas.c          |    3 ++-
 drivers/scsi/libsas/sas_expander.c       |    4 ++++
 drivers/scsi/libsas/sas_host_smp.c       |    3 ---
 drivers/scsi/mpt2sas/mpt2sas_transport.c |    4 ++--
 8 files changed, 18 insertions(+), 13 deletions(-)

Index: block/block/blk-core.c
===================================================================

block/blk-core.c +3 -2
@@ -1783,9 +1783,10 @@ void blk_start_request(struct request *r
 	blk_dequeue_request(req);
 
 	/*
-	 * We are now handing the request to the hardware, add the
-	 * timeout handler.
+	 * We are now handing the request to the hardware, initialize
+	 * resid_len to full count and add the timeout handler.
 	 */
+	req->resid_len = blk_rq_bytes(req);
 	blk_add_timer(req);
 }
 EXPORT_SYMBOL(blk_start_request);

Index: block/drivers/ide/ide-cd.c
===================================================================

drivers/ide/ide-cd.c +2 -2
@@ -699,6 +699,7 @@ static ide_startstop_t cdrom_newpc_intr(
 
 out_end:
 	if (blk_pc_request(rq) && rc == 0) {
+		rq->resid_len = 0;
 		blk_end_request_all(rq, 0);
 		hwif->rq = NULL;
 	} else {

@@ -718,8 +719,7 @@ out_end:
 
 		/* make sure it's fully ended */
 		if (blk_fs_request(rq) == 0) {
-			rq->resid_len = blk_rq_bytes(rq) -
-				(cmd->nbytes - cmd->nleft);
+			rq->resid_len -= cmd->nbytes - cmd->nleft;
 			if (uptodate == 0 && (cmd->tf_flags & IDE_TFLAG_WRITE))
 				rq->resid_len += cmd->last_xfer_len;
 		}

Index: block/drivers/ide/ide-tape.c
===================================================================

drivers/ide/ide-tape.c +1 -1
@@ -380,7 +380,7 @@ static int ide_tape_callback(ide_drive_t
 		}
 
 		tape->first_frame += blocks;
-		rq->resid_len = blk_rq_bytes(rq) - blocks * tape->blk_size;
+		rq->resid_len -= blocks * tape->blk_size;
 
 		if (pc->error) {
 			uptodate = 0;

Index: block/drivers/message/fusion/mptsas.c
===================================================================

drivers/message/fusion/mptsas.c +2 -1
@@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
 		smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
 		memcpy(req->sense, smprep, sizeof(*smprep));
 		req->sense_len = sizeof(*smprep);
-		rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= smprep->ResponseDataLength;
 	} else {
 		printk(MYIOC_s_ERR_FMT "%s: smp passthru reply failed to be returned\n",
 		    ioc->name, __func__);

Index: block/drivers/scsi/libsas/sas_expander.c
===================================================================

drivers/scsi/libsas/sas_expander.c +4 -0
@@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
 	if (ret > 0) {
 		/* positive number is the untransferred residual */
 		rsp->resid_len = ret;
+		req->resid_len = 0;
 		ret = 0;
+	} else if (ret == 0) {
+		rsp->resid_len = 0;
+		req->resid_len = 0;
 	}
 
 	return ret;

Index: block/drivers/scsi/libsas/sas_host_smp.c
===================================================================

drivers/scsi/libsas/sas_host_smp.c +0 -3
@@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
 	resp_data[1] = req_data[1];
 	resp_data[2] = SMP_RESP_FUNC_UNK;
 
-	req->resid_len = blk_rq_bytes(req);
-	rsp->resid_len = blk_rq_bytes(rsp);
-
 	switch (req_data[1]) {
 	case SMP_REPORT_GENERAL:
 		req->resid_len -= 8;

Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
===================================================================

drivers/scsi/mpt2sas/mpt2sas_transport.c +2 -2
@@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
 
 		memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
 		req->sense_len = sizeof(*mpi_reply);
-		rsp->resid_len = blk_rq_bytes(rsp) -
-				 mpi_reply->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= mpi_reply->ResponseDataLength;
 	} else {
 		dtransportprintk(ioc, printk(MPT2SAS_DEBUG_FMT
 		    "%s - no reply\n", ioc->name, __func__));

Index: block/drivers/block/ub.c
===================================================================

drivers/block/ub.c +4 -2
@@ -781,8 +781,10 @@ static void ub_rw_cmd_done(struct ub_dev
 
 	if (cmd->error == 0) {
 		if (blk_pc_request(rq)) {
-			if (cmd->act_len < blk_rq_bytes(rq))
-				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
+			if (cmd->act_len >= rq->resid_len)
+				rq->resid_len = 0;
+			else
+				rq->resid_len -= cmd->act_len;
 			scsi_status = 0;
 		} else {
 			if (cmd->act_len != cmd->len) {
Signature · 1 line
Jens Axboe
#8 lore raw

Re: [PATCH UPDATED2 block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

Hello, Jens.

Quoted 3 lines · Jens Axboe

Tejun, you have to stop replying with updated patches in an existing thread. It's virtually impossible to sort the resulting mess into patches worth applying.

Heh.. yeah, I usually post minor updates as UPDATED patches for review then repost the whole series after accumulating some of them. This patchset's size was on the borderline for proper patchset and I was feeling lazy. Sorry about that. Will repost.

Thanks.

Signature · 1 line
tejun
#9 lore raw

Re: [PATCH UPDATED2 block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

Quoted 11 lines · Tejun Heo

Hello, Jens.

Quoted 3 lines · Jens Axboe

Tejun, you have to stop replying with updated patches in an existing thread. It's virtually impossible to sort the resulting mess into patches worth applying.

Heh.. yeah, I usually post minor updates as UPDATED patches for review then repost the whole series after accumulating some of them. This patchset's size was on the borderline for proper patchset and I was feeling lazy. Sorry about that. Will repost.

Thanks, appreciated :-)

Signature · 1 line
Jens Axboe
#10 lore raw

PATCH 2/3 block: set rq->resid_len to blk_rq_bytes() on issue

In reply to Tejun Heo

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing rq->resid_len, the default value of residue count was changed from full count to zero. The conversion was done under the assumption that when a request fails residue count wasn't defined. However, Boaz and James pointed out that this wasn't true and the residue count should be preserved for failed requests too.

This patchset restores the original behavior by setting rq->resid_len to blk_rq_bytes(rq) on request start and restoring explicit clearing in affected drivers. While at it, take advantage of the fact that rq->resid_len is set to full count where applicable.

* ide-cd: rq->resid_len cleared on pc success

* mptsas: req->resid_len cleared on success

* sas_expander: rsp/req->resid_len cleared on success

* mpt2sas_transport: req->resid_len cleared on success

* ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take advantage of initial full count to simplify code

Boaz Harrosh spotted bug in resid_len initialization. Fixed as suggested.

Signed-off-by Tejun Heo <[email protected]>
Acked-by Borislav Petkov <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Boaz Harrosh <[email protected]>
Cc James Bottomley <[email protected]>
Cc Pete Zaitcev <[email protected]>
Cc Bartlomiej Zolnierkiewicz <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc Eric Moore <[email protected]>
Cc Darrick J. Wong <[email protected]>

---
Fixed as Boaz suggested. Thanks Boaz.

 block/blk-core.c                         |    5 +++--
 drivers/block/ub.c                       |    6 ++++--
 drivers/ide/ide-cd.c                     |    4 ++--
 drivers/ide/ide-tape.c                   |    2 +-
 drivers/message/fusion/mptsas.c          |    3 ++-
 drivers/scsi/libsas/sas_expander.c       |    4 ++++
 drivers/scsi/libsas/sas_host_smp.c       |    3 ---
 drivers/scsi/mpt2sas/mpt2sas_transport.c |    4 ++--
 8 files changed, 18 insertions(+), 13 deletions(-)
block/blk-core.c +3 -2
Index: block/block/blk-core.c
===================================================================
@@ -1783,9 +1783,10 @@ void blk_start_request(struct request *r
 	blk_dequeue_request(req);
 
 	/*
-	 * We are now handing the request to the hardware, add the
-	 * timeout handler.
+	 * We are now handing the request to the hardware, initialize
+	 * resid_len to full count and add the timeout handler.
 	 */
+	req->resid_len = blk_rq_bytes(req);
 	blk_add_timer(req);
 }
 EXPORT_SYMBOL(blk_start_request);
drivers/ide/ide-cd.c +2 -2
Index: block/drivers/ide/ide-cd.c
===================================================================
@@ -699,6 +699,7 @@ static ide_startstop_t cdrom_newpc_intr(
 
 out_end:
 	if (blk_pc_request(rq) && rc == 0) {
+		rq->resid_len = 0;
 		blk_end_request_all(rq, 0);
 		hwif->rq = NULL;
 	} else {

@@ -718,8 +719,7 @@ out_end:
 
 		/* make sure it's fully ended */
 		if (blk_fs_request(rq) == 0) {
-			rq->resid_len = blk_rq_bytes(rq) -
-				(cmd->nbytes - cmd->nleft);
+			rq->resid_len -= cmd->nbytes - cmd->nleft;
 			if (uptodate == 0 && (cmd->tf_flags & IDE_TFLAG_WRITE))
 				rq->resid_len += cmd->last_xfer_len;
 		}
drivers/ide/ide-tape.c +1 -1
Index: block/drivers/ide/ide-tape.c
===================================================================
@@ -380,7 +380,7 @@ static int ide_tape_callback(ide_drive_t
 		}
 
 		tape->first_frame += blocks;
-		rq->resid_len = blk_rq_bytes(rq) - blocks * tape->blk_size;
+		rq->resid_len -= blocks * tape->blk_size;
 
 		if (pc->error) {
 			uptodate = 0;
drivers/message/fusion/mptsas.c +2 -1
Index: block/drivers/message/fusion/mptsas.c
===================================================================
@@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
 		smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
 		memcpy(req->sense, smprep, sizeof(*smprep));
 		req->sense_len = sizeof(*smprep);
-		rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= smprep->ResponseDataLength;
 	} else {
 		printk(MYIOC_s_ERR_FMT "%s: smp passthru reply failed to be returned\n",
 		    ioc->name, __func__);
drivers/scsi/libsas/sas_expander.c +4 -0
Index: block/drivers/scsi/libsas/sas_expander.c
===================================================================
@@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
 	if (ret > 0) {
 		/* positive number is the untransferred residual */
 		rsp->resid_len = ret;
+		req->resid_len = 0;
 		ret = 0;
+	} else if (ret == 0) {
+		rsp->resid_len = 0;
+		req->resid_len = 0;
 	}
 
 	return ret;
drivers/scsi/libsas/sas_host_smp.c +0 -3
Index: block/drivers/scsi/libsas/sas_host_smp.c
===================================================================
@@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
 	resp_data[1] = req_data[1];
 	resp_data[2] = SMP_RESP_FUNC_UNK;
 
-	req->resid_len = blk_rq_bytes(req);
-	rsp->resid_len = blk_rq_bytes(rsp);
-
 	switch (req_data[1]) {
 	case SMP_REPORT_GENERAL:
 		req->resid_len -= 8;
drivers/scsi/mpt2sas/mpt2sas_transport.c +2 -2
Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
===================================================================
@@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
 
 		memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
 		req->sense_len = sizeof(*mpi_reply);
-		rsp->resid_len = blk_rq_bytes(rsp) -
-				 mpi_reply->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= mpi_reply->ResponseDataLength;
 	} else {
 		dtransportprintk(ioc, printk(MPT2SAS_DEBUG_FMT
 		    "%s - no reply\n", ioc->name, __func__));
drivers/block/ub.c +4 -2
Index: block/drivers/block/ub.c
===================================================================
@@ -781,8 +781,10 @@ static void ub_rw_cmd_done(struct ub_dev
 
 	if (cmd->error == 0) {
 		if (blk_pc_request(rq)) {
-			if (cmd->act_len < blk_rq_bytes(rq))
-				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
+			if (cmd->act_len >= rq->resid_len)
+				rq->resid_len = 0;
+			else
+				rq->resid_len -= cmd->act_len;
 			scsi_status = 0;
 		} else {
 			if (cmd->act_len != cmd->len) {
#11 lore raw

PATCH 2/3 block: set rq->resid_len to blk_rq_bytes() on issue

In reply to Tejun Heo

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing rq->resid_len, the default value of residue count was changed from full count to zero. The conversion was done under the assumption that when a request fails residue count wasn't defined. However, Boaz and James pointed out that this wasn't true and the residue count should be preserved for failed requests too.

This patchset restores the original behavior by setting rq->resid_len to blk_rq_bytes(rq) on issue and restoring explicit clearing in affected drivers. While at it, take advantage of the fact that rq->resid_len is set to full count where applicable.

* ide-cd: rq->resid_len cleared on pc success

* mptsas: req->resid_len cleared on success

* sas_expander: rsp/req->resid_len cleared on success

* mpt2sas_transport: req->resid_len cleared on success

* ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take advantage of initial full count to simplify code

Signed-off-by Tejun Heo <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Boaz Harrosh <[email protected]>
Cc James Bottomley <[email protected]>
Cc Pete Zaitcev <[email protected]>
Cc Bartlomiej Zolnierkiewicz <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc Eric Moore <[email protected]>
Cc Darrick J. Wong <[email protected]>

---
ub updated per Pete's comment.

Thanks.

 block/blk-core.c                         |    1 +
 drivers/block/ub.c                       |    6 ++++--
 drivers/ide/ide-cd.c                     |    4 ++--
 drivers/ide/ide-tape.c                   |    2 +-
 drivers/message/fusion/mptsas.c          |    3 ++-
 drivers/scsi/libsas/sas_expander.c       |    4 ++++
 drivers/scsi/libsas/sas_host_smp.c       |    3 ---
 drivers/scsi/mpt2sas/mpt2sas_transport.c |    4 ++--
 8 files changed, 16 insertions(+), 11 deletions(-)
block/blk-core.c +1 -0
Index: block/block/blk-core.c
===================================================================
@@ -2198,6 +2198,7 @@ void blk_rq_bio_prep(struct request_queu
 		rq->buffer = bio_data(bio);
 	}
 	rq->__data_len = bio->bi_size;
+	rq->resid_len = bio->bi_size;
 	rq->bio = rq->biotail = bio;
 
 	if (bio->bi_bdev)
drivers/ide/ide-cd.c +2 -2
Index: block/drivers/ide/ide-cd.c
===================================================================
@@ -699,6 +699,7 @@ static ide_startstop_t cdrom_newpc_intr(
 
 out_end:
 	if (blk_pc_request(rq) && rc == 0) {
+		rq->resid_len = 0;
 		blk_end_request_all(rq, 0);
 		hwif->rq = NULL;
 	} else {

@@ -718,8 +719,7 @@ out_end:
 
 		/* make sure it's fully ended */
 		if (blk_fs_request(rq) == 0) {
-			rq->resid_len = blk_rq_bytes(rq) -
-				(cmd->nbytes - cmd->nleft);
+			rq->resid_len -= cmd->nbytes - cmd->nleft;
 			if (uptodate == 0 && (cmd->tf_flags & IDE_TFLAG_WRITE))
 				rq->resid_len += cmd->last_xfer_len;
 		}
drivers/ide/ide-tape.c +1 -1
Index: block/drivers/ide/ide-tape.c
===================================================================
@@ -380,7 +380,7 @@ static int ide_tape_callback(ide_drive_t
 		}
 
 		tape->first_frame += blocks;
-		rq->resid_len = blk_rq_bytes(rq) - blocks * tape->blk_size;
+		rq->resid_len -= blocks * tape->blk_size;
 
 		if (pc->error) {
 			uptodate = 0;
drivers/message/fusion/mptsas.c +2 -1
Index: block/drivers/message/fusion/mptsas.c
===================================================================
@@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
 		smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
 		memcpy(req->sense, smprep, sizeof(*smprep));
 		req->sense_len = sizeof(*smprep);
-		rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= smprep->ResponseDataLength;
 	} else {
 		printk(MYIOC_s_ERR_FMT "%s: smp passthru reply failed to be returned\n",
 		    ioc->name, __func__);
drivers/scsi/libsas/sas_expander.c +4 -0
Index: block/drivers/scsi/libsas/sas_expander.c
===================================================================
@@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
 	if (ret > 0) {
 		/* positive number is the untransferred residual */
 		rsp->resid_len = ret;
+		req->resid_len = 0;
 		ret = 0;
+	} else if (ret == 0) {
+		rsp->resid_len = 0;
+		req->resid_len = 0;
 	}
 
 	return ret;
drivers/scsi/libsas/sas_host_smp.c +0 -3
Index: block/drivers/scsi/libsas/sas_host_smp.c
===================================================================
@@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
 	resp_data[1] = req_data[1];
 	resp_data[2] = SMP_RESP_FUNC_UNK;
 
-	req->resid_len = blk_rq_bytes(req);
-	rsp->resid_len = blk_rq_bytes(rsp);
-
 	switch (req_data[1]) {
 	case SMP_REPORT_GENERAL:
 		req->resid_len -= 8;
drivers/scsi/mpt2sas/mpt2sas_transport.c +2 -2
Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
===================================================================
@@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
 
 		memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
 		req->sense_len = sizeof(*mpi_reply);
-		rsp->resid_len = blk_rq_bytes(rsp) -
-				 mpi_reply->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= mpi_reply->ResponseDataLength;
 	} else {
 		dtransportprintk(ioc, printk(MPT2SAS_DEBUG_FMT
 		    "%s - no reply\n", ioc->name, __func__));
drivers/block/ub.c +4 -2
Index: block/drivers/block/ub.c
===================================================================
@@ -781,8 +781,10 @@ static void ub_rw_cmd_done(struct ub_dev
 
 	if (cmd->error == 0) {
 		if (blk_pc_request(rq)) {
-			if (cmd->act_len < blk_rq_bytes(rq))
-				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
+			if (cmd->act_len >= rq->resid_len)
+				rq->resid_len = 0;
+			else
+				rq->resid_len -= cmd->act_len;
 			scsi_status = 0;
 		} else {
 			if (cmd->act_len != cmd->len) {
#12 lore raw

PATCH 3/3 bio: always copy back data for copied kernel requests

In reply to Tejun Heo

When a read bio_copy_kern() request fails, the content of the bounce buffer is not copied back. However, as request failure doesn't necessarily mean complete failure, the buffer state can be useful. This behavior is also inconsistent with the user map counterpart and causes the subtle difference between bounced and unbounced IO causes confusion.

This patch makes bio_copy_kern_endio() ignore @err and always copy back data on request completion.

Signed-off-by Tejun Heo <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Boaz Harrosh <[email protected]>
Cc James Bottomley <[email protected]>
---
 fs/bio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/bio.c +1 -1
Index: block/fs/bio.c
===================================================================
@@ -1198,7 +1198,7 @@ static void bio_copy_kern_endio(struct b
 		char *addr = page_address(bvec->bv_page);
 		int len = bmd->iovecs[i].bv_len;
 
-		if (read && !err)
+		if (read)
 			memcpy(p, addr, len);
 
 		__free_page(bvec->bv_page);
#13 lore raw

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

In reply to Tejun Heo

Hello.

Quoted 16 lines · Tejun Heo

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing rq->resid_len, the default value of residue count was changed from full count to zero. The conversion was done under the assumption that when a request fails residue count wasn't defined. However, Boaz and James pointed out that this wasn't true and the residue count should be preserved for failed requests too.

This patchset restores the original behavior by setting rq->resid_len to blk_rq_bytes(rq) on issue and restoring explicit clearing in affected drivers. While at it, take advantage of the fact that rq->resid_len is set to full count where applicable.

* ide-cd: rq->resid_len cleared on pc success

* mptsas: req->resid_len cleared on success

Quoted 8 lines

* sas_expander: rsp/req->resid_len cleared on success

* mpt2sas_transport: req->resid_len cleared on success

* ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take advantage of initial full count to simplify code

Signed-off-by Tejun Heo <[email protected]>

The patch looks er... strange at some places.

drivers/message/fusion/mptsas.c static int mptsas_smp_handler(struct Scs · line 1361

Quoted 11 lines

Index: block/drivers/message/fusion/mptsas.c
===================================================================

drivers/message/fusion/mptsas.c +2 -1
@@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
 		smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
 		memcpy(req->sense, smprep, sizeof(*smprep));
 		req->sense_len = sizeof(*smprep);
-		rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= smprep->ResponseDataLength;

Is negative resid_len intended here? If so, shouldn't it be simply:

		rsp->resid_len = -smprep->ResponseDataLength;

drivers/scsi/libsas/sas_expander.c int sas_smp_handler(struct Scsi_Host *sh · line 1944

Quoted 13 lines

Index: block/drivers/scsi/libsas/sas_expander.c
===================================================================

drivers/scsi/libsas/sas_expander.c +4 -0
@@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
 	if (ret > 0) {
 		/* positive number is the untransferred residual */
 		rsp->resid_len = ret;
+		req->resid_len = 0;
 		ret = 0;
+	} else if (ret == 0) {
+		rsp->resid_len = 0;
+		req->resid_len = 0;

???

drivers/scsi/libsas/sas_host_smp.c int sas_smp_host_handler(struct Scsi_Hos · line 181

Quoted 11 lines

Index: block/drivers/scsi/libsas/sas_host_smp.c
===================================================================

drivers/scsi/libsas/sas_host_smp.c +0 -3
@@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
 	resp_data[1] = req_data[1];
 	resp_data[2] = SMP_RESP_FUNC_UNK;
 
-	req->resid_len = blk_rq_bytes(req);
-	rsp->resid_len = blk_rq_bytes(rsp);
-

???

Quoted 8 lines
drivers/scsi/libsas/sas_host_smp.c +0 -0
 	switch (req_data[1]) {
 	case SMP_REPORT_GENERAL:
 		req->resid_len -= 8;

Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
===================================================================

drivers/scsi/mpt2sas/mpt2sas_transport.c transport_smp_handler(struct Scsi_Host * · line 1174

Quoted 7 lines
drivers/scsi/mpt2sas/mpt2sas_transport.c +2 -2
@@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
 
 		memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
 		req->sense_len = sizeof(*mpi_reply);
-		rsp->resid_len = blk_rq_bytes(rsp) -
-				 mpi_reply->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= mpi_reply->ResponseDataLength;

Again, is negative resid_len intended?

MBR, Sergei

#14 lore raw

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

Hello,

Quoted 14 lines · Sergei Shtylyov

drivers/message/fusion/mptsas.c static int mptsas_smp_handler(struct Scs · line 1361

Quoted 11 lines

Index: block/drivers/message/fusion/mptsas.c
===================================================================

drivers/message/fusion/mptsas.c +2 -1
@@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
         smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
         memcpy(req->sense, smprep, sizeof(*smprep));
         req->sense_len = sizeof(*smprep);
-        rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
+        req->resid_len = 0;
+        rsp->resid_len -= smprep->ResponseDataLength;

Is negative resid_len intended here? If so, shouldn't it be simply:

Quoted 3 lines
        rsp->resid_len = -smprep->ResponseDataLength;

From patch description.

This patchset restores the original behavior by setting rq->resid_len to blk_rq_bytes(rq) on issue and restoring explicit clearing in affected drivers.

So, rsp->resid_len equals the initial request length before the subtraction and after subtraction it becomes the residue count. The original code was

	req->data_len = 0;
	rsp->data_len -= smprep->ResponseDataLength;
Quoted 5 lines
Quoted 5 lines

Index: block/drivers/scsi/libsas/sas_expander.c
===================================================================

drivers/scsi/libsas/sas_expander.c +0 -0
@@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
Quoted 8 lines

drivers/scsi/libsas/sas_expander.c int sas_smp_handler(struct Scsi_Host *sh · line 1944

Quoted 8 lines
drivers/scsi/libsas/sas_expander.c +4 -0
     if (ret > 0) {
         /* positive number is the untransferred residual */
         rsp->resid_len = ret;
+        req->resid_len = 0;
         ret = 0;
+    } else if (ret == 0) {
+        rsp->resid_len = 0;
+        req->resid_len = 0;

Heh... there's a reason I mentioned the original commit. The original code was

	if (ret > 0) {
		/* positive number is the untransferred residual */
		rsp->data_len = ret;
		req->data_len = 0;
		ret = 0;
	} else if (ret == 0) {
		rsp->data_len = 0;
		req->data_len = 0;
	}
Quoted 11 lines

drivers/scsi/libsas/sas_host_smp.c int sas_smp_host_handler(struct Scsi_Hos · line 181

Quoted 11 lines

Index: block/drivers/scsi/libsas/sas_host_smp.c
===================================================================

drivers/scsi/libsas/sas_host_smp.c +0 -3
@@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
     resp_data[1] = req_data[1];
     resp_data[2] = SMP_RESP_FUNC_UNK;
 
-    req->resid_len = blk_rq_bytes(req);
-    rsp->resid_len = blk_rq_bytes(rsp);
-

Cuz it's already set to blk_rq_bytes().

Quoted 9 lines
Quoted 9 lines
drivers/scsi/libsas/sas_host_smp.c +0 -0
     switch (req_data[1]) {
     case SMP_REPORT_GENERAL:
         req->resid_len -= 8;

Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
===================================================================

drivers/scsi/mpt2sas/mpt2sas_transport.c +0 -0
@@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
Quoted 8 lines

drivers/scsi/mpt2sas/mpt2sas_transport.c transport_smp_handler(struct Scsi_Host * · line 1174

Quoted 6 lines
drivers/scsi/mpt2sas/mpt2sas_transport.c +2 -2
         memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
         req->sense_len = sizeof(*mpi_reply);
-        rsp->resid_len = blk_rq_bytes(rsp) -
-                 mpi_reply->ResponseDataLength;
+        req->resid_len = 0;
+        rsp->resid_len -= mpi_reply->ResponseDataLength;

Again, is negative resid_len intended?

Ditto.

Thanks.

Signature · 1 line
tejun
#15 lore raw

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

Hello.

Quoted 23 lines · Tejun Heo
Quoted 14 lines

drivers/message/fusion/mptsas.c static int mptsas_smp_handler(struct Scs · line 1362

Quoted 12 lines

Index: block/drivers/message/fusion/mptsas.c
===================================================================

drivers/message/fusion/mptsas.c +2 -1
@@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
         smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
         memcpy(req->sense, smprep, sizeof(*smprep));
         req->sense_len = sizeof(*smprep);
-        rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
+        req->resid_len = 0;
+        rsp->resid_len -= smprep->ResponseDataLength;

Is negative resid_len intended here? If so, shouldn't it be simply:

Quoted 2 lines
        rsp->resid_len = -smprep->ResponseDataLength;

From patch description.

This patchset restores the original behavior by setting rq->resid_len to blk_rq_bytes(rq) on issue and restoring explicit clearing in affected drivers.

Quoted 7 lines

So, rsp->resid_len equals the initial request length before the subtraction and after subtraction it becomes the residue count. The original code was

	req->data_len = 0;
	rsp->data_len -= smprep->ResponseDataLength;

Ah, I've confused up 'rsp' and 'req' as being one thing. Nevermind
then. :-<

Quoted 18 lines
Quoted 6 lines
Quoted 6 lines

Index: block/drivers/scsi/libsas/sas_expander.c
===================================================================

drivers/scsi/libsas/sas_expander.c +0 -0
@@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
     if (ret > 0) {
Quoted 8 lines

drivers/scsi/libsas/sas_expander.c int sas_smp_handler(struct Scsi_Host *sh · line 1946

Quoted 8 lines
drivers/scsi/libsas/sas_expander.c +4 -0
         /* positive number is the untransferred residual */
         rsp->resid_len = ret;
+        req->resid_len = 0;
         ret = 0;
+    } else if (ret == 0) {
+        rsp->resid_len = 0;
+        req->resid_len = 0;

Heh... there's a reason I mentioned the original commit. The original code was

Quoted 10 lines
	if (ret > 0) {
		/* positive number is the untransferred residual */
		rsp->data_len = ret;
		req->data_len = 0;
		ret = 0;
	} else if (ret == 0) {
		rsp->data_len = 0;
		req->data_len = 0;
	}

But still,

               req->data_len = 0;

is common between both branches, so could be moved after the *if* statement.

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines

Index: block/drivers/scsi/libsas/sas_host_smp.c
===================================================================

drivers/scsi/libsas/sas_host_smp.c +0 -0
@@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
     resp_data[1] = req_data[1];
     resp_data[2] = SMP_RESP_FUNC_UNK;
Quoted 7 lines
Quoted 4 lines

drivers/scsi/libsas/sas_host_smp.c int sas_smp_host_handler(struct Scsi_Hos

Quoted 4 lines
drivers/scsi/libsas/sas_host_smp.c +0 -3
-    req->resid_len = blk_rq_bytes(req);
-    rsp->resid_len = blk_rq_bytes(rsp);
-

Cuz it's already set to blk_rq_bytes().

Mixed them up again... :-<

Quoted 19 lines
Quoted 9 lines
Quoted 9 lines
drivers/scsi/libsas/sas_host_smp.c +0 -0
     switch (req_data[1]) {
     case SMP_REPORT_GENERAL:

         req->resid_len -= 8;

Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
===================================================================

drivers/scsi/mpt2sas/mpt2sas_transport.c +0 -0
@@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
Quoted 9 lines

drivers/scsi/mpt2sas/mpt2sas_transport.c transport_smp_handler(struct Scsi_Host *

Quoted 7 lines
drivers/scsi/mpt2sas/mpt2sas_transport.c +2 -2
         memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
         req->sense_len = sizeof(*mpi_reply);
-        rsp->resid_len = blk_rq_bytes(rsp) -
-                 mpi_reply->ResponseDataLength;
+        req->resid_len = 0;
+        rsp->resid_len -= mpi_reply->ResponseDataLength;

Again, is negative resid_len intended?

Quoted 2 lines

Ditto.

... and again.

Quoted 2 lines

Thanks.

WBR, Sergei

#16 lore raw

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

Hello, Sergei.

Quoted 14 lines · Sergei Shtylyov
Quoted 13 lines

Heh... there's a reason I mentioned the original commit. The original code was

    if (ret > 0) {
        /* positive number is the untransferred residual */
        rsp->data_len = ret;
        req->data_len = 0;
        ret = 0;
    } else if (ret == 0) {
        rsp->data_len = 0;
        req->data_len = 0;
    }
Quoted 6 lines

But still,

               req->data_len = 0;

is common between both branches, so could be moved after the *if* statement.

Yeah, sure, feel free to submit a patch, but I don't know. Does it even matter at all as long as the intention is clear?

Thanks.

Signature · 1 line
tejun
#17 lore raw

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

In reply to Tejun Heo
Quoted 3 lines · Tejun Heo

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing rq->resid_len, the default value of residue count was changed from full count to zero. []

So it's not a residue anymore, right? You should've renamed it to rq->count or something, then. Now we have this:

drivers/block/ub.c static void ub_rw_cmd_done(struct ub_dev · line 785

Quoted 9 lines

+++ block/drivers/block/ub.c

drivers/block/ub.c +1 -2
@@ -781,8 +781,7 @@ static void ub_rw_cmd_done(struct ub_dev
 
 	if (cmd->error == 0) {
 		if (blk_pc_request(rq)) {
-			if (cmd->act_len < blk_rq_bytes(rq))
-				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
+			rq->resid_len -= min(cmd->act_len, rq->resid_len);
 			scsi_status = 0;

You are subtracting resid_len from itself. Just how in the world can this be correct?

Even it if is, in fact, correct, it's such an eggregious violation of good style, that your good programmer's card is going to lose a big coupon and have a hole punched in it.

This is not in Linus' tree yet, but I'm going to take a hard look at this once it shows up.

-- Pete

#18 lore raw

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

Quoted 8 lines · Pete Zaitcev
Quoted 3 lines · Tejun Heo

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing rq->resid_len, the default value of residue count was changed from full count to zero. []

So it's not a residue anymore, right? You should've renamed it to rq->count or something, then. Now we have this:

It still is. It just is restoring the original behavior.

Quoted 13 lines

drivers/block/ub.c static void ub_rw_cmd_done(struct ub_dev · line 785

Quoted 9 lines

+++ block/drivers/block/ub.c

drivers/block/ub.c +1 -2
@@ -781,8 +781,7 @@ static void ub_rw_cmd_done(struct ub_dev
 
 	if (cmd->error == 0) {
 		if (blk_pc_request(rq)) {
-			if (cmd->act_len < blk_rq_bytes(rq))
-				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
+			rq->resid_len -= min(cmd->act_len, rq->resid_len);
 			scsi_status = 0;

You are subtracting resid_len from itself. Just how in the world can this be correct?

Quoted 3 lines

Even it if is, in fact, correct, it's such an eggregious violation of good style, that your good programmer's card is going to lose a big coupon and have a hole punched in it.

The original code was

if (cmd->act_len >= rq->data_len)

	rq->data_len = 0;
 else
	rq->data_len -= cmd->act_len

So, I could have written

if (cmd->act_len >= rq->resid_len)

	rq->resid_len = 0;
 else
	rq->resid_len -= cmd->act_len

Instead I wrote

rq->resid_len -= min(cmd->act_len, rq->resid_len);

It's just capping the amount to be subtracted so that resid_len doesn't underflow. What is so wrong or bad style about that?

Quoted 2 lines

This is not in Linus' tree yet, but I'm going to take a hard look at this once it shows up.

It would be great if you do before it hits Linus's tree.

Thanks.

Signature · 1 line
tejun
#19 lore raw

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

Quoted 7 lines · Tejun Heo

So, I could have written

if (cmd->act_len >= rq->resid_len)

	rq->resid_len = 0;
 else
	rq->resid_len -= cmd->act_len
Quoted 6 lines

Instead I wrote

rq->resid_len -= min(cmd->act_len, rq->resid_len);

It's just capping the amount to be subtracted so that resid_len doesn't underflow. What is so wrong or bad style about that?

Curse of the gifted, I guess. To use a subtraction instead of zero this way looks like a pointless, even mischievous obfuscation to me.

Also, we probably want a stack_dump or a printk when actual length exceeds the requested length, don't we? If it ever happens, we might be overwriting some I/O buffer somewhere.

-- Pete

#20 lore raw

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

Hello,

Quoted 9 lines · Pete Zaitcev
Quoted 9 lines

So, I could have written

if (cmd->act_len >= rq->resid_len)

	rq->resid_len = 0;
 else
	rq->resid_len -= cmd->act_len

Instead I wrote

Quoted 7 lines
Quoted 4 lines

rq->resid_len -= min(cmd->act_len, rq->resid_len);

It's just capping the amount to be subtracted so that resid_len doesn't underflow. What is so wrong or bad style about that?

Curse of the gifted, I guess. To use a subtraction instead of zero this way looks like a pointless, even mischievous obfuscation to me.

Ummm... I don't know. I prefer min/max over if/else when capping values. To me, it makes the intention clearer but you're the maintainer and don't like the style, so I'll update the patch so that it has the if/else clause. :-)

Quoted 3 lines

Also, we probably want a stack_dump or a printk when actual length exceeds the requested length, don't we? If it ever happens, we might be overwriting some I/O buffer somewhere.

It depends on particular implementation. Transport overflow doesn't necessarily become actual buffer overflow depending on hardware and driver implementation. If you think the user needs to be warned about transport overflow, please go ahead and add a warning there.

Thanks.

Signature · 1 line
tejun
#21 lore raw

PATCH 3/3 bio: always copy back data for copied kernel requests

In reply to Tejun Heo

When a read bio_copy_kern() request fails, the content of the bounce buffer is not copied back. However, as request failure doesn't necessarily mean complete failure, the buffer state can be useful. This behavior is also inconsistent with the user map counterpart and causes the subtle difference between bounced and unbounced IO causes confusion.

This patch makes bio_copy_kern_endio() ignore @err and always copy back data on request completion.

Signed-off-by Tejun Heo <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Boaz Harrosh <[email protected]>
Cc James Bottomley <[email protected]>
---
 fs/bio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/bio.c +1 -1
Index: block/fs/bio.c
===================================================================
@@ -1198,7 +1198,7 @@ static void bio_copy_kern_endio(struct b
 		char *addr = page_address(bvec->bv_page);
 		int len = bmd->iovecs[i].bv_len;
 
-		if (read && !err)
+		if (read)
 			memcpy(p, addr, len);
 
 		__free_page(bvec->bv_page);
#22 lore raw

PATCH 3/3 bio: always copy back data for copied kernel requests

In reply to Tejun Heo

When a read bio_copy_kern() request fails, the content of the bounce buffer is not copied back. However, as request failure doesn't necessarily mean complete failure, the buffer state can be useful. This behavior is also inconsistent with the user map counterpart and causes the subtle difference between bounced and unbounced IO causes confusion.

This patch makes bio_copy_kern_endio() ignore @err and always copy back data on request completion.

Signed-off-by Tejun Heo <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Boaz Harrosh <[email protected]>
Cc James Bottomley <[email protected]>
---
 fs/bio.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
fs/bio.c +1 -1
Index: block/fs/bio.c
===================================================================
@@ -1198,7 +1198,7 @@ static void bio_copy_kern_endio(struct b
 		char *addr = page_address(bvec->bv_page);
 		int len = bmd->iovecs[i].bv_len;
 
-		if (read && !err)
+		if (read)
 			memcpy(p, addr, len);
 
 		__free_page(bvec->bv_page);
Acked-by on v1 2/3
#23 lore raw

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

In reply to Tejun Heo
Quoted 14 lines · Tejun Heo

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing rq->resid_len, the default value of residue count was changed from full count to zero. The conversion was done under the assumption that when a request fails residue count wasn't defined. However, Boaz and James pointed out that this wasn't true and the residue count should be preserved for failed requests too.

This patchset restores the original behavior by setting rq->resid_len to blk_rq_bytes(rq) on issue and restoring explicit clearing in affected drivers. While at it, take advantage of the fact that rq->resid_len is set to full count where applicable.

* ide-cd: rq->resid_len cleared on pc success

Quoted 8 lines

* mptsas: req->resid_len cleared on success

* sas_expander: rsp/req->resid_len cleared on success

* mpt2sas_transport: req->resid_len cleared on success

* ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take advantage of initial full count to simplify code

ACK the ide-{cd,tape} bits.

Acked-by Borislav Petkov <[email protected]>

I've converted the ide-atapi part to rq->resid_len and from what I see, the changes are compatible with the latest semantics of rq->resid_len but it won't hurt if you could take a look too :) :

http://git.kernel.org/?p=linux/kernel/git/bp/bp.git;a=commit;h=077e6dba20e74a455a0452379d2a965c7e1b01ad

On a side note, ide_pc_intr() does some transfer padding but that doesn't affect rq->resid_len returning the remaining bytecount back up to block layer.

[..]

Thanks.

Signature · 2 lines
Regards/Gruss,
    Boris.
#24 lore raw

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

Hello, Borislav.

Quoted 21 lines · Borislav Petkov
Quoted 20 lines · Tejun Heo

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing rq->resid_len, the default value of residue count was changed from full count to zero. The conversion was done under the assumption that when a request fails residue count wasn't defined. However, Boaz and James pointed out that this wasn't true and the residue count should be preserved for failed requests too.

This patchset restores the original behavior by setting rq->resid_len to blk_rq_bytes(rq) on issue and restoring explicit clearing in affected drivers. While at it, take advantage of the fact that rq->resid_len is set to full count where applicable.

* ide-cd: rq->resid_len cleared on pc success

* mptsas: req->resid_len cleared on success

* sas_expander: rsp/req->resid_len cleared on success

* mpt2sas_transport: req->resid_len cleared on success

Quoted 8 lines
Quoted 2 lines

* ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take advantage of initial full count to simplify code

ACK the ide-{cd,tape} bits.

Acked-by Borislav Petkov <[email protected]>

I've converted the ide-atapi part to rq->resid_len and from what I see,

Sorry it changed underneath your changes.

Quoted 4 lines

the changes are compatible with the latest semantics of rq->resid_len but it won't hurt if you could take a look too :) :

http://git.kernel.org/?p=linux/kernel/git/bp/bp.git;a=commit;h=077e6dba20e74a455a0452379d2a965c7e1b01ad

Yeap, I'll take a look.

Quoted 3 lines

On a side note, ide_pc_intr() does some transfer padding but that doesn't affect rq->resid_len returning the remaining bytecount back up to block layer.

Hmmm... telling the issuer the actual number of transferred bytes sans padding is the correct behavior, I think.

Thanks.

Signature · 1 line
tejun
#25 lore raw

Re: [PATCH block#for-2.6.31 2/3] block: set rq->resid_len to blk_rq_bytes() on issue

In reply to Borislav Petkov

Hello, Borislav.

Quoted 21 lines · Borislav Petkov
Quoted 20 lines · Tejun Heo

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing rq->resid_len, the default value of residue count was changed from full count to zero. The conversion was done under the assumption that when a request fails residue count wasn't defined. However, Boaz and James pointed out that this wasn't true and the residue count should be preserved for failed requests too.

This patchset restores the original behavior by setting rq->resid_len to blk_rq_bytes(rq) on issue and restoring explicit clearing in affected drivers. While at it, take advantage of the fact that rq->resid_len is set to full count where applicable.

* ide-cd: rq->resid_len cleared on pc success

* mptsas: req->resid_len cleared on success

* sas_expander: rsp/req->resid_len cleared on success

* mpt2sas_transport: req->resid_len cleared on success

Quoted 8 lines
Quoted 2 lines

* ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take advantage of initial full count to simplify code

ACK the ide-{cd,tape} bits.

Acked-by Borislav Petkov <[email protected]>

I've converted the ide-atapi part to rq->resid_len and from what I see,

Sorry it changed underneath your changes.

Quoted 4 lines

the changes are compatible with the latest semantics of rq->resid_len but it won't hurt if you could take a look too :) :

http://git.kernel.org/?p=linux/kernel/git/bp/bp.git;a=commit;h=077e6dba20e74a455a0452379d2a965c7e1b01ad

Yeap, I'll take a look.

Quoted 3 lines

On a side note, ide_pc_intr() does some transfer padding but that doesn't affect rq->resid_len returning the remaining bytecount back up to block layer.

Hmmm... telling the issuer the actual number of transferred bytes sans padding is the correct behavior, I think.

Thanks.

Signature · 1 line
tejun
#26 lore raw

PATCH 2/3 block: set rq->resid_len to blk_rq_bytes() on issue

In reply to Tejun Heo

In commit c3a4d78c580de4edc9ef0f7c59812fb02ceb037f, while introducing rq->resid_len, the default value of residue count was changed from full count to zero. The conversion was done under the assumption that when a request fails residue count wasn't defined. However, Boaz and James pointed out that this wasn't true and the residue count should be preserved for failed requests too.

This patchset restores the original behavior by setting rq->resid_len to blk_rq_bytes(rq) on issue and restoring explicit clearing in affected drivers. While at it, take advantage of the fact that rq->resid_len is set to full count where applicable.

* ide-cd: rq->resid_len cleared on pc success

* mptsas: req->resid_len cleared on success

* sas_expander: rsp/req->resid_len cleared on success

* mpt2sas_transport: req->resid_len cleared on success

* ide-cd, ide-tape, mptsas, sas_host_smp, mpt2sas_transport, ub: take advantage of initial full count to simplify code

Signed-off-by Tejun Heo <[email protected]>
Cc Jens Axboe <[email protected]>
Cc Boaz Harrosh <[email protected]>
Cc James Bottomley <[email protected]>
Cc Pete Zaitcev <[email protected]>
Cc Bartlomiej Zolnierkiewicz <[email protected]>
Cc Borislav Petkov <[email protected]>
Cc Sergei Shtylyov <[email protected]>
Cc Eric Moore <[email protected]>
Cc Darrick J. Wong <[email protected]>
---
 block/blk-core.c                         |    1 +
 drivers/block/ub.c                       |    3 +--
 drivers/ide/ide-cd.c                     |    4 ++--
 drivers/ide/ide-tape.c                   |    2 +-
 drivers/message/fusion/mptsas.c          |    3 ++-
 drivers/scsi/libsas/sas_expander.c       |    4 ++++
 drivers/scsi/libsas/sas_host_smp.c       |    3 ---
 drivers/scsi/mpt2sas/mpt2sas_transport.c |    4 ++--
 8 files changed, 13 insertions(+), 11 deletions(-)
block/blk-core.c +1 -0
Index: block/block/blk-core.c
===================================================================
@@ -2198,6 +2198,7 @@ void blk_rq_bio_prep(struct request_queu
 		rq->buffer = bio_data(bio);
 	}
 	rq->__data_len = bio->bi_size;
+	rq->resid_len = bio->bi_size;
 	rq->bio = rq->biotail = bio;
 
 	if (bio->bi_bdev)
drivers/ide/ide-cd.c +2 -2
Index: block/drivers/ide/ide-cd.c
===================================================================
@@ -699,6 +699,7 @@ static ide_startstop_t cdrom_newpc_intr(
 
 out_end:
 	if (blk_pc_request(rq) && rc == 0) {
+		rq->resid_len = 0;
 		blk_end_request_all(rq, 0);
 		hwif->rq = NULL;
 	} else {

@@ -718,8 +719,7 @@ out_end:
 
 		/* make sure it's fully ended */
 		if (blk_fs_request(rq) == 0) {
-			rq->resid_len = blk_rq_bytes(rq) -
-				(cmd->nbytes - cmd->nleft);
+			rq->resid_len -= cmd->nbytes - cmd->nleft;
 			if (uptodate == 0 && (cmd->tf_flags & IDE_TFLAG_WRITE))
 				rq->resid_len += cmd->last_xfer_len;
 		}
drivers/ide/ide-tape.c +1 -1
Index: block/drivers/ide/ide-tape.c
===================================================================
@@ -380,7 +380,7 @@ static int ide_tape_callback(ide_drive_t
 		}
 
 		tape->first_frame += blocks;
-		rq->resid_len = blk_rq_bytes(rq) - blocks * tape->blk_size;
+		rq->resid_len -= blocks * tape->blk_size;
 
 		if (pc->error) {
 			uptodate = 0;
drivers/message/fusion/mptsas.c +2 -1
Index: block/drivers/message/fusion/mptsas.c
===================================================================
@@ -1357,7 +1357,8 @@ static int mptsas_smp_handler(struct Scs
 		smprep = (SmpPassthroughReply_t *)ioc->sas_mgmt.reply;
 		memcpy(req->sense, smprep, sizeof(*smprep));
 		req->sense_len = sizeof(*smprep);
-		rsp->resid_len = blk_rq_bytes(rsp) - smprep->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= smprep->ResponseDataLength;
 	} else {
 		printk(MYIOC_s_ERR_FMT "%s: smp passthru reply failed to be returned\n",
 		    ioc->name, __func__);
drivers/scsi/libsas/sas_expander.c +4 -0
Index: block/drivers/scsi/libsas/sas_expander.c
===================================================================
@@ -1937,7 +1937,11 @@ int sas_smp_handler(struct Scsi_Host *sh
 	if (ret > 0) {
 		/* positive number is the untransferred residual */
 		rsp->resid_len = ret;
+		req->resid_len = 0;
 		ret = 0;
+	} else if (ret == 0) {
+		rsp->resid_len = 0;
+		req->resid_len = 0;
 	}
 
 	return ret;
drivers/scsi/libsas/sas_host_smp.c +0 -3
Index: block/drivers/scsi/libsas/sas_host_smp.c
===================================================================
@@ -176,9 +176,6 @@ int sas_smp_host_handler(struct Scsi_Hos
 	resp_data[1] = req_data[1];
 	resp_data[2] = SMP_RESP_FUNC_UNK;
 
-	req->resid_len = blk_rq_bytes(req);
-	rsp->resid_len = blk_rq_bytes(rsp);
-
 	switch (req_data[1]) {
 	case SMP_REPORT_GENERAL:
 		req->resid_len -= 8;
drivers/scsi/mpt2sas/mpt2sas_transport.c +2 -2
Index: block/drivers/scsi/mpt2sas/mpt2sas_transport.c
===================================================================
@@ -1170,8 +1170,8 @@ transport_smp_handler(struct Scsi_Host *
 
 		memcpy(req->sense, mpi_reply, sizeof(*mpi_reply));
 		req->sense_len = sizeof(*mpi_reply);
-		rsp->resid_len = blk_rq_bytes(rsp) -
-				 mpi_reply->ResponseDataLength;
+		req->resid_len = 0;
+		rsp->resid_len -= mpi_reply->ResponseDataLength;
 	} else {
 		dtransportprintk(ioc, printk(MPT2SAS_DEBUG_FMT
 		    "%s - no reply\n", ioc->name, __func__));
drivers/block/ub.c +1 -2
Index: block/drivers/block/ub.c
===================================================================
@@ -781,8 +781,7 @@ static void ub_rw_cmd_done(struct ub_dev
 
 	if (cmd->error == 0) {
 		if (blk_pc_request(rq)) {
-			if (cmd->act_len < blk_rq_bytes(rq))
-				rq->resid_len = blk_rq_bytes(rq) - cmd->act_len;
+			rq->resid_len -= min(cmd->act_len, rq->resid_len);
 			scsi_status = 0;
 		} else {
 			if (cmd->act_len != cmd->len) {
#27 lore raw
In reply to Tejun Heo

drivers/block/ub.c static void ub_end_rq(struct request *rq · line 833

Quoted 12 lines · Tejun Heo
drivers/block/ub.c +1 -6
@@ -834,12 +829,7 @@ static void ub_end_rq(struct request *rq
 		error = -EIO;
 		rq->errors = scsi_status;
 	}
-	rqlen = blk_rq_bytes(rq);    /* Oddly enough, this is the residue. */
-	if (__blk_end_request(rq, error, cmd_len)) {
-		printk(KERN_WARNING DRV_NAME
-		    ": __blk_end_request blew, %s-cmd total %u rqlen %ld\n",
-		    blk_pc_request(rq)? "pc": "fs", cmd_len, rqlen);
-	}
+	__blk_end_request_all(rq, error);
 }

I applaud this. We needed blk_end_this_mudafakin_request() for a long time. Why two underscores?

-- Pete

author
#28 lore raw
Quoted 17 lines · Pete Zaitcev

drivers/block/ub.c static void ub_end_rq(struct request *rq · line 833

Quoted 12 lines · Tejun Heo
drivers/block/ub.c +1 -6
@@ -834,12 +829,7 @@ static void ub_end_rq(struct request *rq
 		error = -EIO;
 		rq->errors = scsi_status;
 	}
-	rqlen = blk_rq_bytes(rq);    /* Oddly enough, this is the residue. */
-	if (__blk_end_request(rq, error, cmd_len)) {
-		printk(KERN_WARNING DRV_NAME
-		    ": __blk_end_request blew, %s-cmd total %u rqlen %ld\n",
-		    blk_pc_request(rq)? "pc": "fs", cmd_len, rqlen);
-	}
+	__blk_end_request_all(rq, error);
 }

I applaud this. We needed blk_end_this_mudafakin_request() for a long time. Why two underscores?

The ones with two underscores are to be called with queue lock held. Ones without grab queue lock themselves.

Thanks.

Signature · 1 line
tejun