[PATCH 1/2] ksmbd: remove duplicate flag set in smb2_write

[PATCH 1/2] ksmbd: remove duplicate flag set in smb2_write

[Hyunchul Lee]

The writethrough flag is set again if
is_rdma_channel is false.

Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
---
 fs/ksmbd/smb2pdu.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
index e6f4ccc12f49..553aad4ca6fa 100644
--- a/fs/ksmbd/smb2pdu.c
+++ b/fs/ksmbd/smb2pdu.c
@@ -6506,9 +6506,6 @@ int smb2_write(struct ksmbd_work *work)
 				    le16_to_cpu(req->DataOffset));
 
 		ksmbd_debug(SMB, "flags %u\n", le32_to_cpu(req->Flags));
-		if (le32_to_cpu(req->Flags) & SMB2_WRITEFLAG_WRITE_THROUGH)
-			writethrough = true;
-
 		ksmbd_debug(SMB, "filename %pd, offset %lld, len %zu\n",
 			    fp->filp->f_path.dentry, offset, length);
 		err = ksmbd_vfs_write(work, fp, data_buf, length, &offset,
-- 
2.25.1

[PATCH 2/2] ksmbd: smbd: handle RDMA CM time wait event

[Hyunchul Lee]

After a QP has been disconnected, it stays
in a timewait state for in flight packets.
After the state has completed,
RDMA_CM_EVENT_TIMEWAIT_EXIT is reported.
Disconnect on RDMA_CM_EVENT_TIMEWAIT_EXIT
so that ksmbd can restart.

Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
---
 fs/ksmbd/transport_rdma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/ksmbd/transport_rdma.c b/fs/ksmbd/transport_rdma.c
index d035e060c2f0..4b1a471afcd0 100644
--- a/fs/ksmbd/transport_rdma.c
+++ b/fs/ksmbd/transport_rdma.c
@@ -1535,6 +1535,7 @@ static int smb_direct_cm_handler(struct rdma_cm_id *cm_id,
 		wake_up_interruptible(&t->wait_status);
 		break;
 	}
+	case RDMA_CM_EVENT_TIMEWAIT_EXIT:
 	case RDMA_CM_EVENT_DEVICE_REMOVAL:
 	case RDMA_CM_EVENT_DISCONNECTED: {
 		t->status = SMB_DIRECT_CS_DISCONNECTED;
-- 
2.25.1

Re: [PATCH 2/2] ksmbd: smbd: handle RDMA CM time wait event

[Namjae Jeon]

2022-06-14 8:01 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> After a QP has been disconnected, it stays
> in a timewait state for in flight packets.
> After the state has completed,
> RDMA_CM_EVENT_TIMEWAIT_EXIT is reported.
> Disconnect on RDMA_CM_EVENT_TIMEWAIT_EXIT
> so that ksmbd can restart.
>
> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
Acked-by: Namjae Jeon <linkinjeon@kernel.org>

Thanks!

Re: [PATCH 2/2] ksmbd: smbd: handle RDMA CM time wait event

[Tom Talpey]

On 6/13/2022 7:01 PM, Hyunchul Lee wrote:
> After a QP has been disconnected, it stays
> in a timewait state for in flight packets.
> After the state has completed,
> RDMA_CM_EVENT_TIMEWAIT_EXIT is reported.
> Disconnect on RDMA_CM_EVENT_TIMEWAIT_EXIT
> so that ksmbd can restart.
> 
> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> ---
>   fs/ksmbd/transport_rdma.c | 1 +
>   1 file changed, 1 insertion(+)
> 
> diff --git a/fs/ksmbd/transport_rdma.c b/fs/ksmbd/transport_rdma.c
> index d035e060c2f0..4b1a471afcd0 100644
> --- a/fs/ksmbd/transport_rdma.c
> +++ b/fs/ksmbd/transport_rdma.c
> @@ -1535,6 +1535,7 @@ static int smb_direct_cm_handler(struct rdma_cm_id *cm_id,
>   		wake_up_interruptible(&t->wait_status);
>   		break;
>   	}
> +	case RDMA_CM_EVENT_TIMEWAIT_EXIT:
>   	case RDMA_CM_EVENT_DEVICE_REMOVAL:
>   	case RDMA_CM_EVENT_DISCONNECTED: {
>   		t->status = SMB_DIRECT_CS_DISCONNECTED;

Is this issue seen on all RDMA providers? Because I would normally
expect that an RDMA_CM_EVENT_DISCONNECTED will precede the TIMEWAIT
event. What scenarios have you seen this not occur?

Unless ksmbd wishes to reuse its QP's, which is not currently the
case (right?), there's pretty much no reason to manage QP state and
hang around for TIMEWAIT.

Tom.

Re: [PATCH 2/2] ksmbd: smbd: handle RDMA CM time wait event

[Hyunchul Lee]

2022년 6월 14일 (화) 오후 8:56, Tom Talpey <tom@talpey.com>님이 작성:
>
>
> On 6/13/2022 7:01 PM, Hyunchul Lee wrote:
> > After a QP has been disconnected, it stays
> > in a timewait state for in flight packets.
> > After the state has completed,
> > RDMA_CM_EVENT_TIMEWAIT_EXIT is reported.
> > Disconnect on RDMA_CM_EVENT_TIMEWAIT_EXIT
> > so that ksmbd can restart.
> >
> > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> > ---
> >   fs/ksmbd/transport_rdma.c | 1 +
> >   1 file changed, 1 insertion(+)
> >
> > diff --git a/fs/ksmbd/transport_rdma.c b/fs/ksmbd/transport_rdma.c
> > index d035e060c2f0..4b1a471afcd0 100644
> > --- a/fs/ksmbd/transport_rdma.c
> > +++ b/fs/ksmbd/transport_rdma.c
> > @@ -1535,6 +1535,7 @@ static int smb_direct_cm_handler(struct rdma_cm_id *cm_id,
> >               wake_up_interruptible(&t->wait_status);
> >               break;
> >       }
> > +     case RDMA_CM_EVENT_TIMEWAIT_EXIT:
> >       case RDMA_CM_EVENT_DEVICE_REMOVAL:
> >       case RDMA_CM_EVENT_DISCONNECTED: {
> >               t->status = SMB_DIRECT_CS_DISCONNECTED;
>
> Is this issue seen on all RDMA providers? Because I would normally
> expect that an RDMA_CM_EVENT_DISCONNECTED will precede the TIMEWAIT
> event. What scenarios have you seen this not occur?
>

There was an issue that ksmbd got stuck after attempting to shutdown.
We are trying to reproduce it, but we haven't reproduced it yet,
but It seems to be related to the TIMEWAIT event.

And other drivers such as nvme have disconnected on the TIMEWAIT event.

> Unless ksmbd wishes to reuse its QP's, which is not currently the
> case (right?), there's pretty much no reason to manage QP state and
> hang around for TIMEWAIT.

Right, ksmbd doesn't reuse QP.

>
> Tom.


-- 
Thanks,
Hyunchul

Re: [PATCH 2/2] ksmbd: smbd: handle RDMA CM time wait event

[Tom Talpey]

On 6/14/2022 10:14 PM, Hyunchul Lee wrote:
> 2022년 6월 14일 (화) 오후 8:56, Tom Talpey <tom@talpey.com>님이 작성:
>>
>>
>> On 6/13/2022 7:01 PM, Hyunchul Lee wrote:
>>> After a QP has been disconnected, it stays
>>> in a timewait state for in flight packets.
>>> After the state has completed,
>>> RDMA_CM_EVENT_TIMEWAIT_EXIT is reported.
>>> Disconnect on RDMA_CM_EVENT_TIMEWAIT_EXIT
>>> so that ksmbd can restart.
>>>
>>> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
>>> ---
>>>    fs/ksmbd/transport_rdma.c | 1 +
>>>    1 file changed, 1 insertion(+)
>>>
>>> diff --git a/fs/ksmbd/transport_rdma.c b/fs/ksmbd/transport_rdma.c
>>> index d035e060c2f0..4b1a471afcd0 100644
>>> --- a/fs/ksmbd/transport_rdma.c
>>> +++ b/fs/ksmbd/transport_rdma.c
>>> @@ -1535,6 +1535,7 @@ static int smb_direct_cm_handler(struct rdma_cm_id *cm_id,
>>>                wake_up_interruptible(&t->wait_status);
>>>                break;
>>>        }
>>> +     case RDMA_CM_EVENT_TIMEWAIT_EXIT:
>>>        case RDMA_CM_EVENT_DEVICE_REMOVAL:
>>>        case RDMA_CM_EVENT_DISCONNECTED: {
>>>                t->status = SMB_DIRECT_CS_DISCONNECTED;
>>
>> Is this issue seen on all RDMA providers? Because I would normally
>> expect that an RDMA_CM_EVENT_DISCONNECTED will precede the TIMEWAIT
>> event. What scenarios have you seen this not occur?
>>
> 
> There was an issue that ksmbd got stuck after attempting to shutdown.
> We are trying to reproduce it, but we haven't reproduced it yet,
> but It seems to be related to the TIMEWAIT event.

I don't think it's appropriate to add this case to SMB. I think it's
quite unlikely that it will address anything, because an RDMA provider
must have indicated a CM_EVENT_DISCONNECTED prior to any TIMEWAIT.
So, the QP (and connection) will already have been torn down by ksmbd
at the earlier event. Perhaps ksmbd did not properly drain the QP at
the initial disconnect.

 > And other drivers such as nvme have disconnected on the TIMEWAIT event.

NVME is a completely different upper layer, and has different client/
server transport behavior. The SMB session insulates its peers from
most transport errors, and should not be requesting timewait for
its connections, and definitely not waiting for timewait to expire
before initiating teardown (or recovery). The NFS/RDMA client and
server ignore this event, btw.

>> Unless ksmbd wishes to reuse its QP's, which is not currently the
>> case (right?), there's pretty much no reason to manage QP state and
>> hang around for TIMEWAIT.
> 
> Right, ksmbd doesn't reuse QP.

Then there appears to be no good justification for the change. Sorry,
but it's a NAK from me.

Tom.

Re: [PATCH 2/2] ksmbd: smbd: handle RDMA CM time wait event

[Hyunchul Lee]

2022년 6월 16일 (목) 오전 3:53, Tom Talpey <tom@talpey.com>님이 작성:
>
>
> On 6/14/2022 10:14 PM, Hyunchul Lee wrote:
> > 2022년 6월 14일 (화) 오후 8:56, Tom Talpey <tom@talpey.com>님이 작성:
> >>
> >>
> >> On 6/13/2022 7:01 PM, Hyunchul Lee wrote:
> >>> After a QP has been disconnected, it stays
> >>> in a timewait state for in flight packets.
> >>> After the state has completed,
> >>> RDMA_CM_EVENT_TIMEWAIT_EXIT is reported.
> >>> Disconnect on RDMA_CM_EVENT_TIMEWAIT_EXIT
> >>> so that ksmbd can restart.
> >>>
> >>> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> >>> ---
> >>>    fs/ksmbd/transport_rdma.c | 1 +
> >>>    1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/fs/ksmbd/transport_rdma.c b/fs/ksmbd/transport_rdma.c
> >>> index d035e060c2f0..4b1a471afcd0 100644
> >>> --- a/fs/ksmbd/transport_rdma.c
> >>> +++ b/fs/ksmbd/transport_rdma.c
> >>> @@ -1535,6 +1535,7 @@ static int smb_direct_cm_handler(struct rdma_cm_id *cm_id,
> >>>                wake_up_interruptible(&t->wait_status);
> >>>                break;
> >>>        }
> >>> +     case RDMA_CM_EVENT_TIMEWAIT_EXIT:
> >>>        case RDMA_CM_EVENT_DEVICE_REMOVAL:
> >>>        case RDMA_CM_EVENT_DISCONNECTED: {
> >>>                t->status = SMB_DIRECT_CS_DISCONNECTED;
> >>
> >> Is this issue seen on all RDMA providers? Because I would normally
> >> expect that an RDMA_CM_EVENT_DISCONNECTED will precede the TIMEWAIT
> >> event. What scenarios have you seen this not occur?
> >>
> >
> > There was an issue that ksmbd got stuck after attempting to shutdown.
> > We are trying to reproduce it, but we haven't reproduced it yet,
> > but It seems to be related to the TIMEWAIT event.
>
> I don't think it's appropriate to add this case to SMB. I think it's
> quite unlikely that it will address anything, because an RDMA provider
> must have indicated a CM_EVENT_DISCONNECTED prior to any TIMEWAIT.
> So, the QP (and connection) will already have been torn down by ksmbd
> at the earlier event. Perhaps ksmbd did not properly drain the QP at
> the initial disconnect.
>
>  > And other drivers such as nvme have disconnected on the TIMEWAIT event.
>
> NVME is a completely different upper layer, and has different client/
> server transport behavior. The SMB session insulates its peers from
> most transport errors, and should not be requesting timewait for
> its connections, and definitely not waiting for timewait to expire
> before initiating teardown (or recovery). The NFS/RDMA client and
> server ignore this event, btw.
>

Okay, I got it.
I am looking for the cause and have found some clues.

> >> Unless ksmbd wishes to reuse its QP's, which is not currently the
> >> case (right?), there's pretty much no reason to manage QP state and
> >> hang around for TIMEWAIT.
> >
> > Right, ksmbd doesn't reuse QP.
>
> Then there appears to be no good justification for the change. Sorry,
> but it's a NAK from me.
>

Really thank you for the detailed explanation.

> Tom.



-- 
Thanks,
Hyunchul

Re: [PATCH 1/2] ksmbd: remove duplicate flag set in smb2_write

[Namjae Jeon]

2022-06-14 8:01 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> The writethrough flag is set again if
> is_rdma_channel is false.
>
> Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> ---
>  fs/ksmbd/smb2pdu.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> index e6f4ccc12f49..553aad4ca6fa 100644
> --- a/fs/ksmbd/smb2pdu.c
> +++ b/fs/ksmbd/smb2pdu.c
> @@ -6506,9 +6506,6 @@ int smb2_write(struct ksmbd_work *work)
>  				    le16_to_cpu(req->DataOffset));
>
>  		ksmbd_debug(SMB, "flags %u\n", le32_to_cpu(req->Flags));
Could you move debug print to flags check above also ?

> -		if (le32_to_cpu(req->Flags) & SMB2_WRITEFLAG_WRITE_THROUGH)
> -			writethrough = true;
> -
>  		ksmbd_debug(SMB, "filename %pd, offset %lld, len %zu\n",
>  			    fp->filp->f_path.dentry, offset, length);
>  		err = ksmbd_vfs_write(work, fp, data_buf, length, &offset,
> --
> 2.25.1
>
>

Re: [PATCH 1/2] ksmbd: remove duplicate flag set in smb2_write

[Hyunchul Lee]

2022년 6월 14일 (화) 오전 8:08, Namjae Jeon <linkinjeon@kernel.org>님이 작성:
>
> 2022-06-14 8:01 GMT+09:00, Hyunchul Lee <hyc.lee@gmail.com>:
> > The writethrough flag is set again if
> > is_rdma_channel is false.
> >
> > Signed-off-by: Hyunchul Lee <hyc.lee@gmail.com>
> > ---
> >  fs/ksmbd/smb2pdu.c | 3 ---
> >  1 file changed, 3 deletions(-)
> >
> > diff --git a/fs/ksmbd/smb2pdu.c b/fs/ksmbd/smb2pdu.c
> > index e6f4ccc12f49..553aad4ca6fa 100644
> > --- a/fs/ksmbd/smb2pdu.c
> > +++ b/fs/ksmbd/smb2pdu.c
> > @@ -6506,9 +6506,6 @@ int smb2_write(struct ksmbd_work *work)
> >                                   le16_to_cpu(req->DataOffset));
> >
> >               ksmbd_debug(SMB, "flags %u\n", le32_to_cpu(req->Flags));
> Could you move debug print to flags check above also ?
>

Okay, I will.

> > -             if (le32_to_cpu(req->Flags) & SMB2_WRITEFLAG_WRITE_THROUGH)
> > -                     writethrough = true;
> > -
> >               ksmbd_debug(SMB, "filename %pd, offset %lld, len %zu\n",
> >                           fp->filp->f_path.dentry, offset, length);
> >               err = ksmbd_vfs_write(work, fp, data_buf, length, &offset,
> > --
> > 2.25.1
> >
> >



-- 
Thanks,
Hyunchul