[PATCH 1/2] NBD: set uninitialized devices to size 0

[PATCH 1/2] NBD: set uninitialized devices to size 0

[Paul Clements]

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

This fixes errors with utilities (such as LVM's vgscan) that try to scan 
all devices. Previously this would generate read errors when 
uninitialized nbd devices were scanned:

# vgscan
   Reading all physical volumes.  This may take a while...
   /dev/nbd0: read failed after 0 of 1024 at 0: Input/output error
   /dev/nbd0: read failed after 0 of 1024 at 509804544: Input/output error
   /dev/nbd0: read failed after 0 of 2048 at 0: Input/output error
   /dev/nbd1: read failed after 0 of 1024 at 509804544: Input/output error
   /dev/nbd1: read failed after 0 of 2048 at 0: Input/output error


 From now on, uninitialized nbd devices will have size zero, which 
prevents these errors.

Patch applies and was tested against 2.6.23-rc2-mm2.

Thanks,
Paul

[-- Attachment #2: nbd_size_zero.diff --]
[-- Type: text/x-patch, Size: 1068 bytes --]


Signed-Off-By: Paul Clements <paul.clements@steeleye.com>

--- ./drivers/block/nbd.c.ERROR_RETURNS	2007-08-15 15:41:19.000000000 -0400
+++ ./drivers/block/nbd.c	2007-08-16 13:08:06.000000000 -0400
@@ -589,6 +589,9 @@ static int nbd_ioctl(struct inode *inode
 		printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);
 		if (file)
 			fput(file);
+		lo->bytesize = 0;
+		inode->i_bdev->bd_inode->i_size = 0;
+		set_capacity(lo->disk, 0);
 		return lo->harderror;
 	case NBD_CLEAR_QUE:
 		/*
@@ -666,14 +669,14 @@ static int __init nbd_init(void)
 		mutex_init(&nbd_dev[i].tx_lock);
 		init_waitqueue_head(&nbd_dev[i].active_wq);
 		nbd_dev[i].blksize = 1024;
-		nbd_dev[i].bytesize = 0x7ffffc00ULL << 10; /* 2TB */
+		nbd_dev[i].bytesize = 0;
 		disk->major = NBD_MAJOR;
 		disk->first_minor = i;
 		disk->fops = &nbd_fops;
 		disk->private_data = &nbd_dev[i];
 		disk->flags |= GENHD_FL_SUPPRESS_PARTITION_INFO;
 		sprintf(disk->disk_name, "nbd%d", i);
-		set_capacity(disk, 0x7ffffc00ULL << 1); /* 2 TB */
+		set_capacity(disk, 0);
 		add_disk(disk);
 	}
 

[PATCH 2/2] NBD: allow hung network I/O to be cancelled

[Paul Clements]

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

This patch allows NBD I/O to be cancelled when a network outage occurs. 
Previously, I/O would just hang, and if enough I/O was hung in nbd, the 
system (at least user-level) would completely hang until a TCP timeout 
(default, 15 minutes) occurred.

The patch introduces a new ioctl NBD_SET_TIMEOUT that allows a transmit 
timeout value (in seconds) to be specified. Any network send that 
exceeds the timeout will be cancelled and the nbd connection will be 
shut down. I've tested with various timeout values and 6 seconds seems 
to be a good choice for the timeout. If the NBD_SET_TIMEOUT ioctl is not 
called, you get the old (I/O hang) behavior.

Patch applies and was tested against 2.6.23-rc2-mm2.

Thanks,
Paul

[-- Attachment #2: nbd_xmit_timeout_26.diff --]
[-- Type: text/x-patch, Size: 7309 bytes --]


Signed-Off-By: Paul Clements <paul.clements@steeleye.com>

--- ./drivers/block/nbd.c.SIZE_ZERO	2007-08-16 13:08:06.000000000 -0400
+++ ./drivers/block/nbd.c	2007-08-23 16:42:09.000000000 -0400
@@ -113,12 +113,42 @@ static void nbd_end_request(struct reque
 	spin_unlock_irqrestore(q->queue_lock, flags);
 }
 
+static void sock_shutdown(struct nbd_device *lo, int lock)
+{
+	/* Forcibly shutdown the socket causing all listeners
+	 * to error
+	 *
+	 * FIXME: This code is duplicated from sys_shutdown, but
+	 * there should be a more generic interface rather than
+	 * calling socket ops directly here */
+	if (lock)
+		mutex_lock(&lo->tx_lock);
+	if (lo->sock) {
+		printk(KERN_WARNING "%s: shutting down socket\n",
+			lo->disk->disk_name);
+		lo->sock->ops->shutdown(lo->sock, SEND_SHUTDOWN|RCV_SHUTDOWN);
+		lo->sock = NULL;
+	}
+	if (lock)
+		mutex_unlock(&lo->tx_lock);
+}
+
+static void nbd_xmit_timeout(unsigned long arg)
+{
+	struct task_struct *task = (struct task_struct *)arg;
+
+	printk(KERN_WARNING "nbd: killing hung xmit (%s, pid: %d)\n",
+		task->comm, task->pid);
+	force_sig(SIGKILL, task);
+}
+
 /*
  *  Send or receive packet.
  */
-static int sock_xmit(struct socket *sock, int send, void *buf, int size,
+static int sock_xmit(struct nbd_device *lo, int send, void *buf, int size,
 		int msg_flags)
 {
+	struct socket *sock = lo->sock;
 	int result;
 	struct msghdr msg;
 	struct kvec iov;
@@ -139,9 +169,20 @@ static int sock_xmit(struct socket *sock
 		msg.msg_controllen = 0;
 		msg.msg_flags = msg_flags | MSG_NOSIGNAL;
 
-		if (send)
+		if (send) {
+			struct timer_list ti;
+
+			if (lo->xmit_timeout) {
+				init_timer(&ti);
+				ti.function = nbd_xmit_timeout;
+				ti.data = (unsigned long)current;
+				ti.expires = jiffies + lo->xmit_timeout;
+				add_timer(&ti);
+			}
 			result = kernel_sendmsg(sock, &msg, &iov, 1, size);
-		else
+			if (lo->xmit_timeout)
+				del_timer_sync(&ti);
+		} else
 			result = kernel_recvmsg(sock, &msg, &iov, 1, size, 0);
 
 		if (signal_pending(current)) {
@@ -150,6 +191,7 @@ static int sock_xmit(struct socket *sock
 				current->pid, current->comm,
 				dequeue_signal_lock(current, &current->blocked, &info));
 			result = -EINTR;
+			sock_shutdown(lo, !send);
 			break;
 		}
 
@@ -167,23 +209,22 @@ static int sock_xmit(struct socket *sock
 	return result;
 }
 
-static inline int sock_send_bvec(struct socket *sock, struct bio_vec *bvec,
+static inline int sock_send_bvec(struct nbd_device *lo, struct bio_vec *bvec,
 		int flags)
 {
 	int result;
 	void *kaddr = kmap(bvec->bv_page);
-	result = sock_xmit(sock, 1, kaddr + bvec->bv_offset, bvec->bv_len,
-			flags);
+	result = sock_xmit(lo, 1, kaddr + bvec->bv_offset, bvec->bv_len, flags);
 	kunmap(bvec->bv_page);
 	return result;
 }
 
+/* always call with the tx_lock held */
 static int nbd_send_req(struct nbd_device *lo, struct request *req)
 {
 	int result, i, flags;
 	struct nbd_request request;
 	unsigned long size = req->nr_sectors << 9;
-	struct socket *sock = lo->sock;
 
 	request.magic = htonl(NBD_REQUEST_MAGIC);
 	request.type = htonl(nbd_cmd(req));
@@ -196,8 +237,8 @@ static int nbd_send_req(struct nbd_devic
 			nbdcmd_to_ascii(nbd_cmd(req)),
 			(unsigned long long)req->sector << 9,
 			req->nr_sectors << 9);
-	result = sock_xmit(sock, 1, &request, sizeof(request),
-			(nbd_cmd(req) == NBD_CMD_WRITE)? MSG_MORE: 0);
+	result = sock_xmit(lo, 1, &request, sizeof(request),
+			(nbd_cmd(req) == NBD_CMD_WRITE) ? MSG_MORE : 0);
 	if (result <= 0) {
 		printk(KERN_ERR "%s: Send control failed (result %d)\n",
 				lo->disk->disk_name, result);
@@ -219,7 +260,7 @@ static int nbd_send_req(struct nbd_devic
 				dprintk(DBG_TX, "%s: request %p: sending %d bytes data\n",
 						lo->disk->disk_name, req,
 						bvec->bv_len);
-				result = sock_send_bvec(sock, bvec, flags);
+				result = sock_send_bvec(lo, bvec, flags);
 				if (result <= 0) {
 					printk(KERN_ERR "%s: Send data failed (result %d)\n",
 							lo->disk->disk_name,
@@ -261,11 +302,11 @@ out:
 	return ERR_PTR(err);
 }
 
-static inline int sock_recv_bvec(struct socket *sock, struct bio_vec *bvec)
+static inline int sock_recv_bvec(struct nbd_device *lo, struct bio_vec *bvec)
 {
 	int result;
 	void *kaddr = kmap(bvec->bv_page);
-	result = sock_xmit(sock, 0, kaddr + bvec->bv_offset, bvec->bv_len,
+	result = sock_xmit(lo, 0, kaddr + bvec->bv_offset, bvec->bv_len,
 			MSG_WAITALL);
 	kunmap(bvec->bv_page);
 	return result;
@@ -277,10 +318,9 @@ static struct request *nbd_read_stat(str
 	int result;
 	struct nbd_reply reply;
 	struct request *req;
-	struct socket *sock = lo->sock;
 
 	reply.magic = 0;
-	result = sock_xmit(sock, 0, &reply, sizeof(reply), MSG_WAITALL);
+	result = sock_xmit(lo, 0, &reply, sizeof(reply), MSG_WAITALL);
 	if (result <= 0) {
 		printk(KERN_ERR "%s: Receive control failed (result %d)\n",
 				lo->disk->disk_name, result);
@@ -322,7 +362,7 @@ static struct request *nbd_read_stat(str
 		rq_for_each_bio(bio, req) {
 			struct bio_vec *bvec;
 			bio_for_each_segment(bvec, bio, i) {
-				result = sock_recv_bvec(sock, bvec);
+				result = sock_recv_bvec(lo, bvec);
 				if (result <= 0) {
 					printk(KERN_ERR "%s: Receive data failed (result %d)\n",
 							lo->disk->disk_name,
@@ -401,6 +441,7 @@ static void nbd_clear_que(struct nbd_dev
 	}
 }
 
+
 /*
  * We always wait for result of write, for now. It would be nice to make it optional
  * in future
@@ -511,7 +552,9 @@ static int nbd_ioctl(struct inode *inode
 		sreq.nr_sectors = 0;
                 if (!lo->sock)
 			return -EINVAL;
+		mutex_lock(&lo->tx_lock);
                 nbd_send_req(lo, &sreq);
+		mutex_unlock(&lo->tx_lock);
                 return 0;
  
 	case NBD_CLEAR_SOCK:
@@ -555,6 +598,9 @@ static int nbd_ioctl(struct inode *inode
 		set_blocksize(inode->i_bdev, lo->blksize);
 		set_capacity(lo->disk, lo->bytesize >> 9);
 		return 0;
+	case NBD_SET_TIMEOUT:
+		lo->xmit_timeout = arg * HZ;
+		return 0;
 	case NBD_SET_SIZE_BLOCKS:
 		lo->bytesize = ((u64) arg) * lo->blksize;
 		inode->i_bdev->bd_inode->i_size = lo->bytesize;
@@ -567,22 +613,7 @@ static int nbd_ioctl(struct inode *inode
 		error = nbd_do_it(lo);
 		if (error)
 			return error;
-		/* on return tidy up in case we have a signal */
-		/* Forcibly shutdown the socket causing all listeners
-		 * to error
-		 *
-		 * FIXME: This code is duplicated from sys_shutdown, but
-		 * there should be a more generic interface rather than
-		 * calling socket ops directly here */
-		mutex_lock(&lo->tx_lock);
-		if (lo->sock) {
-			printk(KERN_WARNING "%s: shutting down socket\n",
-				lo->disk->disk_name);
-			lo->sock->ops->shutdown(lo->sock,
-				SEND_SHUTDOWN|RCV_SHUTDOWN);
-			lo->sock = NULL;
-		}
-		mutex_unlock(&lo->tx_lock);
+		sock_shutdown(lo, 1);
 		file = lo->file;
 		lo->file = NULL;
 		nbd_clear_que(lo);
--- ./include/linux/nbd.h.PRISTINE	2007-08-22 13:15:45.000000000 -0400
+++ ./include/linux/nbd.h	2007-08-22 13:17:02.000000000 -0400
@@ -26,6 +26,7 @@
 #define NBD_PRINT_DEBUG	_IO( 0xab, 6 )
 #define NBD_SET_SIZE_BLOCKS	_IO( 0xab, 7 )
 #define NBD_DISCONNECT  _IO( 0xab, 8 )
+#define NBD_SET_TIMEOUT _IO( 0xab, 9 )
 
 enum {
 	NBD_CMD_READ = 0,
@@ -65,6 +66,7 @@ struct nbd_device {
 	int blksize;
 	u64 bytesize;
 	pid_t pid; /* pid of nbd-client, if attached */
+	int xmit_timeout;
 };
 
 #endif

Re: [PATCH 2/2] NBD: allow hung network I/O to be cancelled

[Mike Snitzer]

On 8/24/07, Paul Clements <paul.clements@steeleye.com> wrote:
> This patch allows NBD I/O to be cancelled when a network outage occurs.
> Previously, I/O would just hang, and if enough I/O was hung in nbd, the
> system (at least user-level) would completely hang until a TCP timeout
> (default, 15 minutes) occurred.
>
> The patch introduces a new ioctl NBD_SET_TIMEOUT that allows a transmit
> timeout value (in seconds) to be specified. Any network send that
> exceeds the timeout will be cancelled and the nbd connection will be
> shut down. I've tested with various timeout values and 6 seconds seems
> to be a good choice for the timeout. If the NBD_SET_TIMEOUT ioctl is not
> called, you get the old (I/O hang) behavior.

Hi Paul,

Thanks for implementing this!  Do you happen to have an associated
nbd-client patch for userspace?  If not I'd be happy to coordinate
with you and Wouter on a patch.

regards,
Mike

Re: [PATCH 2/2] NBD: allow hung network I/O to be cancelled

[Paul Clements]

Mike Snitzer wrote:
> On 8/24/07, Paul Clements <paul.clements@steeleye.com> wrote:
>> This patch allows NBD I/O to be cancelled when a network outage occurs.
>> Previously, I/O would just hang, and if enough I/O was hung in nbd, the
>> system (at least user-level) would completely hang until a TCP timeout
>> (default, 15 minutes) occurred.
>>
>> The patch introduces a new ioctl NBD_SET_TIMEOUT that allows a transmit
>> timeout value (in seconds) to be specified. Any network send that
>> exceeds the timeout will be cancelled and the nbd connection will be
>> shut down. I've tested with various timeout values and 6 seconds seems
>> to be a good choice for the timeout. If the NBD_SET_TIMEOUT ioctl is not
>> called, you get the old (I/O hang) behavior.
> 
> Hi Paul,
> 
> Thanks for implementing this!  Do you happen to have an associated
> nbd-client patch for userspace?  If not I'd be happy to coordinate
> with you and Wouter on a patch.

No, I don't. I just basically hardcoded my nbd-client to do a 6 second 
timeout by default, but Wouter will probably want to do something a 
little less hackish for the official nbd-client.

--
Paul

Re: [PATCH 1/2] NBD: set uninitialized devices to size 0

[Andrew Morton]

On Fri, 24 Aug 2007 13:06:39 -0400
Paul Clements <paul.clements@steeleye.com> wrote:

> This fixes errors with utilities (such as LVM's vgscan) that try to scan 
> all devices. Previously this would generate read errors when 
> uninitialized nbd devices were scanned:

I somewhat randomly marked both these as 2.6.24 material.  If you think
that was incorrect, please shout out.

Re: [PATCH 1/2] NBD: set uninitialized devices to size 0

[Bill Davidsen]

Andrew Morton wrote:
> On Fri, 24 Aug 2007 13:06:39 -0400
> Paul Clements <paul.clements@steeleye.com> wrote:
> 
>> This fixes errors with utilities (such as LVM's vgscan) that try to scan 
>> all devices. Previously this would generate read errors when 
>> uninitialized nbd devices were scanned:
> 
> I somewhat randomly marked both these as 2.6.24 material.  If you think
> that was incorrect, please shout out.

I have the feeling that I mentioned nbd issues several releases ago, but 
never got to getting more info on reproducing them. I try not to submit 
bugs I can't reproduce, oftem they're my fault :-(


-- 
Bill Davidsen <davidsen@tmr.com>
   "We have more to fear from the bungling of the incompetent than from
the machinations of the wicked."  - from Slashdot