On 2010-07-08 14:06, Mike Snitzer wrote: > On Thu, Jul 08 2010 at 12:17am -0400, > FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> wrote: > >> On Tue, 6 Jul 2010 09:59:58 -0400 >> Mike Snitzer <snitzer@xxxxxxxxxx> 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 ;) > >> 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. > >> From: FUJITA Tomonori <fujita.tomonori@xxxxxxxxxxxxx> >> 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 <fujita.tomonori@xxxxxxxxxxxxx> > > Acked-by: Mike Snitzer <snitzer@xxxxxxxxxx> > > Jens, it'd be great if we could get this fix in now. I picked up Tomo's patch this morning. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
