[[PATCH,v1] 25/37] [CIFS] SMBD: Support SMBD idle connection timer

Commit Message

Long Li Aug. 2, 2017, 8:10 p.m. UTC

From: Long Li <[e-mail]>

This is part of the keep alive protocol. Each peer maintains a timer, and will send a message to peer when it expires.

Signed-off-by: Long Li <[e-mail]>
---
 fs/cifs/cifsrdma.c | 25 +++++++++++++++++++++++++
 fs/cifs/cifsrdma.h |  6 ++++--
 2 files changed, 29 insertions(+), 2 deletions(-)

Comments

Tom Talpey Aug. 14, 2017, 9:12 p.m. UTC | #1

> -----Original Message-----
> From: [e-mail] [mailto:linux-cifs-
> [e-mail]] On Behalf Of Long Li
> Sent: Wednesday, August 2, 2017 4:11 PM
> To: Steve French <[e-mail]>; [e-mail]; samba-
> [e-mail]; [e-mail]
> Cc: Long Li <[e-mail]>
> Subject: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection timer
> 
> +static int keep_alive_interval = 120;

This is the recommended value, but not the only possibility.

> @@ -1348,6 +1369,10 @@ struct cifs_rdma_info* cifs_create_rdma_session(
>         init_waitqueue_head(&info->wait_send_queue);
>         init_waitqueue_head(&info->wait_reassembly_queue);
> 
> +       INIT_DELAYED_WORK(&info->idle_timer_work, idle_connection_timer);
> +       schedule_delayed_work(&info->idle_timer_work,
> +               info->keep_alive_interval*HZ);
> +

This initialization is ok, but the timer should be rescheduled (extended) any time
any packet is sent. There is no need to perform keepalives on an active SMB Direct
connection.

Tom.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Long Li Aug. 14, 2017, 11:29 p.m. UTC | #2

> -----Original Message-----
> From: Tom Talpey
> Sent: Monday, August 14, 2017 2:12 PM
> To: Long Li <[e-mail]>; Steve French <[e-mail]>;
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Cc: Long Li <[e-mail]>
> Subject: RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection
> timer
> 
> > -----Original Message-----
> > From: [e-mail] [mailto:linux-cifs-
> > [e-mail]] On Behalf Of Long Li
> > Sent: Wednesday, August 2, 2017 4:11 PM
> > To: Steve French <[e-mail]>; [e-mail];
> > samba- [e-mail]; [e-mail]
> > Cc: Long Li <[e-mail]>
> > Subject: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection
> > timer
> >
> > +static int keep_alive_interval = 120;
> 
> This is the recommended value, but not the only possibility.
> 
> > @@ -1348,6 +1369,10 @@ struct cifs_rdma_info*
> cifs_create_rdma_session(
> >         init_waitqueue_head(&info->wait_send_queue);
> >         init_waitqueue_head(&info->wait_reassembly_queue);
> >
> > +       INIT_DELAYED_WORK(&info->idle_timer_work,
> idle_connection_timer);
> > +       schedule_delayed_work(&info->idle_timer_work,
> > +               info->keep_alive_interval*HZ);
> > +
> 
> This initialization is ok, but the timer should be rescheduled (extended) any
> time any packet is sent. There is no need to perform keepalives on an active
> SMB Direct connection.

My feeling is that rescheduling on a work queue for every packet is sent is not efficient, especially under heavy conditions.

Firing it every 120 seconds doesn't seem to be big waste and may actually save some CPU.

> 
> Tom.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Tom Talpey Aug. 14, 2017, 11:42 p.m. UTC | #3

> -----Original Message-----
> From: [e-mail] [mailto:linux-cifs-
> [e-mail]] On Behalf Of Long Li
> Sent: Monday, August 14, 2017 7:30 PM
> To: Tom Talpey <[e-mail]>; Steve French <[e-mail]>;
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection
> timer
> 
> [This sender failed our fraud detection checks and may not be who they appear
> to be. Learn about spoofing at http://aka.ms/LearnAboutSpoofing]
> 
> > -----Original Message-----
> > From: Tom Talpey
> > Sent: Monday, August 14, 2017 2:12 PM
> > To: Long Li <[e-mail]>; Steve French <[e-mail]>;
> > [e-mail]; [e-mail]; linux-
> > [e-mail]
> > Cc: Long Li <[e-mail]>
> > Subject: RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection
> > timer
> >
> > > -----Original Message-----
> > > From: [e-mail] [mailto:linux-cifs-
> > > [e-mail]] On Behalf Of Long Li
> > > Sent: Wednesday, August 2, 2017 4:11 PM
> > > To: Steve French <[e-mail]>; [e-mail];
> > > samba- [e-mail]; [e-mail]
> > > Cc: Long Li <[e-mail]>
> > > Subject: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection
> > > timer
> > >
> > > +static int keep_alive_interval = 120;
> >
> > This is the recommended value, but not the only possibility.
> >
> > > @@ -1348,6 +1369,10 @@ struct cifs_rdma_info*
> > cifs_create_rdma_session(
> > >         init_waitqueue_head(&info->wait_send_queue);
> > >         init_waitqueue_head(&info->wait_reassembly_queue);
> > >
> > > +       INIT_DELAYED_WORK(&info->idle_timer_work,
> > idle_connection_timer);
> > > +       schedule_delayed_work(&info->idle_timer_work,
> > > +               info->keep_alive_interval*HZ);
> > > +
> >
> > This initialization is ok, but the timer should be rescheduled (extended) any
> > time any packet is sent. There is no need to perform keepalives on an active
> > SMB Direct connection.
> 
> My feeling is that rescheduling on a work queue for every packet is sent is not
> efficient, especially under heavy conditions.

That's not what I was suggesting. Cant the timer simply be re-extended to the
120-second interval? I.e. on an active connection, it will never fire because it's
always advancing.

As defined here, it will go off and send a keepalive every 120 seconds. The
idle_connection_timer() routine unconditionally sends it.

> 
> Firing it every 120 seconds doesn't seem to be big waste and may actually save
> some CPU.

Firing the timer, no big deal. Sending the packets and requiring the peer to process
them too, disagree.

Tom.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Long Li Aug. 15, 2017, 12:10 a.m. UTC | #4

> -----Original Message-----
> From: Tom Talpey
> Sent: Monday, August 14, 2017 4:42 PM
> To: Long Li <[e-mail]>; Steve French <[e-mail]>;
> [e-mail]; [e-mail]; linux-
> [e-mail]
> Subject: RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle connection
> timer
> 
> > -----Original Message-----
> > From: [e-mail] [mailto:linux-cifs-
> > [e-mail]] On Behalf Of Long Li
> > Sent: Monday, August 14, 2017 7:30 PM
> > To: Tom Talpey <[e-mail]>; Steve French
> > <[e-mail]>; [e-mail];
> > [e-mail]; linux- [e-mail]
> > Subject: RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle
> > connection timer
> >
> > [This sender failed our fraud detection checks and may not be who they
> > appear to be. Learn about spoofing at
> > http://aka.ms/LearnAboutSpoofing]
> >
> > > -----Original Message-----
> > > From: Tom Talpey
> > > Sent: Monday, August 14, 2017 2:12 PM
> > > To: Long Li <[e-mail]>; Steve French
> > > <[e-mail]>; [e-mail];
> > > [e-mail]; linux- [e-mail]
> > > Cc: Long Li <[e-mail]>
> > > Subject: RE: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle
> > > connection timer
> > >
> > > > -----Original Message-----
> > > > From: [e-mail] [mailto:linux-cifs-
> > > > [e-mail]] On Behalf Of Long Li
> > > > Sent: Wednesday, August 2, 2017 4:11 PM
> > > > To: Steve French <[e-mail]>; [e-mail];
> > > > samba- [e-mail]; [e-mail]
> > > > Cc: Long Li <[e-mail]>
> > > > Subject: [[PATCH v1] 25/37] [CIFS] SMBD: Support SMBD idle
> > > > connection timer
> > > >
> > > > +static int keep_alive_interval = 120;
> > >
> > > This is the recommended value, but not the only possibility.
> > >
> > > > @@ -1348,6 +1369,10 @@ struct cifs_rdma_info*
> > > cifs_create_rdma_session(
> > > >         init_waitqueue_head(&info->wait_send_queue);
> > > >         init_waitqueue_head(&info->wait_reassembly_queue);
> > > >
> > > > +       INIT_DELAYED_WORK(&info->idle_timer_work,
> > > idle_connection_timer);
> > > > +       schedule_delayed_work(&info->idle_timer_work,
> > > > +               info->keep_alive_interval*HZ);
> > > > +
> > >
> > > This initialization is ok, but the timer should be rescheduled
> > > (extended) any time any packet is sent. There is no need to perform
> > > keepalives on an active SMB Direct connection.
> >
> > My feeling is that rescheduling on a work queue for every packet is
> > sent is not efficient, especially under heavy conditions.
> 
> That's not what I was suggesting. Cant the timer simply be re-extended to
> the 120-second interval? I.e. on an active connection, it will never fire
> because it's always advancing.
> 
> As defined here, it will go off and send a keepalive every 120 seconds. The
> idle_connection_timer() routine unconditionally sends it.
> 
> >
> > Firing it every 120 seconds doesn't seem to be big waste and may
> > actually save some CPU.
> 
> Firing the timer, no big deal. Sending the packets and requiring the peer to
> process them too, disagree.

Fair enough. I will fix the code to modify delayed work instead of firing every 120 seconds.

> 
> Tom.
--
To unsubscribe from this list: send the line "unsubscribe linux-cifs" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/fs/cifs/cifsrdma.c b/fs/cifs/cifsrdma.c
index e275834..523c80f 100644
--- a/fs/cifs/cifsrdma.c
+++ b/fs/cifs/cifsrdma.c
@@ -89,6 +89,7 @@  static int send_credit_target = 512;
 static int max_send_size = 8192;
 static int max_fragmented_recv_size = 1024*1024;
 static int max_receive_size = 8192;
+static int keep_alive_interval = 120;
 
 // maximum number of SGEs in a RDMA I/O
 static int max_send_sge = 16;
@@ -1200,6 +1201,25 @@  static void destroy_receive_buffers(struct cifs_rdma_info *info)
 		mempool_free(response, info->response_mempool);
 }
 
+// Implement idle connection timer [MS-SMBD] 3.1.6.2
+static void idle_connection_timer(struct work_struct *work)
+{
+	struct cifs_rdma_info *info = container_of(
+					work, struct cifs_rdma_info,
+					idle_timer_work.work);
+
+	if (info->keep_alive_requested != KEEP_ALIVE_NONE)
+		log_keep_alive("error status info->keep_alive_requested=%d\n",
+				info->keep_alive_requested);
+
+	log_keep_alive("about to send an empty idle message\n");
+	cifs_rdma_post_send_empty(info);
+
+	// setup the next idle timeout work
+	schedule_delayed_work(&info->idle_timer_work,
+				info->keep_alive_interval*HZ);
+}
+
 int cifs_reconnect_rdma_session(struct TCP_Server_Info *server)
 {
 	log_rdma_event("reconnecting rdma session\n");
@@ -1262,6 +1282,7 @@  struct cifs_rdma_info* cifs_create_rdma_session(
 	info->max_send_size = max_send_size;
 	info->max_fragmented_recv_size = max_fragmented_recv_size;
 	info->max_receive_size = max_receive_size;
+	info->keep_alive_interval = keep_alive_interval;
 
 	max_send_sge = min_t(int, max_send_sge,
 		info->id->device->attrs.max_sge);
@@ -1348,6 +1369,10 @@  struct cifs_rdma_info* cifs_create_rdma_session(
 	init_waitqueue_head(&info->wait_send_queue);
 	init_waitqueue_head(&info->wait_reassembly_queue);
 
+	INIT_DELAYED_WORK(&info->idle_timer_work, idle_connection_timer);
+	schedule_delayed_work(&info->idle_timer_work,
+		info->keep_alive_interval*HZ);
+
 	init_waitqueue_head(&info->wait_send_pending);
 	atomic_set(&info->send_pending, 0);
 
diff --git a/fs/cifs/cifsrdma.h b/fs/cifs/cifsrdma.h
index 62d0bb8..dd497ce 100644
--- a/fs/cifs/cifsrdma.h
+++ b/fs/cifs/cifsrdma.h
@@ -73,6 +73,7 @@  struct cifs_rdma_info {
 	int max_fragmented_recv_size;
 	int max_fragmented_send_size;
 	int max_receive_size;
+	int keep_alive_interval;
 	int max_readwrite_size;
 	enum keep_alive_status keep_alive_requested;
 	int protocol;
@@ -101,12 +102,13 @@  struct cifs_rdma_info {
 
 	wait_queue_head_t wait_send_queue;
 
+	bool full_packet_received;
+	struct delayed_work idle_timer_work;;
+
 	// request pool for RDMA send
 	struct kmem_cache *request_cache;
 	mempool_t *request_mempool;
 
-	bool full_packet_received;
-
 	// response pool for RDMA receive
 	struct kmem_cache *response_cache;
 	mempool_t *response_mempool;