Re: add sd_unprep_fn to free discard page

[James Bottomley <James.Bottomley@suse.de>]

On Thu, 2010-07-01 at 16:40 +0300, Boaz Harrosh wrote:
> On 07/01/2010 01:49 PM, FUJITA Tomonori wrote:
> > This patchset fixes page leak issue in discard commands with unprep
> > facility that James posted:
> > 
> > http://marc.info/?l=linux-scsi&m=127791727508214&w=2
> > 
> > The 1/3 patch adds unprep facility to the block layer (identical to
> > what James posted).
> > 
> 
> Alternatively to this patch you could also call scsi_driver->done()
> on all commands. There are only two users (sd sr) it's not that bad to add
> an if (blk_pc_req()) inside these ->done() function.

We could, but it wouldn't fix the problem.  The system issue is that
->done isn't symmetrical to ->prep_rq_fn() ... ->done() sits in the
middle of the SCSI completion path and we could decide to requeue the
command after calling it (that's why we can't free the discard page
either in ->done() or in the midlayer where ->done() is called).

The system solution is a symmetrical pair for ->prep_rq_fn() which is
only called either after all error handling is complete, or we decide to
completely strip the command down for a retry that goes through prep
again.

> > The 2/3 patch frees a page for discard commands by using the unprep
> > facility. James' original patch doesn't work since it accesses to
> > rq->bio in q->unprep_rq_fn. We hit oops since q->unprep_rq_fn is
> > called when all the data buffer (req->bio and scsi_data_buffer) in the
> > request is freed.
> > 
> > I use rq->buffer to keep track of an allocated page as the block layer
> > sets rq->buffer to the address of bio's page. scsi-ml (and llds) don't
> > use rq->buffer (rq->buffer is set to NULL). So I can't say that I like
> > it lots. Any other way to do that?
> > 
> 
> rq->buffer is intended for block-driver use as well as req->special.
> sd+scsi-ml is the block-driver here. req->special is used by scsi-ml
> and rq->buffer is set to NULL inside the call to
> scsi_setup_blk_pc_cmnd/scsi_setup_fs_cmnd. Since you set the ->buffer
> after the call to scsi_setup_blk_pc_cmnd you should be in the clear.
> 
> I think scsi-ml should stop setting rq->buffer to NULL and leave it
> be for ULD use. It is left from the time that LLDs where converted
> to use BIOs, just to make sure out-of-tree drivers crash.

So I buy this more.  There is a slight assymmetry in that we have the
bio in prep, but not in unprep.  Since requests can complete partially
(freeing the bios on the way), there's no real way to avoid this.  I
think the use of ->buffer is a bit unsavoury but it looks acceptable.

James