| Message ID | 20230310153211.10982-1-sprasad@microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | [01/11] cifs: fix tcon status change after tree connect | expand |
Hi Shyam, Shyam Prasad N <nspmangalore@gmail.com> writes: > After cifs_tree_connect, tcon status should not be > set to TID_GOOD. There could still be files that need > reopen. The status should instead be changed to > TID_NEED_FILES_INVALIDATE. That way, after reopen of > files, the status can be changed to TID_GOOD. > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > --- > fs/cifs/cifsglob.h | 2 +- > fs/cifs/connect.c | 14 ++++++++++---- > fs/cifs/dfs.c | 16 +++++++++++----- > fs/cifs/file.c | 10 +++++----- > 4 files changed, 27 insertions(+), 15 deletions(-) With this patch, the status of TID_GOOD is no longer set after reconnecting tcons. I've noticed that when the DFS cache refresher attempted to get a new referral for updating a cached entry but the IPC tcon status was still set to TID_NEED_FILES_INVALIDATE, therefore skipping the I/O as it thought the IPC tcon was disconnected. IOW, the TID_NEED_FILES_INVALIDATE status remains set for both types of tcons after reconnect. Besides, could you please split this patch into two? The first one would fix the checking of tcon statuses by using the appropriate spin lock, and the second would make use of TID_NEED_FILES_INVALIDATE on non-IPC tcons and set the TID_GOOD status afterwards. Thanks.
On Wed, Mar 15, 2023 at 3:49 AM Paulo Alcantara <pc@manguebit.com> wrote: > > Hi Shyam, > > Shyam Prasad N <nspmangalore@gmail.com> writes: > > > After cifs_tree_connect, tcon status should not be > > set to TID_GOOD. There could still be files that need > > reopen. The status should instead be changed to > > TID_NEED_FILES_INVALIDATE. That way, after reopen of > > files, the status can be changed to TID_GOOD. > > > > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > > --- > > fs/cifs/cifsglob.h | 2 +- > > fs/cifs/connect.c | 14 ++++++++++---- > > fs/cifs/dfs.c | 16 +++++++++++----- > > fs/cifs/file.c | 10 +++++----- > > 4 files changed, 27 insertions(+), 15 deletions(-) > > With this patch, the status of TID_GOOD is no longer set after > reconnecting tcons. I've noticed that when the DFS cache refresher > attempted to get a new referral for updating a cached entry but the IPC > tcon status was still set to TID_NEED_FILES_INVALIDATE, therefore > skipping the I/O as it thought the IPC tcon was disconnected. > > IOW, the TID_NEED_FILES_INVALIDATE status remains set for both types of > tcons after reconnect. > > Besides, could you please split this patch into two? The first one > would fix the checking of tcon statuses by using the appropriate spin > lock, and the second would make use of TID_NEED_FILES_INVALIDATE on > non-IPC tcons and set the TID_GOOD status afterwards. > > Thanks. Hi Paulo, Good point. The tcon status were indeed messed up after this patch. Should have checked it earlier. My bad. Let me revert this one and include only the checking of tcon status with the right lock. Attached the patch for that.
Shyam Prasad N <nspmangalore@gmail.com> writes: > On Wed, Mar 15, 2023 at 3:49 AM Paulo Alcantara <pc@manguebit.com> wrote: >> Shyam Prasad N <nspmangalore@gmail.com> writes: >> >> > After cifs_tree_connect, tcon status should not be >> > set to TID_GOOD. There could still be files that need >> > reopen. The status should instead be changed to >> > TID_NEED_FILES_INVALIDATE. That way, after reopen of >> > files, the status can be changed to TID_GOOD. >> > >> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> >> > --- >> > fs/cifs/cifsglob.h | 2 +- >> > fs/cifs/connect.c | 14 ++++++++++---- >> > fs/cifs/dfs.c | 16 +++++++++++----- >> > fs/cifs/file.c | 10 +++++----- >> > 4 files changed, 27 insertions(+), 15 deletions(-) >> >> With this patch, the status of TID_GOOD is no longer set after >> reconnecting tcons. I've noticed that when the DFS cache refresher >> attempted to get a new referral for updating a cached entry but the IPC >> tcon status was still set to TID_NEED_FILES_INVALIDATE, therefore >> skipping the I/O as it thought the IPC tcon was disconnected. >> >> IOW, the TID_NEED_FILES_INVALIDATE status remains set for both types of >> tcons after reconnect. >> >> Besides, could you please split this patch into two? The first one >> would fix the checking of tcon statuses by using the appropriate spin >> lock, and the second would make use of TID_NEED_FILES_INVALIDATE on >> non-IPC tcons and set the TID_GOOD status afterwards. > > Good point. The tcon status were indeed messed up after this patch. > Should have checked it earlier. My bad. > > Let me revert this one and include only the checking of tcon status > with the right lock. > > Attached the patch for that. Thanks for the patch! Looks good to me. BTW, don't you think we need to get rid of unnecessary ses status check in __refresh_tcon() as well ... spin_lock(&ipc->tc_lock); if (ses->ses_status != SES_GOOD || ipc->status != TID_GOOD) {
On Fri, Mar 17, 2023 at 2:29 AM Paulo Alcantara <pc@manguebit.com> wrote: > > Shyam Prasad N <nspmangalore@gmail.com> writes: > > > On Wed, Mar 15, 2023 at 3:49 AM Paulo Alcantara <pc@manguebit.com> wrote: > >> Shyam Prasad N <nspmangalore@gmail.com> writes: > >> > >> > After cifs_tree_connect, tcon status should not be > >> > set to TID_GOOD. There could still be files that need > >> > reopen. The status should instead be changed to > >> > TID_NEED_FILES_INVALIDATE. That way, after reopen of > >> > files, the status can be changed to TID_GOOD. > >> > > >> > Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> > >> > --- > >> > fs/cifs/cifsglob.h | 2 +- > >> > fs/cifs/connect.c | 14 ++++++++++---- > >> > fs/cifs/dfs.c | 16 +++++++++++----- > >> > fs/cifs/file.c | 10 +++++----- > >> > 4 files changed, 27 insertions(+), 15 deletions(-) > >> > >> With this patch, the status of TID_GOOD is no longer set after > >> reconnecting tcons. I've noticed that when the DFS cache refresher > >> attempted to get a new referral for updating a cached entry but the IPC > >> tcon status was still set to TID_NEED_FILES_INVALIDATE, therefore > >> skipping the I/O as it thought the IPC tcon was disconnected. > >> > >> IOW, the TID_NEED_FILES_INVALIDATE status remains set for both types of > >> tcons after reconnect. > >> > >> Besides, could you please split this patch into two? The first one > >> would fix the checking of tcon statuses by using the appropriate spin > >> lock, and the second would make use of TID_NEED_FILES_INVALIDATE on > >> non-IPC tcons and set the TID_GOOD status afterwards. > > > > Good point. The tcon status were indeed messed up after this patch. > > Should have checked it earlier. My bad. > > > > Let me revert this one and include only the checking of tcon status > > with the right lock. > > > > Attached the patch for that. > > Thanks for the patch! Looks good to me. > > BTW, don't you think we need to get rid of unnecessary ses status check > in __refresh_tcon() as well > > ... > spin_lock(&ipc->tc_lock); > if (ses->ses_status != SES_GOOD || ipc->status != TID_GOOD) { Good point. Here's the updated patch.
Shyam Prasad N <nspmangalore@gmail.com> writes: > Here's the updated patch. Thanks! Reviewed-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
updated for-next with this newer version of the patch On Fri, Mar 17, 2023 at 7:35 AM Paulo Alcantara <pc@manguebit.com> wrote: > > Shyam Prasad N <nspmangalore@gmail.com> writes: > > > Here's the updated patch. > > Thanks! > > Reviewed-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index a99883f16d94..8a37b1553dc6 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -137,7 +137,7 @@ enum tid_status_enum { TID_NEED_RECON, TID_NEED_TCON, TID_IN_TCON, - TID_NEED_FILES_INVALIDATE, /* currently unused */ + TID_NEED_FILES_INVALIDATE, TID_IN_FILES_INVALIDATE }; diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c index 5233f14f0636..3d07729c91a1 100644 --- a/fs/cifs/connect.c +++ b/fs/cifs/connect.c @@ -4038,9 +4038,15 @@ int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon, const stru /* only send once per connect */ spin_lock(&tcon->tc_lock); - if (tcon->ses->ses_status != SES_GOOD || - (tcon->status != TID_NEW && - tcon->status != TID_NEED_TCON)) { + if (tcon->status != TID_GOOD && + tcon->status != TID_NEW && + tcon->status != TID_NEED_RECON) { + spin_unlock(&tcon->tc_lock); + return -EHOSTDOWN; + } + + if (tcon->status == TID_NEED_FILES_INVALIDATE || + tcon->status == TID_GOOD) { spin_unlock(&tcon->tc_lock); return 0; } @@ -4051,7 +4057,7 @@ int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon, const stru if (rc) { spin_lock(&tcon->tc_lock); if (tcon->status == TID_IN_TCON) - tcon->status = TID_NEED_TCON; + tcon->status = TID_NEED_RECON; spin_unlock(&tcon->tc_lock); } else { spin_lock(&tcon->tc_lock); diff --git a/fs/cifs/dfs.c b/fs/cifs/dfs.c index b64d20374b9c..d37af02902c5 100644 --- a/fs/cifs/dfs.c +++ b/fs/cifs/dfs.c @@ -479,9 +479,15 @@ int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon, const stru /* only send once per connect */ spin_lock(&tcon->tc_lock); - if (tcon->ses->ses_status != SES_GOOD || - (tcon->status != TID_NEW && - tcon->status != TID_NEED_TCON)) { + if (tcon->status != TID_GOOD && + tcon->status != TID_NEW && + tcon->status != TID_NEED_RECON) { + spin_unlock(&tcon->tc_lock); + return -EHOSTDOWN; + } + + if (tcon->status == TID_NEED_FILES_INVALIDATE || + tcon->status == TID_GOOD) { spin_unlock(&tcon->tc_lock); return 0; } @@ -529,12 +535,12 @@ int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon, const stru if (rc) { spin_lock(&tcon->tc_lock); if (tcon->status == TID_IN_TCON) - tcon->status = TID_NEED_TCON; + tcon->status = TID_NEED_RECON; spin_unlock(&tcon->tc_lock); } else { spin_lock(&tcon->tc_lock); if (tcon->status == TID_IN_TCON) - tcon->status = TID_GOOD; + tcon->status = TID_NEED_FILES_INVALIDATE; spin_unlock(&tcon->tc_lock); tcon->need_reconnect = false; } diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 4d4a2d82636d..96d865e108f4 100644 --- a/fs/cifs/file.c +++ b/fs/cifs/file.c @@ -174,13 +174,13 @@ cifs_mark_open_files_invalid(struct cifs_tcon *tcon) struct list_head *tmp1; /* only send once per connect */ - spin_lock(&tcon->ses->ses_lock); - if ((tcon->ses->ses_status != SES_GOOD) || (tcon->status != TID_NEED_RECON)) { - spin_unlock(&tcon->ses->ses_lock); + spin_lock(&tcon->tc_lock); + if (tcon->status != TID_NEED_FILES_INVALIDATE) { + spin_unlock(&tcon->tc_lock); return; } tcon->status = TID_IN_FILES_INVALIDATE; - spin_unlock(&tcon->ses->ses_lock); + spin_unlock(&tcon->tc_lock); /* list all files open on tree connection and mark them invalid */ spin_lock(&tcon->open_file_lock); @@ -194,7 +194,7 @@ cifs_mark_open_files_invalid(struct cifs_tcon *tcon) invalidate_all_cached_dirs(tcon); spin_lock(&tcon->tc_lock); if (tcon->status == TID_IN_FILES_INVALIDATE) - tcon->status = TID_NEED_TCON; + tcon->status = TID_GOOD; spin_unlock(&tcon->tc_lock); /*
After cifs_tree_connect, tcon status should not be set to TID_GOOD. There could still be files that need reopen. The status should instead be changed to TID_NEED_FILES_INVALIDATE. That way, after reopen of files, the status can be changed to TID_GOOD. Signed-off-by: Shyam Prasad N <sprasad@microsoft.com> --- fs/cifs/cifsglob.h | 2 +- fs/cifs/connect.c | 14 ++++++++++---- fs/cifs/dfs.c | 16 +++++++++++----- fs/cifs/file.c | 10 +++++----- 4 files changed, 27 insertions(+), 15 deletions(-)