| 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 |
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 >
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
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 --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);
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(-)