On Sunday 17 May 2009 16:30:48 James Bottomley wrote:
> On Sat, 2009-05-16 at 21:26 +0200, Bartlomiej Zolnierkiewicz wrote:
> > Nowadays eh_lock_door_done() uses blk_get_request() instead of
> > scsi_allocate_request().
>
> I appreciate any attempt to clean up the rather parlous state of our
> documentation. However, unfortunately, this isn't a simple search and
To be honest, I get a very different feeling here...
My two previous obviously correct and trivial patches fixing documentation
for scsi_device_lookup_by_target() and scsi_eh_restore_cmnd() seem to be
just lost (they were posted on 27th April)...
> replace job ... what you've done here is transform an obviously
> incorrect set of statements into a more plausible but still incorrect
> set of statements.
>
> > Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@xxxxxxxxx>
> > ---
> > on top of linux-next
> >
> > drivers/scsi/scsi_error.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > Index: b/drivers/scsi/scsi_error.c
> > ===================================================================
> > --- a/drivers/scsi/scsi_error.c
> > +++ b/drivers/scsi/scsi_error.c
> > @@ -1451,7 +1451,7 @@ static void eh_lock_door_done(struct req
> > * @sdev: SCSI device to prevent medium removal
> > *
> > * Locking:
> > - * We must be called from process context; scsi_allocate_request()
> > + * We must be called from process context; blk_get_request()
> > * may sleep.
>
> Actually, it may or may not depending upon what the user tells it in the
> gfp argument, so the best course here is to kill everything after the
> semicolon.
>
> > * Notes:
> > @@ -1459,11 +1459,11 @@ static void eh_lock_door_done(struct req
> > * head of the devices request queue, and continue.
> > *
> > * Bugs:
> > - * scsi_allocate_request() may sleep waiting for existing requests to
> > + * blk_get_request() may sleep waiting for existing requests to
> > * be processed. However, since we haven't kicked off any request
> > * processing for this host, this may deadlock.
>
> May have been true for scsi_allocate_request, certainly untrue for
> blk_get_request, since we've gone to great pains to eliminate these
> problems.
Okay, I recall the trick used there now (AKA "batching" processes)...
> > - * If scsi_allocate_request() fails for what ever reason, we
> > + * If blk_get_request() fails for what ever reason, we
> > * completely forget to lock the door.
>
> Misleading, since blk_get_request() cannot fail given the gfp flags
> we're using.
>
> So the correct way to fix all of this (including a code change to
> prevent the spurious null check on blk_get_request() return) is below.
Agreed, this version is much better.
> James
>
> ---
>
> diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
> index 0c2c73b..a2a47c3 100644
> --- a/drivers/scsi/scsi_error.c
> +++ b/drivers/scsi/scsi_error.c
> @@ -1451,28 +1451,21 @@ static void eh_lock_door_done(struct request *req, int uptodate)
> * @sdev: SCSI device to prevent medium removal
> *
> * Locking:
> - * We must be called from process context; scsi_allocate_request()
> - * may sleep.
> + * We must be called from process context.
> *
> * Notes:
> * We queue up an asynchronous "ALLOW MEDIUM REMOVAL" request on the
> * head of the devices request queue, and continue.
> - *
> - * Bugs:
> - * scsi_allocate_request() may sleep waiting for existing requests to
> - * be processed. However, since we haven't kicked off any request
> - * processing for this host, this may deadlock.
> - *
> - * If scsi_allocate_request() fails for what ever reason, we
> - * completely forget to lock the door.
> */
> static void scsi_eh_lock_door(struct scsi_device *sdev)
> {
> struct request *req;
>
> + /*
> + * blk_get_request with GFP_KERNEL (__GFP_WAIT) sleeps until a
> + * request becomes available
> + */
> req = blk_get_request(sdev->request_queue, READ, GFP_KERNEL);
> - if (!req)
> - return;
>
> req->cmd[0] = ALLOW_MEDIUM_REMOVAL;
> req->cmd[1] = 0;
--
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
