All of lore.kernel.org
 help / color / mirror / Atom feed
From: James Bottomley <James.Bottomley@SteelEye.com>
To: Alan Stern <stern@rowland.harvard.edu>
Cc: SCSI development list <linux-scsi@vger.kernel.org>
Subject: Re: [PATCH] SCSI core: fix leakage of scsi_cmnd's
Date: Thu, 08 Sep 2005 10:56:29 -0500	[thread overview]
Message-ID: <1126194989.4845.24.camel@mulgrave> (raw)
In-Reply-To: <Pine.LNX.4.44L0.0509081045230.4503-100000@iolanthe.rowland.org>

> The SCSI core has a problem with leakage of scsi_cmnd structs.  It occurs
> when a request is requeued; the request keeps its associated scsi_cmnd so
> that the core doesn't need to assign a new one.  However, the routines
> that read the device queue sometimes delete entries without bothering to
> free the associated scsi_cmnd.  Among other things, this bug manifests as
> error-handler processes remaining alive when a hot-pluggable device is
> unplugged in the middle of executing a command.
> 
> This patch (as559) fixes the problem by calling scsi_put_command and 
> scsi_release_buffers at the appropriate times.  It also reorganizes a 
> couple of bits of code, adding a new subroutine to find the scsi_cmnd 
> associated with a requeued request.

Nate Dailey also had a patch for this, a reply to which is incomplete
still in my drafts folder.

The bottom line is that I don't think any modifications to the prep_fn()
are necessary.  It's guarded by REQ_DONTPREP, so is never called again
if we actually manage to prepare a command fully.  The returns from it
are:

BLKPREP_DEFER:  Requeue the command for re-preparation (no resources
should be allocated)

BLKPREP_KILL: destroy the command (no resources should be allocated)

BLKPREP_OK: Command is prepared, resources are allocated, REQ_DONTPREP
is set to prevent any additional prep_fn() call.

So in the DEFER or KILL case, all you need to worry about is resources
you may have allocated in the current prep_fn() invocation.  It seems to
me that we do release these correctly, unless I'm missing something?

The second problem is a bug (also spotted by Nate).  However, what I
think we should be doing in this case is calling __scsi_done with
DID_NO_CONNECT which should clean up correctly and also send the error
indications back up the stack to the correct sources (that's what we do
in scsi_dispatch_cmd() for this problem).

James



  reply	other threads:[~2005-09-08 15:56 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2005-09-08 14:56 [PATCH] SCSI core: fix leakage of scsi_cmnd's Alan Stern
2005-09-08 15:56 ` James Bottomley [this message]
2005-09-08 16:44   ` Alan Stern
2005-09-08 17:13     ` James Bottomley
2005-09-08 20:49       ` Alan Stern
2005-09-09 18:40         ` James Bottomley
2005-09-13 17:00           ` Alan Stern
2005-09-13 21:34             ` James Bottomley
2005-09-14 13:59               ` Alan Stern

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1126194989.4845.24.camel@mulgrave \
    --to=james.bottomley@steeleye.com \
    --cc=linux-scsi@vger.kernel.org \
    --cc=stern@rowland.harvard.edu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.