[PATCH 0/3] Small changes for rnbd-srv

[PATCH 0/3] Small changes for rnbd-srv

[Guoqing Jiang]

Hi,

Pls review the three patches.

Thanks,
Guoqing    

Guoqing Jiang (3):
  rnbd-srv: fix the return value of rnbd_srv_rdma_ev
  rnbd-srv: make process_msg_close returns void
  rnbd-srv: remove redundant setting of blk_open_flags

 drivers/block/rnbd/rnbd-srv-dev.c | 1 -
 drivers/block/rnbd/rnbd-srv.c     | 9 ++++-----
 2 files changed, 4 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/3] rnbd-srv: fix the return value of rnbd_srv_rdma_ev

[Guoqing Jiang]

Since process_msg_open could fail, we should return 'ret'
instead of '0' at the end of function.

Fixes: 2de6c8de192b ("block/rnbd: server: main functionality")
Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/block/rnbd/rnbd-srv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
index 3f6c268e04ef..9182d45cb9be 100644
--- a/drivers/block/rnbd/rnbd-srv.c
+++ b/drivers/block/rnbd/rnbd-srv.c
@@ -403,7 +403,7 @@ static int rnbd_srv_rdma_ev(void *priv,
 	}
 
 	rtrs_srv_resp_rdma(id, ret);
-	return 0;
+	return ret;
 }
 
 static struct rnbd_srv_sess_dev
-- 
2.34.1

Re: [PATCH 1/3] rnbd-srv: fix the return value of rnbd_srv_rdma_ev

[Chaitanya Kulkarni]

On 8/30/22 05:39, Guoqing Jiang wrote:
> Since process_msg_open could fail, we should return 'ret'
> instead of '0' at the end of function.
> 
> Fixes: 2de6c8de192b ("block/rnbd: server: main functionality")
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---

Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

-ck

Re: [PATCH 1/3] rnbd-srv: fix the return value of rnbd_srv_rdma_ev

[Haris Iqbal]

On Tue, Aug 30, 2022 at 2:39 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
> Since process_msg_open could fail, we should return 'ret'
> instead of '0' at the end of function.
>
> Fixes: 2de6c8de192b ("block/rnbd: server: main functionality")
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---
>  drivers/block/rnbd/rnbd-srv.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
> index 3f6c268e04ef..9182d45cb9be 100644
> --- a/drivers/block/rnbd/rnbd-srv.c
> +++ b/drivers/block/rnbd/rnbd-srv.c
> @@ -403,7 +403,7 @@ static int rnbd_srv_rdma_ev(void *priv,
>         }
>
>         rtrs_srv_resp_rdma(id, ret);
> -       return 0;
> +       return ret;

I think the point here was to process the failure through
rtrs_srv_resp_rdma() function. If you notice how the return of rdma_ev
is processed by RTRS, in case of a failure return; it tries to send a
response back through send_io_resp_imm(). Same would happen in the
function rtrs_srv_resp_rdma().

If we call rtrs_srv_resp_rdma() with the error, and return the err
back to the caller of rdma_ev, we may end up sending err response more
than once.

>  }
>
>  static struct rnbd_srv_sess_dev
> --
> 2.34.1
>

Re: [PATCH 1/3] rnbd-srv: fix the return value of rnbd_srv_rdma_ev

[Guoqing Jiang]

On 8/30/22 9:28 PM, Haris Iqbal wrote:
> On Tue, Aug 30, 2022 at 2:39 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>> Since process_msg_open could fail, we should return 'ret'
>> instead of '0' at the end of function.
>>
>> Fixes: 2de6c8de192b ("block/rnbd: server: main functionality")
>> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
>> ---
>>   drivers/block/rnbd/rnbd-srv.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
>> index 3f6c268e04ef..9182d45cb9be 100644
>> --- a/drivers/block/rnbd/rnbd-srv.c
>> +++ b/drivers/block/rnbd/rnbd-srv.c
>> @@ -403,7 +403,7 @@ static int rnbd_srv_rdma_ev(void *priv,
>>          }
>>
>>          rtrs_srv_resp_rdma(id, ret);
>> -       return 0;
>> +       return ret;
> I think the point here was to process the failure through
> rtrs_srv_resp_rdma() function. If you notice how the return of rdma_ev
> is processed by RTRS, in case of a failure return; it tries to send a
> response back through send_io_resp_imm(). Same would happen in the
> function rtrs_srv_resp_rdma().
>
> If we call rtrs_srv_resp_rdma() with the error, and return the err
> back to the caller of rdma_ev, we may end up sending err response more
> than once.

Thanks for the explanation, I am wondering if it makes sense to call
rtrs_srv_resp_rdma when ret == 0, or let's just add a comment here.

Thanks,
Guoqing

Re: [PATCH 1/3] rnbd-srv: fix the return value of rnbd_srv_rdma_ev

[Jinpu Wang]

On Tue, Aug 30, 2022 at 5:10 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
>
>
>
> On 8/30/22 9:28 PM, Haris Iqbal wrote:
> > On Tue, Aug 30, 2022 at 2:39 PM Guoqing Jiang <guoqing.jiang@linux.dev> wrote:
> >> Since process_msg_open could fail, we should return 'ret'
> >> instead of '0' at the end of function.
> >>
> >> Fixes: 2de6c8de192b ("block/rnbd: server: main functionality")
> >> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> >> ---
> >>   drivers/block/rnbd/rnbd-srv.c | 2 +-
> >>   1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
> >> index 3f6c268e04ef..9182d45cb9be 100644
> >> --- a/drivers/block/rnbd/rnbd-srv.c
> >> +++ b/drivers/block/rnbd/rnbd-srv.c
> >> @@ -403,7 +403,7 @@ static int rnbd_srv_rdma_ev(void *priv,
> >>          }
> >>
> >>          rtrs_srv_resp_rdma(id, ret);
> >> -       return 0;
> >> +       return ret;
> > I think the point here was to process the failure through
> > rtrs_srv_resp_rdma() function. If you notice how the return of rdma_ev
> > is processed by RTRS, in case of a failure return; it tries to send a
> > response back through send_io_resp_imm(). Same would happen in the
> > function rtrs_srv_resp_rdma().
> >
> > If we call rtrs_srv_resp_rdma() with the error, and return the err
> > back to the caller of rdma_ev, we may end up sending err response more
> > than once.
>
> Thanks for the explanation, I am wondering if it makes sense to call
> rtrs_srv_resp_rdma when ret == 0,
As haris mentioned above, rtrs_srv_resp_rdma will send back the
confirmation to the client side.
ret==0 means the operation is finished successfully, negative value means error.

> let's just add a comment here.
would be good to add a comment.
>
> Thanks,
> Guoqing
Thx!

[PATCH 2/3] rnbd-srv: make process_msg_close returns void

[Guoqing Jiang]

Change the return type to void given it always returns 0.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
process_msg_sess_info also returns 0, to avoid conflict with rdma tree,
will send it in next merge window.

 drivers/block/rnbd/rnbd-srv.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/block/rnbd/rnbd-srv.c b/drivers/block/rnbd/rnbd-srv.c
index 9182d45cb9be..026681a70c46 100644
--- a/drivers/block/rnbd/rnbd-srv.c
+++ b/drivers/block/rnbd/rnbd-srv.c
@@ -339,7 +339,7 @@ void rnbd_srv_sess_dev_force_close(struct rnbd_srv_sess_dev *sess_dev,
 	mutex_unlock(&sess->lock);
 }
 
-static int process_msg_close(struct rnbd_srv_session *srv_sess,
+static void process_msg_close(struct rnbd_srv_session *srv_sess,
 			     void *data, size_t datalen, const void *usr,
 			     size_t usrlen)
 {
@@ -351,13 +351,12 @@ static int process_msg_close(struct rnbd_srv_session *srv_sess,
 	sess_dev = rnbd_get_sess_dev(le32_to_cpu(close_msg->device_id),
 				      srv_sess);
 	if (IS_ERR(sess_dev))
-		return 0;
+		return;
 
 	rnbd_put_sess_dev(sess_dev);
 	mutex_lock(&srv_sess->lock);
 	rnbd_srv_destroy_dev_session_sysfs(sess_dev);
 	mutex_unlock(&srv_sess->lock);
-	return 0;
 }
 
 static int process_msg_open(struct rnbd_srv_session *srv_sess,
@@ -387,7 +386,7 @@ static int rnbd_srv_rdma_ev(void *priv,
 	case RNBD_MSG_IO:
 		return process_rdma(srv_sess, id, data, datalen, usr, usrlen);
 	case RNBD_MSG_CLOSE:
-		ret = process_msg_close(srv_sess, data, datalen, usr, usrlen);
+		process_msg_close(srv_sess, data, datalen, usr, usrlen);
 		break;
 	case RNBD_MSG_OPEN:
 		ret = process_msg_open(srv_sess, usr, usrlen, data, datalen);
-- 
2.34.1

Re: [PATCH 2/3] rnbd-srv: make process_msg_close returns void

[Chaitanya Kulkarni]

On 8/30/22 05:39, Guoqing Jiang wrote:
> Change the return type to void given it always returns 0.
> 
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---


Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

Re: [PATCH 2/3] rnbd-srv: make process_msg_close returns void

[Haris Iqbal]

On Tue, Aug 30, 2022 at 3:04 PM Chaitanya Kulkarni
<chaitanyak@nvidia.com> wrote:
>
> On 8/30/22 05:39, Guoqing Jiang wrote:
> > Change the return type to void given it always returns 0.
> >
> > Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> > ---
>
>
> Looks good.
>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

This looks good,

Acked-by: Md Haris Iqbal <haris.iqbal@ionos.com>

>
>

[PATCH 3/3] rnbd-srv: remove redundant setting of blk_open_flags

[Guoqing Jiang]

It is not necessary since it is set later just before function
return success.

Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
---
 drivers/block/rnbd/rnbd-srv-dev.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/block/rnbd/rnbd-srv-dev.c b/drivers/block/rnbd/rnbd-srv-dev.c
index c63017f6e421..38131fa5718d 100644
--- a/drivers/block/rnbd/rnbd-srv-dev.c
+++ b/drivers/block/rnbd/rnbd-srv-dev.c
@@ -21,7 +21,6 @@ struct rnbd_dev *rnbd_dev_open(const char *path, fmode_t flags)
 	if (!dev)
 		return ERR_PTR(-ENOMEM);
 
-	dev->blk_open_flags = flags;
 	dev->bdev = blkdev_get_by_path(path, flags, THIS_MODULE);
 	ret = PTR_ERR_OR_ZERO(dev->bdev);
 	if (ret)
-- 
2.34.1

Re: [PATCH 3/3] rnbd-srv: remove redundant setting of blk_open_flags

[Chaitanya Kulkarni]

On 8/30/22 05:39, Guoqing Jiang wrote:
> It is not necessary since it is set later just before function
> return success.
> 
> Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> ---


Looks good.

Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

Re: [PATCH 3/3] rnbd-srv: remove redundant setting of blk_open_flags

[Haris Iqbal]

On Tue, Aug 30, 2022 at 3:02 PM Chaitanya Kulkarni
<chaitanyak@nvidia.com> wrote:
>
> On 8/30/22 05:39, Guoqing Jiang wrote:
> > It is not necessary since it is set later just before function
> > return success.
> >
> > Signed-off-by: Guoqing Jiang <guoqing.jiang@linux.dev>
> > ---
>
>
> Looks good.
>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>

This looks good,

Acked-by: Md Haris Iqbal <haris.iqbal@ionos.com>

>
>