[PATCH] sd: fix cache flushing on module removal (and individual device removal)

[PATCH] sd: fix cache flushing on module removal (and individual device removal)

[James Bottomley]

The fix isn't actually in sd: it's in scsi_device_get().  I modified it
to allow devices to be returned in SDEV_CANCEL, but not SDEV_DEL.  This
means that the device_remove_driver, which occurs in device_del() in
scsi_remove_device() after the device has gone into SDEV_CANCEL is now
effective at flushing the cache.

James

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 94df671..3294863 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -851,7 +851,7 @@ EXPORT_SYMBOL(scsi_track_queue_full);
  */
 int scsi_device_get(struct scsi_device *sdev)
 {
-	if (sdev->sdev_state == SDEV_DEL || sdev->sdev_state == SDEV_CANCEL)
+	if (sdev->sdev_state == SDEV_DEL)
 		return -ENXIO;
 	if (!get_device(&sdev->sdev_gendev))
 		return -ENXIO;

Re: [PATCH] sd: fix cache flushing on module removal (and individual device removal)

[James Bottomley]

On Thu, 2006-08-31 at 17:08 -0400, James Bottomley wrote:
> The fix isn't actually in sd: it's in scsi_device_get().  I modified it
> to allow devices to be returned in SDEV_CANCEL, but not SDEV_DEL.  This
> means that the device_remove_driver, which occurs in device_del() in
> scsi_remove_device() after the device has gone into SDEV_CANCEL is now
> effective at flushing the cache.

Actually, lets try that again, but with the correct patch.

James

Index: linux-2.6/drivers/scsi/scsi.c
===================================================================
--- linux-2.6.orig/drivers/scsi/scsi.c
+++ linux-2.6/drivers/scsi/scsi.c
@@ -835,14 +835,14 @@ EXPORT_SYMBOL(scsi_track_queue_full);
  */
 int scsi_device_get(struct scsi_device *sdev)
 {
-	if (sdev->sdev_state == SDEV_DEL || sdev->sdev_state == SDEV_CANCEL)
+	if (sdev->sdev_state == SDEV_DEL)
 		return -ENXIO;
 	if (!get_device(&sdev->sdev_gendev))
 		return -ENXIO;
-	if (!try_module_get(sdev->host->hostt->module)) {
-		put_device(&sdev->sdev_gendev);
-		return -ENXIO;
-	}
+	/* We can fail this if we're doing SCSI operations
+	 * from module exit (like cache flush) */
+	try_module_get(sdev->host->hostt->module);
+
 	return 0;
 }
 EXPORT_SYMBOL(scsi_device_get);
@@ -857,7 +857,10 @@ EXPORT_SYMBOL(scsi_device_get);
  */
 void scsi_device_put(struct scsi_device *sdev)
 {
-	module_put(sdev->host->hostt->module);
+	/* The module refcount will be zero if scsi_device_get()
+	 * was called from a module removal routine */
+	if (likely(module_refcount(sdev->host->hostt->module) != 0))
+		module_put(sdev->host->hostt->module);
 	put_device(&sdev->sdev_gendev);
 }
 EXPORT_SYMBOL(scsi_device_put);

Re: [PATCH] sd: fix cache flushing on module removal (and individual device removal)

[Stefan Richter]

James Bottomley wrote:
[...]
> --- linux-2.6.orig/drivers/scsi/scsi.c
> +++ linux-2.6/drivers/scsi/scsi.c
> @@ -835,14 +835,14 @@ EXPORT_SYMBOL(scsi_track_queue_full);
>   */
>  int scsi_device_get(struct scsi_device *sdev)
>  {
> -	if (sdev->sdev_state == SDEV_DEL || sdev->sdev_state == SDEV_CANCEL)
> +	if (sdev->sdev_state == SDEV_DEL)
>  		return -ENXIO;
>  	if (!get_device(&sdev->sdev_gendev))
>  		return -ENXIO;
> -	if (!try_module_get(sdev->host->hostt->module)) {
> -		put_device(&sdev->sdev_gendev);
> -		return -ENXIO;
> -	}
> +	/* We can fail this if we're doing SCSI operations
> +	 * from module exit (like cache flush) */
> +	try_module_get(sdev->host->hostt->module);
> +
>  	return 0;
>  }
>  EXPORT_SYMBOL(scsi_device_get);
> @@ -857,7 +857,10 @@ EXPORT_SYMBOL(scsi_device_get);
>   */
>  void scsi_device_put(struct scsi_device *sdev)
>  {
> -	module_put(sdev->host->hostt->module);
> +	/* The module refcount will be zero if scsi_device_get()
> +	 * was called from a module removal routine */
> +	if (likely(module_refcount(sdev->host->hostt->module) != 0))
> +		module_put(sdev->host->hostt->module);
>  	put_device(&sdev->sdev_gendev);
>  }
>  EXPORT_SYMBOL(scsi_device_put);

Somehow the (void)try_module_get(...) looks dangerous to me. Is it
really safe to always ignore failures to get the module? Why would we
want to ignore failures? Couldn't there be border cases where a
module_getter/_putter in a concurrent code path disturbs
scsi_device_get/_put's underlying assumptions?
-- 
Stefan Richter
-=====-=-==- =--= ----=
http://arcgraph.de/sr/

Re: [PATCH] sd: fix cache flushing on module removal (and individual device removal)

[James Bottomley]

On Fri, 2006-09-01 at 11:02 +0200, Stefan Richter wrote:
> Somehow the (void)try_module_get(...) looks dangerous to me. Is it
> really safe to always ignore failures to get the module? Why would we
> want to ignore failures? Couldn't there be border cases where a
> module_getter/_putter in a concurrent code path disturbs
> scsi_device_get/_put's underlying assumptions?

As long as we don't do spurious module_puts, yes.  However, there looks
to be another nasty module race that's orthogonal to this, in that the
final scsi_host_put() of a module doesn't necessarily wait for the host
actually to be released, so it's possible to free the host template when
the module exit finishes and still have a partially functioning host.

James