| Message ID | 20220830123904.26671-2-guoqing.jiang@linux.dev (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Small changes for rnbd-srv | expand |
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
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 >
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
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!
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
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(-)