[PATCH 0/5] Five nbd patches

[PATCH 0/5] Five nbd patches

[Bart Van Assche]

Hi Jens,

These patches are what I came up with after having reviewed the nbd source code.
Please consider these patches for the next merge window.

Thanks,

Bart.

Bart Van Assche (5):
  nbd: Use NULL to represent a pointer
  nbd: Remove superfluous casts
  nbd: Improve the documentation of the locking assumptions
  nbd: Remove a local variable from nbd_send_cmd()
  nbd: Fix signal handling

 drivers/block/nbd.c        | 41 ++++++++++++++++++++------------------
 include/trace/events/nbd.h |  2 +-
 2 files changed, 23 insertions(+), 20 deletions(-)

[PATCH 1/5] nbd: Use NULL to represent a pointer

[Bart Van Assche]

This patch fixes the following sparse warnings:

drivers/block/nbd.c: note: in included file (through include/trace/trace_events.h, include/trace/define_trace.h, include/trace/events/nbd.h):
./include/trace/events/nbd.h:61:1: warning: Using plain integer as NULL pointer
drivers/block/nbd.c: note: in included file (through include/trace/perf.h, include/trace/define_trace.h, include/trace/events/nbd.h):
./include/trace/events/nbd.h:61:1: warning: Using plain integer as NULL pointer

Cc: Christoph Hellwig <hch@lst.de>
Cc: Josef Bacik <jbacik@fb.com>
Cc: Yu Kuai <yukuai3@huawei.com>
Cc: Markus Pargmann <mpa@pengutronix.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 include/trace/events/nbd.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/trace/events/nbd.h b/include/trace/events/nbd.h
index 9849956f34d8..390d98a05c9d 100644
--- a/include/trace/events/nbd.h
+++ b/include/trace/events/nbd.h
@@ -72,7 +72,7 @@ DECLARE_EVENT_CLASS(nbd_send_request,
 	),
 
 	TP_fast_assign(
-		__entry->nbd_request = 0;
+		__entry->nbd_request = NULL;
 		__entry->dev_index = index;
 		__entry->request = rq;
 	),

[PATCH 2/5] nbd: Remove superfluous casts

[Bart Van Assche]

In Linux kernel code it is preferred not to use a cast when converting a
void pointer to another pointer type.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Josef Bacik <jbacik@fb.com>
Cc: Yu Kuai <yukuai3@huawei.com>
Cc: Markus Pargmann <mpa@pengutronix.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/nbd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 9d4ec9273bf9..90760f27824d 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -222,7 +222,7 @@ static ssize_t pid_show(struct device *dev,
 			struct device_attribute *attr, char *buf)
 {
 	struct gendisk *disk = dev_to_disk(dev);
-	struct nbd_device *nbd = (struct nbd_device *)disk->private_data;
+	struct nbd_device *nbd = disk->private_data;
 
 	return sprintf(buf, "%d\n", nbd->pid);
 }
@@ -236,7 +236,7 @@ static ssize_t backend_show(struct device *dev,
 		struct device_attribute *attr, char *buf)
 {
 	struct gendisk *disk = dev_to_disk(dev);
-	struct nbd_device *nbd = (struct nbd_device *)disk->private_data;
+	struct nbd_device *nbd = disk->private_data;
 
 	return sprintf(buf, "%s\n", nbd->backend ?: "");
 }

[PATCH 3/5] nbd: Improve the documentation of the locking assumptions

[Bart Van Assche]

Document locking assumptions with lockdep_assert_held() instead of source
code comments. The advantage of lockdep_assert_held() is that it is
verified at runtime if lockdep is enabled in the kernel config.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Josef Bacik <jbacik@fb.com>
Cc: Yu Kuai <yukuai3@huawei.com>
Cc: Markus Pargmann <mpa@pengutronix.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/nbd.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 90760f27824d..05f69710afe8 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -588,7 +588,6 @@ static inline int was_interrupted(int result)
 	return result == -ERESTARTSYS || result == -EINTR;
 }
 
-/* always call with the tx_lock held */
 static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
 {
 	struct request *req = blk_mq_rq_from_pdu(cmd);
@@ -605,6 +604,9 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
 	u32 nbd_cmd_flags = 0;
 	int sent = nsock->sent, skip = 0;
 
+	lockdep_assert_held(&cmd->lock);
+	lockdep_assert_held(&nsock->tx_lock);
+
 	iov_iter_kvec(&from, ITER_SOURCE, &iov, 1, sizeof(request));
 
 	type = req_to_nbd_cmd_type(req);
@@ -1015,6 +1017,8 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
 	struct nbd_sock *nsock;
 	int ret;
 
+	lockdep_assert_held(&cmd->lock);
+
 	config = nbd_get_config_unlocked(nbd);
 	if (!config) {
 		dev_err_ratelimited(disk_to_dev(nbd->disk),

[PATCH 4/5] nbd: Remove a local variable from nbd_send_cmd()

[Bart Van Assche]

blk_rq_bytes() returns an unsigned int while 'size' has type unsigned long.
This is confusing. Improve code readability by removing the local variable
'size'.

Cc: Christoph Hellwig <hch@lst.de>
Cc: Josef Bacik <jbacik@fb.com>
Cc: Yu Kuai <yukuai3@huawei.com>
Cc: Markus Pargmann <mpa@pengutronix.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/nbd.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 05f69710afe8..29e43ab1650c 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -597,7 +597,6 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
 	struct nbd_request request = {.magic = htonl(NBD_REQUEST_MAGIC)};
 	struct kvec iov = {.iov_base = &request, .iov_len = sizeof(request)};
 	struct iov_iter from;
-	unsigned long size = blk_rq_bytes(req);
 	struct bio *bio;
 	u64 handle;
 	u32 type;
@@ -646,7 +645,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
 	request.type = htonl(type | nbd_cmd_flags);
 	if (type != NBD_CMD_FLUSH) {
 		request.from = cpu_to_be64((u64)blk_rq_pos(req) << 9);
-		request.len = htonl(size);
+		request.len = htonl(blk_rq_bytes(req));
 	}
 	handle = nbd_cmd_handle(cmd);
 	request.cookie = cpu_to_be64(handle);

[PATCH 5/5] nbd: Fix signal handling

[Bart Van Assche]

Both nbd_send_cmd() and nbd_handle_cmd() return either a negative error
number or a positive blk_status_t value. nbd_queue_rq() converts these
return values into a blk_status_t value. There is a bug in the conversion
code: if nbd_send_cmd() returns BLK_STS_RESOURCE, nbd_queue_rq() should
return BLK_STS_RESOURCE instead of BLK_STS_OK. Fix this, move the
conversion code into nbd_handle_cmd() and fix the remaining sparse warnings.

This patch fixes the following sparse warnings:

drivers/block/nbd.c:673:32: warning: incorrect type in return expression (different base types)
drivers/block/nbd.c:673:32:    expected int
drivers/block/nbd.c:673:32:    got restricted blk_status_t [usertype]
drivers/block/nbd.c:714:48: warning: incorrect type in return expression (different base types)
drivers/block/nbd.c:714:48:    expected int
drivers/block/nbd.c:714:48:    got restricted blk_status_t [usertype]
drivers/block/nbd.c:1120:21: warning: incorrect type in assignment (different base types)
drivers/block/nbd.c:1120:21:    expected int [assigned] ret
drivers/block/nbd.c:1120:21:    got restricted blk_status_t [usertype]
drivers/block/nbd.c:1125:16: warning: incorrect type in return expression (different base types)
drivers/block/nbd.c:1125:16:    expected restricted blk_status_t
drivers/block/nbd.c:1125:16:    got int [assigned] ret

Cc: Christoph Hellwig <hch@lst.de>
Cc: Josef Bacik <jbacik@fb.com>
Cc: Yu Kuai <yukuai3@huawei.com>
Cc: Markus Pargmann <mpa@pengutronix.de>
Fixes: fc17b6534eb8 ("blk-mq: switch ->queue_rq return value to blk_status_t")
Cc: stable@vger.kernel.org
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/block/nbd.c | 28 ++++++++++++++--------------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 29e43ab1650c..22a79a62cc4e 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -588,6 +588,10 @@ static inline int was_interrupted(int result)
 	return result == -ERESTARTSYS || result == -EINTR;
 }
 
+/*
+ * Returns BLK_STS_RESOURCE if the caller should retry after a delay. Returns
+ * -EAGAIN if the caller should requeue @cmd. Returns -EIO if sending failed.
+ */
 static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
 {
 	struct request *req = blk_mq_rq_from_pdu(cmd);
@@ -670,7 +674,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
 				nsock->sent = sent;
 			}
 			set_bit(NBD_CMD_REQUEUED, &cmd->flags);
-			return BLK_STS_RESOURCE;
+			return (__force int)BLK_STS_RESOURCE;
 		}
 		dev_err_ratelimited(disk_to_dev(nbd->disk),
 			"Send control failed (result %d)\n", result);
@@ -711,7 +715,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
 					nsock->pending = req;
 					nsock->sent = sent;
 					set_bit(NBD_CMD_REQUEUED, &cmd->flags);
-					return BLK_STS_RESOURCE;
+					return (__force int)BLK_STS_RESOURCE;
 				}
 				dev_err(disk_to_dev(nbd->disk),
 					"Send data failed (result %d)\n",
@@ -1008,7 +1012,7 @@ static int wait_for_reconnect(struct nbd_device *nbd)
 	return !test_bit(NBD_RT_DISCONNECTED, &config->runtime_flags);
 }
 
-static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
+static blk_status_t nbd_handle_cmd(struct nbd_cmd *cmd, int index)
 {
 	struct request *req = blk_mq_rq_from_pdu(cmd);
 	struct nbd_device *nbd = cmd->nbd;
@@ -1022,14 +1026,14 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
 	if (!config) {
 		dev_err_ratelimited(disk_to_dev(nbd->disk),
 				    "Socks array is empty\n");
-		return -EINVAL;
+		return BLK_STS_IOERR;
 	}
 
 	if (index >= config->num_connections) {
 		dev_err_ratelimited(disk_to_dev(nbd->disk),
 				    "Attempted send on invalid socket\n");
 		nbd_config_put(nbd);
-		return -EINVAL;
+		return BLK_STS_IOERR;
 	}
 	cmd->status = BLK_STS_OK;
 again:
@@ -1052,7 +1056,7 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
 			 */
 			sock_shutdown(nbd);
 			nbd_config_put(nbd);
-			return -EIO;
+			return BLK_STS_IOERR;
 		}
 		goto again;
 	}
@@ -1065,7 +1069,7 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
 	blk_mq_start_request(req);
 	if (unlikely(nsock->pending && nsock->pending != req)) {
 		nbd_requeue_cmd(cmd);
-		ret = 0;
+		ret = BLK_STS_OK;
 		goto out;
 	}
 	/*
@@ -1084,19 +1088,19 @@ static int nbd_handle_cmd(struct nbd_cmd *cmd, int index)
 				    "Request send failed, requeueing\n");
 		nbd_mark_nsock_dead(nbd, nsock, 1);
 		nbd_requeue_cmd(cmd);
-		ret = 0;
+		ret = BLK_STS_OK;
 	}
 out:
 	mutex_unlock(&nsock->tx_lock);
 	nbd_config_put(nbd);
-	return ret;
+	return ret < 0 ? BLK_STS_IOERR : (__force blk_status_t)ret;
 }
 
 static blk_status_t nbd_queue_rq(struct blk_mq_hw_ctx *hctx,
 			const struct blk_mq_queue_data *bd)
 {
 	struct nbd_cmd *cmd = blk_mq_rq_to_pdu(bd->rq);
-	int ret;
+	blk_status_t ret;
 
 	/*
 	 * Since we look at the bio's to send the request over the network we
@@ -1116,10 +1120,6 @@ static blk_status_t nbd_queue_rq(struct blk_mq_hw_ctx *hctx,
 	 * appropriate.
 	 */
 	ret = nbd_handle_cmd(cmd, hctx->queue_num);
-	if (ret < 0)
-		ret = BLK_STS_IOERR;
-	else if (!ret)
-		ret = BLK_STS_OK;
 	mutex_unlock(&cmd->lock);
 
 	return ret;

Re: [PATCH 5/5] nbd: Fix signal handling

[Christoph Hellwig]

On Fri, May 10, 2024 at 01:23:13PM -0700, Bart Van Assche wrote:
> Both nbd_send_cmd() and nbd_handle_cmd() return either a negative error
> number or a positive blk_status_t value.

Eww.  Please split these into separate values instead.  There is a reason
why blk_status_t is a separate type with sparse checks, and drivers
really shouldn't do avoid with that for a tiny micro-optimization of
the calling convention (if this even is one and not just the driver
being sloppy).

Re: [PATCH 5/5] nbd: Fix signal handling

[Bart Van Assche]

On 5/20/24 05:41, Christoph Hellwig wrote:
> On Fri, May 10, 2024 at 01:23:13PM -0700, Bart Van Assche wrote:
>> Both nbd_send_cmd() and nbd_handle_cmd() return either a negative error
>> number or a positive blk_status_t value.
> 
> Eww.  Please split these into separate values instead.  There is a reason
> why blk_status_t is a separate type with sparse checks, and drivers
> really shouldn't do avoid with that for a tiny micro-optimization of
> the calling convention (if this even is one and not just the driver
> being sloppy).

How about the (untested) patch below?

Thanks,

Bart.

diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
index 22a79a62cc4e..4ee76c39e3a5 100644
--- a/drivers/block/nbd.c
+++ b/drivers/block/nbd.c
@@ -588,11 +588,17 @@ static inline int was_interrupted(int result)
  	return result == -ERESTARTSYS || result == -EINTR;
  }

+struct send_res {
+	int result;
+	blk_status_t status;
+};
+
  /*
   * Returns BLK_STS_RESOURCE if the caller should retry after a delay. Returns
   * -EAGAIN if the caller should requeue @cmd. Returns -EIO if sending failed.
   */
-static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
+static struct send_res nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd,
+				    int index)
  {
  	struct request *req = blk_mq_rq_from_pdu(cmd);
  	struct nbd_config *config = nbd->config;
@@ -614,13 +620,13 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)

  	type = req_to_nbd_cmd_type(req);
  	if (type == U32_MAX)
-		return -EIO;
+		return (struct send_res){ .result = -EIO };

  	if (rq_data_dir(req) == WRITE &&
  	    (config->flags & NBD_FLAG_READ_ONLY)) {
  		dev_err_ratelimited(disk_to_dev(nbd->disk),
  				    "Write on read-only\n");
-		return -EIO;
+		return (struct send_res){ .result = -EIO };
  	}

  	if (req->cmd_flags & REQ_FUA)
@@ -674,11 +680,11 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
  				nsock->sent = sent;
  			}
  			set_bit(NBD_CMD_REQUEUED, &cmd->flags);
-			return (__force int)BLK_STS_RESOURCE;
+			return (struct send_res){ .status = BLK_STS_RESOURCE };
  		}
  		dev_err_ratelimited(disk_to_dev(nbd->disk),
  			"Send control failed (result %d)\n", result);
-		return -EAGAIN;
+		return (struct send_res){ .result = -EAGAIN };
  	}
  send_pages:
  	if (type != NBD_CMD_WRITE)
@@ -715,12 +721,14 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
  					nsock->pending = req;
  					nsock->sent = sent;
  					set_bit(NBD_CMD_REQUEUED, &cmd->flags);
-					return (__force int)BLK_STS_RESOURCE;
+					return (struct send_res){
+						.status = BLK_STS_RESOURCE
+					};
  				}
  				dev_err(disk_to_dev(nbd->disk),
  					"Send data failed (result %d)\n",
  					result);
-				return -EAGAIN;
+				return (struct send_res){ .result = -EAGAIN };
  			}
  			/*
  			 * The completion might already have come in,
@@ -737,7 +745,7 @@ static int nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd, int index)
  	trace_nbd_payload_sent(req, handle);
  	nsock->pending = NULL;
  	nsock->sent = 0;
-	return 0;
+	return (struct send_res){};
  }

  static int nbd_read_reply(struct nbd_device *nbd, struct socket *sock,
@@ -1018,7 +1026,8 @@ static blk_status_t nbd_handle_cmd(struct nbd_cmd *cmd, int index)
  	struct nbd_device *nbd = cmd->nbd;
  	struct nbd_config *config;
  	struct nbd_sock *nsock;
-	int ret;
+	struct send_res send_res;
+	blk_status_t ret;

  	lockdep_assert_held(&cmd->lock);

@@ -1076,14 +1085,15 @@ static blk_status_t nbd_handle_cmd(struct nbd_cmd *cmd, int index)
  	 * Some failures are related to the link going down, so anything that
  	 * returns EAGAIN can be retried on a different socket.
  	 */
-	ret = nbd_send_cmd(nbd, cmd, index);
-	/*
-	 * Access to this flag is protected by cmd->lock, thus it's safe to set
-	 * the flag after nbd_send_cmd() succeed to send request to server.
-	 */
-	if (!ret)
+	send_res = nbd_send_cmd(nbd, cmd, index);
+	ret = send_res.result < 0 ? BLK_STS_IOERR : send_res.status;
+	if (ret == BLK_STS_OK) {
+		/*
+		 * cmd->lock is held. Hence, it's safe to set this flag after
+		 * nbd_send_cmd() succeeded sending the request to the server.
+		 */
  		__set_bit(NBD_CMD_INFLIGHT, &cmd->flags);
-	else if (ret == -EAGAIN) {
+	} else if (send_res.result == -EAGAIN) {
  		dev_err_ratelimited(disk_to_dev(nbd->disk),
  				    "Request send failed, requeueing\n");
  		nbd_mark_nsock_dead(nbd, nsock, 1);
@@ -1093,7 +1103,7 @@ static blk_status_t nbd_handle_cmd(struct nbd_cmd *cmd, int index)
  out:
  	mutex_unlock(&nsock->tx_lock);
  	nbd_config_put(nbd);
-	return ret < 0 ? BLK_STS_IOERR : (__force blk_status_t)ret;
+	return ret;
  }

  static blk_status_t nbd_queue_rq(struct blk_mq_hw_ctx *hctx,

Re: [PATCH 0/5] Five nbd patches

[Jens Axboe]

On Fri, 10 May 2024 13:23:08 -0700, Bart Van Assche wrote:
> These patches are what I came up with after having reviewed the nbd source code.
> Please consider these patches for the next merge window.
> 
> Thanks,
> 
> Bart.
> 
> [...]

Applied, thanks!

[1/5] nbd: Use NULL to represent a pointer
      commit: 08190cc4d8a62f2a07b4158751afd3a01638c4c5
[2/5] nbd: Remove superfluous casts
      commit: 40639e9a0f6e49edd4ef520e6c0e070e1a04a330
[3/5] nbd: Improve the documentation of the locking assumptions
      commit: 2a6751e052ab4789630bc889c814037068723bc1
[4/5] nbd: Remove a local variable from nbd_send_cmd()
      commit: f6cb9a2c3d2e893a8d493d34ed3e0400fe8afe28
[5/5] nbd: Fix signal handling
      commit: e56d4b633fffea9510db468085bed0799cba4ecd

Best regards,
-- 
Jens Axboe