[PATCH 00 of 10] blkfront pvops updates, v2

[PATCH 00 of 10] blkfront pvops updates, v2

[Daniel Stodden]

Updates since the last time:

 * Make sure everything applies against xen/frontend. That fortunately
   made no difference.

 * Two more loopholes:

    - The path freeing info in xenbus_remove took a more carefully
      sync with an unfortunate bdops->release. This is because the
      locking can't just nest. Fixed by resyncing the info pointer
      through disk->private_data.

    - Similar effect during bdev open. This also needs to anticipate
      the not-so-unlikely case where not only the gendisk but the
      entire info struct was deleted just before entry. Again, syncing
      on disk->private_data once holding bd_mutex does the trick.

    I chose to just merge those in.

 * To be honest, that dev_warn the last patch added was broken -- doh.

 * Found the switch [again] for HG to strip those headers -- cheers.

Feedback certainly welcome. I am aware this thing looks manic, but at
it stands, we recently happend to come across exactly that family of
races in XCP again, so I really want these paths fixed.

Thanks,
Daniel

[PATCH 01 of 10] xenbus: Make xenbus_switch_state transactional (again)

[Daniel Stodden]

[-- Attachment #1: Type: text/plain, Size: 750 bytes --]

According to the comments, this was how it's been done years ago, but
apparently took an xbt pointer from elsewhere back then. The code was
removed because of consistency issues: cancellation wont't roll back
the saved xbdev->state.

Still, unsolicited writes to the state field remain an issue,
especially if device shutdown takes thread synchronization, and subtle
races cause accidental recreation of the device node.

Fixed by reintroducing the transaction. An internal one is sufficient,
so the xbdev->state value remains consistent.

Also fixes the original hack to prevent infinite recursion. Instead of
bailing out on the first attempt to switch to Closing, checks call
depth now.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>



[-- Attachment #2: xenbus-switch-transaction.diff --]
[-- Type: text/x-patch, Size: 4572 bytes --]

According to the comments, this was how it's been done years ago, but
apparently took an xbt pointer from elsewhere back then. The code was
removed because of consistency issues: cancellation wont't roll back
the saved xbdev->state.

Still, unsolicited writes to the state field remain an issue,
especially if device shutdown takes thread synchronization, and subtle
races cause accidental recreation of the device node.

Fixed by reintroducing the transaction. An internal one is sufficient,
so the xbdev->state value remains consistent.

Also fixes the original hack to prevent infinite recursion. Instead of
bailing out on the first attempt to switch to Closing, checks call
depth now.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>

diff -r be2785bfda86 -r b863073c3633 drivers/xen/xenbus/xenbus_client.c
--- a/drivers/xen/xenbus/xenbus_client.c	Fri Apr 30 14:58:59 2010 -0700
+++ b/drivers/xen/xenbus/xenbus_client.c	Fri Apr 30 14:58:59 2010 -0700
@@ -134,6 +134,64 @@
 }
 EXPORT_SYMBOL_GPL(xenbus_watch_pathfmt);
 
+static void xenbus_switch_fatal(struct xenbus_device *, int, int,
+				const char *, ...);
+
+static int
+__xenbus_switch_state(struct xenbus_device *dev,
+		      enum xenbus_state state, int depth)
+{
+	/* We check whether the state is currently set to the given value, and
+	   if not, then the state is set.  We don't want to unconditionally
+	   write the given state, because we don't want to fire watches
+	   unnecessarily.  Furthermore, if the node has gone, we don't write
+	   to it, as the device will be tearing down, and we don't want to
+	   resurrect that directory.
+
+	   Note that, because of this cached value of our state, this
+	   function will not take a caller's Xenstore transaction
+	   (something it was trying to in the past) because dev->state
+	   would not get reset if the transaction was aborted.
+	 */
+
+	struct xenbus_transaction xbt;
+	int current_state;
+	int err, abort;
+
+	if (state == dev->state)
+		return 0;
+
+again:
+	abort = 1;
+
+	err = xenbus_transaction_start(&xbt);
+	if (err) {
+		xenbus_switch_fatal(dev, depth, err, "starting transaction");
+		return 0;
+	}
+
+	err = xenbus_scanf(xbt, dev->nodename, "state", "%d", &current_state);
+	if (err != 1)
+		goto abort;
+
+	err = xenbus_printf(xbt, dev->nodename, "state", "%d", state);
+	if (err) {
+		xenbus_switch_fatal(dev, depth, err, "writing new state");
+		goto abort;
+	}
+
+	abort = 0;
+abort:
+	err = xenbus_transaction_end(xbt, abort);
+	if (err) {
+		if (err == -EAGAIN && !abort)
+			goto again;
+		xenbus_switch_fatal(dev, depth, err, "ending transaction");
+	} else
+		dev->state = state;
+
+	return 0;
+}
 
 /**
  * xenbus_switch_state
@@ -146,42 +204,9 @@
  */
 int xenbus_switch_state(struct xenbus_device *dev, enum xenbus_state state)
 {
-	/* We check whether the state is currently set to the given value, and
-	   if not, then the state is set.  We don't want to unconditionally
-	   write the given state, because we don't want to fire watches
-	   unnecessarily.  Furthermore, if the node has gone, we don't write
-	   to it, as the device will be tearing down, and we don't want to
-	   resurrect that directory.
+	return __xenbus_switch_state(dev, state, 0);
+}
 
-	   Note that, because of this cached value of our state, this function
-	   will not work inside a Xenstore transaction (something it was
-	   trying to in the past) because dev->state would not get reset if
-	   the transaction was aborted.
-
-	 */
-
-	int current_state;
-	int err;
-
-	if (state == dev->state)
-		return 0;
-
-	err = xenbus_scanf(XBT_NIL, dev->nodename, "state", "%d",
-			   &current_state);
-	if (err != 1)
-		return 0;
-
-	err = xenbus_printf(XBT_NIL, dev->nodename, "state", "%d", state);
-	if (err) {
-		if (state != XenbusStateClosing) /* Avoid looping */
-			xenbus_dev_fatal(dev, err, "writing new state");
-		return err;
-	}
-
-	dev->state = state;
-
-	return 0;
-}
 EXPORT_SYMBOL_GPL(xenbus_switch_state);
 
 int xenbus_frontend_closed(struct xenbus_device *dev)
@@ -285,6 +310,23 @@
 EXPORT_SYMBOL_GPL(xenbus_dev_fatal);
 
 /**
+ * Equivalent to xenbus_dev_fatal(dev, err, fmt, args), but helps
+ * avoiding recursion within xenbus_switch_state.
+ */
+static void xenbus_switch_fatal(struct xenbus_device *dev, int depth, int err,
+				const char *fmt, ...)
+{
+	va_list ap;
+
+	va_start(ap, fmt);
+	xenbus_va_dev_error(dev, err, fmt, ap);
+	va_end(ap);
+
+	if (!depth)
+		__xenbus_switch_state(dev, XenbusStateClosing, 1);
+}
+
+/**
  * xenbus_grant_ring
  * @dev: xenbus device
  * @ring_mfn: mfn of ring to grant

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 02 of 10] blkfront: Fix backtrace in del_gendisk

[Daniel Stodden]

[-- Attachment #1: Type: text/plain, Size: 218 bytes --]

The call to del_gendisk follows an non-refcounted gd->queue
pointer. We release the last ref in blk_cleanup_queue. Fixed by
reordering releases accordingly.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>



[-- Attachment #2: blkfront-cleanup-queue.diff --]
[-- Type: text/x-patch, Size: 839 bytes --]

The call to del_gendisk follows an non-refcounted gd->queue
pointer. We release the last ref in blk_cleanup_queue. Fixed by
reordering releases accordingly.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>

diff -r b863073c3633 -r 649bc0003f9a drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
+++ b/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
@@ -1021,14 +1021,14 @@
 	/* Flush gnttab callback work. Must be done with no locks held. */
 	flush_scheduled_work();
 
-	blk_cleanup_queue(info->rq);
-	info->rq = NULL;
-
 	minor = info->gd->first_minor;
 	nr_minors = info->gd->minors;
 	del_gendisk(info->gd);
 	xlbd_release_minors(minor, nr_minors);
 
+	blk_cleanup_queue(info->rq);
+	info->rq = NULL;
+
  out:
 	if (info->xbdev)
 		xenbus_frontend_closed(info->xbdev);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 03 of 10] blkfront: Fix gendisk leak

[Daniel Stodden]

[-- Attachment #1: Type: text/plain, Size: 60 bytes --]

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>



[-- Attachment #2: blkfront-del-gendisk.diff --]
[-- Type: text/x-patch, Size: 440 bytes --]

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>

diff -r 649bc0003f9a -r 1bbb4a55cfe3 drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
+++ b/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
@@ -1029,6 +1029,9 @@
 	blk_cleanup_queue(info->rq);
 	info->rq = NULL;
 
+	put_disk(info->gd);
+	info->gd = NULL;
+
  out:
 	if (info->xbdev)
 		xenbus_frontend_closed(info->xbdev);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 04 of 10] blkfront: Clean up vbd release

[Daniel Stodden]

[-- Attachment #1: Type: text/plain, Size: 402 bytes --]

* Current blkfront_closing is rather a xlvbd_release_gendisk.
   Renamed in preparation of later patches (need the name again).

 * Removed the misleading comment -- this only applied to the backend
   switch handler, and the queue is already flushed btw.

 * Break out the xenbus call, callers know better when to switch
   frontend state.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>



[-- Attachment #2: blkfront-vbd-release.diff --]
[-- Type: text/x-patch, Size: 3425 bytes --]

 * Current blkfront_closing is rather a xlvbd_release_gendisk.
   Renamed in preparation of later patches (need the name again).

 * Removed the misleading comment -- this only applied to the backend
   switch handler, and the queue is already flushed btw.

 * Break out the xenbus call, callers know better when to switch
   frontend state.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>

diff -r 1bbb4a55cfe3 -r 87be4d8c53a4 drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
+++ b/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
@@ -534,6 +534,39 @@
 	return err;
 }
 
+static void xlvbd_release_gendisk(struct blkfront_info *info)
+{
+	unsigned int minor, nr_minors;
+	unsigned long flags;
+
+	if (info->rq == NULL)
+		return;
+
+	spin_lock_irqsave(&blkif_io_lock, flags);
+
+	/* No more blkif_request(). */
+	blk_stop_queue(info->rq);
+
+	/* No more gnttab callback work. */
+	gnttab_cancel_free_callback(&info->callback);
+	spin_unlock_irqrestore(&blkif_io_lock, flags);
+
+	/* Flush gnttab callback work. Must be done with no locks held. */
+	flush_scheduled_work();
+
+	del_gendisk(info->gd);
+
+	minor = info->gd->first_minor;
+	nr_minors = info->gd->minors;
+	xlbd_release_minors(minor, nr_minors);
+
+	blk_cleanup_queue(info->rq);
+	info->rq = NULL;
+
+	put_disk(info->gd);
+	info->gd = NULL;
+}
+
 static void kick_pending_request_queues(struct blkfront_info *info)
 {
 	if (!RING_FULL(&info->ring)) {
@@ -995,49 +1028,6 @@
 }
 
 /**
- * Handle the change of state of the backend to Closing.  We must delete our
- * device-layer structures now, to ensure that writes are flushed through to
- * the backend.  Once is this done, we can switch to Closed in
- * acknowledgement.
- */
-static void blkfront_closing(struct blkfront_info *info)
-{
-	unsigned int minor, nr_minors;
-	unsigned long flags;
-
-
-	if (info->rq == NULL)
-		goto out;
-
-	spin_lock_irqsave(&blkif_io_lock, flags);
-
-	/* No more blkif_request(). */
-	blk_stop_queue(info->rq);
-
-	/* No more gnttab callback work. */
-	gnttab_cancel_free_callback(&info->callback);
-	spin_unlock_irqrestore(&blkif_io_lock, flags);
-
-	/* Flush gnttab callback work. Must be done with no locks held. */
-	flush_scheduled_work();
-
-	minor = info->gd->first_minor;
-	nr_minors = info->gd->minors;
-	del_gendisk(info->gd);
-	xlbd_release_minors(minor, nr_minors);
-
-	blk_cleanup_queue(info->rq);
-	info->rq = NULL;
-
-	put_disk(info->gd);
-	info->gd = NULL;
-
- out:
-	if (info->xbdev)
-		xenbus_frontend_closed(info->xbdev);
-}
-
-/**
  * Callback received when the backend's state changes.
  */
 static void blkback_changed(struct xenbus_device *dev,
@@ -1075,8 +1065,11 @@
 		if (info->users > 0)
 			xenbus_dev_error(dev, -EBUSY,
 					 "Device in use; refusing to close");
-		else
-			blkfront_closing(info);
+		else {
+			xlvbd_release_gendisk(info);
+			xenbus_frontend_closed(info->xbdev);
+		}
+
 		mutex_unlock(&bd->bd_mutex);
 		bdput(bd);
 		break;
@@ -1127,11 +1120,13 @@
 		struct xenbus_device *dev = info->xbdev;
 
 		if (!dev) {
-			blkfront_closing(info);
+			xlvbd_release_gendisk(info);
 			kfree(info);
 		} else if (xenbus_read_driver_state(dev->otherend)
-			   == XenbusStateClosing && info->is_ready)
-			blkfront_closing(info);
+			   == XenbusStateClosing && info->is_ready) {
+			xlvbd_release_gendisk(info);
+			xenbus_frontend_closed(dev);
+		}
 	}
 	return 0;
 }

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 05 of 10] blkfront: Lock blkfront_info when closing

[Daniel Stodden]

[-- Attachment #1: Type: text/plain, Size: 420 bytes --]

The bdev .open/.release fops race against backend switches to Closing,
handled by the XenBus thread.

The original code attempted to serialize block device holders and
xenbus only via bd_mutex. This is insufficient, the info->bd pointer
may already be stale (or null) while xenbus tries to bump up the
refcount.

Protect blkfront_info with a dedicated mutex.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>



[-- Attachment #2: blkfront-xenbus-closing.diff --]
[-- Type: text/x-patch, Size: 2767 bytes --]

The bdev .open/.release fops race against backend switches to Closing,
handled by the XenBus thread.

The original code attempted to serialize block device holders and
xenbus only via bd_mutex. This is insufficient, the info->bd pointer
may already be stale (or null) while xenbus tries to bump up the
refcount.

Protect blkfront_info with a dedicated mutex.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>

diff -r 87be4d8c53a4 -r fbf1ab5b4c61 drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
+++ b/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
@@ -77,6 +77,7 @@
  */
 struct blkfront_info
 {
+	struct mutex mutex;
 	struct xenbus_device *xbdev;
 	struct gendisk *gd;
 	int vdevice;
@@ -804,7 +805,6 @@
 	return err;
 }
 
-
 /**
  * Entry point to this code when a new device is created.  Allocate the basic
  * structures and the ring buffer for communication with the backend, and
@@ -836,6 +836,7 @@
 		return -ENOMEM;
 	}
 
+	mutex_init(&info->mutex);
 	info->xbdev = dev;
 	info->vdevice = vdevice;
 	info->connected = BLKIF_STATE_DISCONNECTED;
@@ -951,6 +952,43 @@
 	return err;
 }
 
+static void
+blkfront_closing(struct blkfront_info *info)
+{
+	struct xenbus_device *xbdev = info->xbdev;
+	struct block_device *bdev = NULL;
+
+	mutex_lock(&info->mutex);
+
+	if (xbdev->state == XenbusStateClosing) {
+		mutex_unlock(&info->mutex);
+		return;
+	}
+
+	if (info->gd)
+		bdev = bdget_disk(info->gd, 0);
+
+	mutex_unlock(&info->mutex);
+
+	if (!bdev) {
+		xenbus_frontend_closed(xbdev);
+		return;
+	}
+
+	mutex_lock(&bdev->bd_mutex);
+
+	if (info->users) {
+		xenbus_dev_error(xbdev, -EBUSY,
+				 "Device in use; refusing to close");
+		xenbus_switch_state(xbdev, XenbusStateClosing);
+	} else {
+		xlvbd_release_gendisk(info);
+		xenbus_frontend_closed(xbdev);
+	}
+
+	mutex_unlock(&bdev->bd_mutex);
+	bdput(bdev);
+}
 
 /*
  * Invoked when the backend is finally 'ready' (and has told produced
@@ -1034,7 +1072,6 @@
 			    enum xenbus_state backend_state)
 {
 	struct blkfront_info *info = dev_get_drvdata(&dev->dev);
-	struct block_device *bd;
 
 	dev_dbg(&dev->dev, "blkfront:blkback_changed to state %d.\n", backend_state);
 
@@ -1053,25 +1090,7 @@
 		break;
 
 	case XenbusStateClosing:
-		if (info->gd == NULL) {
-			xenbus_frontend_closed(dev);
-			break;
-		}
-		bd = bdget_disk(info->gd, 0);
-		if (bd == NULL)
-			xenbus_dev_fatal(dev, -ENODEV, "bdget failed");
-
-		mutex_lock(&bd->bd_mutex);
-		if (info->users > 0)
-			xenbus_dev_error(dev, -EBUSY,
-					 "Device in use; refusing to close");
-		else {
-			xlvbd_release_gendisk(info);
-			xenbus_frontend_closed(info->xbdev);
-		}
-
-		mutex_unlock(&bd->bd_mutex);
-		bdput(bd);
+		blkfront_closing(info);
 		break;
 	}
 }

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 06 of 10] blkfront: Fix blkfront backend switch race (bdev open)

[Daniel Stodden]

[-- Attachment #1: Type: text/plain, Size: 328 bytes --]

We need not mind if users grab a late handle on a closing disk. We
probably even should not. But we have to make sure it's not a dead
one already

Let the bdev deal with a gendisk deleted under its feet. Takes the
info mutex to decide a race against backend closing.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>



[-- Attachment #2: blkfront-bdev-open.diff --]
[-- Type: text/x-patch, Size: 1163 bytes --]

We need not mind if users grab a late handle on a closing disk. We
probably even should not. But we have to make sure it's not a dead
one already

Let the bdev deal with a gendisk deleted under its feet. Takes the
info mutex to decide a race against backend closing.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>

diff -r fbf1ab5b4c61 -r 5bfa144b3a9b drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
+++ b/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
@@ -1120,12 +1120,27 @@
 
 static int blkif_open(struct block_device *bdev, fmode_t mode)
 {
-	struct blkfront_info *info = bdev->bd_disk->private_data;
+	struct gendisk *disk = bdev->bd_disk;
+	struct blkfront_info *info;
+	int err = 0;
 
-	if (!info->xbdev)
-		return -ENODEV;
-	info->users++;
-	return 0;
+	info = disk->private_data;
+	if (!info)
+		/* xbdev gone */
+		return -ERESTARTSYS;
+
+	mutex_lock(&info->mutex);
+
+	if (!info->gd)
+		/* xbdev is closed */
+		err = -ERESTARTSYS;
+
+	mutex_unlock(&info->mutex);
+
+	if (!err)
+		++info->users;
+
+	return err;
 }
 
 static int blkif_release(struct gendisk *disk, fmode_t mode)

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 07 of 10] blkfront: Fix blkfront backend switch race (bdev release)

[Daniel Stodden]

[-- Attachment #1: Type: text/plain, Size: 407 bytes --]

We cannot read backend state within bdev operations, because it risks
grabbing the state change before xenbus gets to do it.

Fixed by tracking deferral with a frontend switch to Closing. State
exposure isn't strictly necessary, but the backends won't mind.

For a 'clean' deferral this seems actually a more decent protocol than
raising errors.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>



[-- Attachment #2: blkfront-bdev-release.diff --]
[-- Type: text/x-patch, Size: 1903 bytes --]

We cannot read backend state within bdev operations, because it risks
grabbing the state change before xenbus gets to do it.

Fixed by tracking deferral with a frontend switch to Closing. State
exposure isn't strictly necessary, but the backends won't mind.

For a 'clean' deferral this seems actually a more decent protocol than
raising errors.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>

diff -r 5bfa144b3a9b -r 848b3f5aa723 drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
+++ b/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
@@ -1146,22 +1146,38 @@
 static int blkif_release(struct gendisk *disk, fmode_t mode)
 {
 	struct blkfront_info *info = disk->private_data;
-	info->users--;
-	if (info->users == 0) {
-		/* Check whether we have been instructed to close.  We will
-		   have ignored this request initially, as the device was
-		   still mounted. */
-		struct xenbus_device *dev = info->xbdev;
+	struct block_device *bdev;
+	struct xenbus_device *xbdev;
 
-		if (!dev) {
-			xlvbd_release_gendisk(info);
-			kfree(info);
-		} else if (xenbus_read_driver_state(dev->otherend)
-			   == XenbusStateClosing && info->is_ready) {
-			xlvbd_release_gendisk(info);
-			xenbus_frontend_closed(dev);
-		}
+	if (--info->users)
+		return 0;
+
+	bdev = bdget_disk(disk, 0);
+	bdput(bdev);
+
+	/*
+	 * Check if we have been instructed to close. We will have
+	 * deferred this request, because the bdev was still open.
+	 */
+
+	mutex_lock(&info->mutex);
+	xbdev = info->xbdev;
+
+	if (xbdev && xbdev->state == XenbusStateClosing) {
+		/* pending switch to state closed */
+		xlvbd_release_gendisk(info);
+		xenbus_frontend_closed(info->xbdev);
 	}
+
+	mutex_unlock(&info->mutex);
+
+	if (!xbdev) {
+		/* sudden device removal */
+		xlvbd_release_gendisk(info);
+		disk->private_data = NULL;
+		kfree(info);
+	}
+
 	return 0;
 }
 

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 08 of 10] blkfront: Lock blockfront_info during xbdev removal

[Daniel Stodden]

[-- Attachment #1: Type: text/plain, Size: 236 bytes --]

Same approach as blkfront_closing:
 * Grab the bdev safely, holding the info mutex.
 * Zap xbdev safely, holding the info mutex.
 * Try bdev removal safely, holding bd_mutex.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>



[-- Attachment #2: blkfront-xenbus-remove.diff --]
[-- Type: text/x-patch, Size: 1592 bytes --]

Same approach as blkfront_closing:
 * Grab the bdev safely, holding the info mutex.
 * Zap xbdev safely, holding the info mutex.
 * Try bdev removal safely, holding bd_mutex.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>

diff -r 848b3f5aa723 -r 4d4cc7268176 drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
+++ b/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
@@ -1095,18 +1095,47 @@
 	}
 }
 
-static int blkfront_remove(struct xenbus_device *dev)
+static int blkfront_remove(struct xenbus_device *xbdev)
 {
-	struct blkfront_info *info = dev_get_drvdata(&dev->dev);
+	struct blkfront_info *info = dev_get_drvdata(&xbdev->dev);
+	struct block_device *bdev = NULL;
+	struct gendisk *disk;
 
-	dev_dbg(&dev->dev, "blkfront_remove: %s removed\n", dev->nodename);
+	dev_dbg(&xbdev->dev, "%s removed", xbdev->nodename);
 
 	blkif_free(info, 0);
 
-	if(info->users == 0)
+	mutex_lock(&info->mutex);
+
+	disk = info->gd;
+	if (disk)
+		bdev = bdget_disk(disk, 0);
+
+	info->xbdev = NULL;
+	mutex_unlock(&info->mutex);
+
+	if (!bdev) {
 		kfree(info);
-	else
-		info->xbdev = NULL;
+		return 0;
+	}
+
+	/*
+	 * The xbdev was removed before we reached the Closed
+	 * state. See if it's safe to remove the disk. If the bdev
+	 * isn't closed yet, we let release take care of it.
+	 */
+
+	mutex_lock(&bdev->bd_mutex);
+	info = disk->private_data;
+
+	if (info && !info->users) {
+		xlvbd_release_gendisk(info);
+		disk->private_data = NULL;
+		kfree(info);
+	}
+
+	mutex_unlock(&bdev->bd_mutex);
+	bdput(bdev);
 
 	return 0;
 }

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 09 of 10] blkfront: Remove obsolete info->users

[Daniel Stodden]

[-- Attachment #1: Type: text/plain, Size: 113 bytes --]

This is just bd_openers, protected by the bd_mutex.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>



[-- Attachment #2: blkfront-bd-openers.diff --]
[-- Type: text/x-patch, Size: 1474 bytes --]

This is just bd_openers, protected by the bd_mutex.

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>

diff -r 4d4cc7268176 -r 798c5b306104 drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
+++ b/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
@@ -94,12 +94,6 @@
 	unsigned long shadow_free;
 	int feature_barrier;
 	int is_ready;
-
-	/**
-	 * The number of people holding this device open.  We won't allow a
-	 * hot-unplug unless this is 0.
-	 */
-	int users;
 };
 
 static DEFINE_SPINLOCK(blkif_io_lock);
@@ -977,7 +971,7 @@
 
 	mutex_lock(&bdev->bd_mutex);
 
-	if (info->users) {
+	if (bdev->bd_openers) {
 		xenbus_dev_error(xbdev, -EBUSY,
 				 "Device in use; refusing to close");
 		xenbus_switch_state(xbdev, XenbusStateClosing);
@@ -1128,7 +1122,7 @@
 	mutex_lock(&bdev->bd_mutex);
 	info = disk->private_data;
 
-	if (info && !info->users) {
+	if (info && !bdev->bd_openers) {
 		xlvbd_release_gendisk(info);
 		disk->private_data = NULL;
 		kfree(info);
@@ -1166,9 +1160,6 @@
 
 	mutex_unlock(&info->mutex);
 
-	if (!err)
-		++info->users;
-
 	return err;
 }
 
@@ -1178,12 +1169,12 @@
 	struct block_device *bdev;
 	struct xenbus_device *xbdev;
 
-	if (--info->users)
-		return 0;
-
 	bdev = bdget_disk(disk, 0);
 	bdput(bdev);
 
+	if (bdev->bd_openers)
+		return 0;
+
 	/*
 	 * Check if we have been instructed to close. We will have
 	 * deferred this request, because the bdev was still open.

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

[PATCH 10 of 10] blkfront: Klog the unclean release path

[Daniel Stodden]

[-- Attachment #1: Type: text/plain, Size: 60 bytes --]

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>



[-- Attachment #2: blkfront-vbd-release-log.diff --]
[-- Type: text/x-patch, Size: 1023 bytes --]

Signed-off-by: Daniel Stodden <daniel.stodden@citrix.com>

diff -r 798c5b306104 -r 5cb920bc0e83 drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
+++ b/drivers/block/xen-blkfront.c	Fri Apr 30 14:58:59 2010 -0700
@@ -1122,6 +1122,10 @@
 	mutex_lock(&bdev->bd_mutex);
 	info = disk->private_data;
 
+	dev_warn(disk_to_dev(disk),
+		 "%s was hot-unplugged, %d stale handles\n",
+		 xbdev->nodename, bdev->bd_openers);
+
 	if (info && !bdev->bd_openers) {
 		xlvbd_release_gendisk(info);
 		disk->private_data = NULL;
@@ -1185,6 +1189,7 @@
 
 	if (xbdev && xbdev->state == XenbusStateClosing) {
 		/* pending switch to state closed */
+		dev_info(disk_to_dev(bdev->bd_disk), "releasing disk\n");
 		xlvbd_release_gendisk(info);
 		xenbus_frontend_closed(info->xbdev);
 	}
@@ -1193,6 +1198,7 @@
 
 	if (!xbdev) {
 		/* sudden device removal */
+		dev_info(disk_to_dev(bdev->bd_disk), "releasing disk\n");
 		xlvbd_release_gendisk(info);
 		disk->private_data = NULL;
 		kfree(info);

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH 00 of 10] blkfront pvops updates, v2

[Jeremy Fitzhardinge]

On 04/30/2010 03:01 PM, Daniel Stodden wrote:
>  * Found the switch [again] for HG to strip those headers -- cheers.
>   

The ideal for me is if you have a git tree I can pull from, but failing
that, the most useful format for mailed patches is something that "git
am" can accept.  It looks like it will take the attachments OK, but it
cats the mail body onto the attachment, so it ends up with the commit
comment twice.  If you have a non-mangling mailer, it's probably easiest
if you just include the full commit comment+patch in the mail body (it's
easier to comment on the patches in mail that way too).

> Oh beautiful. This apparently strips the first comment line together
> with the patch header. Looks buggy.
>
> F*! 
>
> Sorry. Fix/resend?
>   

Did it lose something other than the subject?  git am picked it up from
the email Subject: line, so it didn't matter it was missing from the
patch comment itself.

    J

Re: [PATCH 00 of 10] blkfront pvops updates, v2

[Daniel Stodden]

On Fri, 2010-04-30 at 18:16 -0400, Jeremy Fitzhardinge wrote:
> On 04/30/2010 03:01 PM, Daniel Stodden wrote:
> >  * Found the switch [again] for HG to strip those headers -- cheers.
> >   
> 
> The ideal for me is if you have a git tree I can pull from, 

There's still hope to get one. Didn't happen yet. Ian?

> but failing
> that, the most useful format for mailed patches is something that "git
> am" can accept.  It looks like it will take the attachments OK, but it
> cats the mail body onto the attachment, so it ends up with the commit
> comment twice.  If you have a non-mangling mailer, it's probably easiest
> if you just include the full commit comment+patch in the mail body (it's
> easier to comment on the patches in mail that way too).

Okay, whatever works.

> > Oh beautiful. This apparently strips the first comment line together
> > with the patch header. Looks buggy.
> >
> > F*! 
> >
> > Sorry. Fix/resend?
> >   
> 
> Did it lose something other than the subject?  

No.

> git am picked it up from
> the email Subject: line, so it didn't matter it was missing from the
> patch comment itself.

Good.

Thanks,
Daniel