[PATCH] scsi_error: fix scsi_eh_lock_door() documentation

[PATCH] scsi_error: fix scsi_eh_lock_door() documentation

[Bartlomiej Zolnierkiewicz]

Nowadays eh_lock_door_done() uses blk_get_request() instead of
scsi_allocate_request().

Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com>
---
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.
  *
  * 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.
  *
- *	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.
  */
 static void scsi_eh_lock_door(struct scsi_device *sdev)

Re: [PATCH] scsi_error: fix scsi_eh_lock_door() documentation

[James Bottomley]

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
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@gmail.com>
> ---
> 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.

> - *	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.

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;

Re: [PATCH] scsi_error: fix scsi_eh_lock_door() documentation

[Bartlomiej Zolnierkiewicz]

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@gmail.com>
> > ---
> > 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;