diff mbox series

cifs: fix msleep() is imprecise

Message ID 1607591258-13865-1-git-send-email-yejune.deng@gmail.com (mailing list archive)
State New, archived
Headers show
Series cifs: fix msleep() is imprecise | expand

Commit Message

Yejune Deng Dec. 10, 2020, 9:07 a.m. UTC
See Documentation/timers/timers-howto.rst, msleep() is not
for (1ms - 20ms), There is a more advanced API is used.

Signed-off-by: Yejune Deng <yejune.deng@gmail.com>
---
 fs/cifs/cifsfs.c    |  4 ++--
 fs/cifs/connect.c   | 14 +++++++-------
 fs/cifs/file.c      |  6 +++---
 fs/cifs/smbdirect.c |  2 +-
 4 files changed, 13 insertions(+), 13 deletions(-)

Comments

Steve French Jan. 6, 2021, 4:26 a.m. UTC | #1
This patch seems reasonable at first glance, but I was a little
concerned that we don't see many users yet of fsleep.  Has there been
pushback on converting "yield" situations from using msleep to fsleep?

On Thu, Dec 10, 2020 at 3:09 AM Yejune Deng <yejune.deng@gmail.com> wrote:
>
> See Documentation/timers/timers-howto.rst, msleep() is not
> for (1ms - 20ms), There is a more advanced API is used.
>
> Signed-off-by: Yejune Deng <yejune.deng@gmail.com>
> ---
>  fs/cifs/cifsfs.c    |  4 ++--
>  fs/cifs/connect.c   | 14 +++++++-------
>  fs/cifs/file.c      |  6 +++---
>  fs/cifs/smbdirect.c |  2 +-
>  4 files changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index 472cb77..d35ce52 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -664,10 +664,10 @@ static void cifs_umount_begin(struct super_block *sb)
>                 cifs_dbg(FYI, "wake up tasks now - umount begin not complete\n");
>                 wake_up_all(&tcon->ses->server->request_q);
>                 wake_up_all(&tcon->ses->server->response_q);
> -               msleep(1); /* yield */
> +               fsleep(1000); /* yield */
>                 /* we have to kick the requests once more */
>                 wake_up_all(&tcon->ses->server->response_q);
> -               msleep(1);
> +               fsleep(1000);
>         }
>
>         return;
> diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> index 44f9cce..62a9c64 100644
> --- a/fs/cifs/connect.c
> +++ b/fs/cifs/connect.c
> @@ -538,7 +538,7 @@ static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
>                 if (rc) {
>                         cifs_dbg(FYI, "reconnect error %d\n", rc);
>                         mutex_unlock(&server->srv_mutex);
> -                       msleep(3000);
> +                       ssleep(3);
>                 } else {
>                         atomic_inc(&tcpSesReconnectCount);
>                         set_credits(server, 1);
> @@ -621,7 +621,7 @@ static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
>                 server->bigbuf = (char *)cifs_buf_get();
>                 if (!server->bigbuf) {
>                         cifs_server_dbg(VFS, "No memory for large SMB response\n");
> -                       msleep(3000);
> +                       ssleep(3);
>                         /* retry will check if exiting */
>                         return false;
>                 }
> @@ -634,7 +634,7 @@ static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
>                 server->smallbuf = (char *)cifs_small_buf_get();
>                 if (!server->smallbuf) {
>                         cifs_server_dbg(VFS, "No memory for SMB response\n");
> -                       msleep(1000);
> +                       ssleep(1);
>                         /* retry will check if exiting */
>                         return false;
>                 }
> @@ -729,7 +729,7 @@ static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
>                          * to clear and app threads to set tcpStatus
>                          * CifsNeedReconnect if server hung.
>                          */
> -                       usleep_range(1000, 2000);
> +                       fsleep(1000);
>                         length = 0;
>                         continue;
>                 }
> @@ -790,7 +790,7 @@ static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
>                  */
>                 cifs_dbg(FYI, "RFC 1002 negative session response\n");
>                 /* give server a second to clean up */
> -               msleep(1000);
> +               ssleep(1);
>                 /*
>                  * Always try 445 first on reconnect since we get NACK
>                  * on some if we ever connected to port 139 (the NACK
> @@ -944,7 +944,7 @@ static void clean_demultiplex_info(struct TCP_Server_Info *server)
>                  * response and going ahead and killing cifsd.
>                  */
>                 cifs_dbg(FYI, "Wait for exit from demultiplex thread\n");
> -               msleep(46000);
> +               ssleep(46);
>                 /*
>                  * If threads still have not exited they are probably never
>                  * coming home not much else we can do but free the memory.
> @@ -3655,7 +3655,7 @@ static void rfc1002mangle(char *target, char *source, unsigned int length)
>                  * significant slowing down on mount
>                  * for everyone else
>                  */
> -               usleep_range(1000, 2000);
> +               fsleep(1000);
>         }
>         /*
>          * else the negprot may still work without this
> diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> index be46fab..75538a8 100644
> --- a/fs/cifs/file.c
> +++ b/fs/cifs/file.c
> @@ -283,7 +283,7 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
>  cifs_down_write(struct rw_semaphore *sem)
>  {
>         while (!down_write_trylock(sem))
> -               msleep(10);
> +               fsleep(10000);
>  }
>
>  static void cifsFileInfo_put_work(struct work_struct *work);
> @@ -2828,7 +2828,7 @@ size_t get_numpages(const size_t wsize, const size_t len, size_t *cur_len)
>
>                         if (wsize < wdata->bytes) {
>                                 add_credits_and_wake_if(server, &credits, 0);
> -                               msleep(1000);
> +                               ssleep(1);
>                         }
>                 } while (wsize < wdata->bytes);
>                 wdata->credits = credits;
> @@ -3563,7 +3563,7 @@ static int cifs_resend_rdata(struct cifs_readdata *rdata,
>
>                         if (rsize < rdata->bytes) {
>                                 add_credits_and_wake_if(server, &credits, 0);
> -                               msleep(1000);
> +                               ssleep(1);
>                         }
>                 } while (rsize < rdata->bytes);
>                 rdata->credits = credits;
> diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> index b029ed3..84f97f8 100644
> --- a/fs/cifs/smbdirect.c
> +++ b/fs/cifs/smbdirect.c
> @@ -1372,7 +1372,7 @@ void smbd_destroy(struct TCP_Server_Info *server)
>         wake_up_interruptible_all(&info->wait_mr);
>         while (atomic_read(&info->mr_used_count)) {
>                 mutex_unlock(&server->srv_mutex);
> -               msleep(1000);
> +               ssleep(1);
>                 mutex_lock(&server->srv_mutex);
>         }
>         destroy_mr_list(info);
> --
> 1.9.1
>
Yejune Deng Jan. 7, 2021, 11:08 a.m. UTC | #2
No。I just see Documentation/timers/timers-howto.rst and don't
recommend using msleep() for (1ms - 20ms). It recommends using
usleep_range(). And fsleep() is flexible sleeping.


On Wed, Jan 6, 2021 at 12:27 PM Steve French <smfrench@gmail.com> wrote:
>
> This patch seems reasonable at first glance, but I was a little
> concerned that we don't see many users yet of fsleep.  Has there been
> pushback on converting "yield" situations from using msleep to fsleep?
>
> On Thu, Dec 10, 2020 at 3:09 AM Yejune Deng <yejune.deng@gmail.com> wrote:
> >
> > See Documentation/timers/timers-howto.rst, msleep() is not
> > for (1ms - 20ms), There is a more advanced API is used.
> >
> > Signed-off-by: Yejune Deng <yejune.deng@gmail.com>
> > ---
> >  fs/cifs/cifsfs.c    |  4 ++--
> >  fs/cifs/connect.c   | 14 +++++++-------
> >  fs/cifs/file.c      |  6 +++---
> >  fs/cifs/smbdirect.c |  2 +-
> >  4 files changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> > index 472cb77..d35ce52 100644
> > --- a/fs/cifs/cifsfs.c
> > +++ b/fs/cifs/cifsfs.c
> > @@ -664,10 +664,10 @@ static void cifs_umount_begin(struct super_block *sb)
> >                 cifs_dbg(FYI, "wake up tasks now - umount begin not complete\n");
> >                 wake_up_all(&tcon->ses->server->request_q);
> >                 wake_up_all(&tcon->ses->server->response_q);
> > -               msleep(1); /* yield */
> > +               fsleep(1000); /* yield */
> >                 /* we have to kick the requests once more */
> >                 wake_up_all(&tcon->ses->server->response_q);
> > -               msleep(1);
> > +               fsleep(1000);
> >         }
> >
> >         return;
> > diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
> > index 44f9cce..62a9c64 100644
> > --- a/fs/cifs/connect.c
> > +++ b/fs/cifs/connect.c
> > @@ -538,7 +538,7 @@ static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
> >                 if (rc) {
> >                         cifs_dbg(FYI, "reconnect error %d\n", rc);
> >                         mutex_unlock(&server->srv_mutex);
> > -                       msleep(3000);
> > +                       ssleep(3);
> >                 } else {
> >                         atomic_inc(&tcpSesReconnectCount);
> >                         set_credits(server, 1);
> > @@ -621,7 +621,7 @@ static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
> >                 server->bigbuf = (char *)cifs_buf_get();
> >                 if (!server->bigbuf) {
> >                         cifs_server_dbg(VFS, "No memory for large SMB response\n");
> > -                       msleep(3000);
> > +                       ssleep(3);
> >                         /* retry will check if exiting */
> >                         return false;
> >                 }
> > @@ -634,7 +634,7 @@ static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
> >                 server->smallbuf = (char *)cifs_small_buf_get();
> >                 if (!server->smallbuf) {
> >                         cifs_server_dbg(VFS, "No memory for SMB response\n");
> > -                       msleep(1000);
> > +                       ssleep(1);
> >                         /* retry will check if exiting */
> >                         return false;
> >                 }
> > @@ -729,7 +729,7 @@ static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
> >                          * to clear and app threads to set tcpStatus
> >                          * CifsNeedReconnect if server hung.
> >                          */
> > -                       usleep_range(1000, 2000);
> > +                       fsleep(1000);
> >                         length = 0;
> >                         continue;
> >                 }
> > @@ -790,7 +790,7 @@ static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
> >                  */
> >                 cifs_dbg(FYI, "RFC 1002 negative session response\n");
> >                 /* give server a second to clean up */
> > -               msleep(1000);
> > +               ssleep(1);
> >                 /*
> >                  * Always try 445 first on reconnect since we get NACK
> >                  * on some if we ever connected to port 139 (the NACK
> > @@ -944,7 +944,7 @@ static void clean_demultiplex_info(struct TCP_Server_Info *server)
> >                  * response and going ahead and killing cifsd.
> >                  */
> >                 cifs_dbg(FYI, "Wait for exit from demultiplex thread\n");
> > -               msleep(46000);
> > +               ssleep(46);
> >                 /*
> >                  * If threads still have not exited they are probably never
> >                  * coming home not much else we can do but free the memory.
> > @@ -3655,7 +3655,7 @@ static void rfc1002mangle(char *target, char *source, unsigned int length)
> >                  * significant slowing down on mount
> >                  * for everyone else
> >                  */
> > -               usleep_range(1000, 2000);
> > +               fsleep(1000);
> >         }
> >         /*
> >          * else the negprot may still work without this
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c
> > index be46fab..75538a8 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -283,7 +283,7 @@ int cifs_posix_open(char *full_path, struct inode **pinode,
> >  cifs_down_write(struct rw_semaphore *sem)
> >  {
> >         while (!down_write_trylock(sem))
> > -               msleep(10);
> > +               fsleep(10000);
> >  }
> >
> >  static void cifsFileInfo_put_work(struct work_struct *work);
> > @@ -2828,7 +2828,7 @@ size_t get_numpages(const size_t wsize, const size_t len, size_t *cur_len)
> >
> >                         if (wsize < wdata->bytes) {
> >                                 add_credits_and_wake_if(server, &credits, 0);
> > -                               msleep(1000);
> > +                               ssleep(1);
> >                         }
> >                 } while (wsize < wdata->bytes);
> >                 wdata->credits = credits;
> > @@ -3563,7 +3563,7 @@ static int cifs_resend_rdata(struct cifs_readdata *rdata,
> >
> >                         if (rsize < rdata->bytes) {
> >                                 add_credits_and_wake_if(server, &credits, 0);
> > -                               msleep(1000);
> > +                               ssleep(1);
> >                         }
> >                 } while (rsize < rdata->bytes);
> >                 rdata->credits = credits;
> > diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
> > index b029ed3..84f97f8 100644
> > --- a/fs/cifs/smbdirect.c
> > +++ b/fs/cifs/smbdirect.c
> > @@ -1372,7 +1372,7 @@ void smbd_destroy(struct TCP_Server_Info *server)
> >         wake_up_interruptible_all(&info->wait_mr);
> >         while (atomic_read(&info->mr_used_count)) {
> >                 mutex_unlock(&server->srv_mutex);
> > -               msleep(1000);
> > +               ssleep(1);
> >                 mutex_lock(&server->srv_mutex);
> >         }
> >         destroy_mr_list(info);
> > --
> > 1.9.1
> >
>
>
> --
> Thanks,
>
> Steve
Aurélien Aptel Jan. 7, 2021, 2:03 p.m. UTC | #3
Yejune Deng <yejune.deng@gmail.com> writes:
> No。I just see Documentation/timers/timers-howto.rst and don't
> recommend using msleep() for (1ms - 20ms). It recommends using
> usleep_range(). And fsleep() is flexible sleeping.

I think what Steve is asking is does using fsleep() changes anything
regarding yielding i.e. does it affect how tasks/processes get
scheduled. AFAIK, this msleep(1) in the code is to give a hint and let
the kernel run other tasks, in particular the ones waiting on the
request and response queue.

Cheers,
diff mbox series

Patch

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 472cb77..d35ce52 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -664,10 +664,10 @@  static void cifs_umount_begin(struct super_block *sb)
 		cifs_dbg(FYI, "wake up tasks now - umount begin not complete\n");
 		wake_up_all(&tcon->ses->server->request_q);
 		wake_up_all(&tcon->ses->server->response_q);
-		msleep(1); /* yield */
+		fsleep(1000); /* yield */
 		/* we have to kick the requests once more */
 		wake_up_all(&tcon->ses->server->response_q);
-		msleep(1);
+		fsleep(1000);
 	}
 
 	return;
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index 44f9cce..62a9c64 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -538,7 +538,7 @@  static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
 		if (rc) {
 			cifs_dbg(FYI, "reconnect error %d\n", rc);
 			mutex_unlock(&server->srv_mutex);
-			msleep(3000);
+			ssleep(3);
 		} else {
 			atomic_inc(&tcpSesReconnectCount);
 			set_credits(server, 1);
@@ -621,7 +621,7 @@  static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
 		server->bigbuf = (char *)cifs_buf_get();
 		if (!server->bigbuf) {
 			cifs_server_dbg(VFS, "No memory for large SMB response\n");
-			msleep(3000);
+			ssleep(3);
 			/* retry will check if exiting */
 			return false;
 		}
@@ -634,7 +634,7 @@  static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
 		server->smallbuf = (char *)cifs_small_buf_get();
 		if (!server->smallbuf) {
 			cifs_server_dbg(VFS, "No memory for SMB response\n");
-			msleep(1000);
+			ssleep(1);
 			/* retry will check if exiting */
 			return false;
 		}
@@ -729,7 +729,7 @@  static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
 			 * to clear and app threads to set tcpStatus
 			 * CifsNeedReconnect if server hung.
 			 */
-			usleep_range(1000, 2000);
+			fsleep(1000);
 			length = 0;
 			continue;
 		}
@@ -790,7 +790,7 @@  static inline int reconn_setup_dfs_targets(struct cifs_sb_info *cifs_sb,
 		 */
 		cifs_dbg(FYI, "RFC 1002 negative session response\n");
 		/* give server a second to clean up */
-		msleep(1000);
+		ssleep(1);
 		/*
 		 * Always try 445 first on reconnect since we get NACK
 		 * on some if we ever connected to port 139 (the NACK
@@ -944,7 +944,7 @@  static void clean_demultiplex_info(struct TCP_Server_Info *server)
 		 * response and going ahead and killing cifsd.
 		 */
 		cifs_dbg(FYI, "Wait for exit from demultiplex thread\n");
-		msleep(46000);
+		ssleep(46);
 		/*
 		 * If threads still have not exited they are probably never
 		 * coming home not much else we can do but free the memory.
@@ -3655,7 +3655,7 @@  static void rfc1002mangle(char *target, char *source, unsigned int length)
 		 * significant slowing down on mount
 		 * for everyone else
 		 */
-		usleep_range(1000, 2000);
+		fsleep(1000);
 	}
 	/*
 	 * else the negprot may still work without this
diff --git a/fs/cifs/file.c b/fs/cifs/file.c
index be46fab..75538a8 100644
--- a/fs/cifs/file.c
+++ b/fs/cifs/file.c
@@ -283,7 +283,7 @@  int cifs_posix_open(char *full_path, struct inode **pinode,
 cifs_down_write(struct rw_semaphore *sem)
 {
 	while (!down_write_trylock(sem))
-		msleep(10);
+		fsleep(10000);
 }
 
 static void cifsFileInfo_put_work(struct work_struct *work);
@@ -2828,7 +2828,7 @@  size_t get_numpages(const size_t wsize, const size_t len, size_t *cur_len)
 
 			if (wsize < wdata->bytes) {
 				add_credits_and_wake_if(server, &credits, 0);
-				msleep(1000);
+				ssleep(1);
 			}
 		} while (wsize < wdata->bytes);
 		wdata->credits = credits;
@@ -3563,7 +3563,7 @@  static int cifs_resend_rdata(struct cifs_readdata *rdata,
 
 			if (rsize < rdata->bytes) {
 				add_credits_and_wake_if(server, &credits, 0);
-				msleep(1000);
+				ssleep(1);
 			}
 		} while (rsize < rdata->bytes);
 		rdata->credits = credits;
diff --git a/fs/cifs/smbdirect.c b/fs/cifs/smbdirect.c
index b029ed3..84f97f8 100644
--- a/fs/cifs/smbdirect.c
+++ b/fs/cifs/smbdirect.c
@@ -1372,7 +1372,7 @@  void smbd_destroy(struct TCP_Server_Info *server)
 	wake_up_interruptible_all(&info->wait_mr);
 	while (atomic_read(&info->mr_used_count)) {
 		mutex_unlock(&server->srv_mutex);
-		msleep(1000);
+		ssleep(1);
 		mutex_lock(&server->srv_mutex);
 	}
 	destroy_mr_list(info);