All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] Ensure blktap reports I/O errors back to guest
@ 2006-12-01 16:20 Daniel P. Berrange
  2006-12-01 16:55 ` Andrew Warfield
  0 siblings, 1 reply; 2+ messages in thread
From: Daniel P. Berrange @ 2006-12-01 16:20 UTC (permalink / raw)
  To: xen-devel

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

There are a number of flaws in the blktap userspace daemon when dealing 
with I/O errors.

 - The backends which use AIO check the io_events.res member to determine
   if an I/O error occurred. Which is good. But when calling the callback
   to signal completion of the I/O, they pass the io_events.res2 member

   Now this seems fine at first glance[1]

     "res is the usual result of an I/O operation: the number of bytes 
      transfered, or a negative error code. res2 is a second status  
      value which will be returned to the user"

   Except that

      "currently (2.6.0-test9), callers of aio_complete() within the 
       kernel always set res2 to zero."

   And this hasn't changed anytime since 2.6.0, so by passing through
   the status from 'res2', the callback thinks the I/O operation succeeded
   even when it failed :-(

   The fix is simple instead of passing 'res2', just pass

      ep->res == io->u.c.nbytes ? 0 : 1

   This would solve the error reporting to the guest, except that there
   is a second flaw...

 - The tapdisk I/O completion callback checks the status parameter
   passed in, syslog's it and then returns. It never bothers to send
   the I/O completion response back to the blktap kernel driver when
   a failure occurrs.

   Fortunately the fix for this is also simple. Instead of returning
   from the callback when dealing with an error, we simply toggle the
   status field for the pending response to BLKIF_RSP_ERROR and then
   continue with the normal codepath. So the error eventually gets
   back to the guest.


The scenario I used to discover the problem and test the patch is thus:

 - In dom0  create a filesystem with only 200 MB of free space
 - Create a 1 GB sparse file on this volume.
 - Configure the guest so this sparse file appears as /dev/xvdb
 - In the domU create a single partition on /dev/xvdb and format
   it with ext3.
 - In the DomU, mount /dev/xvdb1 on /mnt and then run

      dd if=/dev/zero of=/mnt/data.bin bs=1GB count=1


Without this patch, the 'dd' command would succeed in writing 1 GB of data
even though the underlying disk in Dom0 was only 200 MB in size. More complex
tests of copying a whole directory heirarchy across resulted in catastrophic
data corruption of the filessytem itself. Manual fsck was needed to fixup
the filesystem & there were many very bad errors needing fixing.


With this patch applied the DomU sees the I/O failures and kernel  logs
messages

Dec  1 11:02:53 dhcp-5-203 kernel: end_request: I/O error, dev xvdc, sector 722127
Dec  1 11:02:53 dhcp-5-203 kernel: end_request: I/O error, dev xvdc, sector 730327
Dec  1 11:02:53 dhcp-5-203 kernel: end_request: I/O error, dev xvdc, sector 738527
Dec  1 11:02:53 dhcp-5-203 kernel: end_request: I/O error, dev xvdc, sector 746727
Dec  1 11:02:53 dhcp-5-203 kernel: end_request: I/O error, dev xvdc, sector 754927
Dec  1 11:02:53 dhcp-5-203 kernel: end_request: I/O error, dev xvdc, sector 763127
Dec  1 11:02:53 dhcp-5-203 kernel: end_request: I/O error, dev xvdc, sector 771327
Dec  1 11:02:53 dhcp-5-203 kernel: end_request: I/O error, dev xvdc, sector 779527
Dec  1 11:02:53 dhcp-5-203 kernel: end_request: I/O error, dev xvdc, sector 792399

It will retry the I/O operation until it runs out of sectors to try, and then
fail the operation. The filesystem is not seriously damaged - ext3 journal
recovery will trivially cleanup if the guest is rebooted after the disk in
Dom0 is enlarged.

   Signed-off-by: Daniel P. Berrange <berrange@redhat.com>

Regards,
Dan.

[1] http://lwn.net/Articles/24366/
-- 
|=- Red Hat, Engineering, Emerging Technologies, Boston.  +1 978 392 2496 -=|
|=-           Perl modules: http://search.cpan.org/~danberr/              -=|
|=-               Projects: http://freshmeat.net/~danielpb/               -=|
|=-  GnuPG: 7D3B9505   F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505  -=| 

[-- Attachment #2: xen-blktap-report-errors.patch --]
[-- Type: text/plain, Size: 2298 bytes --]

diff -u xen-3.0.3_0-src.orig/tools/blktap/drivers/block-aio.c xen-3.0.3_0-src/tools/blktap/drivers/block-aio.c
--- xen-3.0.3_0-src.orig/tools/blktap/drivers/block-aio.c	2006-11-16 14:10:25.000000000 -0500
+++ xen-3.0.3_0-src/tools/blktap/drivers/block-aio.c	2006-12-01 11:00:18.000000000 -0500
@@ -358,12 +358,8 @@
 		struct pending_aio *pio;
 		
 		pio = &prv->pending_aio[(long)io->data];
-		
-		if (ep->res != io->u.c.nbytes) {
-			/* TODO: handle this case better. */
-			DPRINTF("AIO did less than I asked it to. \n");
-		}
-		rsp += pio->cb(s, ep->res2, pio->id, pio->private);
+		rsp += pio->cb(s, ep->res == io->u.c.nbytes ? 0 : 1,
+			       pio->id, pio->private);
 
 		prv->iocb_free[prv->iocb_free_count++] = io;
 	}
diff -u xen-3.0.3_0-src.orig/tools/blktap/drivers/block-qcow.c xen-3.0.3_0-src/tools/blktap/drivers/block-qcow.c
--- xen-3.0.3_0-src.orig/tools/blktap/drivers/block-qcow.c	2006-10-15 08:22:03.000000000 -0400
+++ xen-3.0.3_0-src/tools/blktap/drivers/block-qcow.c	2006-12-01 11:03:27.000000000 -0500
@@ -1145,13 +1145,6 @@
 
                 pio = &prv->pending_aio[(long)io->data];
 
-                if (ep->res != io->u.c.nbytes) {
-                        /* TODO: handle this case better. */
-			ptr = (int *)&ep->res;
-                        DPRINTF("AIO did less than I asked it to "
-				"[%lu,%lu,%d]\n", 
-				ep->res, io->u.c.nbytes, *ptr);
-                }
 		aio_unlock(prv, pio->sector);
 		if (pio->id >= 0) {
 			if (prv->crypt_method)
@@ -1162,7 +1155,7 @@
 						&prv->aes_decrypt_key);
 			prv->nr_reqs[pio->qcow_idx]--;
 			if (prv->nr_reqs[pio->qcow_idx] == 0) 
-				rsp += pio->cb(s, ep->res2, pio->id, 
+			        rsp += pio->cb(s, ep->res == io->u.c.nbytes ? 0 : 1, pio->id, 
 					       pio->private);
 		} else if (pio->id == -2) free(pio->buf);
 
diff -u xen-3.0.3_0-src.orig/tools/blktap/drivers/tapdisk.c xen-3.0.3_0-src/tools/blktap/drivers/tapdisk.c
--- xen-3.0.3_0-src.orig/tools/blktap/drivers/tapdisk.c	2006-10-15 08:22:03.000000000 -0400
+++ xen-3.0.3_0-src/tools/blktap/drivers/tapdisk.c	2006-12-01 11:00:04.000000000 -0500
@@ -445,8 +445,7 @@
 	}
 	
 	if (res != 0) {
-		DPRINTF("*** request error %d! \n", res);
-		return 0;
+	        blkif->pending_list[idx].status = BLKIF_RSP_ERROR;
 	}
 
 	blkif->pending_list[idx].count--;

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

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

^ permalink raw reply	[flat|nested] 2+ messages in thread
end of thread, other threads:[~2006-12-01 16:55 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2006-12-01 16:20 [PATCH] Ensure blktap reports I/O errors back to guest Daniel P. Berrange
2006-12-01 16:55 ` Andrew Warfield

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.