scsi: unify the error handling of the prep functions

PATCH In review FUJITA Tomonori 4 participants 8 messages Jul 5, 2010 – Jul 8, 2010

Subsystems (2)

SCSI Device mapper

lore

Share & tools

lore

FUJITA Tomonori

author

2010-07-05 04:00 UTC #0 lore raw

PATCH 1/1 scsi: unify the error handling of the prep functions

This can be applied to the block's for-2.6.36.

=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] scsi: unify the error handling of the prep functions

This unifies the error handling of the prep functions (and fix the leak of a page allocated for discard in the case of BLKPREP_KILL or BLK_PREP_DEFER).

The error handling of the prep path is very messy. Some errors are handled in the prep functions while some are in scsi_prep_return().

Let's handle all the errors in scsi_prep_return().

Signed-off-by FUJITA Tomonori <[email protected]>

---
 drivers/scsi/scsi_lib.c |   26 +++++++++++++-------------
 drivers/scsi/sd.c       |    6 ++++--
 2 files changed, 17 insertions(+), 15 deletions(-)

drivers/scsi/scsi_lib.c +13 ~~-13~~

index ee83619..f9b3f7b 100644
@@ -1008,14 +1008,7 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 	}
 
 	return BLKPREP_OK ;
-
 err_exit:
-	scsi_release_buffers(cmd);
-	if (error == BLKPREP_KILL)
-		scsi_put_command(cmd);
-	else /* BLKPREP_DEFER */
-		scsi_unprep_request(cmd->request);
-
 	return error;
 }
 EXPORT_SYMBOL(scsi_init_io);

@@ -1177,6 +1170,17 @@ int scsi_prep_state_check(struct scsi_device *sdev, struct request *req)
 }
 EXPORT_SYMBOL(scsi_prep_state_check);
 
+static void scsi_prep_error(struct request *req)
+{
+	struct scsi_cmnd *cmd = req->special;
+
+	if (!cmd)
+		return;
+
+	scsi_release_buffers(cmd);
+	scsi_unprep_request(req);
+}
+
 int scsi_prep_return(struct request_queue *q, struct request *req, int ret)
 {
 	struct scsi_device *sdev = q->queuedata;

@@ -1185,14 +1189,10 @@ int scsi_prep_return(struct request_queue *q, struct request *req, int ret)
 	case BLKPREP_KILL:
 		req->errors = DID_NO_CONNECT << 16;
 		/* release the command and kill it */
-		if (req->special) {
-			struct scsi_cmnd *cmd = req->special;
-			scsi_release_buffers(cmd);
-			scsi_put_command(cmd);
-			req->special = NULL;
-		}
+		scsi_prep_error(req);
 		break;
 	case BLKPREP_DEFER:
+		scsi_prep_error(req);
 		/*
 		 * If we defer, the blk_peek_request() returns NULL, but the
 		 * queue must be restarted, so we plug here if no returning

drivers/scsi/sd.c +4 -2

index aa6b48b..88ba0c5 100644
@@ -473,8 +473,10 @@ static int scsi_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
 
 static void sd_unprep_fn(struct request_queue *q, struct request *rq)
 {
-	if (rq->cmd_flags & REQ_DISCARD)
-		__free_page(virt_to_page(rq->buffer));
+	if (rq->cmd_flags & REQ_DISCARD) {
+		free_page((unsigned long)rq->buffer);
+		rq->buffer = NULL;
+	}
 }
 
 /**

Signature · 1 line

1.6.5

Mike Snitzer

Acked-by on v1 1/1

2010-07-05 13:45 UTC #1 lore raw

Quoted 10 lines · FUJITA Tomonori

This can be applied to the block's for-2.6.36.
=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] scsi: unify the error handling of the prep functions
This unifies the error handling of the prep functions (and fix the leak of a page allocated for discard in the case of BLKPREP_KILL or BLK_PREP_DEFER).

Quoted 6 lines

The error handling of the prep path is very messy. Some errors are handled in the prep functions while some are in scsi_prep_return().
Let's handle all the errors in scsi_prep_return().
Signed-off-by FUJITA Tomonori <[email protected]>

This patch addresses the discard page leak too (scsi_prep_error calls scsi_unprep_request which calls blk_unprep_request).

Acked-by Mike Snitzer <[email protected]>

Mike Snitzer

2010-07-06 13:59 UTC #2 lore raw

PATCH v2 1/1 scsi: address leak in scsi_setup_discard_cmnd error path (was: Re: scsi: unify the error handling of the prep functions)

Quoted 16 lines · Mike Snitzer

Quoted 13 lines · FUJITA Tomonori
This can be applied to the block's for-2.6.36.
=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] scsi: unify the error handling of the prep functions
This unifies the error handling of the prep functions (and fix the leak of a page allocated for discard in the case of BLKPREP_KILL or BLK_PREP_DEFER).
The error handling of the prep path is very messy. Some errors are handled in the prep functions while some are in scsi_prep_return().

Quoted 8 lines

Quoted 3 lines
Let's handle all the errors in scsi_prep_return().
Signed-off-by FUJITA Tomonori <[email protected]>
This patch addresses the discard page leak too (scsi_prep_error calls scsi_unprep_request which calls blk_unprep_request).
Acked-by Mike Snitzer <[email protected]>

Hi,

Your unified error handling forces a BLKPREP_KILL or BLKPREP_DEFER return from scsi_setup_discard_cmnd to depend on prep cleanup even though BLKPREP_OK was not returned. And this works, but it runs counter to the rules James previously shared: http://lkml.org/lkml/2010/7/1/512

"The rules are pretty clear: Unprep is only called if the request gets prepped ... that means you have to return BLKPREP_OK. Defer or kill assume there's no teardown to do, so the allocation (if it took place) must be reversed before returning them."

All this being said, I'm OK with your patch (as my ack implies) but I can see that it will take the request prep error handling in a direction James may prefer to avoid.

Here is a minimalist patch that 1) removes the discard request's page leak 2) preserves the prep cleanup rules covered above. Fixing the leak is a priority, introducing additional error path code sharing/cleanup is secondary and can come later.

=
From: Mike Snitzer <[email protected]>
Subject: [PATCH v2] scsi: address leak in scsi_setup_discard_cmnd error path

Currently, a discard request's page will not get cleaned up in the scsi_setup_discard_cmnd error path. A scsi_setup_discard_cmnd return other than BLKPREP_OK will not cause a discard request to get completely cleaned up (scsi_prep_return will not set REQ_DONTPREP unless BLKPREP_OK was returned).

This fix eliminates the leak while preserving the rule that:
Unprep is only called if the request gets prepped (meaning a return BLKPREP_OK). Defer or kill assume there's no teardown to do, so any allocation must be reversed before returning them.

Signed-off-by Mike Snitzer <[email protected]>

---
 drivers/scsi/sd.c |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

drivers/scsi/sd.c +7 -1

index 0994ab6..08e08bd 100644
@@ -468,6 +468,10 @@ static int scsi_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
 	blk_add_request_payload(rq, page, len);
 	ret = scsi_setup_blk_pc_cmnd(sdp, rq);
 	rq->buffer = page_address(page);
+	if (ret != BLKPREP_OK) {
+		__free_page(page);
+		rq->buffer = NULL;
+	}
 	return ret;
 }

@@ -485,8 +489,10 @@ static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
 
 static void sd_unprep_fn(struct request_queue *q, struct request *rq)
 {
-	if (rq->cmd_flags & REQ_DISCARD)
+	if (rq->cmd_flags & REQ_DISCARD) {
 		__free_page(virt_to_page(rq->buffer));
+		rq->buffer = NULL;
+	}
 }
 
 /**

Christoph Hellwig

Reviewed-by on v2 1/1

2010-07-07 19:44 UTC #3 lore raw

Re: [PATCH v2] scsi: address leak in scsi_setup_discard_cmnd error path (was: Re: scsi: unify the error handling of the prep functions)

Quoted 4 lines

Here is a minimalist patch that 1) removes the discard request's page leak 2) preserves the prep cleanup rules covered above. Fixing the leak is a priority, introducing additional error path code sharing/cleanup is secondary and can come later.

I much prefer this approach.

Quoted 1 line

Signed-off-by Mike Snitzer <[email protected]>

Reviewed-by Christoph Hellwig <[email protected]>

Jens Axboe

2010-07-07 19:46 UTC #4 lore raw

Re: [PATCH v2] scsi: address leak in scsi_setup_discard_cmnd error path

Quoted 6 lines · Christoph Hellwig

Quoted 4 lines
Here is a minimalist patch that 1) removes the discard request's page leak 2) preserves the prep cleanup rules covered above. Fixing the leak is a priority, introducing additional error path code sharing/cleanup is secondary and can come later.
I much prefer this approach.

Same here, it's the correct layering. I'll queue this up, thanks.

Signature · 1 line

Jens Axboe

FUJITA Tomonori

author

2010-07-08 04:17 UTC #5 lore raw

Re: [PATCH v2] scsi: address leak in scsi_setup_discard_cmnd error path (was: Re: scsi: unify the error handling of the prep functions)

In reply to Mike Snitzer

Quoted 4 lines · On Tue, 6 Jul 2010 09:59:58 -0400 Mike Snitzer <[email protected]> wrote:

Here is a minimalist patch that 1) removes the discard request's page leak 2) preserves the prep cleanup rules covered above. Fixing the leak is a priority, introducing additional error path code sharing/cleanup is secondary and can come later.

This patch doesn't work. I hit kernel oops since now this patch frees a discard page twice.

If scsi_init_io hits BLKPREP_DEFER (e.g. due to scsi_init_sgtable), scsi_setup_blk_pc_cmnd calls scsi_unprep_request. So sd_unprep_fn frees a page. Then, scsi_setup_discard_cmnd frees the same page again.

I talked to James on irc, he prefers to strictly apply the rule that we should unprep on only the requests that got prepped.

So we need to remove scsi_unprep_request in scsi_init_io error path. The following patch solves all the issues (hopefully).

=
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] scsi: fix discard page leak

We leak a page allocated for discard on some error conditions (e.g. scsi_prep_state_check returns BLKPREP_DEFER in scsi_setup_blk_pc_cmnd).

We unprep on requests that weren't prepped in the error path of scsi_init_io. It makes the error path to clean up scsi commands messy.

Let's strictly apply the rule that we can't unprep on a request that wasn't prepped.

Calling just scsi_put_command() in the error path of scsi_init_io() is enough. We don't set REQ_DONTPREP yet.

scsi_setup_discard_cmnd can safely free a page on the error case with the above rule.

Signed-off-by FUJITA Tomonori <[email protected]>

---
 drivers/scsi/scsi_lib.c |    7 ++-----
 drivers/scsi/sd.c       |   10 ++++++++--
 2 files changed, 10 insertions(+), 7 deletions(-)

drivers/scsi/scsi_lib.c +2 -5

index ee83619..b8de389 100644
@@ -1011,11 +1011,8 @@ int scsi_init_io(struct scsi_cmnd *cmd, gfp_t gfp_mask)
 
 err_exit:
 	scsi_release_buffers(cmd);
-	if (error == BLKPREP_KILL)
-		scsi_put_command(cmd);
-	else /* BLKPREP_DEFER */
-		scsi_unprep_request(cmd->request);
-
+	scsi_put_command(cmd);
+	cmd->request->special = NULL;
 	return error;
 }
 EXPORT_SYMBOL(scsi_init_io);

drivers/scsi/sd.c +8 -2

index 0994ab6..1d0c4b7 100644
@@ -468,6 +468,10 @@ static int scsi_setup_discard_cmnd(struct scsi_device *sdp, struct request *rq)
 	blk_add_request_payload(rq, page, len);
 	ret = scsi_setup_blk_pc_cmnd(sdp, rq);
 	rq->buffer = page_address(page);
+	if (ret != BLKPREP_OK) {
+		__free_page(page);
+		rq->buffer = NULL;
+	}
 	return ret;
 }

@@ -485,8 +489,10 @@ static int scsi_setup_flush_cmnd(struct scsi_device *sdp, struct request *rq)
 
 static void sd_unprep_fn(struct request_queue *q, struct request *rq)
 {
-	if (rq->cmd_flags & REQ_DISCARD)
-		__free_page(virt_to_page(rq->buffer));
+	if (rq->cmd_flags & REQ_DISCARD) {
+		free_page((unsigned long)rq->buffer);
+		rq->buffer = NULL;
+	}
 }
 
 /**

Signature · 1 line

1.6.5

Mike Snitzer

Acked-by

2010-07-08 12:06 UTC #6 lore raw

Re: [PATCH v2] scsi: address leak in scsi_setup_discard_cmnd error path (was: Re: scsi: unify the error handling of the prep functions)

Quoted 10 lines · FUJITA Tomonori

Quoted 4 lines · On Tue, 6 Jul 2010 09:59:58 -0400 Mike Snitzer <[email protected]> wrote:
Here is a minimalist patch that 1) removes the discard request's page leak 2) preserves the prep cleanup rules covered above. Fixing the leak is a priority, introducing additional error path code sharing/cleanup is secondary and can come later.
This patch doesn't work. I hit kernel oops since now this patch frees a discard page twice.

I load tested my v2 patch quite a bit but didn't hit the oops. Guess you're just lucky ;)

Quoted 3 lines

If scsi_init_io hits BLKPREP_DEFER (e.g. due to scsi_init_sgtable), scsi_setup_blk_pc_cmnd calls scsi_unprep_request. So sd_unprep_fn frees a page. Then, scsi_setup_discard_cmnd frees the same page again.

OK, thanks for catching this.

Quoted 13 lines

From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] scsi: fix discard page leak
We leak a page allocated for discard on some error conditions (e.g. scsi_prep_state_check returns BLKPREP_DEFER in scsi_setup_blk_pc_cmnd).
We unprep on requests that weren't prepped in the error path of scsi_init_io. It makes the error path to clean up scsi commands messy.
Let's strictly apply the rule that we can't unprep on a request that wasn't prepped.

Quoted 7 lines

Calling just scsi_put_command() in the error path of scsi_init_io() is enough. We don't set REQ_DONTPREP yet.
scsi_setup_discard_cmnd can safely free a page on the error case with the above rule.
Signed-off-by FUJITA Tomonori <[email protected]>

Acked-by Mike Snitzer <[email protected]>

Jens, it'd be great if we could get this fix in now.

Jens Axboe

2010-07-08 12:10 UTC #7 lore raw

Re: [PATCH v2] scsi: address leak in scsi_setup_discard_cmnd error path

Quoted 39 lines · Mike Snitzer

Quoted 10 lines · FUJITA Tomonori
Quoted 4 lines · On Tue, 6 Jul 2010 09:59:58 -0400 Mike Snitzer <[email protected]> wrote:
Here is a minimalist patch that 1) removes the discard request's page leak 2) preserves the prep cleanup rules covered above. Fixing the leak is a priority, introducing additional error path code sharing/cleanup is secondary and can come later.
This patch doesn't work. I hit kernel oops since now this patch frees a discard page twice.
I load tested my v2 patch quite a bit but didn't hit the oops. Guess you're just lucky ;)
Quoted 3 lines
If scsi_init_io hits BLKPREP_DEFER (e.g. due to scsi_init_sgtable), scsi_setup_blk_pc_cmnd calls scsi_unprep_request. So sd_unprep_fn frees a page. Then, scsi_setup_discard_cmnd frees the same page again.
OK, thanks for catching this.
Quoted 16 lines
From: FUJITA Tomonori <[email protected]>
Subject: [PATCH] scsi: fix discard page leak
We leak a page allocated for discard on some error conditions (e.g. scsi_prep_state_check returns BLKPREP_DEFER in scsi_setup_blk_pc_cmnd).
We unprep on requests that weren't prepped in the error path of scsi_init_io. It makes the error path to clean up scsi commands messy.
Let's strictly apply the rule that we can't unprep on a request that wasn't prepped.
Calling just scsi_put_command() in the error path of scsi_init_io() is enough. We don't set REQ_DONTPREP yet.

Quoted 8 lines

Quoted 4 lines
scsi_setup_discard_cmnd can safely free a page on the error case with the above rule.
Signed-off-by FUJITA Tomonori <[email protected]>
Acked-by Mike Snitzer <[email protected]>
Jens, it'd be great if we could get this fix in now.

I picked up Tomo's patch this morning.

Signature · 1 line

Jens Axboe