Skip to contentSkip to conversation

nbd driver for 2.5+: fix locking issues with ioctl UI

RFC PATCH In review Lou Langholtz 7 participants 25 messages Jun 25, 2003 – Jun 30, 2003 1 message not in the archive

Subsystems (2)
Block layer Network block device (NBD)
lore
Share & tools
lore

Lou Langholtz

author
#0 lore raw

RFC PATCH 1/1 nbd driver for 2.5+: fix locking issues with ioctl UI

Please review and comment...

This is the sixth patch to the NBD driver and it fixes a variety of outstanding locking issues with the ioctl user interface. This patch modifies both drivers/block/nbd.c and include/linux/nbd.h. It's intended to be applied incrementally (on top of the fifth patch).

Of major note is that since the linux 2.5 change in the block layer code, the distributions of the nbd-client user tool that I am familiar with (including the one in my anbd distribution up through release 0.4) will already be unable to correctly set the size of the served block device. This happens because nbd-client opens the nbd, calls the set size ioctl on its descriptor and eventually calls the do_it ioctl with that same still opened descriptor. The set size ioctl essentially just calls set_capacity() on the relevant gendisk structure. In the new block layer code however, the nbd descriptor has to be released after the set size ioctl call and then re-opened in order for the correct size to be registered by other openers. This could also be worked around in the driver but it has not been done (yet).

That said, this seems like an opportunistic time to further break compatibility with the existing nbd-client user tool and do away with the problematic components of the ioctl user interface. This patch deprecates two ioctl's (NBD_SET_SOCK and NBD_CLEAR_SOCK) and changes the usage of the NBD_DO_IT ioctl to take the socket descriptor as its argument. As such, NBD_DO_IT rolls all the functionality of the three ioctls into just one. It simplifies the user interface and makes resource locking much easier. Consequently, several race conditions can be cleaned up in the driver itself (rather than via work arounds in nbd-client).

There are other ways of course... the new NBD_DO_IT style interface could be introduced instead as another ioctl completely and these 3 ioctls could be maintained for backward compatibility for a while longer. That seems contradictory though to the idea of keeping the newest code the cleanest but at least it doesn't break existing nbd-client tools.

So look at the patch if you're interested and let me know what you think. Please don't make it part of the kernel distro though, at least not yet. I'll be "disconnected" from Thursday through Sunday so I won't be able to do any fixes should this introduce more problems than it fixes till next week.

drivers/block/nbd.c +150 -159
diff -urN linux-2.5.73-p5/drivers/block/nbd.c linux-2.5.73-p6/drivers/block/nbd.c
@@ -36,6 +36,11 @@
  * 03-06-24 Remove unneeded blksize_bits field from nbd_device struct.
  *   <[email protected]>
  * 03-06-24 Cleanup PARANOIA usage & code. <[email protected]>
+ * 03-06-24 Fix resource locking issues with ioctl user interface. Note that
+ *   this change is incompatible with existing user tools (nbd-client) and
+ *   require them to be updated. Actually, the linux 2.5 block layer redisign
+ *   already required existing user tools to be updated to properly set the
+ *   correct device size. <[email protected]>
  *
  * possible FIXME: make set_sock / set_blksize / set_size / do_it one syscall
  * why not: would need verify_area and friends, would share yet another 

@@ -137,21 +142,26 @@
 }
 #endif /* NDEBUG */
 
-static void nbd_end_request(struct request *req)
+static void request_end_while_locked(struct request *req)
 {
 	int uptodate = (req->errors == 0) ? 1 : 0;
-	request_queue_t *q = req->q;
-	unsigned long flags;
 
 	dprintk(DBG_BLKDEV, "%s: request %p: %s\n", req->rq_disk->disk_name,
 			req, uptodate? "done": "failed");
+	if (!end_that_request_first(req, uptodate, req->nr_sectors))
+		end_that_request_last(req);
 #ifdef PARANOIA
 	requests_out++;
 #endif
+}
+
+static void request_end(struct request *req)
+{
+	unsigned long flags;
+	request_queue_t *q = req->q;
+
 	spin_lock_irqsave(q->queue_lock, flags);
-	if (!end_that_request_first(req, uptodate, req->nr_sectors)) {
-		end_that_request_last(req);
-	}
+	request_end_while_locked(req);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 }
 

@@ -239,7 +249,7 @@
 	return result;
 }
 
-void nbd_send_req(struct nbd_device *lo, struct request *req)
+static void nbd_send_req(struct nbd_device *lo, struct request *req)
 {
 	int result, i, flags;
 	struct nbd_request request;

@@ -252,13 +262,7 @@
 	request.len = htonl(size);
 	memcpy(request.handle, &req, sizeof(req));
 
-	down(&lo->tx_lock);
-
-	if (!sock || !lo->sock) {
-		printk(KERN_ERR "%s: Attempted send on closed socket\n",
-				lo->disk->disk_name);
-		goto error_out;
-	}
+	BUG_ON(sock == NULL);
 
 	dprintk(DBG_TX, "%s: request %p: sending control (%s@%llu,%luB)\n",
 			lo->disk->disk_name, req,

@@ -297,11 +301,9 @@
 			}
 		}
 	}
-	up(&lo->tx_lock);
 	return;
 
-      error_out:
-	up(&lo->tx_lock);
+error_out:
 	req->errors++;
 }
 

@@ -398,28 +400,15 @@
 	return req;
 }
 
-void nbd_do_it(struct nbd_device *lo)
+static int nbd_clear_que(struct nbd_device *lo)
 {
 	struct request *req;
 
-#ifdef PARANOIA
-	BUG_ON(lo->magic != LO_MAGIC);
-#endif
-	while ((req = nbd_read_stat(lo)) != NULL)
-		nbd_end_request(req);
-	printk(KERN_NOTICE "%s: req should never be null\n",
-			lo->disk->disk_name);
-	return;
-}
-
-void nbd_clear_que(struct nbd_device *lo)
-{
-	struct request *req;
-
-#ifdef PARANOIA
-	BUG_ON(lo->magic != LO_MAGIC);
-#endif
-
+	down(&lo->tx_lock);
+	if (lo->sock) {
+		up(&lo->tx_lock);
+		return -EBUSY;
+	}
 	do {
 		req = NULL;
 		spin_lock(&lo->queue_lock);

@@ -430,16 +419,18 @@
 		spin_unlock(&lo->queue_lock);
 		if (req) {
 			req->errors++;
-			nbd_end_request(req);
+			request_end(req);
 		}
 	} while (req);
+	up(&lo->tx_lock);
+	return 0;
 }
 
 /*
  * We always wait for result of write, for now. It would be nice to make it optional
  * in future
  * if ((req->cmd == WRITE) && (lo->flags & NBD_WRITE_NOCHK)) 
- *   { printk( "Warning: Ignoring result!\n"); nbd_end_request( req ); }
+ *   { printk( "Warning: Ignoring result!\n"); request_end( req ); }
  */
 
 static void do_nbd_request(request_queue_t * q)

@@ -448,75 +439,69 @@
 	
 	while ((req = elv_next_request(q)) != NULL) {
 		struct nbd_device *lo;
+		int notready;
 
 		blkdev_dequeue_request(req);
+#ifdef PARANOIA
+		requests_in++;
+#endif
+		req->errors = 0;
 		dprintk(DBG_BLKDEV, "%s: request %p: dequeued (flags=%lx)\n",
 				req->rq_disk->disk_name, req, req->flags);
 
 		if (!(req->flags & REQ_CMD))
-			goto error_out;
+			goto fail_request;
 
 		lo = req->rq_disk->private_data;
 #ifdef PARANOIA
 		BUG_ON(lo->magic != LO_MAGIC);
 #endif
-		if (!lo->file) {
-			printk(KERN_ERR "%s: Request when not-ready\n",
-					lo->disk->disk_name);
-			goto error_out;
-		}
+
 		nbd_cmd(req) = NBD_CMD_READ;
 		if (rq_data_dir(req) == WRITE) {
 			nbd_cmd(req) = NBD_CMD_WRITE;
 			if (lo->flags & NBD_READ_ONLY) {
 				printk(KERN_ERR "%s: Write on read-only\n",
 						lo->disk->disk_name);
-				goto error_out;
+				goto fail_request;
 			}
 		}
-#ifdef PARANOIA
-		requests_in++;
-#endif
 
-		req->errors = 0;
 		spin_unlock_irq(q->queue_lock);
-
-		spin_lock(&lo->queue_lock);
-
-		if (!lo->file) {
-			spin_unlock(&lo->queue_lock);
-			printk(KERN_ERR "%s: failed between accept and semaphore, file lost\n",
-					lo->disk->disk_name);
-			req->errors++;
-			nbd_end_request(req);
-			spin_lock_irq(q->queue_lock);
-			continue;
-		}
-
-		list_add(&req->queuelist, &lo->queue_head);
-		spin_unlock(&lo->queue_lock);
-
-		nbd_send_req(lo, req);
-
-		if (req->errors) {
-			printk(KERN_ERR "%s: Request send failed\n",
-					lo->disk->disk_name);
+		down(&lo->tx_lock);
+		if (lo->sock) {
+			notready = 0;
 			spin_lock(&lo->queue_lock);
-			list_del_init(&req->queuelist);
+			list_add(&req->queuelist, &lo->queue_head);
 			spin_unlock(&lo->queue_lock);
-			nbd_end_request(req);
-			spin_lock_irq(q->queue_lock);
-			continue;
+			nbd_send_req(lo, req);
+			if (req->errors) {
+				printk(KERN_ERR "%s: Request send failed\n",
+						lo->disk->disk_name);
+				spin_lock(&lo->queue_lock);
+				list_del_init(&req->queuelist);
+				spin_unlock(&lo->queue_lock);
+			}
 		}
-
+		else
+			notready = 1;
+		up(&lo->tx_lock);
 		spin_lock_irq(q->queue_lock);
+		if (notready) {
+			printk(KERN_ERR "%s: Request when not-ready\n",
+					lo->disk->disk_name);
+			req->errors++;
+			request_end_while_locked(req);
+		}
 		continue;
 
-	      error_out:
+fail_request:
+		/*
+		 * Fail the request: anyone waiting on a read or write gets
+		 * an error and can move on to their close() call.
+		 */
 		req->errors++;
-		spin_unlock(q->queue_lock);
-		nbd_end_request(req);
-		spin_lock(q->queue_lock);
+		request_end_while_locked(req);
 	}
 	return;
 }

@@ -548,12 +533,86 @@
 	return 0;
 }
 
+static int nbd_do_it(struct nbd_device *lo, unsigned int fd)
+{
+	struct file *file;
+	struct inode *inode;
+	struct socket *sock;
+	struct request *req;
+
+	file = fget(fd);
+	if (!file)
+		return -EBADF;
+	inode = file->f_dentry->d_inode;
+	if (!inode->i_sock) {
+		fput(file);
+		return -ENOTSOCK;
+	}
+	sock = SOCKET_I(inode);
+	if (sock->type != SOCK_STREAM) {
+		fput(file);
+		return -EPROTONOSUPPORT;
+	}
+
+	down(&lo->tx_lock);
+	if (lo->sock) {
+		up(&lo->tx_lock);
+		fput(file);
+		return -EBUSY;
+	}
+	lo->sock = sock;
+	up(&lo->tx_lock);
+
+	/* no need for rx lock since here is the rx... */
+	dprintk(DBG_IOCTL, "%s: nbd_do_it: commencing read loop\n",
+			lo->disk->disk_name);
+	while ((req = nbd_read_stat(lo)) != NULL)
+		request_end(req);
+	dprintk(DBG_IOCTL, "%s: nbd_do_it: read loop finished\n",
+			lo->disk->disk_name);
+
+	down(&lo->tx_lock);
+	lo->sock = NULL;
+	up(&lo->tx_lock);
+
+	fput(file);
+	return lo->harderror;
+}
+
+static int nbd_disconnect(struct nbd_device *lo)
+{
+	int error;
+	struct request sreq;
+
+	printk(KERN_INFO "%s: NBD_DISCONNECT\n", lo->disk->disk_name);
+
+	sreq.flags = REQ_SPECIAL;
+	nbd_cmd(&sreq) = NBD_CMD_DISC;
+
+	/*
+	 * Set sreq to sane values in case server implementation
+	 * fails to check the request type first and also to keep
+	 * debugging output cleaner.
+	 */
+	sreq.sector = 0;
+	sreq.nr_sectors = 0;
+
+	down(&lo->tx_lock);
+	if (lo->sock) {
+		error = 0;
+		nbd_send_req(lo, &sreq);
+	}
+	else {
+		error = -ENOTCONN;
+	}
+	up(&lo->tx_lock);
+	return error;
+}
+
 static int nbd_ioctl(struct inode *inode, struct file *file,
 		     unsigned int cmd, unsigned long arg)
 {
 	struct nbd_device *lo = inode->i_bdev->bd_disk->private_data;
-	int error;
-	struct request sreq ;
 
 #ifdef PARANOIA
 	BUG_ON(lo->magic != LO_MAGIC);

@@ -565,57 +624,15 @@
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	switch (cmd) {
+	case NBD_CLEAR_SOCK: /* deprecated! */
+	case NBD_SET_SOCK: /* deprecated! */
+		printk(KERN_WARNING "%s: %s[%d] called deprecated ioctl %s\n",
+				lo->disk->disk_name,
+				current->comm, current->pid,
+				ioctl_cmd_to_ascii(cmd));
+		return -EINVAL;
 	case NBD_DISCONNECT:
-	        printk(KERN_INFO "%s: NBD_DISCONNECT\n", lo->disk->disk_name);
-		sreq.flags = REQ_SPECIAL;
-		nbd_cmd(&sreq) = NBD_CMD_DISC;
-		/*
-		 * Set these to sane values in case server implementation
-		 * fails to check the request type first and also to keep
-		 * debugging output cleaner.
-		 */
-		sreq.sector = 0;
-		sreq.nr_sectors = 0;
-                if (!lo->sock)
-			return -EINVAL;
-                nbd_send_req(lo, &sreq);
-                return 0 ;
- 
-	case NBD_CLEAR_SOCK:
-		nbd_clear_que(lo);
-		spin_lock(&lo->queue_lock);
-		if (!list_empty(&lo->queue_head)) {
-			spin_unlock(&lo->queue_lock);
-			printk(KERN_ERR "%s: Some requests are in progress -> can not turn off.\n",
-					lo->disk->disk_name);
-			return -EBUSY;
-		}
-		file = lo->file;
-		if (!file) {
-			spin_unlock(&lo->queue_lock);
-			return -EINVAL;
-		}
-		lo->file = NULL;
-		lo->sock = NULL;
-		spin_unlock(&lo->queue_lock);
-		fput(file);
-		return 0;
-	case NBD_SET_SOCK:
-		if (lo->file)
-			return -EBUSY;
-		error = -EINVAL;
-		file = fget(arg);
-		if (file) {
-			inode = file->f_dentry->d_inode;
-			if (inode->i_sock) {
-				lo->file = file;
-				lo->sock = SOCKET_I(inode);
-				error = 0;
-			} else {
-				fput(file);
-			}
-		}
-		return error;
+		return nbd_disconnect(lo);
 	case NBD_SET_BLKSIZE:
 		if ((arg & (arg-1)) || (arg < 512) || (arg > PAGE_SIZE))
 			return -EINVAL;

@@ -632,34 +649,9 @@
 		set_capacity(lo->disk, lo->bytesize >> 9);
 		return 0;
 	case NBD_DO_IT:
-		if (!lo->file)
-			return -EINVAL;
-		nbd_do_it(lo);
-		/* 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 */
-		down(&lo->tx_lock);
-		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;
-		up(&lo->tx_lock);
-		spin_lock(&lo->queue_lock);
-		file = lo->file;
-		lo->file = NULL;
-		spin_unlock(&lo->queue_lock);
-		nbd_clear_que(lo);
-		printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);
-		if (file)
-			fput(file);
-		return lo->harderror;
+		return nbd_do_it(lo, arg);
 	case NBD_CLEAR_QUE:
-		nbd_clear_que(lo);
-		return 0;
+		return nbd_clear_que(lo);
 	case NBD_PRINT_DEBUG:
 #ifdef PARANOIA
 		printk(KERN_INFO "%s: next = %p, prev = %p. Global: in %d, out %d\n",

@@ -731,7 +723,6 @@
 	for (i = 0; i < MAX_NBD; i++) {
 		struct gendisk *disk = nbd_dev[i].disk;
 		nbd_dev[i].refcnt = 0;
-		nbd_dev[i].file = NULL;
 #ifdef PARANOIA
 		nbd_dev[i].magic = LO_MAGIC;
 #endif
include/linux/nbd.h +1 -1
diff -urN linux-2.5.73-p5/include/linux/nbd.h linux-2.5.73-p6/include/linux/nbd.h
@@ -8,6 +8,7 @@
  * 2003/06/24 Louis D. Langholtz <[email protected]>
  *            Removed unneeded blksize_bits field from nbd_device struct.
  *            Cleanup PARANOIA usage & code.
+ *            Removed unneeded file field from nbd_device struct.
  */
 
 #ifndef LINUX_NBD_H

@@ -42,7 +43,6 @@
 #define NBD_READ_ONLY 0x0001
 #define NBD_WRITE_NOCHK 0x0002
 	struct socket * sock;
-	struct file * file; 	/* If == NULL, device is not ready, yet	*/
 #ifdef PARANOIA
 	int magic;		/* FIXME: not if debugging is off	*/
 #endif
#1 lore raw
Quoted 4 lines · Lou Langholtz

That said, this seems like an opportunistic time to further break
compatibility with the existing nbd-client user tool and do away with the problematic components of the ioctl user interface.

Is a suitably modified userspace tool available?

author
#2 lore raw
Quoted 10 lines · Andrew Morton
Quoted 3 lines · Lou Langholtz

That said, this seems like an opportunistic time to further break compatibility with the existing nbd-client user tool and do away with the problematic components of the ioctl user interface.

Is a suitably modified userspace tool available?

Seems like a pandora's box (and/or catch-22)... A modified userspace tool is not yet available that I know of. I have a hacked version that I've been using to test things with of course. But it's not in a very nice state to release. And then Pavel's distro of the tool is probably what most people use anyway. I have a seperate distro in part because I wanted to just play with things at first and thought sharing the sources might be useful to someone. Etc. Anyways, I wanted to get a feeling first from the kernel hacking community how important back compatibility was to them w.r.t. this driver. Since the original jumbo patch also got criticized against for supporting multiple linux version in the driver sources (via the LINUX_VERSION macros), seems like there's some split in what people may prefer. I wanted to generate comments on these specific changes to solidify what changes would really be needed in the user space tool before also distributing a changed nbd-client. It would also help to have something like a version ioctl or something that could be keyed on to provide the correct support. On the other hand, I'm not sure that Pavel even realized that the block layer changes impacted the semantics of the sizing ioctl (such that the user-space tool needs to re-open the file descriptor to get the proper size in). I didn't realize this myself even till only a few days ago but I had been wondering all along why sizing hadn't been working right. Sigh. Just to many other things to also do. The driver could also fix this sizing issue but coding the change their feels more like a hack than in the client tool. On the bright side, Pavel's nbd-client could probably be ifdef'd quite easily to work-around/fix the sizing issue and also this new proposed ioctl interface. Let me catch up w/ LKML mail and see what others have to say so far. I can probably produce a diff for Pavel easily enough (more easily than for my distro of the same) so that we won't have these issues anymore. But then full circle, it all depends on how the community would like to see these issues resolved (ie. maybe they want the sizing fixed in the driver instead and personally it's a close balance).

author
#3 lore raw
In reply to Andrew Morton
Quoted 9 lines · Andrew Morton
Quoted 3 lines · Lou Langholtz

That said, this seems like an opportunistic time to further break compatibility with the existing nbd-client user tool and do away with the problematic components of the ioctl user interface.

Is a suitably modified userspace tool available?

Here is an updated patch 6 (I called it 6.1) that fixes some additional locking issues as well as fixing the header file so it can be used by user applications too (like the nbd-client tool). This patch is also incremental against patch 5 (that cleaned up PARANOI stuff). The updated header file could be split in two files. For now though, this is enough change at once (IMO).

I have also attached a patch to Pavel's nbd-2.0 release nbd tools that updates the nbd-client to work with linux 2.5 as well as 2.5.74 (assuming the aforementioned patch 6.1 made it into 2.5.74). Handling is switched at compile time however and uses <linux/version.h> to do the switching. This will have problems of course if the builder doesn't pay close attention to where there header file are coming from or tries to run the same binary on a different kernel release. Etc.

Again though, I'm submitting this as food for thought (this may not be ready to get into kernel distribution), and also to help get the nbd-client user space tool sync'd with developments that had long ago taken place in the kernel. The comments received so far have helped enormously at resolving the various issues. Keep them coming! :-)

I will be "disconnected" though shortly till Monday.

drivers/block/nbd.c +195 -171
diff -urN linux-2.5.73-p5/drivers/block/nbd.c linux-2.5.73-p6.1/drivers/block/nbd.c
@@ -36,6 +36,11 @@
  * 03-06-24 Remove unneeded blksize_bits field from nbd_device struct.
  *   <[email protected]>
  * 03-06-24 Cleanup PARANOIA usage & code. <[email protected]>
+ * 03-06-24 Fix resource locking issues with ioctl user interface. Note that
+ *   this change is incompatible with existing user tools (nbd-client) and
+ *   require them to be updated. Actually, the linux 2.5 block layer redisign
+ *   already required existing user tools to be updated to properly set the
+ *   correct device size. <[email protected]>
  *
  * possible FIXME: make set_sock / set_blksize / set_size / do_it one syscall
  * why not: would need verify_area and friends, would share yet another 

@@ -137,21 +142,26 @@
 }
 #endif /* NDEBUG */
 
-static void nbd_end_request(struct request *req)
+static void request_end_while_locked(struct request *req)
 {
 	int uptodate = (req->errors == 0) ? 1 : 0;
-	request_queue_t *q = req->q;
-	unsigned long flags;
 
 	dprintk(DBG_BLKDEV, "%s: request %p: %s\n", req->rq_disk->disk_name,
 			req, uptodate? "done": "failed");
+	if (!end_that_request_first(req, uptodate, req->nr_sectors))
+		end_that_request_last(req);
 #ifdef PARANOIA
 	requests_out++;
 #endif
+}
+
+static void request_end(struct request *req)
+{
+	unsigned long flags;
+	request_queue_t *q = req->q;
+
 	spin_lock_irqsave(q->queue_lock, flags);
-	if (!end_that_request_first(req, uptodate, req->nr_sectors)) {
-		end_that_request_last(req);
-	}
+	request_end_while_locked(req);
 	spin_unlock_irqrestore(q->queue_lock, flags);
 }
 

@@ -239,7 +249,7 @@
 	return result;
 }
 
-void nbd_send_req(struct nbd_device *lo, struct request *req)
+static void nbd_send_req(struct nbd_device *lo, struct request *req)
 {
 	int result, i, flags;
 	struct nbd_request request;

@@ -252,13 +262,7 @@
 	request.len = htonl(size);
 	memcpy(request.handle, &req, sizeof(req));
 
-	down(&lo->tx_lock);
-
-	if (!sock || !lo->sock) {
-		printk(KERN_ERR "%s: Attempted send on closed socket\n",
-				lo->disk->disk_name);
-		goto error_out;
-	}
+	BUG_ON(sock == NULL);
 
 	dprintk(DBG_TX, "%s: request %p: sending control (%s@%llu,%luB)\n",
 			lo->disk->disk_name, req,

@@ -297,11 +301,9 @@
 			}
 		}
 	}
-	up(&lo->tx_lock);
 	return;
 
-      error_out:
-	up(&lo->tx_lock);
+error_out:
 	req->errors++;
 }
 

@@ -398,28 +400,15 @@
 	return req;
 }
 
-void nbd_do_it(struct nbd_device *lo)
-{
-	struct request *req;
-
-#ifdef PARANOIA
-	BUG_ON(lo->magic != LO_MAGIC);
-#endif
-	while ((req = nbd_read_stat(lo)) != NULL)
-		nbd_end_request(req);
-	printk(KERN_NOTICE "%s: req should never be null\n",
-			lo->disk->disk_name);
-	return;
-}
-
-void nbd_clear_que(struct nbd_device *lo)
+static int nbd_clear_que(struct nbd_device *lo)
 {
 	struct request *req;
 
-#ifdef PARANOIA
-	BUG_ON(lo->magic != LO_MAGIC);
-#endif
-
+	down(&lo->tx_lock);
+	if (lo->sock) {
+		up(&lo->tx_lock);
+		return -EBUSY;
+	}
 	do {
 		req = NULL;
 		spin_lock(&lo->queue_lock);

@@ -430,16 +419,18 @@
 		spin_unlock(&lo->queue_lock);
 		if (req) {
 			req->errors++;
-			nbd_end_request(req);
+			request_end(req);
 		}
 	} while (req);
+	up(&lo->tx_lock);
+	return 0;
 }
 
 /*
  * We always wait for result of write, for now. It would be nice to make it optional
  * in future
  * if ((req->cmd == WRITE) && (lo->flags & NBD_WRITE_NOCHK)) 
- *   { printk( "Warning: Ignoring result!\n"); nbd_end_request( req ); }
+ *   { printk( "Warning: Ignoring result!\n"); request_end( req ); }
  */
 
 static void do_nbd_request(request_queue_t * q)

@@ -448,75 +439,69 @@
 	
 	while ((req = elv_next_request(q)) != NULL) {
 		struct nbd_device *lo;
+		int notready;
 
 		blkdev_dequeue_request(req);
+#ifdef PARANOIA
+		requests_in++;
+#endif
+		req->errors = 0;
 		dprintk(DBG_BLKDEV, "%s: request %p: dequeued (flags=%lx)\n",
 				req->rq_disk->disk_name, req, req->flags);
 
 		if (!(req->flags & REQ_CMD))
-			goto error_out;
+			goto fail_request;
 
 		lo = req->rq_disk->private_data;
 #ifdef PARANOIA
 		BUG_ON(lo->magic != LO_MAGIC);
 #endif
-		if (!lo->file) {
-			printk(KERN_ERR "%s: Request when not-ready\n",
-					lo->disk->disk_name);
-			goto error_out;
-		}
+
 		nbd_cmd(req) = NBD_CMD_READ;
 		if (rq_data_dir(req) == WRITE) {
 			nbd_cmd(req) = NBD_CMD_WRITE;
 			if (lo->flags & NBD_READ_ONLY) {
 				printk(KERN_ERR "%s: Write on read-only\n",
 						lo->disk->disk_name);
-				goto error_out;
+				goto fail_request;
 			}
 		}
-#ifdef PARANOIA
-		requests_in++;
-#endif
 
-		req->errors = 0;
 		spin_unlock_irq(q->queue_lock);
-
-		spin_lock(&lo->queue_lock);
-
-		if (!lo->file) {
-			spin_unlock(&lo->queue_lock);
-			printk(KERN_ERR "%s: failed between accept and semaphore, file lost\n",
-					lo->disk->disk_name);
-			req->errors++;
-			nbd_end_request(req);
-			spin_lock_irq(q->queue_lock);
-			continue;
-		}
-
-		list_add(&req->queuelist, &lo->queue_head);
-		spin_unlock(&lo->queue_lock);
-
-		nbd_send_req(lo, req);
-
-		if (req->errors) {
-			printk(KERN_ERR "%s: Request send failed\n",
-					lo->disk->disk_name);
+		down(&lo->tx_lock);
+		if (lo->sock) {
+			notready = 0;
 			spin_lock(&lo->queue_lock);
-			list_del_init(&req->queuelist);
+			list_add(&req->queuelist, &lo->queue_head);
 			spin_unlock(&lo->queue_lock);
-			nbd_end_request(req);
-			spin_lock_irq(q->queue_lock);
-			continue;
+			nbd_send_req(lo, req);
+			if (req->errors) {
+				printk(KERN_ERR "%s: Request send failed\n",
+						lo->disk->disk_name);
+				spin_lock(&lo->queue_lock);
+				list_del_init(&req->queuelist);
+				spin_unlock(&lo->queue_lock);
+			}
 		}
-
+		else
+			notready = 1;
+		up(&lo->tx_lock);
 		spin_lock_irq(q->queue_lock);
+		if (notready) {
+			printk(KERN_ERR "%s: Request when not-ready\n",
+					lo->disk->disk_name);
+			req->errors++;
+			request_end_while_locked(req);
+		}
 		continue;
 
-	      error_out:
+fail_request:
+		/*
+		 * Fail the request: anyone waiting on a read or write gets
+		 * an error and can move on to their close() call.
+		 */
 		req->errors++;
-		spin_unlock(q->queue_lock);
-		nbd_end_request(req);
-		spin_lock(q->queue_lock);
+		request_end_while_locked(req);
 	}
 	return;
 }

@@ -548,12 +533,128 @@
 	return 0;
 }
 
+static int nbd_do_it(struct nbd_device *lo, unsigned int fd)
+{
+	struct file *file;
+	struct inode *inode;
+	struct socket *sock;
+	struct request *req;
+
+	file = fget(fd);
+	if (!file)
+		return -EBADF;
+	inode = file->f_dentry->d_inode;
+	if (!inode->i_sock) {
+		fput(file);
+		return -ENOTSOCK;
+	}
+	sock = SOCKET_I(inode);
+	if (sock->type != SOCK_STREAM) {
+		fput(file);
+		return -EPROTONOSUPPORT;
+	}
+
+	down(&lo->tx_lock);
+	if (lo->sock) {
+		up(&lo->tx_lock);
+		fput(file);
+		return -EBUSY;
+	}
+	lo->sock = sock;
+	up(&lo->tx_lock);
+
+	/* no need for rx lock since here is the rx... */
+	dprintk(DBG_IOCTL, "%s: nbd_do_it: commencing read loop\n",
+			lo->disk->disk_name);
+	while ((req = nbd_read_stat(lo)) != NULL)
+		request_end(req);
+	dprintk(DBG_IOCTL, "%s: nbd_do_it: read loop finished\n",
+			lo->disk->disk_name);
+
+	down(&lo->tx_lock);
+	lo->sock = NULL;
+	up(&lo->tx_lock);
+
+	fput(file);
+	return lo->harderror;
+}
+
+static int nbd_disconnect(struct nbd_device *lo)
+{
+	int error;
+	struct request sreq;
+
+	printk(KERN_INFO "%s: NBD_DISCONNECT\n", lo->disk->disk_name);
+
+	sreq.flags = REQ_SPECIAL;
+	nbd_cmd(&sreq) = NBD_CMD_DISC;
+
+	/*
+	 * Set sreq to sane values in case server implementation
+	 * fails to check the request type first and also to keep
+	 * debugging output cleaner.
+	 */
+	sreq.sector = 0;
+	sreq.nr_sectors = 0;
+
+	down(&lo->tx_lock);
+	if (lo->sock) {
+		error = 0;
+		nbd_send_req(lo, &sreq);
+	}
+	else {
+		error = -ENOTCONN;
+	}
+	up(&lo->tx_lock);
+	return error;
+}
+
+static int nbd_set_blksize(struct nbd_device *lo, unsigned long arg)
+{
+	if ((arg & (arg-1)) || (arg < 512) || (arg > PAGE_SIZE))
+		return -EINVAL;
+	down(&lo->tx_lock);
+	if (lo->sock) {
+		up(&lo->tx_lock);
+		return -EBUSY;
+	}
+	lo->blksize = arg;
+	lo->bytesize &= ~(lo->blksize-1); 
+	set_capacity(lo->disk, lo->bytesize >> 9);
+	up(&lo->tx_lock);
+	return 0;
+}
+
+static int nbd_set_size(struct nbd_device *lo, unsigned long arg)
+{
+	down(&lo->tx_lock);
+	if (lo->sock) {
+		up(&lo->tx_lock);
+		return -EBUSY;
+	}
+	lo->bytesize = arg & ~(lo->blksize-1); 
+	set_capacity(lo->disk, lo->bytesize >> 9);
+	up(&lo->tx_lock);
+	return 0;
+}
+
+static int nbd_set_size_blocks(struct nbd_device *lo, unsigned long arg)
+{
+	down(&lo->tx_lock);
+	if (lo->sock) {
+		up(&lo->tx_lock);
+		return -EBUSY;
+	}
+	lo->bytesize = ((u64) arg) * lo->blksize;
+	set_capacity(lo->disk, lo->bytesize >> 9);
+	up(&lo->tx_lock);
+	return 0;
+}
+
 static int nbd_ioctl(struct inode *inode, struct file *file,
 		     unsigned int cmd, unsigned long arg)
 {
 	struct nbd_device *lo = inode->i_bdev->bd_disk->private_data;
-	int error;
-	struct request sreq ;
 
 #ifdef PARANOIA
 	BUG_ON(lo->magic != LO_MAGIC);

@@ -565,101 +666,25 @@
 	if (!capable(CAP_SYS_ADMIN))
 		return -EPERM;
 	switch (cmd) {
+	case NBD_CLEAR_SOCK: /* deprecated! */
+	case NBD_SET_SOCK: /* deprecated! */
+		printk(KERN_WARNING "%s: %s[%d] called deprecated ioctl %s\n",
+				lo->disk->disk_name,
+				current->comm, current->pid,
+				ioctl_cmd_to_ascii(cmd));
+		return -EINVAL;
 	case NBD_DISCONNECT:
-	        printk(KERN_INFO "%s: NBD_DISCONNECT\n", lo->disk->disk_name);
-		sreq.flags = REQ_SPECIAL;
-		nbd_cmd(&sreq) = NBD_CMD_DISC;
-		/*
-		 * Set these to sane values in case server implementation
-		 * fails to check the request type first and also to keep
-		 * debugging output cleaner.
-		 */
-		sreq.sector = 0;
-		sreq.nr_sectors = 0;
-                if (!lo->sock)
-			return -EINVAL;
-                nbd_send_req(lo, &sreq);
-                return 0 ;
- 
-	case NBD_CLEAR_SOCK:
-		nbd_clear_que(lo);
-		spin_lock(&lo->queue_lock);
-		if (!list_empty(&lo->queue_head)) {
-			spin_unlock(&lo->queue_lock);
-			printk(KERN_ERR "%s: Some requests are in progress -> can not turn off.\n",
-					lo->disk->disk_name);
-			return -EBUSY;
-		}
-		file = lo->file;
-		if (!file) {
-			spin_unlock(&lo->queue_lock);
-			return -EINVAL;
-		}
-		lo->file = NULL;
-		lo->sock = NULL;
-		spin_unlock(&lo->queue_lock);
-		fput(file);
-		return 0;
-	case NBD_SET_SOCK:
-		if (lo->file)
-			return -EBUSY;
-		error = -EINVAL;
-		file = fget(arg);
-		if (file) {
-			inode = file->f_dentry->d_inode;
-			if (inode->i_sock) {
-				lo->file = file;
-				lo->sock = SOCKET_I(inode);
-				error = 0;
-			} else {
-				fput(file);
-			}
-		}
-		return error;
+		return nbd_disconnect(lo);
 	case NBD_SET_BLKSIZE:
-		if ((arg & (arg-1)) || (arg < 512) || (arg > PAGE_SIZE))
-			return -EINVAL;
-		lo->blksize = arg;
-		lo->bytesize &= ~(lo->blksize-1); 
-		set_capacity(lo->disk, lo->bytesize >> 9);
-		return 0;
+		return nbd_set_blksize(lo, arg);
 	case NBD_SET_SIZE:
-		lo->bytesize = arg & ~(lo->blksize-1); 
-		set_capacity(lo->disk, lo->bytesize >> 9);
-		return 0;
+		return nbd_set_size(lo, arg);
 	case NBD_SET_SIZE_BLOCKS:
-		lo->bytesize = ((u64) arg) * lo->blksize;
-		set_capacity(lo->disk, lo->bytesize >> 9);
-		return 0;
+		return nbd_set_size_blocks(lo, arg);
 	case NBD_DO_IT:
-		if (!lo->file)
-			return -EINVAL;
-		nbd_do_it(lo);
-		/* 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 */
-		down(&lo->tx_lock);
-		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;
-		up(&lo->tx_lock);
-		spin_lock(&lo->queue_lock);
-		file = lo->file;
-		lo->file = NULL;
-		spin_unlock(&lo->queue_lock);
-		nbd_clear_que(lo);
-		printk(KERN_WARNING "%s: queue cleared\n", lo->disk->disk_name);
-		if (file)
-			fput(file);
-		return lo->harderror;
+		return nbd_do_it(lo, arg);
 	case NBD_CLEAR_QUE:
-		nbd_clear_que(lo);
-		return 0;
+		return nbd_clear_que(lo);
 	case NBD_PRINT_DEBUG:
 #ifdef PARANOIA
 		printk(KERN_INFO "%s: next = %p, prev = %p. Global: in %d, out %d\n",

@@ -731,7 +756,6 @@
 	for (i = 0; i < MAX_NBD; i++) {
 		struct gendisk *disk = nbd_dev[i].disk;
 		nbd_dev[i].refcnt = 0;
-		nbd_dev[i].file = NULL;
 #ifdef PARANOIA
 		nbd_dev[i].magic = LO_MAGIC;
 #endif
include/linux/nbd.h +34 -31

86 lines not on this page · raw message

nbd-client.c +22 -3
diff -urN nbd/nbd-client.c nbd-patched/nbd-client.c
@@ -22,6 +22,7 @@
 #include <fcntl.h>
 #include <syslog.h>
 #include <stdlib.h>
+#include <linux/version.h>
 
 #define MY_NAME "nbd_client"
 #ifndef __GNUC__

@@ -62,7 +63,7 @@
 	unsigned long size;
 	char buf[256] = "\0\0\0\0\0\0\0\0\0";
 	int blocksize=1024;
-	char *hostname;
+	char *hostname, *nbdname;
 	int swap=0;
 
 	logging();

@@ -117,10 +118,11 @@
 
 	if (argc==0) goto errmsg;
 	sock = opennet(hostname, port);
-	nbd = open(argv[0], O_RDWR);
+	nbdname = argv[0];
+	nbd = open(nbdname, O_RDWR);
 	if (nbd < 0)
 	  err("Can not open NBD: %m");
-	++argv; --argc; /* skip device */
+	++argv; --argc; /* skip device command line argument */
 
 	if (argc>1) goto errmsg;
 	if (argc!=0) swap=1;

@@ -182,10 +184,18 @@
 			err("Ioctl/1 failed: %m\n");
 	}
 #endif
+#if (LINUX_VERSION_CODE >= KERNEL_VERSION(2,5,0))
+	close(nbd);
+	nbd = open(nbdname, O_RDWR);
+	if (nbd < 0)
+	  err("Can not re-open NBD: %m");
+#endif
 
+#if (LINUX_VERSION_CODE < KERNEL_VERSION(2,5,74))
 	ioctl(nbd, NBD_CLEAR_SOCK);
 	if (ioctl(nbd, NBD_SET_SOCK, sock) < 0)
 		err("Ioctl/2 failed: %m\n");
+#endif
 
 #ifndef SO_SWAPPING
 	if (swap)

@@ -202,14 +212,23 @@
 	if (fork())
 		exit(0);
 
+#if (LINUX_VERSION_CODE < KERNEL_VERSION(2,5,74))
 	if (ioctl(nbd, NBD_DO_IT) < 0)
 		fprintf(stderr, "Kernel call returned: %m");
 	else
 		fprintf(stderr, "Kernel call returned.");
+#else
+	if (ioctl(nbd, NBD_DO_IT, sock) < 0)
+		fprintf(stderr, "Kernel call returned: %m");
+	else
+		fprintf(stderr, "Kernel call returned.");
+#endif
 	printf("Closing: que, ");
 	ioctl(nbd, NBD_CLEAR_QUE);
+#if (LINUX_VERSION_CODE < KERNEL_VERSION(2,5,74))
 	printf("sock, ");
 	ioctl(nbd, NBD_CLEAR_SOCK);
+#endif
 	printf("done\n");
 	return 0;
 }
#4 lore raw
Quoted 7 lines · Lou Langholtz

I have also attached a patch to Pavel's nbd-2.0 release nbd tools that updates the nbd-client to work with linux 2.5 as well as 2.5.74 (assuming the aforementioned patch 6.1 made it into 2.5.74). Handling is switched at compile time however and uses <linux/version.h> to do the switching. This will have problems of course if the builder doesn't pay close attention to where there header file are coming from or tries to run the same binary on a different kernel release. Etc.

That's broken. You must make sure that a binary works with different kernels or at least make it fail gracefully. Using <linux/version.h> from userspace is absolutely not acceptable, just don't use kernel headers at all but a local copy of <linux/nbd.h>.

author
#5 lore raw
Quoted 13 lines · Christoph Hellwig
Quoted 3 lines · Lou Langholtz

I have also attached a patch to Pavel's nbd-2.0 release nbd tools that updates the nbd-client to work with linux 2.5 as well as 2.5.74 (assuming the aforementioned patch 6.1 made it into 2.5.74). Handling is switched at compile time however and uses <linux/version.h> to do the switching. This will have problems of course if the builder doesn't pay close attention to where there header file are coming from or tries to run the same binary on a different kernel release. Etc.

That's broken. You must make sure that a binary works with different kernels or at least make it fail gracefully. Using <linux/version.h> from userspace is absolutely not acceptable, just don't use kernel headers at all but a local copy of <linux/nbd.h>.

Yes. To be fair though, the binary (and the driver too) was broken on linux 2.5 kernels long before I even proposed any changes to the nbd driver. I'm trying to fix that. But it's a puzzle that has to have pieces moved out of the way first. With constraints like making one patch per fundemental change, it's more of a challenge trying to keep things in sync with user space. I'd like to see binary (runtime) compatibility too, but it's a bigger step to implement that. In the meantime, the hack/patch I submitted to nbd-client seems like a step forward. At least it works its way around several of the incompatibilities and lets people find out what other problem may lie ahead. I just found another problem for example with the disconnect function in nbd-client that will need to be fixed in order to be able to unload the module. A future step will be to change the compile time switching to a runtime switch, but I'm not sold on any one way to implement this yet. If you have something in mind for this, let me know. For example, should the ioctls be used to somehow notify the user process of the differing implementation. Like returning EINVAL for NBD_SET_SOCK. That'd tell nbd-tool that the nbd driver thinks something about the ioctl was invalid but not what. I wanted to return EDEPRECATED instead but I haven't found that errno yet. I could overload an errno but that seems ugly too. Or the driver could have a NBD_GET_VERSION ioctl. Is there precedence for that? I haven't come accross it yet.

How would you propose these issues be solved? Keep in touch!! Thanks!!

#6 lore raw
Quoted 3 lines · Lou Langholtz

Yes. To be fair though, the binary (and the driver too) was broken on linux 2.5 kernels long before I even proposed any changes to the nbd driver. I'm trying to fix that.

Hey, I didn't say changing the interface is wrong. But if possible do it in a way that the new userspace can still support the old kernel driver.

Quoted 5 lines

NBD_SET_SOCK. That'd tell nbd-tool that the nbd driver thinks something about the ioctl was invalid but not what. I wanted to return EDEPRECATED instead but I haven't found that errno yet. I could overload an errno but that seems ugly too. Or the driver could have a NBD_GET_VERSION ioctl. Is there precedence for that? I haven't come accross it yet.

That's one choice. At least md and device mapper do that.

author
#7 lore raw
Quoted 10 lines · Christoph Hellwig
Quoted 3 lines · Lou Langholtz

Yes. To be fair though, the binary (and the driver too) was broken on linux 2.5 kernels long before I even proposed any changes to the nbd driver. I'm trying to fix that.

Hey, I didn't say changing the interface is wrong. But if possible do it in a way that the new userspace can still support the old kernel driver.

Agreed! Will do, but how???

Quoted 7 lines
Quoted 3 lines

NBD_SET_SOCK. That'd tell nbd-tool that the nbd driver thinks something about the ioctl was invalid but not what. I wanted to return EDEPRECATED instead but I haven't found that errno yet. I could overload an errno but that seems ugly too. Or the driver could have a NBD_GET_VERSION ioctl. Is there precedence for that? I haven't come accross it yet.

That's one choice. At least md and device mapper do that.

Cool! I'll take a look at these. Is this the prefered way then? There's probably a lot of need for this generally speaking. Thought about using /proc for this too. And then sysfs is gaining favor so maybe in there? Any preference?

#8 lore raw
Quoted 5 lines · Lou Langholtz
Quoted 4 lines

Hey, I didn't say changing the interface is wrong. But if possible do it in a way that the new userspace can still support the old kernel driver.

Agreed! Will do, but how???

if (detected_new_driver) {
new_code();
} else {
old_code();
}

in the userland app. if structures change in an incompatible way you need _v1 and _v2 versions of them of course.

And what to use for detected_new_driver? Probably the new ABI version ioctl..

Quoted 2 lines

Cool! I'll take a look at these. Is this the prefered way then? There's probably a lot of need for this generally speaking.

Yeah. And please do like device-mapper with major and minor versions. major as in completly incompatible and minor as in support old protocol but has new featues in addition.

Quoted 2 lines

Thought about using
/proc for this too.

Bad idea :)

Quoted 1 line

And then sysfs is gaining favor so maybe in there?

Doesn't sound like a fit either. Maybe you could do a small own fs to control nbd, but I'm not familar enough with the actual API to comment on this more.

#9 lore raw

Anyone for NBD maintainer [was Re: [RFC][PATCH] nbd driver for 2.5+: fix locking issues with ioctl UI]

In reply to Lou Langholtz

Hi!

Quoted 10 lines
Quoted 6 lines
Quoted 5 lines

That said, this seems like an opportunistic time to further break compatibility with the existing nbd-client user tool and do away with the problematic components of the ioctl user interface.

Is a suitably modified userspace tool available?

Here is an updated patch 6 (I called it 6.1) that fixes some additional locking issues as well as fixing the header file so it can be used by

...

BTW Anyone wanting to become nbd maintainer? I'm not much interested in nbd these days...
Pavel

Signature · 2 lines
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
author
#10 lore raw

Re: Anyone for NBD maintainer [was Re: [RFC][PATCH] nbd driver for 2.5+: fix locking issues with ioctl UI]

Quoted 6 lines · Pavel Machek

. . .
BTW Anyone wanting to become nbd maintainer? I'm not much interested in nbd these days...
Pavel

I'd be honoured too but not sure what exactly that entails. What other stuff would I have to do besides actively developing it? I won't be able to do anything on the Internet till Monday either.

#11 lore raw

Re: Anyone for NBD maintainer [was Re: [RFC][PATCH] nbd driver for 2.5+: fix locking issues with ioctl UI]

Hi!

Quoted 8 lines
Quoted 5 lines

BTW Anyone wanting to become nbd maintainer? I'm not much interested in nbd these days...
Pavel

I'd be honoured too but not sure what exactly that entails. What other stuff would I have to do besides actively developing it? I won't be able to do anything on the Internet till Monday either.

Well, maintainer gets patches from people and should decide which are good and which are not...

Anyway if you give me your sf.net name, I can probably add you as a developer to the nbd.sf.net... but: I'd like you to keep userland code backward compatible. Ie. new userland code should still work with 2.4.x kernel.
Pavel

Signature · 2 lines
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
author
#12 lore raw

Re: Anyone for NBD maintainer [was Re: [RFC][PATCH] nbd driver for 2.5+: fix locking issues with ioctl UI]

Quoted 4 lines · Pavel Machek
Quoted 3 lines

. . .I'd be honoured too but not sure what exactly that entails. What other stuff would I have to do besides actively developing it? I won't be able to do anything on the Internet till Monday either.

Quoted 10 lines

Well, maintainer gets patches from people and should decide which are good and which are not...

Anyway if you give me your sf.net name, I can probably add you as a developer to the nbd.sf.net... but: I'd like you to keep userland code backward compatible. Ie. new userland code should still work with 2.4.x kernel.
Pavel

Okay. Haven't heard anyone say lose the compatibility in favor of the cleanup so I'll work on the back compatibility more in the driver. Same with userland tools. I'd better get a sf.net name then apparantly. Thanks ;-)

A message that is not in the archive (sent off-list or not yet received): Pine.LNX.4.10.10306251645580.11076-100000@clements.sc.steeleye.com

#14 lore raw

NBD maintainer change [was Re: Anyone for NBD maintainer]

Hi!

Quoted 6 lines
Quoted 2 lines

BTW Anyone wanting to become nbd maintainer? I'm not much interested in nbd these days...

I would like to take this on, if you don't want to do it anymore. I've been keeping up with the nbd source for quite a while now, so I think I'm ready to give it a shot.

Linus, I no longer have time/interest in maintaining NBD. Please apply,

Pavel

MAINTAINERS +2 -2
@@ -1265,8 +1265,8 @@
 S:	Maintained
 
 NETWORK BLOCK DEVICE
-P:	Pavel Machek
-M:	[email protected]
+P:	Paul Clements
+M:	[email protected]
 S:	Maintained
 
 NETWORK DEVICE DRIVERS
Signature · 2 lines
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
#15 lore raw
In reply to Lou Langholtz

Lou,

a few comments on the patch...

Quoted 4 lines · Lou Langholtz

Please review and comment...

This is the sixth patch to the NBD driver and it fixes a variety of outstanding locking issues with the ioctl user interface. This patch

This patch introduces a locking hierarchy between the lo->tx_lock and lo->queue_lock. The existing driver does not have this limitation. I would feel a lot better about this patch if you were to recode it to avoid this.

Also, I noticed that you've removed the forcible shutdown of the socket at the end of NBD_DO_IT. Was there a particular reason for that?

Quoted 4 lines

... the new NBD_DO_IT style interface
could be introduced instead as another ioctl completely and these 3 ioctls could be maintained for backward compatibility for a while longer.

It would be really nice if the driver remained (as much as possible) compatible with the 2.4 version...unless you have a really good reason to break things... :)

--
Paul

#16 lore raw
Quoted 4 lines · Paul Clements

This patch introduces a locking hierarchy between the lo->tx_lock and lo->queue_lock. The existing driver does not have this limitation. I would feel a lot better about this patch if you were to recode it to avoid this.

?????

One of them is a semaphore, another - a spinlock. Of course there always had been a hierarchy - you can't call down() under a spinlock, the damn thing can schedule.

author
#17 lore raw
In reply to Paul Clements
Quoted 6 lines · Paul Clements

. . .
This patch introduces a locking hierarchy between the lo->tx_lock and lo->queue_lock. The existing driver does not have this limitation. I would feel a lot better about this patch if you were to recode it to avoid this.

Did Al's statements regarding this make this feel better? The code could be redone so that the queuelist is added to before the down but then more often it will have to be removed from since the lo->sock check wouldn't have been done yet. On the other hand I've been thinking that I might be able to take advantage of the irq locked condition imposed by the q->queue_lock and just use nbd_lock to replace q->queue_lock then. Al and Andrew seem to have a much deeper understanding though for spinlocking though so I'll defer to there comments on this idea (of replacing lo->queue_lock by use of nbd_lock). This has the added attraction of already having nbd_lock locked when in do_nbd_request.

Quoted 4 lines

Also, I noticed that you've removed the forcible shutdown of the socket at the end of NBD_DO_IT. Was there a particular reason for that?

Yes. The forcible shutdown that was put in place (while closing one race condition) still left a panicable race condition with the nbd-client tool. I forget exactly what this was at the moment. Suffice it to say that since the user space tool opened the socket to begin with, it seems a better design to have the user space tool do the close as well. When nbd-client exits, it'd effect close of this socket anyway even if killed. What still needs to be done though, is to have the implementation of the NBD_DISCONNECT ioctl in the driver wait until nbd_do_it sets lo->sock back to NULL. That way the NBD_CLEAR_QUE ioctl can return -EBUSY if lo->sock, while not getting called by nbd-client till lo->sock == NULL so that the que clear works.

Quoted 9 lines
Quoted 3 lines

... the new NBD_DO_IT style interface could be introduced instead as another ioctl completely and these 3 ioctls could be maintained for backward compatibility for a while longer.

It would be really nice if the driver remained (as much as possible) compatible with the 2.4 version...unless you have a really good reason to break things... :)

So you'd prefer to have a new ioctl then to do this rolled together NBD_DO_IT function? Say NBD_RUN or something that takes the sock argument. That will require the nbd_device structure to maintain that file pointer again and possibly leave open the races that seem inherent to having the three seperate ioctl's. Someone else long ago commented in the driver "possible FIXME: make set_sock / set_blksize / set_size / do_it one syscall. Why not: would need verify_area and friends, would share yet another structure with userland". So it seems someone was long ago realizing there were problems having the three seperate ioctls for set_sock, do_it, and clear_sock. Of course this doesn't address set_size, set_blksize, but I don't see them as problematic w.r.t. locking once set_sock, do_it, and clear_sock are rolled together.

Seems like we're better off deprecating at least the set_sock and clear_sock ioctls to return some error code and having the nbd-client tool runtime switch off of something like their return value (or a release level value as was also suggested). It's maybe more painful in requiring people to update their nbd-client's but we live with those kinds of required updates all the time (example mod-utils insmod, rmmod, from Rusty to boot 2.5 kernels). The benefit will be memory saving and better kernel stability. I'm more torn between the idea of deprecating all three ioctls and adding a NBD_RUN versus just deprecating NBD_SET_SOCK and NBD_CLEAR_SOCK and changing the NBD_DO_IT ioctl to require the socket descriptor.

Comments?

author
#18 lore raw
Quoted 1 line · Lou Langholtz

. . . On the other hand I've been thinking that I might be able to take advantage of the irq locked condition imposed by the q->queue_lock and just use nbd_lock to replace q->queue_lock then. Al and Andrew seem to have a much deeper understanding though for spinlocking though so I'll defer to there comments on this idea (of replacing lo->queue_lock by use of nbd_lock). This has the added attraction of already having nbd_lock locked when in do_nbd_request.. . .

Typo! Above should have read "just use nbd_lock to replace lo->queue_lock" (another spinlock_t per nbd_device). Anyways... would using the one nbd_lock to also protect the lo->queue_list work better than using the queue_lock per nbd_device I'm wondering. According to the prior discusions about spinlocks this should be better. I don't have a picture right now of wether that even works or not. Gotta run though, thanks!

#19 lore raw
In reply to Lou Langholtz
Quoted 3 lines · Lou Langholtz

Did Al's statements regarding this make this feel better? The code could be redone so that the queuelist is added to before the down but then more often it will have to be removed from since the lo->sock check

It wasn't that I was worried about someone calling down() under spinlock, obviously that'd be a problem. However, if the queue_lock were to be changed back to a semaphore (as it used to be, pre-2.4.16 or so) this could lead to problems. I guess I was just wondering why the tx_lock was pulled out of nbd_send_req(). It just seems to make the code harder to follow with the overlapping locking, the duplicated checks for sock == NULL, and it also means the tx_lock gets held (a little bit) longer...

Quoted 2 lines

that since the user space tool opened the socket to begin with, it seems a better design to have the user space tool do the close as well.

I agree, but I thought that the shutdown was causing different behavior...

Quoted 3 lines

When
nbd-client exits, it'd effect close of this socket anyway even if killed.

Does the socket close at exit have the same effect as a socket shutdown? If so, then I guess the shutdown is unnecessary...

Quoted 2 lines

So you'd prefer to have a new ioctl then to do this rolled together NBD_DO_IT function? Say NBD_RUN or something that takes the sock

I do like the combined ioctl, as it seems to make the code a little bit cleaner and safer. But, it would also be nice to maintain compatibility with the existing userland tools. Maybe if the driver could support both new and old interfaces (at least for now), then users could gradually move over to the new interface(s)?

--
Paul

author
#20 lore raw
Quoted 6 lines · Paul Clements

. . . I guess I was just wondering why the
tx_lock was pulled out of nbd_send_req(). It just seems to make the code harder to follow with the overlapping locking, the duplicated checks for sock == NULL, and it also means the tx_lock gets held (a little bit) longer...

Reviewing the code some more, I'm not sure why I moved the tx_lock out from nbd_send_req(). Some possible explanations are:

1. nbd_send_req() was big enough without the locking in it.
2. keeps the locking at the same level as the spin_unlock_irq() which makes lock analysis a little easier. it's also a more consistant level to have at for consistantly locking accross all the ioctl handling.
3. the next patch I had done (patch 7) implements a default blocking strategy and having the lock be outside nbd_send_req made analysis a little easier again. as in the last reason, the locking then fell into place more consistantly level wise.
4. in order to have the locking inside nbd_send_req it would seem it'd help to make nbd_send_req return a value that could be checked.

I'm not convinced by any of my own reasons though. Maybe it should be inside. There aren't duplicated checks for sock == null except in different execution paths so it doesn't get checked twice in a row if that's what you mean. Just larger executable size as any inlining causes.

Quoted 7 lines
Quoted 3 lines

that since the user space tool opened the socket to begin with, it seems a better design to have the user space tool do the close as well.

I agree, but I thought that the shutdown was causing different behavior...

What behavior would the shutdown cause that close doesn't also cause?

Quoted 9 lines
Quoted 5 lines

When
nbd-client exits, it'd effect close of this socket anyway even if killed.

Does the socket close at exit have the same effect as a socket shutdown? If so, then I guess the shutdown is unnecessary...

I believe so. I thought someone from steeleye put the call in to begin with just in order to close up a race condition gap and that's better handled by not having the three seperate ioctls. My understanding is that the shutdown is analogous to calling the shutdown() system call on the socket which is just a way to individually shutdown the read side or write side of the socket no? In anycase, I believe close (once really finished) has the same net effect.

Quoted 10 lines
Quoted 3 lines

So you'd prefer to have a new ioctl then to do this rolled together NBD_DO_IT function? Say NBD_RUN or something that takes the sock

I do like the combined ioctl, as it seems to make the code a little bit cleaner and safer. But, it would also be nice to maintain compatibility with the existing userland tools. Maybe if the driver could support both new and old interfaces (at least for now), then users could gradually move over to the new interface(s)?

Agreed then. That's what I'd done in my original jumbo nbd patch (maintain suport for old and new ioctls). So I'll work on that next week. In the meantime, the nbd-client tool currently can't correctly set the size of the device and either that needs to be worked around in the driver (I'd done that in the original jumbo patch), or the nbd-client tool needs to be updated (the patch I'd mailed out for nbd-client works around the sizing issue by re-opening the nbd). To be clear, that's not something any of the changes that have gone in so far broke nor address. It's a consequence of how bd_set_size() is called in fs/block_dev.c do_open(). One of the other drivers (in drivers/block I believe) works around the problem by updating the info in the inode but it seems kind of like a hack. Will someone who has worked on the fs/block_dev.c file recently chime in on this issue of how to properly effect size?

Thanks for your input on this!!!

#21 lore raw
Quoted 7 lines · Lou Langholtz

Reviewing the code some more, I'm not sure why I moved the tx_lock out from nbd_send_req(). Some possible explanations are:

2. keeps the locking at the same level as the spin_unlock_irq() which makes lock analysis a little easier. it's also a more consistant level to have at for consistantly locking accross all the ioctl handling.

But, unless the locks are overlapping (which they did not used to be) there really is nothing to analyze in the first place...it's just obviously correct, right?

Quoted 2 lines

4. in order to have the locking inside nbd_send_req it would seem it'd help to make nbd_send_req return a value that could be checked.

That's not an unreasonable thing to do...it'd be a much more minor change. I guess I'm just having a hard time seeing the benefit of rearranging all this code. Unless there's some substantial benefit, it seems wiser to keep the changes as minimal as possible...after all, the current code does work.

Quoted 6 lines

week. In the meantime, the nbd-client tool currently can't correctly set the size of the device and either that needs to be worked around in the driver (I'd done that in the original jumbo patch), or the nbd-client tool needs to be updated (the patch I'd mailed out for nbd-client works around the sizing issue by re-opening the nbd). To be clear, that's not something any of the changes that have gone in so far broke nor address.

I'm in favor of doing the minor change in the driver to maintain compatibility with existing userland tools. It's a pretty simple fix...

The patch will be coming shortly...

--
Paul

author
#22 lore raw
Quoted 8 lines · Paul Clements

. . .
That's not an unreasonable thing to do...it'd be a much more minor change. I guess I'm just having a hard time seeing the benefit of rearranging all this code. Unless there's some substantial benefit, it seems wiser to keep the changes as minimal as possible...after all, the current code does work.
. . .

Sorry for the confusion. I think you're looking at the nbd code more from the standpoint of fixing problem areas and integration with the changes from 2.5. I've had my eye meanwhile on a more robust implemenation and enhanced design. The difference being that what I've submitted so far are just the changes I've needed along that path which also achieve the former target. But the changes clearly aren't ideal for the former perspective - in not being minimalistic changes. Just nobody else has completely fixed problematic areas of the driver liking locking races so I figure if the code also fixes these problematic areas it's worth sharing with this audience. YMMV of course.

#23 lore raw

PATCH 1/1 nbd: maintain compatibility with existing nbd tools

In reply to Lou Langholtz
Quoted 8 lines · Lou Langholtz

[ ... ] In the meantime, the nbd-client tool currently can't correctly set the size of the device and either that needs to be worked around in the driver (I'd done that in the original jumbo patch), or the nbd-client tool needs to be updated (the patch I'd mailed out for nbd-client works around the sizing issue by re-opening the nbd). To be clear, that's not something any of the changes that have gone in so far broke nor address. It's a consequence of how bd_set_size() is called in fs/block_dev.c do_open().

And here's the (tiny) patch for nbd to maintain compatibility with the existing nbd-client tool. Compiled and tested on a couple machines. Please apply.

Thanks,
Paul

nbd.c renamed from nbd.c.ORIG +5 -2
@@ -465,15 +468,18 @@
 			lo->blksize_bits++;
 			temp >>= 1;
 		}
-		lo->bytesize &= ~(lo->blksize-1); 
+		lo->bytesize &= ~(lo->blksize-1);
+		inode->i_bdev->bd_inode->i_size = lo->bytesize;
 		set_capacity(lo->disk, lo->bytesize >> 9);
 		return 0;
 	case NBD_SET_SIZE:
-		lo->bytesize = arg & ~(lo->blksize-1); 
+		lo->bytesize = arg & ~(lo->blksize-1);
+		inode->i_bdev->bd_inode->i_size = lo->bytesize;
 		set_capacity(lo->disk, lo->bytesize >> 9);
 		return 0;
 	case NBD_SET_SIZE_BLOCKS:
 		lo->bytesize = ((u64) arg) << lo->blksize_bits;
+		inode->i_bdev->bd_inode->i_size = lo->bytesize;
 		set_capacity(lo->disk, lo->bytesize >> 9);
 		return 0;
 	case NBD_DO_IT:
#24 lore raw

Re: [PATCH] nbd: maintain compatibility with existing nbd tools

Hi!

Quoted 9 lines
Quoted 8 lines

[ ... ] In the meantime, the nbd-client tool currently can't correctly set the size of the device and either that needs to be worked around in the driver (I'd done that in the original jumbo patch), or the nbd-client tool needs to be updated (the patch I'd mailed out for nbd-client works around the sizing issue by re-opening the nbd). To be clear, that's not something any of the changes that have gone in so far broke nor address. It's a consequence of how bd_set_size() is called in fs/block_dev.c do_open().

Quoted 3 lines

And here's the (tiny) patch for nbd to maintain compatibility with the existing nbd-client tool. Compiled and tested on a couple machines. Please apply.

You are the maintainer, you can go to the linus directly. (I hope Linus took that MAINTAINERS patch). Or you can send this to Rusty 'trivial patch monkey' russel. It seems easy enough.

Pavel

[Aha, if you wanted *Andrew* to apply it... I guess it is better to say directly who do you want to take it.]

Quoted 2 lines

Thanks,
Paul

Content-Description: nbd patch

Quoted 23 lines
nbd.c renamed from nbd.c.ORIG +5 -2
@@ -465,15 +468,18 @@
 			lo->blksize_bits++;
 			temp >>= 1;
 		}
-		lo->bytesize &= ~(lo->blksize-1); 
+		lo->bytesize &= ~(lo->blksize-1);
+		inode->i_bdev->bd_inode->i_size = lo->bytesize;
 		set_capacity(lo->disk, lo->bytesize >> 9);
 		return 0;
 	case NBD_SET_SIZE:
-		lo->bytesize = arg & ~(lo->blksize-1); 
+		lo->bytesize = arg & ~(lo->blksize-1);
+		inode->i_bdev->bd_inode->i_size = lo->bytesize;
 		set_capacity(lo->disk, lo->bytesize >> 9);
 		return 0;
 	case NBD_SET_SIZE_BLOCKS:
 		lo->bytesize = ((u64) arg) << lo->blksize_bits;
+		inode->i_bdev->bd_inode->i_size = lo->bytesize;
 		set_capacity(lo->disk, lo->bytesize >> 9);
 		return 0;
 	case NBD_DO_IT:
Signature · 2 lines
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]
#25 lore raw

Re: [PATCH 2.5.73] nbd: maintain compatibility with existing nbd tools

Quoted 9 lines · Pavel Machek
Quoted 9 lines
Quoted 8 lines

[ ... ] In the meantime, the nbd-client tool currently can't correctly set the size of the device and either that needs to be worked around in the driver (I'd done that in the original jumbo patch), or the nbd-client tool needs to be updated (the patch I'd mailed out for nbd-client works around the sizing issue by re-opening the nbd). To be clear, that's not something any of the changes that have gone in so far broke nor address. It's a consequence of how bd_set_size() is called in fs/block_dev.c do_open().

Quoted 6 lines
Quoted 3 lines

And here's the (tiny) patch for nbd to maintain compatibility with the existing nbd-client tool. Compiled and tested on a couple machines. Please apply.

You are the maintainer, you can go to the linus directly. (I hope Linus took that MAINTAINERS patch).

Not yet, I don't think...

Quoted 2 lines

[Aha, if you wanted *Andrew* to apply it... I guess it is better to say directly who do you want to take it.]

Well, it's just that Andrew seems particularly willing to help on this, so that's why I sent to him...

At any rate, Andrew, here's a cleaned up version of the patch... please apply.

Thanks,
Paul

drivers/block/nbd.c renamed from drivers/block/nbd.c.PRISTINE +5 -2
@@ -503,15 +503,18 @@
 			lo->blksize_bits++;
 			temp >>= 1;
 		}
-		lo->bytesize &= ~(lo->blksize-1); 
+		lo->bytesize &= ~(lo->blksize-1);
+		inode->i_bdev->bd_inode->i_size = lo->bytesize;
 		set_capacity(lo->disk, lo->bytesize >> 9);
 		return 0;
 	case NBD_SET_SIZE:
-		lo->bytesize = arg & ~(lo->blksize-1); 
+		lo->bytesize = arg & ~(lo->blksize-1);
+		inode->i_bdev->bd_inode->i_size = lo->bytesize;
 		set_capacity(lo->disk, lo->bytesize >> 9);
 		return 0;
 	case NBD_SET_SIZE_BLOCKS:
 		lo->bytesize = ((u64) arg) << lo->blksize_bits;
+		inode->i_bdev->bd_inode->i_size = lo->bytesize;
 		set_capacity(lo->disk, lo->bytesize >> 9);
 		return 0;
 	case NBD_DO_IT: