[patch] xen-blkback: sync I/O after backend disconnected

[patch] xen-blkback: sync I/O after backend disconnected

[Joe Jin]

When backend disconnect, sync IO requests to the disk.

Signed-off-by: Joe Jin <joe.jin@oracle.com>
Cc: Jens Axboe <jaxboe@fusionio.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
---
 drivers/block/xen-blkback/xenbus.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index 1a87f5b..da39d61 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -35,6 +35,7 @@ static void connect(struct backend_info *);
 static int connect_ring(struct backend_info *);
 static void backend_changed(struct xenbus_watch *, const char **,
 			    unsigned int);
+static void xen_vbd_sync(struct xen_vbd *vbd);
 
 struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be)
 {
@@ -232,6 +233,7 @@ static void xen_blkif_disconnect(struct xen_blkif *blkif)
 		free_vm_area(blkif->blk_ring_area);
 		blkif->blk_rings.common.sring = NULL;
 	}
+	xen_vbd_sync(&blkif->vbd);
 }
 
 void xen_blkif_free(struct xen_blkif *blkif)
@@ -332,6 +334,12 @@ static void xen_vbd_free(struct xen_vbd *vbd)
 	vbd->bdev = NULL;
 }
 
+static void xen_vbd_sync(struct xen_vbd *vbd)
+{
+	if (vbd->bdev)
+		fsync_bdev(vbd->bdev);
+}
+
 static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle,
 			  unsigned major, unsigned minor, int readonly,
 			  int cdrom)

[patch] xen-blkback: sync I/O after backend disconnected

[Joe Jin]

When backend disconnect, sync IO requests to the disk.

Signed-off-by: Joe Jin <joe.jin@oracle.com>
Cc: Jens Axboe <jaxboe@fusionio.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Ian Campbell <Ian.Campbell@eu.citrix.com>
---
 drivers/block/xen-blkback/xenbus.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/block/xen-blkback/xenbus.c b/drivers/block/xen-blkback/xenbus.c
index 1a87f5b..da39d61 100644
--- a/drivers/block/xen-blkback/xenbus.c
+++ b/drivers/block/xen-blkback/xenbus.c
@@ -35,6 +35,7 @@ static void connect(struct backend_info *);
 static int connect_ring(struct backend_info *);
 static void backend_changed(struct xenbus_watch *, const char **,
 			    unsigned int);
+static void xen_vbd_sync(struct xen_vbd *vbd);
 
 struct xenbus_device *xen_blkbk_xenbus(struct backend_info *be)
 {
@@ -232,6 +233,7 @@ static void xen_blkif_disconnect(struct xen_blkif *blkif)
 		free_vm_area(blkif->blk_ring_area);
 		blkif->blk_rings.common.sring = NULL;
 	}
+	xen_vbd_sync(&blkif->vbd);
 }
 
 void xen_blkif_free(struct xen_blkif *blkif)
@@ -332,6 +334,12 @@ static void xen_vbd_free(struct xen_vbd *vbd)
 	vbd->bdev = NULL;
 }
 
+static void xen_vbd_sync(struct xen_vbd *vbd)
+{
+	if (vbd->bdev)
+		fsync_bdev(vbd->bdev);
+}
+
 static int xen_vbd_create(struct xen_blkif *blkif, blkif_vdev_t handle,
 			  unsigned major, unsigned minor, int readonly,
 			  int cdrom)

Re: [patch] xen-blkback: sync I/O after backend disconnected

[Christoph Hellwig]

On Mon, Aug 15, 2011 at 12:55:02PM +0800, Joe Jin wrote:
> When backend disconnect, sync IO requests to the disk.

Care to explain why?

Also you'll just need a sync_blockdev, fsync_bdev does far to many
things that don't make any sense when you don't have a file system
mounted on a device.

Re: [Xen-devel] Re: [patch] xen-blkback: sync I/O after backend disconnected

[Konrad Rzeszutek Wilk]

On Mon, Aug 15, 2011 at 10:46:10AM -0400, Christoph Hellwig wrote:
> On Mon, Aug 15, 2011 at 12:55:02PM +0800, Joe Jin wrote:
> > When backend disconnect, sync IO requests to the disk.
> 
> Care to explain why?

I was thinking it might be a good idea to do that when a disk
(file, LVM, real block device) is released from a guest just in case
there is some outstanding I/Os. But then I realized that we
bypasses the page cache anyhow - so there should be no outstanding I/O
requests - unless they are in the disk queue.

And the guest would normally issues a FLUSH when unmounting the
disk. Hm, I wonder what the conditions are when we forcibly kill the
guest - there might be outstanding I/Os in the disk's cache -
at which point we should probably sync the write cache, no?

> 
> Also you'll just need a sync_blockdev, fsync_bdev does far to many
> things that don't make any sense when you don't have a file system
> mounted on a device.
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xensource.com
> http://lists.xensource.com/xen-devel

Re: [patch] xen-blkback: sync I/O after backend disconnected

[Joe Jin]

On 08/15/11 22:46, Christoph Hellwig wrote:
> On Mon, Aug 15, 2011 at 12:55:02PM +0800, Joe Jin wrote:
>> When backend disconnect, sync IO requests to the disk.
> 
> Care to explain why?

When backend disconnect, I think we'd better flush all dirty data 
to the disk ASAP.

> 
> Also you'll just need a sync_blockdev, fsync_bdev does far to many
> things that don't make any sense when you don't have a file system
> mounted on a device.
> 

xen-blkback support physical device and loopback file, so I think
here should be fsync_bdev()? 

Thanks,
Joe

Re: [patch] xen-blkback: sync I/O after backend disconnected

[Joe Jin]

On 08/15/11 22:46, Christoph Hellwig wrote:
> On Mon, Aug 15, 2011 at 12:55:02PM +0800, Joe Jin wrote:
>> When backend disconnect, sync IO requests to the disk.
> 
> Care to explain why?

When backend disconnect, I think we'd better flush all dirty data 
to the disk ASAP.

> 
> Also you'll just need a sync_blockdev, fsync_bdev does far to many
> things that don't make any sense when you don't have a file system
> mounted on a device.
> 

xen-blkback support physical device and loopback file, so I think
here should be fsync_bdev()? 

Thanks,
Joe

Re: [patch] xen-blkback: sync I/O after backend disconnected

[Christoph Hellwig]

On Tue, Aug 16, 2011 at 02:56:30PM +0800, Joe Jin wrote:
> On 08/15/11 22:46, Christoph Hellwig wrote:
> > On Mon, Aug 15, 2011 at 12:55:02PM +0800, Joe Jin wrote:
> >> When backend disconnect, sync IO requests to the disk.
> > 
> > Care to explain why?
> 
> When backend disconnect, I think we'd better flush all dirty data 
> to the disk ASAP.

Then please state this in the commit log, and even better the code
also.

> > Also you'll just need a sync_blockdev, fsync_bdev does far to many
> > things that don't make any sense when you don't have a file system
> > mounted on a device.
> > 
> 
> xen-blkback support physical device and loopback file, so I think
> here should be fsync_bdev()? 

No, and in fact sync_blockdev is also wrong when thinking about it.
blkback always submits bios directly to the device, so flushing any
kernel caches won't do you any help.  The only thing that might make
sense to flush would be the disk write cache using a blkdev_issue_flush.

Re: [Xen-devel] Re: [patch] xen-blkback: sync I/O after backend disconnected

[Ian Jackson]

Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] Re: [patch] xen-blkback: sync I/O after backend disconnected"):
> And the guest would normally issues a FLUSH when unmounting the
> disk. Hm, I wonder what the conditions are when we forcibly kill the
> guest - there might be outstanding I/Os in the disk's cache -
> at which point we should probably sync the write cache, no?

If we forcibly kill the guest we don't care about its IO in flight.
After all we are throwing away everything that the guest has in its
queue.

Bear in mind that the reason for forcibly killing (or perhaps forcibly
detaching) might be that the underlying device has wedged somehow.  It
would be annoying if that prevented even a force detach.

Or to put it in other words: force detach and force kill should be
lossy.  Their whole point is to be the lossy version.

Ian.

Re: [Xen-devel] Re: [patch] xen-blkback: sync I/O after backend disconnected

[Konrad Rzeszutek Wilk]

On Thu, Aug 25, 2011 at 05:15:08PM +0100, Ian Jackson wrote:
> Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] Re: [patch] xen-blkback: sync I/O after backend disconnected"):
> > And the guest would normally issues a FLUSH when unmounting the
> > disk. Hm, I wonder what the conditions are when we forcibly kill the
> > guest - there might be outstanding I/Os in the disk's cache -
> > at which point we should probably sync the write cache, no?
> 
> If we forcibly kill the guest we don't care about its IO in flight.

Why don't we care about it? It might have become wedged but that does
not mean we don't want to preserve as much as possible.

> After all we are throwing away everything that the guest has in its
> queue.

We end up finishing all of the outstanding I/Os that guest has sent.
When all of the bio_submit callbacks have finished then we release
the guest backend. So adding in a 'SYNC' to the queue would force
the disk cache to flush all the writes at least.

> 
> Bear in mind that the reason for forcibly killing (or perhaps forcibly
> detaching) might be that the underlying device has wedged somehow.  It
> would be annoying if that prevented even a force detach.
> 
> Or to put it in other words: force detach and force kill should be
> lossy.  Their whole point is to be the lossy version.

Hm, I think you still end up holding on the request queue and not
freeing the blkback thread and all of its goodies until the bio callbacks
have completed. When the device gets wedged they become wedged too.
You have to wait for the error handler to give up and then the avalanche
of 'write page lost' and 'error secxtor XX' along with the -ENXIO being
returned ends up in the bio callbacks.

What I am saying is that the force detach is still stuck waiting
if the device has wedged - irregardless of this patch.

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

Re: Re: [patch] xen-blkback: sync I/O after backend disconnected

[Konrad Rzeszutek Wilk]

On Thu, Aug 25, 2011 at 05:15:08PM +0100, Ian Jackson wrote:
> Konrad Rzeszutek Wilk writes ("Re: [Xen-devel] Re: [patch] xen-blkback: sync I/O after backend disconnected"):
> > And the guest would normally issues a FLUSH when unmounting the
> > disk. Hm, I wonder what the conditions are when we forcibly kill the
> > guest - there might be outstanding I/Os in the disk's cache -
> > at which point we should probably sync the write cache, no?
> 
> If we forcibly kill the guest we don't care about its IO in flight.

Why don't we care about it? It might have become wedged but that does
not mean we don't want to preserve as much as possible.

> After all we are throwing away everything that the guest has in its
> queue.

We end up finishing all of the outstanding I/Os that guest has sent.
When all of the bio_submit callbacks have finished then we release
the guest backend. So adding in a 'SYNC' to the queue would force
the disk cache to flush all the writes at least.

> 
> Bear in mind that the reason for forcibly killing (or perhaps forcibly
> detaching) might be that the underlying device has wedged somehow.  It
> would be annoying if that prevented even a force detach.
> 
> Or to put it in other words: force detach and force kill should be
> lossy.  Their whole point is to be the lossy version.

Hm, I think you still end up holding on the request queue and not
freeing the blkback thread and all of its goodies until the bio callbacks
have completed. When the device gets wedged they become wedged too.
You have to wait for the error handler to give up and then the avalanche
of 'write page lost' and 'error secxtor XX' along with the -ENXIO being
returned ends up in the bio callbacks.

What I am saying is that the force detach is still stuck waiting
if the device has wedged - irregardless of this patch.

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