On Thu, Oct 17, 2024 at 05:47:53PM +0200, Kevin Wolf wrote:
> Am 17.10.2024 um 13:36 hat Ming Lei geschrieben:
> > nbd driver sends request header and payload with multiple call of
> > sock_sendmsg, and partial sending can't be avoided. However, nbd driver
> > returns BLK_STS_RESOURCE to block core in this situation. This way causes
> > one issue: request->tag may change in the next run of nbd_queue_rq(), but
> > the original old tag has been sent as part of header cookie, this way
> > confuses nbd driver reply handling, since the real request can't be
> > retrieved any more with the obsolete old tag.
> >
> > Fix it by retrying sending directly, this way is reasonable & safe since
> > nothing can move on if the current hw queue(socket) has pending request,
> > and unnecessary requeue can be avoided in this way.
> >
> > Cc: vincent.chen@xxxxxxxxxx
> > Cc: Leon Schuermann <leon@is.currently.online>
> > Cc: Bart Van Assche <bvanassche@xxxxxxx>
> > Reported-by: Kevin Wolf <kwolf@xxxxxxxxxx>
> > Signed-off-by: Ming Lei <ming.lei@xxxxxxxxxx>
> > ---
> > Kevin,
> > Please test this version, thanks!
>
> The NBD errors seem to go away with this.
>
> I'm not sure about side effects, though. Isn't the idea behind EINTR
> that you return to userspace to let it handle a signal? Looping in the
Well, the retry can be done in one work function, then userspace can get
chance to handle signal.
> kernel doesn't quite achieve this, so do we delay/prevent signal
> delivery with this? On the other hand, if it were completely prevented,
> then this should become an infinite loop, which it didn't in my test.
If retry can't succeed in the request's deadline, it will fail.
>
> > drivers/block/nbd.c | 35 +++++++++++++++++++++++++++++++++--
> > 1 file changed, 33 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/block/nbd.c b/drivers/block/nbd.c
> > index b852050d8a96..ef84071041e3 100644
> > --- a/drivers/block/nbd.c
> > +++ b/drivers/block/nbd.c
> > @@ -701,8 +701,9 @@ static blk_status_t nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd,
> > if (sent) {
> > nsock->pending = req;
> > nsock->sent = sent;
> > + } else {
> > + set_bit(NBD_CMD_REQUEUED, &cmd->flags);
> > }
> > - set_bit(NBD_CMD_REQUEUED, &cmd->flags);
> > return BLK_STS_RESOURCE;
> > }
> > dev_err_ratelimited(disk_to_dev(nbd->disk),
> > @@ -743,7 +744,6 @@ static blk_status_t nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd,
> > */
> > nsock->pending = req;
> > nsock->sent = sent;
> > - set_bit(NBD_CMD_REQUEUED, &cmd->flags);
> > return BLK_STS_RESOURCE;
> > }
> > dev_err(disk_to_dev(nbd->disk),
> > @@ -778,6 +778,35 @@ static blk_status_t nbd_send_cmd(struct nbd_device *nbd, struct nbd_cmd *cmd,
> > return BLK_STS_OK;
> > }
> >
> > +/*
> > + * Send pending nbd request and payload, part of them have been sent
> > + * already, so we have to send them all with current request, and can't
> > + * return BLK_STS_RESOURCE, otherwise request tag may be changed in next
> > + * retry
> > + */
> > +static blk_status_t nbd_send_pending_cmd(struct nbd_device *nbd,
> > + struct nbd_cmd *cmd)
> > +{
> > + struct request *req = blk_mq_rq_from_pdu(cmd);
> > + unsigned long deadline = READ_ONCE(req->deadline);
> > + unsigned int wait_ms = 2;
> > + blk_status_t res;
> > +
> > + WARN_ON_ONCE(test_bit(NBD_CMD_REQUEUED, &cmd->flags));
> > +
> > + while (true) {
> > + res = nbd_send_cmd(nbd, cmd, cmd->index);
> > + if (res != BLK_STS_RESOURCE)
> > + return res;
> > + if (READ_ONCE(jiffies) + msecs_to_jiffies(wait_ms) >= deadline)
> > + break;
> > + msleep(wait_ms);
> > + wait_ms *= 2;
> > + }
> > +
> > + return BLK_STS_IOERR;
> > +}
> > +
> > static int nbd_read_reply(struct nbd_device *nbd, struct socket *sock,
> > struct nbd_reply *reply)
> > {
> > @@ -1111,6 +1140,8 @@ static blk_status_t nbd_handle_cmd(struct nbd_cmd *cmd, int index)
> > goto out;
> > }
> > ret = nbd_send_cmd(nbd, cmd, index);
> > + if (ret == BLK_STS_RESOURCE && nsock->pending == req)
> > + ret = nbd_send_pending_cmd(nbd, cmd);
>
> Is there a reason to call nbd_send_cmd() outside of the new loop first
> instead of going to the loop directly? It's always better to only have
> a single code path.
IMO, it is better to add new cold code path for handling the unusual
pending request, and nbd_send_cmd() has been too complicated to maintain.
Thanks,
Ming
