LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH 0/9] nbd: cleanups
@ 2015-02-12 20:57 Markus Pargmann
  2015-02-12 20:57 ` [PATCH 1/9] Documentation: nbd: Reformat to allow more documentation Markus Pargmann
                   ` (8 more replies)
  0 siblings, 9 replies; 21+ messages in thread
From: Markus Pargmann @ 2015-02-12 20:57 UTC (permalink / raw)
  To: nbd-general; +Cc: linux-kernel, Markus Pargmann

Hi,

here are some cleanups for nbd.

Patch 3 removes the internal kernel header of nbd as it is only used for nbd.c.
We can define the device struct at the top of nbd.c instead.
Patch 6 replaces the previously used dprint macro with dev_dbg. dev_dbg()
should work as well as dprint did.

The other patches change some minor things.

Best Regards,

Markus


Markus Pargmann (9):
  Documentation: nbd: Reformat to allow more documentation
  Documentation: nbd: Add list of module parameters
  nbd: Remove kernel internal header
  nbd: Replace kthread_create with kthread_run
  nbd: Fix device bytesize type
  nbd: Restructure debugging prints
  nbd: Remove fixme that was already fixed
  nbd: Return error code directly
  nbd: Return error pointer directly

 Documentation/blockdev/nbd.txt |  48 +++++++++-----
 drivers/block/nbd.c            | 138 ++++++++++++++++-------------------------
 include/linux/nbd.h            |  46 --------------
 3 files changed, 83 insertions(+), 149 deletions(-)
 delete mode 100644 include/linux/nbd.h

-- 
2.1.4


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 1/9] Documentation: nbd: Reformat to allow more documentation
  2015-02-12 20:57 [PATCH 0/9] nbd: cleanups Markus Pargmann
@ 2015-02-12 20:57 ` Markus Pargmann
  2015-02-12 20:57 ` [PATCH 2/9] Documentation: nbd: Add list of module parameters Markus Pargmann
                   ` (7 subsequent siblings)
  8 siblings, 0 replies; 21+ messages in thread
From: Markus Pargmann @ 2015-02-12 20:57 UTC (permalink / raw)
  To: nbd-general; +Cc: linux-kernel, Markus Pargmann

Reformat the existing documentation to have more structure. This allows
for more documentation seperated from the existing paragraphs.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 Documentation/blockdev/nbd.txt | 34 +++++++++++++++++++---------------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/Documentation/blockdev/nbd.txt b/Documentation/blockdev/nbd.txt
index 271e607304da..337946bd460e 100644
--- a/Documentation/blockdev/nbd.txt
+++ b/Documentation/blockdev/nbd.txt
@@ -1,17 +1,21 @@
-                      Network Block Device (TCP version)
-                                       
-   What is it: With this compiled in the kernel (or as a module), Linux
-   can use a remote server as one of its block devices. So every time
-   the client computer wants to read, e.g., /dev/nb0, it sends a
-   request over TCP to the server, which will reply with the data read.
-   This can be used for stations with low disk space (or even diskless)
-   to borrow disk space from another computer.
-   Unlike NFS, it is possible to put any filesystem on it, etc.
+Network Block Device (TCP version)
+==================================
 
-   For more information, or to download the nbd-client and nbd-server
-   tools, go to http://nbd.sf.net/.
+1) Overview
+-----------
 
-   The nbd kernel module need only be installed on the client
-   system, as the nbd-server is completely in userspace. In fact,
-   the nbd-server has been successfully ported to other operating
-   systems, including Windows.
+What is it: With this compiled in the kernel (or as a module), Linux
+can use a remote server as one of its block devices. So every time
+the client computer wants to read, e.g., /dev/nb0, it sends a
+request over TCP to the server, which will reply with the data read.
+This can be used for stations with low disk space (or even diskless)
+to borrow disk space from another computer.
+Unlike NFS, it is possible to put any filesystem on it, etc.
+
+For more information, or to download the nbd-client and nbd-server
+tools, go to http://nbd.sf.net/.
+
+The nbd kernel module need only be installed on the client
+system, as the nbd-server is completely in userspace. In fact,
+the nbd-server has been successfully ported to other operating
+systems, including Windows.
-- 
2.1.4


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 2/9] Documentation: nbd: Add list of module parameters
  2015-02-12 20:57 [PATCH 0/9] nbd: cleanups Markus Pargmann
  2015-02-12 20:57 ` [PATCH 1/9] Documentation: nbd: Reformat to allow more documentation Markus Pargmann
@ 2015-02-12 20:57 ` Markus Pargmann
  2015-02-14 10:29   ` [Nbd] " Wouter Verhelst
  2015-02-12 20:57 ` [PATCH 3/9] nbd: Remove kernel internal header Markus Pargmann
                   ` (6 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Markus Pargmann @ 2015-02-12 20:57 UTC (permalink / raw)
  To: nbd-general; +Cc: linux-kernel, Markus Pargmann

Add a list of available module parameters as attachment to the
documentation.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 Documentation/blockdev/nbd.txt | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/blockdev/nbd.txt b/Documentation/blockdev/nbd.txt
index 337946bd460e..db242ea2bce8 100644
--- a/Documentation/blockdev/nbd.txt
+++ b/Documentation/blockdev/nbd.txt
@@ -19,3 +19,13 @@ The nbd kernel module need only be installed on the client
 system, as the nbd-server is completely in userspace. In fact,
 the nbd-server has been successfully ported to other operating
 systems, including Windows.
+
+A) NBD parameters
+-----------------
+
+max_part
+	Number of partitions per device (default: 0).
+
+nbds_max
+	Number of block devices that should be initialized (default: 16).
+
-- 
2.1.4


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 3/9] nbd: Remove kernel internal header
  2015-02-12 20:57 [PATCH 0/9] nbd: cleanups Markus Pargmann
  2015-02-12 20:57 ` [PATCH 1/9] Documentation: nbd: Reformat to allow more documentation Markus Pargmann
  2015-02-12 20:57 ` [PATCH 2/9] Documentation: nbd: Add list of module parameters Markus Pargmann
@ 2015-02-12 20:57 ` Markus Pargmann
  2015-02-14 10:30   ` [Nbd] " Wouter Verhelst
  2015-02-12 20:57 ` [PATCH 4/9] nbd: Replace kthread_create with kthread_run Markus Pargmann
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Markus Pargmann @ 2015-02-12 20:57 UTC (permalink / raw)
  To: nbd-general; +Cc: linux-kernel, Markus Pargmann

The header is not included anywhere. Remove it and include the private
nbd_device struct in nbd.c.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 22 ++++++++++++++++++++++
 include/linux/nbd.h | 46 ----------------------------------------------
 2 files changed, 22 insertions(+), 46 deletions(-)
 delete mode 100644 include/linux/nbd.h

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 4bc2a5cb9935..58c2b20ad17b 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -38,6 +38,28 @@
 
 #include <linux/nbd.h>
 
+struct nbd_device {
+	int flags;
+	int harderror;		/* Code of hard error			*/
+	struct socket * sock;	/* If == NULL, device is not ready, yet	*/
+	int magic;
+
+	spinlock_t queue_lock;
+	struct list_head queue_head;	/* Requests waiting result */
+	struct request *active_req;
+	wait_queue_head_t active_wq;
+	struct list_head waiting_queue;	/* Requests to be sent */
+	wait_queue_head_t waiting_wq;
+
+	struct mutex tx_lock;
+	struct gendisk *disk;
+	int blksize;
+	u64 bytesize;
+	pid_t pid; /* pid of nbd-client, if attached */
+	int xmit_timeout;
+	int disconnect; /* a disconnect has been requested by user */
+};
+
 #define NBD_MAGIC 0x68797548
 
 #ifdef NDEBUG
diff --git a/include/linux/nbd.h b/include/linux/nbd.h
deleted file mode 100644
index f62f78aef4ac..000000000000
--- a/include/linux/nbd.h
+++ /dev/null
@@ -1,46 +0,0 @@
-/*
- * 1999 Copyright (C) Pavel Machek, pavel@ucw.cz. This code is GPL.
- * 1999/11/04 Copyright (C) 1999 VMware, Inc. (Regis "HPReg" Duchesne)
- *            Made nbd_end_request() use the io_request_lock
- * 2001 Copyright (C) Steven Whitehouse
- *            New nbd_end_request() for compatibility with new linux block
- *            layer code.
- * 2003/06/24 Louis D. Langholtz <ldl@aros.net>
- *            Removed unneeded blksize_bits field from nbd_device struct.
- *            Cleanup PARANOIA usage & code.
- * 2004/02/19 Paul Clements
- *            Removed PARANOIA, plus various cleanup and comments
- */
-#ifndef LINUX_NBD_H
-#define LINUX_NBD_H
-
-
-#include <linux/wait.h>
-#include <linux/mutex.h>
-#include <uapi/linux/nbd.h>
-
-struct request;
-
-struct nbd_device {
-	int flags;
-	int harderror;		/* Code of hard error			*/
-	struct socket * sock;	/* If == NULL, device is not ready, yet	*/
-	int magic;
-
-	spinlock_t queue_lock;
-	struct list_head queue_head;	/* Requests waiting result */
-	struct request *active_req;
-	wait_queue_head_t active_wq;
-	struct list_head waiting_queue;	/* Requests to be sent */
-	wait_queue_head_t waiting_wq;
-
-	struct mutex tx_lock;
-	struct gendisk *disk;
-	int blksize;
-	u64 bytesize;
-	pid_t pid; /* pid of nbd-client, if attached */
-	int xmit_timeout;
-	int disconnect; /* a disconnect has been requested by user */
-};
-
-#endif
-- 
2.1.4


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 4/9] nbd: Replace kthread_create with kthread_run
  2015-02-12 20:57 [PATCH 0/9] nbd: cleanups Markus Pargmann
                   ` (2 preceding siblings ...)
  2015-02-12 20:57 ` [PATCH 3/9] nbd: Remove kernel internal header Markus Pargmann
@ 2015-02-12 20:57 ` Markus Pargmann
  2015-02-12 20:57 ` [PATCH 5/9] nbd: Fix device bytesize type Markus Pargmann
                   ` (4 subsequent siblings)
  8 siblings, 0 replies; 21+ messages in thread
From: Markus Pargmann @ 2015-02-12 20:57 UTC (permalink / raw)
  To: nbd-general; +Cc: linux-kernel, Markus Pargmann

kthread_run includes the wake_up_process() call, so instead of
kthread_create() followed by wake_up_process() we can use this macro.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 58c2b20ad17b..c07160c25a94 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -728,13 +728,13 @@ static int __nbd_ioctl(struct block_device *bdev, struct nbd_device *nbd,
 		else
 			blk_queue_flush(nbd->disk->queue, 0);
 
-		thread = kthread_create(nbd_thread, nbd, "%s",
-					nbd->disk->disk_name);
+		thread = kthread_run(nbd_thread, nbd, "%s",
+				     nbd->disk->disk_name);
 		if (IS_ERR(thread)) {
 			mutex_lock(&nbd->tx_lock);
 			return PTR_ERR(thread);
 		}
-		wake_up_process(thread);
+
 		error = nbd_do_it(nbd);
 		kthread_stop(thread);
 
-- 
2.1.4


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 5/9] nbd: Fix device bytesize type
  2015-02-12 20:57 [PATCH 0/9] nbd: cleanups Markus Pargmann
                   ` (3 preceding siblings ...)
  2015-02-12 20:57 ` [PATCH 4/9] nbd: Replace kthread_create with kthread_run Markus Pargmann
@ 2015-02-12 20:57 ` Markus Pargmann
  2015-02-12 20:57 ` [PATCH 6/9] nbd: Restructure debugging prints Markus Pargmann
                   ` (3 subsequent siblings)
  8 siblings, 0 replies; 21+ messages in thread
From: Markus Pargmann @ 2015-02-12 20:57 UTC (permalink / raw)
  To: nbd-general; +Cc: linux-kernel, Markus Pargmann

The block subsystem uses loff_t to store the device size. Change the
type for nbd_device bytesize to loff_t.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index c07160c25a94..13c8371cbf4c 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -32,6 +32,7 @@
 #include <net/sock.h>
 #include <linux/net.h>
 #include <linux/kthread.h>
+#include <linux/types.h>
 
 #include <asm/uaccess.h>
 #include <asm/types.h>
@@ -54,7 +55,7 @@ struct nbd_device {
 	struct mutex tx_lock;
 	struct gendisk *disk;
 	int blksize;
-	u64 bytesize;
+	loff_t bytesize;
 	pid_t pid; /* pid of nbd-client, if attached */
 	int xmit_timeout;
 	int disconnect; /* a disconnect has been requested by user */
-- 
2.1.4


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 6/9] nbd: Restructure debugging prints
  2015-02-12 20:57 [PATCH 0/9] nbd: cleanups Markus Pargmann
                   ` (4 preceding siblings ...)
  2015-02-12 20:57 ` [PATCH 5/9] nbd: Fix device bytesize type Markus Pargmann
@ 2015-02-12 20:57 ` Markus Pargmann
  2015-02-12 21:08   ` Joe Perches
  2015-02-12 20:57 ` [PATCH 7/9] nbd: Remove fixme that was already fixed Markus Pargmann
                   ` (2 subsequent siblings)
  8 siblings, 1 reply; 21+ messages in thread
From: Markus Pargmann @ 2015-02-12 20:57 UTC (permalink / raw)
  To: nbd-general; +Cc: linux-kernel, Markus Pargmann

dprintk has some name collisions with other frameworks and drivers. It
is also not necessary to have these custom debug print filters. Dynamic
debug offers the same amount of filtered debugging.

This patch replaces all dprintks with dev_dbg(). It also removes the
ioctl dprintk which prints the ingoing ioctls which should be
replaceable by strace or similar stuff.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 86 ++++++++++++-----------------------------------------
 1 file changed, 19 insertions(+), 67 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 13c8371cbf4c..60a38b06a79b 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -63,21 +63,6 @@ struct nbd_device {
 
 #define NBD_MAGIC 0x68797548
 
-#ifdef NDEBUG
-#define dprintk(flags, fmt...)
-#else /* NDEBUG */
-#define dprintk(flags, fmt...) do { \
-	if (debugflags & (flags)) printk(KERN_DEBUG fmt); \
-} while (0)
-#define DBG_IOCTL       0x0004
-#define DBG_INIT        0x0010
-#define DBG_EXIT        0x0020
-#define DBG_BLKDEV      0x0100
-#define DBG_RX          0x0200
-#define DBG_TX          0x0400
-static unsigned int debugflags;
-#endif /* NDEBUG */
-
 static unsigned int nbds_max = 16;
 static struct nbd_device *nbd_dev;
 static int max_part;
@@ -94,27 +79,6 @@ static int max_part;
  */
 static DEFINE_SPINLOCK(nbd_lock);
 
-#ifndef NDEBUG
-static const char *ioctl_cmd_to_ascii(int cmd)
-{
-	switch (cmd) {
-	case NBD_SET_SOCK: return "set-sock";
-	case NBD_SET_BLKSIZE: return "set-blksize";
-	case NBD_SET_SIZE: return "set-size";
-	case NBD_SET_TIMEOUT: return "set-timeout";
-	case NBD_SET_FLAGS: return "set-flags";
-	case NBD_DO_IT: return "do-it";
-	case NBD_CLEAR_SOCK: return "clear-sock";
-	case NBD_CLEAR_QUE: return "clear-que";
-	case NBD_PRINT_DEBUG: return "print-debug";
-	case NBD_SET_SIZE_BLOCKS: return "set-size-blocks";
-	case NBD_DISCONNECT: return "disconnect";
-	case BLKROSET: return "set-read-only";
-	case BLKFLSBUF: return "flush-buffer-cache";
-	}
-	return "unknown";
-}
-
 static const char *nbdcmd_to_ascii(int cmd)
 {
 	switch (cmd) {
@@ -126,16 +90,15 @@ static const char *nbdcmd_to_ascii(int cmd)
 	}
 	return "invalid";
 }
-#endif /* NDEBUG */
 
-static void nbd_end_request(struct request *req)
+static void nbd_end_request(struct nbd_device *nbd, struct request *req)
 {
 	int error = req->errors ? -EIO : 0;
 	struct request_queue *q = req->q;
 	unsigned long flags;
 
-	dprintk(DBG_BLKDEV, "%s: request %p: %s\n", req->rq_disk->disk_name,
-			req, error ? "failed" : "done");
+	dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: %s\n",
+		req->rq_disk->disk_name, req, error ? "failed" : "done");
 
 	spin_lock_irqsave(q->queue_lock, flags);
 	__blk_end_request_all(req, error);
@@ -276,11 +239,9 @@ static int nbd_send_req(struct nbd_device *nbd, struct request *req)
 	}
 	memcpy(request.handle, &req, sizeof(req));
 
-	dprintk(DBG_TX, "%s: request %p: sending control (%s@%llu,%uB)\n",
-			nbd->disk->disk_name, req,
-			nbdcmd_to_ascii(nbd_cmd(req)),
-			(unsigned long long)blk_rq_pos(req) << 9,
-			blk_rq_bytes(req));
+	dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: sending control (%s@%llu,%uB)\n",
+		nbd->disk->disk_name, req, nbdcmd_to_ascii(nbd_cmd(req)),
+		(unsigned long long)blk_rq_pos(req) << 9, blk_rq_bytes(req));
 	result = sock_xmit(nbd, 1, &request, sizeof(request),
 			(nbd_cmd(req) == NBD_CMD_WRITE) ? MSG_MORE : 0);
 	if (result <= 0) {
@@ -300,8 +261,8 @@ static int nbd_send_req(struct nbd_device *nbd, struct request *req)
 			flags = 0;
 			if (!rq_iter_last(bvec, iter))
 				flags = MSG_MORE;
-			dprintk(DBG_TX, "%s: request %p: sending %d bytes data\n",
-					nbd->disk->disk_name, req, bvec.bv_len);
+			dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: sending %d bytes data\n",
+				nbd->disk->disk_name, req, bvec.bv_len);
 			result = sock_send_bvec(nbd, &bvec, flags);
 			if (result <= 0) {
 				dev_err(disk_to_dev(nbd->disk),
@@ -394,8 +355,8 @@ static struct request *nbd_read_stat(struct nbd_device *nbd)
 		return req;
 	}
 
-	dprintk(DBG_RX, "%s: request %p: got reply\n",
-			nbd->disk->disk_name, req);
+	dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: got reply\n",
+		nbd->disk->disk_name, req);
 	if (nbd_cmd(req) == NBD_CMD_READ) {
 		struct req_iterator iter;
 		struct bio_vec bvec;
@@ -408,7 +369,7 @@ static struct request *nbd_read_stat(struct nbd_device *nbd)
 				req->errors++;
 				return req;
 			}
-			dprintk(DBG_RX, "%s: request %p: got %d bytes data\n",
+			dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: got %d bytes data\n",
 				nbd->disk->disk_name, req, bvec.bv_len);
 		}
 	}
@@ -449,7 +410,7 @@ static int nbd_do_it(struct nbd_device *nbd)
 	}
 
 	while ((req = nbd_read_stat(nbd)) != NULL)
-		nbd_end_request(req);
+		nbd_end_request(nbd, req);
 
 	device_remove_file(disk_to_dev(nbd->disk), &pid_attr);
 	nbd->pid = 0;
@@ -478,7 +439,7 @@ static void nbd_clear_que(struct nbd_device *nbd)
 				 queuelist);
 		list_del_init(&req->queuelist);
 		req->errors++;
-		nbd_end_request(req);
+		nbd_end_request(nbd, req);
 	}
 
 	while (!list_empty(&nbd->waiting_queue)) {
@@ -486,7 +447,7 @@ static void nbd_clear_que(struct nbd_device *nbd)
 				 queuelist);
 		list_del_init(&req->queuelist);
 		req->errors++;
-		nbd_end_request(req);
+		nbd_end_request(nbd, req);
 	}
 }
 
@@ -530,7 +491,7 @@ static void nbd_handle_req(struct nbd_device *nbd, struct request *req)
 	if (nbd_send_req(nbd, req) != 0) {
 		dev_err(disk_to_dev(nbd->disk), "Request send failed\n");
 		req->errors++;
-		nbd_end_request(req);
+		nbd_end_request(nbd, req);
 	} else {
 		spin_lock(&nbd->queue_lock);
 		list_add_tail(&req->queuelist, &nbd->queue_head);
@@ -545,7 +506,7 @@ static void nbd_handle_req(struct nbd_device *nbd, struct request *req)
 
 error_out:
 	req->errors++;
-	nbd_end_request(req);
+	nbd_end_request(nbd, req);
 }
 
 static int nbd_thread(void *data)
@@ -593,8 +554,8 @@ static void do_nbd_request(struct request_queue *q)
 
 		spin_unlock_irq(q->queue_lock);
 
-		dprintk(DBG_BLKDEV, "%s: request %p: dequeued (flags=%x)\n",
-				req->rq_disk->disk_name, req, req->cmd_type);
+		dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: dequeued (flags=%x)\n",
+			req->rq_disk->disk_name, req, req->cmd_type);
 
 		nbd = req->rq_disk->private_data;
 
@@ -604,7 +565,7 @@ static void do_nbd_request(struct request_queue *q)
 			dev_err(disk_to_dev(nbd->disk),
 				"Attempted send on closed socket\n");
 			req->errors++;
-			nbd_end_request(req);
+			nbd_end_request(nbd, req);
 			spin_lock_irq(q->queue_lock);
 			continue;
 		}
@@ -791,10 +752,6 @@ static int nbd_ioctl(struct block_device *bdev, fmode_t mode,
 
 	BUG_ON(nbd->magic != NBD_MAGIC);
 
-	/* Anyone capable of this syscall can do *real bad* things */
-	dprintk(DBG_IOCTL, "%s: nbd_ioctl cmd=%s(0x%x) arg=%lu\n",
-		nbd->disk->disk_name, ioctl_cmd_to_ascii(cmd), cmd, arg);
-
 	mutex_lock(&nbd->tx_lock);
 	error = __nbd_ioctl(bdev, nbd, cmd, arg);
 	mutex_unlock(&nbd->tx_lock);
@@ -884,7 +841,6 @@ static int __init nbd_init(void)
 	}
 
 	printk(KERN_INFO "nbd: registered device at major %d\n", NBD_MAJOR);
-	dprintk(DBG_INIT, "nbd: debugflags=0x%x\n", debugflags);
 
 	for (i = 0; i < nbds_max; i++) {
 		struct gendisk *disk = nbd_dev[i].disk;
@@ -943,7 +899,3 @@ module_param(nbds_max, int, 0444);
 MODULE_PARM_DESC(nbds_max, "number of network block devices to initialize (default: 16)");
 module_param(max_part, int, 0444);
 MODULE_PARM_DESC(max_part, "number of partitions per device (default: 0)");
-#ifndef NDEBUG
-module_param(debugflags, int, 0644);
-MODULE_PARM_DESC(debugflags, "flags for controlling debug output");
-#endif
-- 
2.1.4


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 7/9] nbd: Remove fixme that was already fixed
  2015-02-12 20:57 [PATCH 0/9] nbd: cleanups Markus Pargmann
                   ` (5 preceding siblings ...)
  2015-02-12 20:57 ` [PATCH 6/9] nbd: Restructure debugging prints Markus Pargmann
@ 2015-02-12 20:57 ` Markus Pargmann
  2015-02-12 20:57 ` [PATCH 8/9] nbd: Return error code directly Markus Pargmann
  2015-02-12 20:57 ` [PATCH 9/9] nbd: Return error pointer directly Markus Pargmann
  8 siblings, 0 replies; 21+ messages in thread
From: Markus Pargmann @ 2015-02-12 20:57 UTC (permalink / raw)
  To: nbd-general; +Cc: linux-kernel, Markus Pargmann

The mentioned problem is not present anymore.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 60a38b06a79b..3a3e0057e991 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -105,14 +105,11 @@ static void nbd_end_request(struct nbd_device *nbd, struct request *req)
 	spin_unlock_irqrestore(q->queue_lock, flags);
 }
 
+/*
+ * Forcibly shutdown the socket causing all listeners to error
+ */
 static void sock_shutdown(struct nbd_device *nbd, 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(&nbd->tx_lock);
 	if (nbd->sock) {
-- 
2.1.4


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 8/9] nbd: Return error code directly
  2015-02-12 20:57 [PATCH 0/9] nbd: cleanups Markus Pargmann
                   ` (6 preceding siblings ...)
  2015-02-12 20:57 ` [PATCH 7/9] nbd: Remove fixme that was already fixed Markus Pargmann
@ 2015-02-12 20:57 ` Markus Pargmann
  2015-02-12 20:57 ` [PATCH 9/9] nbd: Return error pointer directly Markus Pargmann
  8 siblings, 0 replies; 21+ messages in thread
From: Markus Pargmann @ 2015-02-12 20:57 UTC (permalink / raw)
  To: nbd-general; +Cc: linux-kernel, Markus Pargmann

By returning the error code directly, we can avoid the jump label
error_out.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 3a3e0057e991..f2c1973c486a 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -244,7 +244,7 @@ static int nbd_send_req(struct nbd_device *nbd, struct request *req)
 	if (result <= 0) {
 		dev_err(disk_to_dev(nbd->disk),
 			"Send control failed (result %d)\n", result);
-		goto error_out;
+		return -EIO;
 	}
 
 	if (nbd_cmd(req) == NBD_CMD_WRITE) {
@@ -265,14 +265,11 @@ static int nbd_send_req(struct nbd_device *nbd, struct request *req)
 				dev_err(disk_to_dev(nbd->disk),
 					"Send data failed (result %d)\n",
 					result);
-				goto error_out;
+				return -EIO;
 			}
 		}
 	}
 	return 0;
-
-error_out:
-	return -EIO;
 }
 
 static struct request *nbd_find_request(struct nbd_device *nbd,
-- 
2.1.4


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 9/9] nbd: Return error pointer directly
  2015-02-12 20:57 [PATCH 0/9] nbd: cleanups Markus Pargmann
                   ` (7 preceding siblings ...)
  2015-02-12 20:57 ` [PATCH 8/9] nbd: Return error code directly Markus Pargmann
@ 2015-02-12 20:57 ` Markus Pargmann
  8 siblings, 0 replies; 21+ messages in thread
From: Markus Pargmann @ 2015-02-12 20:57 UTC (permalink / raw)
  To: nbd-general; +Cc: linux-kernel, Markus Pargmann

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/block/nbd.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index f2c1973c486a..170c148dc036 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -280,7 +280,7 @@ static struct request *nbd_find_request(struct nbd_device *nbd,
 
 	err = wait_event_interruptible(nbd->active_wq, nbd->active_req != xreq);
 	if (unlikely(err))
-		goto out;
+		return ERR_PTR(err);
 
 	spin_lock(&nbd->queue_lock);
 	list_for_each_entry_safe(req, tmp, &nbd->queue_head, queuelist) {
@@ -292,10 +292,7 @@ static struct request *nbd_find_request(struct nbd_device *nbd,
 	}
 	spin_unlock(&nbd->queue_lock);
 
-	err = -ENOENT;
-
-out:
-	return ERR_PTR(err);
+	return ERR_PTR(-ENOENT);
 }
 
 static inline int sock_recv_bvec(struct nbd_device *nbd, struct bio_vec *bvec)
-- 
2.1.4


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 6/9] nbd: Restructure debugging prints
  2015-02-12 20:57 ` [PATCH 6/9] nbd: Restructure debugging prints Markus Pargmann
@ 2015-02-12 21:08   ` Joe Perches
  2015-02-13  9:58     ` Markus Pargmann
  0 siblings, 1 reply; 21+ messages in thread
From: Joe Perches @ 2015-02-12 21:08 UTC (permalink / raw)
  To: Markus Pargmann; +Cc: nbd-general, linux-kernel

On Thu, 2015-02-12 at 21:57 +0100, Markus Pargmann wrote:
> dprintk has some name collisions with other frameworks and drivers. It
> is also not necessary to have these custom debug print filters. Dynamic
> debug offers the same amount of filtered debugging.
> 
> This patch replaces all dprintks with dev_dbg(). It also removes the
> ioctl dprintk which prints the ingoing ioctls which should be
> replaceable by strace or similar stuff.

Perhaps add

#define nbd_dbg(nbd, fmt, ...)					\
	dev_dbg(disk_to_dev((nbd)->disk), "%s: " fmt,		\
		nbd->disk->disk_name, ##__VA_ARGS__)

(or function with %pV)

> diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
[]
> +static void nbd_end_request(struct nbd_device *nbd, struct request *req)
>  {
>  	int error = req->errors ? -EIO : 0;
>  	struct request_queue *q = req->q;
>  	unsigned long flags;
>  
> -	dprintk(DBG_BLKDEV, "%s: request %p: %s\n", req->rq_disk->disk_name,
> -			req, error ? "failed" : "done");
> +	dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: %s\n",
> +		req->rq_disk->disk_name, req, error ? "failed" : "done");

so this becomes

	nbd_dbg(nbd, "request %p: %s\n",
		req, error ? "failed" : "done");

> @@ -276,11 +239,9 @@ static int nbd_send_req(struct nbd_device *nbd, struct request *req)
>  	}
>  	memcpy(request.handle, &req, sizeof(req));
>  
> -	dprintk(DBG_TX, "%s: request %p: sending control (%s@%llu,%uB)\n",
> -			nbd->disk->disk_name, req,
> -			nbdcmd_to_ascii(nbd_cmd(req)),
> -			(unsigned long long)blk_rq_pos(req) << 9,
> -			blk_rq_bytes(req));
> +	dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: sending control (%s@%llu,%uB)\n",
> +		nbd->disk->disk_name, req, nbdcmd_to_ascii(nbd_cmd(req)),
> +		(unsigned long long)blk_rq_pos(req) << 9, blk_rq_bytes(req));

	nbd_dbg(nbd, "request %p: sending control (%s@%llu,%uB)\n",
		req, nbdcmd_to_ascii(nbd_cmd(req)),
		(unsigned long long)blk_rq_pos(req) << 9, blk_rq_bytes(req));

> @@ -300,8 +261,8 @@ static int nbd_send_req(struct nbd_device *nbd, struct request *req)
>  			flags = 0;
>  			if (!rq_iter_last(bvec, iter))
>  				flags = MSG_MORE;
> -			dprintk(DBG_TX, "%s: request %p: sending %d bytes data\n",
> -					nbd->disk->disk_name, req, bvec.bv_len);
> +			dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: sending %d bytes data\n",
> +				nbd->disk->disk_name, req, bvec.bv_len);

etc...



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 6/9] nbd: Restructure debugging prints
  2015-02-12 21:08   ` Joe Perches
@ 2015-02-13  9:58     ` Markus Pargmann
  2015-02-13 10:05       ` Joe Perches
  0 siblings, 1 reply; 21+ messages in thread
From: Markus Pargmann @ 2015-02-13  9:58 UTC (permalink / raw)
  To: Joe Perches; +Cc: nbd-general, linux-kernel

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

On Thu, Feb 12, 2015 at 01:08:48PM -0800, Joe Perches wrote:
> On Thu, 2015-02-12 at 21:57 +0100, Markus Pargmann wrote:
> > dprintk has some name collisions with other frameworks and drivers. It
> > is also not necessary to have these custom debug print filters. Dynamic
> > debug offers the same amount of filtered debugging.
> > 
> > This patch replaces all dprintks with dev_dbg(). It also removes the
> > ioctl dprintk which prints the ingoing ioctls which should be
> > replaceable by strace or similar stuff.
> 
> Perhaps add
> 
> #define nbd_dbg(nbd, fmt, ...)					\
> 	dev_dbg(disk_to_dev((nbd)->disk), "%s: " fmt,		\
> 		nbd->disk->disk_name, ##__VA_ARGS__)

I am not really happy with those custom debug print macros. What do you
think about an inline function 'nbd_to_dev' instead?

> 
> (or function with %pV)
> 
> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> []
> > +static void nbd_end_request(struct nbd_device *nbd, struct request *req)
> >  {
> >  	int error = req->errors ? -EIO : 0;
> >  	struct request_queue *q = req->q;
> >  	unsigned long flags;
> >  
> > -	dprintk(DBG_BLKDEV, "%s: request %p: %s\n", req->rq_disk->disk_name,
> > -			req, error ? "failed" : "done");
> > +	dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: %s\n",
> > +		req->rq_disk->disk_name, req, error ? "failed" : "done");
> 
> so this becomes
> 
> 	nbd_dbg(nbd, "request %p: %s\n",
> 		req, error ? "failed" : "done");

so this would be:
 	nbd_dbg(nbd_to_dev(nbd), "request %p: %s\n",
 		req, error ? "failed" : "done");

Best regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 6/9] nbd: Restructure debugging prints
  2015-02-13  9:58     ` Markus Pargmann
@ 2015-02-13 10:05       ` Joe Perches
  2015-02-13 11:24         ` Markus Pargmann
  0 siblings, 1 reply; 21+ messages in thread
From: Joe Perches @ 2015-02-13 10:05 UTC (permalink / raw)
  To: Markus Pargmann; +Cc: nbd-general, linux-kernel

On Fri, 2015-02-13 at 10:58 +0100, Markus Pargmann wrote:
> On Thu, Feb 12, 2015 at 01:08:48PM -0800, Joe Perches wrote:
> > On Thu, 2015-02-12 at 21:57 +0100, Markus Pargmann wrote:
> > > dprintk has some name collisions with other frameworks and drivers. It
> > > is also not necessary to have these custom debug print filters. Dynamic
> > > debug offers the same amount of filtered debugging.
> > > 
> > > This patch replaces all dprintks with dev_dbg(). It also removes the
> > > ioctl dprintk which prints the ingoing ioctls which should be
> > > replaceable by strace or similar stuff.
> > 
> > Perhaps add
> > 
> > #define nbd_dbg(nbd, fmt, ...)					\
> > 	dev_dbg(disk_to_dev((nbd)->disk), "%s: " fmt,		\
> > 		nbd->disk->disk_name, ##__VA_ARGS__)
> 
> I am not really happy with those custom debug print macros. What do you
> think about an inline function 'nbd_to_dev' instead?

Wouldn't that change the output?
What would the output look like?

> > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > []
> > > +static void nbd_end_request(struct nbd_device *nbd, struct request *req)
> > >  {
> > >  	int error = req->errors ? -EIO : 0;
> > >  	struct request_queue *q = req->q;
> > >  	unsigned long flags;
> > >  
> > > -	dprintk(DBG_BLKDEV, "%s: request %p: %s\n", req->rq_disk->disk_name,
> > > -			req, error ? "failed" : "done");
> > > +	dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: %s\n",
> > > +		req->rq_disk->disk_name, req, error ? "failed" : "done");
> > 
> > so this becomes
> > 
> > 	nbd_dbg(nbd, "request %p: %s\n",
> > 		req, error ? "failed" : "done");
> 
> so this would be:
>  	nbd_dbg(nbd_to_dev(nbd), "request %p: %s\n",
>  		req, error ? "failed" : "done");

I don't see much value in that style,
but I don't manage the code either.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 6/9] nbd: Restructure debugging prints
  2015-02-13 10:05       ` Joe Perches
@ 2015-02-13 11:24         ` Markus Pargmann
  2015-02-13 11:48           ` Joe Perches
  0 siblings, 1 reply; 21+ messages in thread
From: Markus Pargmann @ 2015-02-13 11:24 UTC (permalink / raw)
  To: Joe Perches; +Cc: nbd-general, linux-kernel

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

Hi,

On Fri, Feb 13, 2015 at 02:05:27AM -0800, Joe Perches wrote:
> On Fri, 2015-02-13 at 10:58 +0100, Markus Pargmann wrote:
> > On Thu, Feb 12, 2015 at 01:08:48PM -0800, Joe Perches wrote:
> > > On Thu, 2015-02-12 at 21:57 +0100, Markus Pargmann wrote:
> > > > dprintk has some name collisions with other frameworks and drivers. It
> > > > is also not necessary to have these custom debug print filters. Dynamic
> > > > debug offers the same amount of filtered debugging.
> > > > 
> > > > This patch replaces all dprintks with dev_dbg(). It also removes the
> > > > ioctl dprintk which prints the ingoing ioctls which should be
> > > > replaceable by strace or similar stuff.
> > > 
> > > Perhaps add
> > > 
> > > #define nbd_dbg(nbd, fmt, ...)					\
> > > 	dev_dbg(disk_to_dev((nbd)->disk), "%s: " fmt,		\
> > > 		nbd->disk->disk_name, ##__VA_ARGS__)
> > 
> > I am not really happy with those custom debug print macros. What do you
> > think about an inline function 'nbd_to_dev' instead?
> 
> Wouldn't that change the output?
> What would the output look like?

I meant a function that just translates struct nbd_device* to struct
device*. That wouldn't change the output.

> 
> > > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > > []
> > > > +static void nbd_end_request(struct nbd_device *nbd, struct request *req)
> > > >  {
> > > >  	int error = req->errors ? -EIO : 0;
> > > >  	struct request_queue *q = req->q;
> > > >  	unsigned long flags;
> > > >  
> > > > -	dprintk(DBG_BLKDEV, "%s: request %p: %s\n", req->rq_disk->disk_name,
> > > > -			req, error ? "failed" : "done");
> > > > +	dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: %s\n",
> > > > +		req->rq_disk->disk_name, req, error ? "failed" : "done");
> > > 
> > > so this becomes
> > > 
> > > 	nbd_dbg(nbd, "request %p: %s\n",
> > > 		req, error ? "failed" : "done");
> > 
> > so this would be:
> >  	nbd_dbg(nbd_to_dev(nbd), "request %p: %s\n",
> >  		req, error ? "failed" : "done");
> 
> I don't see much value in that style,
> but I don't manage the code either.

Oh sorry, I meant to write:
	dev_dbg(nbd_to_dev(nbd), "request %p: %s\n",
		req, error ? "failed" : "done");

So the normal dev_dbg call is still there and the expression to get the
device from a nbd_device struct is shorter.

Thanks,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 6/9] nbd: Restructure debugging prints
  2015-02-13 11:24         ` Markus Pargmann
@ 2015-02-13 11:48           ` Joe Perches
  2015-02-15 22:20             ` Markus Pargmann
  0 siblings, 1 reply; 21+ messages in thread
From: Joe Perches @ 2015-02-13 11:48 UTC (permalink / raw)
  To: Markus Pargmann; +Cc: nbd-general, linux-kernel

On Fri, 2015-02-13 at 12:24 +0100, Markus Pargmann wrote:
> On Fri, Feb 13, 2015 at 02:05:27AM -0800, Joe Perches wrote:
> > On Fri, 2015-02-13 at 10:58 +0100, Markus Pargmann wrote:
> > > On Thu, Feb 12, 2015 at 01:08:48PM -0800, Joe Perches wrote:
> > > > On Thu, 2015-02-12 at 21:57 +0100, Markus Pargmann wrote:
> > > > > dprintk has some name collisions with other frameworks and drivers. It
> > > > > is also not necessary to have these custom debug print filters. Dynamic
> > > > > debug offers the same amount of filtered debugging.
> > > > > 
> > > > > This patch replaces all dprintks with dev_dbg(). It also removes the
> > > > > ioctl dprintk which prints the ingoing ioctls which should be
> > > > > replaceable by strace or similar stuff.
> > > > 
> > > > Perhaps add
> > > > 
> > > > #define nbd_dbg(nbd, fmt, ...)					\
> > > > 	dev_dbg(disk_to_dev((nbd)->disk), "%s: " fmt,		\
> > > > 		nbd->disk->disk_name, ##__VA_ARGS__)
> > > 
> > > I am not really happy with those custom debug print macros. What do you
> > > think about an inline function 'nbd_to_dev' instead?
> > 
> > Wouldn't that change the output?
> > What would the output look like?
> 
> I meant a function that just translates struct nbd_device* to struct
> device*. That wouldn't change the output.
> > > > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > > > []
> > > > > +static void nbd_end_request(struct nbd_device *nbd, struct request *req)
> > > > >  {
> > > > >  	int error = req->errors ? -EIO : 0;
> > > > >  	struct request_queue *q = req->q;
> > > > >  	unsigned long flags;
> > > > >  
> > > > > -	dprintk(DBG_BLKDEV, "%s: request %p: %s\n", req->rq_disk->disk_name,
> > > > > -			req, error ? "failed" : "done");
> > > > > +	dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: %s\n",
> > > > > +		req->rq_disk->disk_name, req, error ? "failed" : "done");
> > > > 
> > > > so this becomes
> > > > 
> > > > 	nbd_dbg(nbd, "request %p: %s\n",
> > > > 		req, error ? "failed" : "done");
> > > 
> > > so this would be:
> > >  	nbd_dbg(nbd_to_dev(nbd), "request %p: %s\n",
> > >  		req, error ? "failed" : "done");
> > 
> > I don't see much value in that style,
> > but I don't manage the code either.
> 
> Oh sorry, I meant to write:
> 	dev_dbg(nbd_to_dev(nbd), "request %p: %s\n",
> 		req, error ? "failed" : "done");
> 
> So the normal dev_dbg call is still there and the expression to get the
> device from a nbd_device struct is shorter.

Is nbd->disk->disk_name the same string as
disk_to_dev((nbd)->disk)->name?

What's the output of the conversion in this patch?

-	dprintk(DBG_BLKDEV, "%s: request %p: %s\n", req->rq_disk->disk_name,
-			req, error ? "failed" : "done"); 
+	dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: %s\n",
+		req->rq_disk->disk_name, req, error ? "failed" : "done");

Should this conversion be as you wrote above

	dev_dbg(nbd_to_dev(nbd), "request %p: %s\n",
 		req, error ? "failed" : "done");

If so, then there's probably not much use in a
custom nbd_dbg macro.

There is some value though in classifying blocks of
debugging output akin to the use of netif_msg_<type>.

That is lost with these conversions.



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Nbd] [PATCH 2/9] Documentation: nbd: Add list of module parameters
  2015-02-12 20:57 ` [PATCH 2/9] Documentation: nbd: Add list of module parameters Markus Pargmann
@ 2015-02-14 10:29   ` Wouter Verhelst
  2015-02-15 21:53     ` Markus Pargmann
  0 siblings, 1 reply; 21+ messages in thread
From: Wouter Verhelst @ 2015-02-14 10:29 UTC (permalink / raw)
  To: Markus Pargmann; +Cc: nbd-general, linux-kernel

On Thu, Feb 12, 2015 at 09:57:30PM +0100, Markus Pargmann wrote:
> +max_part
> +	Number of partitions per device (default: 0).

About that. Wouldn't it be better to change that default? Something like
16 makes more sense to me.

-- 
It is easy to love a country that is famous for chocolate and beer

  -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Nbd] [PATCH 3/9] nbd: Remove kernel internal header
  2015-02-12 20:57 ` [PATCH 3/9] nbd: Remove kernel internal header Markus Pargmann
@ 2015-02-14 10:30   ` Wouter Verhelst
  2015-02-15 21:56     ` Markus Pargmann
  0 siblings, 1 reply; 21+ messages in thread
From: Wouter Verhelst @ 2015-02-14 10:30 UTC (permalink / raw)
  To: Markus Pargmann; +Cc: nbd-general, linux-kernel

On Thu, Feb 12, 2015 at 09:57:31PM +0100, Markus Pargmann wrote:
> The header is not included anywhere. Remove it and include the private
> nbd_device struct in nbd.c.

It exists mostly for the benefit of userspace trying to speak the NBD
protocol. I've stopped trying to depend on it (since nbd-server needs to
run on !Linux, too), but there are other implementations that might want
to use it.

nbd.h is part of a public API. Let's not drop it.

-- 
It is easy to love a country that is famous for chocolate and beer

  -- Barack Obama, speaking in Brussels, Belgium, 2014-03-26

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Nbd] [PATCH 2/9] Documentation: nbd: Add list of module parameters
  2015-02-14 10:29   ` [Nbd] " Wouter Verhelst
@ 2015-02-15 21:53     ` Markus Pargmann
  0 siblings, 0 replies; 21+ messages in thread
From: Markus Pargmann @ 2015-02-15 21:53 UTC (permalink / raw)
  To: Wouter Verhelst; +Cc: nbd-general, linux-kernel

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

On Sat, Feb 14, 2015 at 11:29:22AM +0100, Wouter Verhelst wrote:
> On Thu, Feb 12, 2015 at 09:57:30PM +0100, Markus Pargmann wrote:
> > +max_part
> > +	Number of partitions per device (default: 0).
> 
> About that. Wouldn't it be better to change that default? Something like
> 16 makes more sense to me.

Setting this should not be necessary at all. I am not sure how to
achieve that. But first of all the existing parameters should be documented.

Best regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [Nbd] [PATCH 3/9] nbd: Remove kernel internal header
  2015-02-14 10:30   ` [Nbd] " Wouter Verhelst
@ 2015-02-15 21:56     ` Markus Pargmann
  0 siblings, 0 replies; 21+ messages in thread
From: Markus Pargmann @ 2015-02-15 21:56 UTC (permalink / raw)
  To: Wouter Verhelst; +Cc: nbd-general, linux-kernel

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

On Sat, Feb 14, 2015 at 11:30:24AM +0100, Wouter Verhelst wrote:
> On Thu, Feb 12, 2015 at 09:57:31PM +0100, Markus Pargmann wrote:
> > The header is not included anywhere. Remove it and include the private
> > nbd_device struct in nbd.c.
> 
> It exists mostly for the benefit of userspace trying to speak the NBD
> protocol. I've stopped trying to depend on it (since nbd-server needs to
> run on !Linux, too), but there are other implementations that might want
> to use it.
> 
> nbd.h is part of a public API. Let's not drop it.

This is just about the kernel internal header include/linux/nbd.h. It is
not about the uapi header. I don't want to remove the protocol header.

The header this patch is about defines only 'struct nbd_device' which is
as far as I can tell only used by nbd.c.

Best regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 6/9] nbd: Restructure debugging prints
  2015-02-13 11:48           ` Joe Perches
@ 2015-02-15 22:20             ` Markus Pargmann
  2015-02-16  0:06               ` Joe Perches
  0 siblings, 1 reply; 21+ messages in thread
From: Markus Pargmann @ 2015-02-15 22:20 UTC (permalink / raw)
  To: Joe Perches; +Cc: nbd-general, linux-kernel

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

On Fri, Feb 13, 2015 at 03:48:06AM -0800, Joe Perches wrote:
> On Fri, 2015-02-13 at 12:24 +0100, Markus Pargmann wrote:
> > On Fri, Feb 13, 2015 at 02:05:27AM -0800, Joe Perches wrote:
> > > On Fri, 2015-02-13 at 10:58 +0100, Markus Pargmann wrote:
> > > > On Thu, Feb 12, 2015 at 01:08:48PM -0800, Joe Perches wrote:
> > > > > On Thu, 2015-02-12 at 21:57 +0100, Markus Pargmann wrote:
> > > > > > dprintk has some name collisions with other frameworks and drivers. It
> > > > > > is also not necessary to have these custom debug print filters. Dynamic
> > > > > > debug offers the same amount of filtered debugging.
> > > > > > 
> > > > > > This patch replaces all dprintks with dev_dbg(). It also removes the
> > > > > > ioctl dprintk which prints the ingoing ioctls which should be
> > > > > > replaceable by strace or similar stuff.
> > > > > 
> > > > > Perhaps add
> > > > > 
> > > > > #define nbd_dbg(nbd, fmt, ...)					\
> > > > > 	dev_dbg(disk_to_dev((nbd)->disk), "%s: " fmt,		\
> > > > > 		nbd->disk->disk_name, ##__VA_ARGS__)
> > > > 
> > > > I am not really happy with those custom debug print macros. What do you
> > > > think about an inline function 'nbd_to_dev' instead?
> > > 
> > > Wouldn't that change the output?
> > > What would the output look like?
> > 
> > I meant a function that just translates struct nbd_device* to struct
> > device*. That wouldn't change the output.
> > > > > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > > > > []
> > > > > > +static void nbd_end_request(struct nbd_device *nbd, struct request *req)
> > > > > >  {
> > > > > >  	int error = req->errors ? -EIO : 0;
> > > > > >  	struct request_queue *q = req->q;
> > > > > >  	unsigned long flags;
> > > > > >  
> > > > > > -	dprintk(DBG_BLKDEV, "%s: request %p: %s\n", req->rq_disk->disk_name,
> > > > > > -			req, error ? "failed" : "done");
> > > > > > +	dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: %s\n",
> > > > > > +		req->rq_disk->disk_name, req, error ? "failed" : "done");
> > > > > 
> > > > > so this becomes
> > > > > 
> > > > > 	nbd_dbg(nbd, "request %p: %s\n",
> > > > > 		req, error ? "failed" : "done");
> > > > 
> > > > so this would be:
> > > >  	nbd_dbg(nbd_to_dev(nbd), "request %p: %s\n",
> > > >  		req, error ? "failed" : "done");
> > > 
> > > I don't see much value in that style,
> > > but I don't manage the code either.
> > 
> > Oh sorry, I meant to write:
> > 	dev_dbg(nbd_to_dev(nbd), "request %p: %s\n",
> > 		req, error ? "failed" : "done");
> > 
> > So the normal dev_dbg call is still there and the expression to get the
> > device from a nbd_device struct is shorter.
> 
> Is nbd->disk->disk_name the same string as
> disk_to_dev((nbd)->disk)->name?
> 
> What's the output of the conversion in this patch?

Oh yes, thanks, that's twice the device name then. Will fix that.

> 
> -	dprintk(DBG_BLKDEV, "%s: request %p: %s\n", req->rq_disk->disk_name,
> -			req, error ? "failed" : "done"); 
> +	dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: %s\n",
> +		req->rq_disk->disk_name, req, error ? "failed" : "done");
> 
> Should this conversion be as you wrote above
> 
> 	dev_dbg(nbd_to_dev(nbd), "request %p: %s\n",
>  		req, error ? "failed" : "done");
> 
> If so, then there's probably not much use in a
> custom nbd_dbg macro.
> 
> There is some value though in classifying blocks of
> debugging output akin to the use of netif_msg_<type>.
> 
> That is lost with these conversions.

It is still possible to enable/disable particular dev_dbg calls using
the dynamic debug interface. It is not as simple as the classification
before but on the other hand it will probably not be used very often.

dev_dbg() also allows enabling the debug output while the kernel is
running. The previous dprintk() was only available when the kernel was
compiled with "NDEBUG" defined.

Best regards,

Markus

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 6/9] nbd: Restructure debugging prints
  2015-02-15 22:20             ` Markus Pargmann
@ 2015-02-16  0:06               ` Joe Perches
  0 siblings, 0 replies; 21+ messages in thread
From: Joe Perches @ 2015-02-16  0:06 UTC (permalink / raw)
  To: Markus Pargmann; +Cc: nbd-general, linux-kernel

On Sun, 2015-02-15 at 23:20 +0100, Markus Pargmann wrote:
> On Fri, Feb 13, 2015 at 03:48:06AM -0800, Joe Perches wrote:
> > On Fri, 2015-02-13 at 12:24 +0100, Markus Pargmann wrote:
> > > On Fri, Feb 13, 2015 at 02:05:27AM -0800, Joe Perches wrote:
> > > > On Fri, 2015-02-13 at 10:58 +0100, Markus Pargmann wrote:
> > > > > On Thu, Feb 12, 2015 at 01:08:48PM -0800, Joe Perches wrote:
> > > > > > On Thu, 2015-02-12 at 21:57 +0100, Markus Pargmann wrote:
> > > > > > > dprintk has some name collisions with other frameworks and drivers. It
> > > > > > > is also not necessary to have these custom debug print filters. Dynamic
> > > > > > > debug offers the same amount of filtered debugging.
> > > > > > > 
> > > > > > > This patch replaces all dprintks with dev_dbg(). It also removes the
> > > > > > > ioctl dprintk which prints the ingoing ioctls which should be
> > > > > > > replaceable by strace or similar stuff.
> > > > > > 
> > > > > > Perhaps add
> > > > > > 
> > > > > > #define nbd_dbg(nbd, fmt, ...)					\
> > > > > > 	dev_dbg(disk_to_dev((nbd)->disk), "%s: " fmt,		\
> > > > > > 		nbd->disk->disk_name, ##__VA_ARGS__)
> > > > > 
> > > > > I am not really happy with those custom debug print macros. What do you
> > > > > think about an inline function 'nbd_to_dev' instead?
> > > > 
> > > > Wouldn't that change the output?
> > > > What would the output look like?
> > > 
> > > I meant a function that just translates struct nbd_device* to struct
> > > device*. That wouldn't change the output.
> > > > > > > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > > > > > []
> > > > > > > +static void nbd_end_request(struct nbd_device *nbd, struct request *req)
> > > > > > >  {
> > > > > > >  	int error = req->errors ? -EIO : 0;
> > > > > > >  	struct request_queue *q = req->q;
> > > > > > >  	unsigned long flags;
> > > > > > >  
> > > > > > > -	dprintk(DBG_BLKDEV, "%s: request %p: %s\n", req->rq_disk->disk_name,
> > > > > > > -			req, error ? "failed" : "done");
> > > > > > > +	dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: %s\n",
> > > > > > > +		req->rq_disk->disk_name, req, error ? "failed" : "done");
> > > > > > 
> > > > > > so this becomes
> > > > > > 
> > > > > > 	nbd_dbg(nbd, "request %p: %s\n",
> > > > > > 		req, error ? "failed" : "done");
> > > > > 
> > > > > so this would be:
> > > > >  	nbd_dbg(nbd_to_dev(nbd), "request %p: %s\n",
> > > > >  		req, error ? "failed" : "done");
> > > > 
> > > > I don't see much value in that style,
> > > > but I don't manage the code either.
> > > 
> > > Oh sorry, I meant to write:
> > > 	dev_dbg(nbd_to_dev(nbd), "request %p: %s\n",
> > > 		req, error ? "failed" : "done");
> > > 
> > > So the normal dev_dbg call is still there and the expression to get the
> > > device from a nbd_device struct is shorter.
> > 
> > Is nbd->disk->disk_name the same string as
> > disk_to_dev((nbd)->disk)->name?
> > 
> > What's the output of the conversion in this patch?
> 
> Oh yes, thanks, that's twice the device name then. Will fix that.
> 
> > 
> > -	dprintk(DBG_BLKDEV, "%s: request %p: %s\n", req->rq_disk->disk_name,
> > -			req, error ? "failed" : "done"); 
> > +	dev_dbg(disk_to_dev(nbd->disk), "%s: request %p: %s\n",
> > +		req->rq_disk->disk_name, req, error ? "failed" : "done");
> > 
> > Should this conversion be as you wrote above
> > 
> > 	dev_dbg(nbd_to_dev(nbd), "request %p: %s\n",
> >  		req, error ? "failed" : "done");
> > 
> > If so, then there's probably not much use in a
> > custom nbd_dbg macro.
> > 
> > There is some value though in classifying blocks of
> > debugging output akin to the use of netif_msg_<type>.
> > 
> > That is lost with these conversions.
> 
> It is still possible to enable/disable particular dev_dbg calls using
> the dynamic debug interface.

Yes, true, but not by type, only all/none/specific.

I've previously proposed mechanisms to categorize
dynamic debugging output by type.




^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2015-02-16  0:06 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-02-12 20:57 [PATCH 0/9] nbd: cleanups Markus Pargmann
2015-02-12 20:57 ` [PATCH 1/9] Documentation: nbd: Reformat to allow more documentation Markus Pargmann
2015-02-12 20:57 ` [PATCH 2/9] Documentation: nbd: Add list of module parameters Markus Pargmann
2015-02-14 10:29   ` [Nbd] " Wouter Verhelst
2015-02-15 21:53     ` Markus Pargmann
2015-02-12 20:57 ` [PATCH 3/9] nbd: Remove kernel internal header Markus Pargmann
2015-02-14 10:30   ` [Nbd] " Wouter Verhelst
2015-02-15 21:56     ` Markus Pargmann
2015-02-12 20:57 ` [PATCH 4/9] nbd: Replace kthread_create with kthread_run Markus Pargmann
2015-02-12 20:57 ` [PATCH 5/9] nbd: Fix device bytesize type Markus Pargmann
2015-02-12 20:57 ` [PATCH 6/9] nbd: Restructure debugging prints Markus Pargmann
2015-02-12 21:08   ` Joe Perches
2015-02-13  9:58     ` Markus Pargmann
2015-02-13 10:05       ` Joe Perches
2015-02-13 11:24         ` Markus Pargmann
2015-02-13 11:48           ` Joe Perches
2015-02-15 22:20             ` Markus Pargmann
2015-02-16  0:06               ` Joe Perches
2015-02-12 20:57 ` [PATCH 7/9] nbd: Remove fixme that was already fixed Markus Pargmann
2015-02-12 20:57 ` [PATCH 8/9] nbd: Return error code directly Markus Pargmann
2015-02-12 20:57 ` [PATCH 9/9] nbd: Return error pointer directly Markus Pargmann

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).