diff mbox series

[01/11] cifs: fix tcon status change after tree connect

Message ID 20230310153211.10982-1-sprasad@microsoft.com
State New
Headers show
Series [01/11] cifs: fix tcon status change after tree connect | expand

Commit Message

Shyam Prasad N March 10, 2023, 3:32 p.m. UTC
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(-)

Comments

Paulo Alcantara March 14, 2023, 10:19 p.m. UTC | #1
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.
Shyam Prasad N March 16, 2023, 10:57 a.m. UTC | #2
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.
Paulo Alcantara March 16, 2023, 8:59 p.m. UTC | #3
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) {
Shyam Prasad N March 17, 2023, 10:48 a.m. UTC | #4
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.
Paulo Alcantara March 17, 2023, 12:35 p.m. UTC | #5
Shyam Prasad N <nspmangalore@gmail.com> writes:

> Here's the updated patch.

Thanks!

Reviewed-by: Paulo Alcantara (SUSE) <pc@manguebit.com>
Steve French March 17, 2023, 6:25 p.m. UTC | #6
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 mbox series

Patch

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);
 
 	/*