diff mbox series

[CIFS] cleanup and clarify status of tree connections

Message ID CAH2r5msLJ166+yuHWQnV7_10_SZ3R1RmMwgLyGMBbggcYks1hQ@mail.gmail.com (mailing list archive)
State New, archived
Headers show
Series [CIFS] cleanup and clarify status of tree connections | expand

Commit Message

Steve French March 27, 2022, 9:14 p.m. UTC 
Currently the way the tid (tree connection) status is tracked
is confusing.  The same enum is used for structs cifs_tcon
and cifs_ses and TCP_Server_info, but each of these three has
different states that they transition among.  The current
code also unnecessarily uses camelCase.

Convert from use of statusEnum to a new tid_status_enum for
tree connections.  The valid states for a tid are:

            TID_NEW = 0,
            TID_GOOD,
            TID_EXITING,
            TID_NEED_RECON,
            TID_NEED_TCON,
            TID_IN_TCON,
            TID_NEED_FILES_INVALIDATE, /* unused, considering removing
in future */
            TID_IN_FILES_INVALIDATE

It also removes CifsNeedTcon CifsInTcon CifsNeedFilesInvalidate and
CifsInFilesInvalidate from the statusEnum used for session and
TCP_Server_Info since they are not relevant for those.

A follow on patch will fix the places where we use the
tcon->need_reconnect flag to be more consistent with the tid->status

See attached.

Also FYI - Shyam had a work in progress patch to fix and clarify the
ses->status enum

Comments

Steve French March 28, 2022, 1:47 a.m. UTC | #1 
Updated patch to fix one place I missed pointed out by the kernel test robot.

See attached.

On Sun, Mar 27, 2022 at 4:14 PM Steve French <smfrench@gmail.com> wrote:
>
> Currently the way the tid (tree connection) status is tracked
> is confusing.  The same enum is used for structs cifs_tcon
> and cifs_ses and TCP_Server_info, but each of these three has
> different states that they transition among.  The current
> code also unnecessarily uses camelCase.
>
> Convert from use of statusEnum to a new tid_status_enum for
> tree connections.  The valid states for a tid are:
>
>             TID_NEW = 0,
>             TID_GOOD,
>             TID_EXITING,
>             TID_NEED_RECON,
>             TID_NEED_TCON,
>             TID_IN_TCON,
>             TID_NEED_FILES_INVALIDATE, /* unused, considering removing
> in future */
>             TID_IN_FILES_INVALIDATE
>
> It also removes CifsNeedTcon CifsInTcon CifsNeedFilesInvalidate and
> CifsInFilesInvalidate from the statusEnum used for session and
> TCP_Server_Info since they are not relevant for those.
>
> A follow on patch will fix the places where we use the
> tcon->need_reconnect flag to be more consistent with the tid->status
>
> See attached.
>
> Also FYI - Shyam had a work in progress patch to fix and clarify the
> ses->status enum
>
> --
> Thanks,
>
> Steve
Shyam Prasad N March 28, 2022, 4:27 p.m. UTC | #2 
On Mon, Mar 28, 2022 at 7:17 AM Steve French <smfrench@gmail.com> wrote:
>
> Updated patch to fix one place I missed pointed out by the kernel test robot.
>
> See attached.
>
> On Sun, Mar 27, 2022 at 4:14 PM Steve French <smfrench@gmail.com> wrote:
> >
> > Currently the way the tid (tree connection) status is tracked
> > is confusing.  The same enum is used for structs cifs_tcon
> > and cifs_ses and TCP_Server_info, but each of these three has
> > different states that they transition among.  The current
> > code also unnecessarily uses camelCase.
> >
> > Convert from use of statusEnum to a new tid_status_enum for
> > tree connections.  The valid states for a tid are:
> >
> >             TID_NEW = 0,
> >             TID_GOOD,
> >             TID_EXITING,
> >             TID_NEED_RECON,
> >             TID_NEED_TCON,
> >             TID_IN_TCON,
> >             TID_NEED_FILES_INVALIDATE, /* unused, considering removing
> > in future */
> >             TID_IN_FILES_INVALIDATE
> >
> > It also removes CifsNeedTcon CifsInTcon CifsNeedFilesInvalidate and
> > CifsInFilesInvalidate from the statusEnum used for session and
> > TCP_Server_Info since they are not relevant for those.
> >
> > A follow on patch will fix the places where we use the
> > tcon->need_reconnect flag to be more consistent with the tid->status
> >
> > See attached.
> >
> > Also FYI - Shyam had a work in progress patch to fix and clarify the
> > ses->status enum
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve

Please try to maintain the old values in the new enum.
Rest looks good.
Steve French March 28, 2022, 5:13 p.m. UTC | #3 
On Mon, Mar 28, 2022 at 11:27 AM Shyam Prasad N <nspmangalore@gmail.com> wrote:
>
> On Mon, Mar 28, 2022 at 7:17 AM Steve French <smfrench@gmail.com> wrote:
> >
> > Updated patch to fix one place I missed pointed out by the kernel test robot.
> >
> > See attached.
> >
> > On Sun, Mar 27, 2022 at 4:14 PM Steve French <smfrench@gmail.com> wrote:
> > >
> > > Currently the way the tid (tree connection) status is tracked
> > > is confusing.  The same enum is used for structs cifs_tcon
> > > and cifs_ses and TCP_Server_info, but each of these three has
> > > different states that they transition among.  The current
> > > code also unnecessarily uses camelCase.
> > >
> > > Convert from use of statusEnum to a new tid_status_enum for
> > > tree connections.  The valid states for a tid are:
> > >
> > >             TID_NEW = 0,
> > >             TID_GOOD,
> > >             TID_EXITING,
> > >             TID_NEED_RECON,
> > >             TID_NEED_TCON,
> > >             TID_IN_TCON,
> > >             TID_NEED_FILES_INVALIDATE, /* unused, considering removing
> > > in future */
> > >             TID_IN_FILES_INVALIDATE
> > >
> > > It also removes CifsNeedTcon CifsInTcon CifsNeedFilesInvalidate and
> > > CifsInFilesInvalidate from the statusEnum used for session and
> > > TCP_Server_Info since they are not relevant for those.
> > >
> > > A follow on patch will fix the places where we use the
> > > tcon->need_reconnect flag to be more consistent with the tid->status
> > >
> > > See attached.
> > >
> > > Also FYI - Shyam had a work in progress patch to fix and clarify the
> > > ses->status enum
> > >
> > > --
> > > Thanks,
> > >
> > > Steve
> >
> >
> >
> > --
> > Thanks,
> >
> > Steve
>
> Please try to maintain the old values in the new enum.
> Rest looks good.

They are mostly the same:
- for the statusEnum, only the ones at the end were removed, and those
don't apply to session or tcp_server (socket)
- for the tid->status the values for the first 4 didn't change

I did check carefully everwhere tidStatus was set/checked.

It shouldn't change any behavior.

The key question for next patch is whether we should check
tcon->need_reconnect which is what is used in all but one place or
check tid->status for need recon (which is done in only one place,
added by patch "cifs: check reconnects for channels of active tcons
too")

Thanks,

Steve
ronnie sahlberg March 28, 2022, 10:03 p.m. UTC | #4 
Looks good to me.

Reviewed-by me

On Mon, Mar 28, 2022 at 12:49 PM Steve French <smfrench@gmail.com> wrote:
>
> Updated patch to fix one place I missed pointed out by the kernel test robot.
>
> See attached.
>
> On Sun, Mar 27, 2022 at 4:14 PM Steve French <smfrench@gmail.com> wrote:
> >
> > Currently the way the tid (tree connection) status is tracked
> > is confusing.  The same enum is used for structs cifs_tcon
> > and cifs_ses and TCP_Server_info, but each of these three has
> > different states that they transition among.  The current
> > code also unnecessarily uses camelCase.
> >
> > Convert from use of statusEnum to a new tid_status_enum for
> > tree connections.  The valid states for a tid are:
> >
> >             TID_NEW = 0,
> >             TID_GOOD,
> >             TID_EXITING,
> >             TID_NEED_RECON,
> >             TID_NEED_TCON,
> >             TID_IN_TCON,
> >             TID_NEED_FILES_INVALIDATE, /* unused, considering removing
> > in future */
> >             TID_IN_FILES_INVALIDATE
> >
> > It also removes CifsNeedTcon CifsInTcon CifsNeedFilesInvalidate and
> > CifsInFilesInvalidate from the statusEnum used for session and
> > TCP_Server_Info since they are not relevant for those.
> >
> > A follow on patch will fix the places where we use the
> > tcon->need_reconnect flag to be more consistent with the tid->status
> >
> > See attached.
> >
> > Also FYI - Shyam had a work in progress patch to fix and clarify the
> > ses->status enum
> >
> > --
> > Thanks,
> >
> > Steve
>
>
>
> --
> Thanks,
>
> Steve
diff mbox series

Patch

From 7e5c8c02911ba8d7e61d4fbd130215318343cf60 Mon Sep 17 00:00:00 2001
From: Steve French <stfrench@microsoft.com>
Date: Sun, 27 Mar 2022 16:07:30 -0500
Subject: [PATCH] smb3: cleanup and clarify status of tree connections

Currently the way the tid (tree connection) status is tracked
is confusing.  The same enum is used for structs cifs_tcon
and cifs_ses and TCP_Server_info, but each of these three has
different states that they transition among.  The current
code also unnecessarily uses camelCase.

Convert from use of statusEnum to a new tid_status_enum for
tree connections.  The valid states for a tid are:

        TID_NEW = 0,
        TID_GOOD,
        TID_EXITING,
        TID_NEED_RECON,
        TID_NEED_TCON,
        TID_IN_TCON,
        TID_NEED_FILES_INVALIDATE, /* unused, considering removing in future */
        TID_IN_FILES_INVALIDATE

It also removes CifsNeedTcon, CifsInTcon, CifsNeedFilesInvalidate and
CifsInFilesInvalidate from the statusEnum used for session and
TCP_Server_Info since they are not relevant for those.

A follow on patch will fix the places where we use the
tcon->need_reconnect flag to be more consistent with the tid->status.

Signed-off-by: Steve French <stfrench@microsoft.com>
---
 fs/cifs/cifs_debug.c |  2 +-
 fs/cifs/cifsfs.c     |  4 ++--
 fs/cifs/cifsglob.h   | 18 +++++++++++++-----
 fs/cifs/cifssmb.c    | 11 +++++------
 fs/cifs/connect.c    | 32 ++++++++++++++++----------------
 fs/cifs/misc.c       |  2 +-
 fs/cifs/smb2pdu.c    |  4 ++--
 7 files changed, 40 insertions(+), 33 deletions(-)

diff --git a/fs/cifs/cifs_debug.c b/fs/cifs/cifs_debug.c
index ea00e1a91250..9d334816eac0 100644
--- a/fs/cifs/cifs_debug.c
+++ b/fs/cifs/cifs_debug.c
@@ -94,7 +94,7 @@  static void cifs_debug_tcon(struct seq_file *m, struct cifs_tcon *tcon)
 		   le32_to_cpu(tcon->fsDevInfo.DeviceCharacteristics),
 		   le32_to_cpu(tcon->fsAttrInfo.Attributes),
 		   le32_to_cpu(tcon->fsAttrInfo.MaxPathNameComponentLength),
-		   tcon->tidStatus);
+		   tcon->status);
 	if (dev_type == FILE_DEVICE_DISK)
 		seq_puts(m, " type: DISK ");
 	else if (dev_type == FILE_DEVICE_CD_ROM)
diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index 6e5246122ee2..969ec2308775 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -699,14 +699,14 @@  static void cifs_umount_begin(struct super_block *sb)
 	tcon = cifs_sb_master_tcon(cifs_sb);
 
 	spin_lock(&cifs_tcp_ses_lock);
-	if ((tcon->tc_count > 1) || (tcon->tidStatus == CifsExiting)) {
+	if ((tcon->tc_count > 1) || (tcon->status == TID_EXITING)) {
 		/* we have other mounts to same share or we have
 		   already tried to force umount this and woken up
 		   all waiting network requests, nothing to do */
 		spin_unlock(&cifs_tcp_ses_lock);
 		return;
 	} else if (tcon->tc_count == 1)
-		tcon->tidStatus = CifsExiting;
+		tcon->status = CifsExiting;
 	spin_unlock(&cifs_tcp_ses_lock);
 
 	/* cancel_brl_requests(tcon); */ /* BB mark all brl mids as exiting */
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h
index ad3cd6053f4e..cd9127510a55 100644
--- a/fs/cifs/cifsglob.h
+++ b/fs/cifs/cifsglob.h
@@ -115,10 +115,18 @@  enum statusEnum {
 	CifsInNegotiate,
 	CifsNeedSessSetup,
 	CifsInSessSetup,
-	CifsNeedTcon,
-	CifsInTcon,
-	CifsNeedFilesInvalidate,
-	CifsInFilesInvalidate
+};
+
+/* associated with each tree connection to the server */
+enum tid_status_enum {
+	TID_NEW = 0,
+	TID_GOOD,
+	TID_EXITING,
+	TID_NEED_RECON,
+	TID_NEED_TCON,
+	TID_IN_TCON,
+	TID_NEED_FILES_INVALIDATE, /* currently unused */
+	TID_IN_FILES_INVALIDATE
 };
 
 enum securityEnum {
@@ -1032,7 +1040,7 @@  struct cifs_tcon {
 	char *password;		/* for share-level security */
 	__u32 tid;		/* The 4 byte tree id */
 	__u16 Flags;		/* optional support bits */
-	enum statusEnum tidStatus;
+	enum tid_status_enum status;
 	atomic_t num_smbs_sent;
 	union {
 		struct {
diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c
index 071e2f21a7db..aca9338b0877 100644
--- a/fs/cifs/cifssmb.c
+++ b/fs/cifs/cifssmb.c
@@ -75,12 +75,11 @@  cifs_mark_open_files_invalid(struct cifs_tcon *tcon)
 
 	/* only send once per connect */
 	spin_lock(&cifs_tcp_ses_lock);
-	if (tcon->ses->status != CifsGood ||
-	    tcon->tidStatus != CifsNeedReconnect) {
+	if ((tcon->ses->status != CifsGood) || (tcon->status != TID_NEED_RECON)) {
 		spin_unlock(&cifs_tcp_ses_lock);
 		return;
 	}
-	tcon->tidStatus = CifsInFilesInvalidate;
+	tcon->status = TID_IN_FILES_INVALIDATE;
 	spin_unlock(&cifs_tcp_ses_lock);
 
 	/* list all files open on tree connection and mark them invalid */
@@ -100,8 +99,8 @@  cifs_mark_open_files_invalid(struct cifs_tcon *tcon)
 	mutex_unlock(&tcon->crfid.fid_mutex);
 
 	spin_lock(&cifs_tcp_ses_lock);
-	if (tcon->tidStatus == CifsInFilesInvalidate)
-		tcon->tidStatus = CifsNeedTcon;
+	if (tcon->status == TID_IN_FILES_INVALIDATE)
+		tcon->status = TID_NEED_TCON;
 	spin_unlock(&cifs_tcp_ses_lock);
 
 	/*
@@ -136,7 +135,7 @@  cifs_reconnect_tcon(struct cifs_tcon *tcon, int smb_command)
 	 * have tcon) are allowed as we start force umount
 	 */
 	spin_lock(&cifs_tcp_ses_lock);
-	if (tcon->tidStatus == CifsExiting) {
+	if (tcon->status == TID_EXITING) {
 		if (smb_command != SMB_COM_WRITE_ANDX &&
 		    smb_command != SMB_COM_OPEN_ANDX &&
 		    smb_command != SMB_COM_TREE_DISCONNECT) {
diff --git a/fs/cifs/connect.c b/fs/cifs/connect.c
index d6f8ccc7bfe2..ee3b7c15e884 100644
--- a/fs/cifs/connect.c
+++ b/fs/cifs/connect.c
@@ -245,7 +245,7 @@  cifs_mark_tcp_ses_conns_for_reconnect(struct TCP_Server_Info *server,
 
 		list_for_each_entry(tcon, &ses->tcon_list, tcon_list) {
 			tcon->need_reconnect = true;
-			tcon->tidStatus = CifsNeedReconnect;
+			tcon->status = TID_NEED_RECON;
 		}
 		if (ses->tcon_ipc)
 			ses->tcon_ipc->need_reconnect = true;
@@ -2207,7 +2207,7 @@  cifs_get_smb_ses(struct TCP_Server_Info *server, struct smb3_fs_context *ctx)
 
 static int match_tcon(struct cifs_tcon *tcon, struct smb3_fs_context *ctx)
 {
-	if (tcon->tidStatus == CifsExiting)
+	if (tcon->status == TID_EXITING)
 		return 0;
 	if (strncmp(tcon->treeName, ctx->UNC, MAX_TREE_SIZE))
 		return 0;
@@ -4486,12 +4486,12 @@  int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon, const stru
 	/* only send once per connect */
 	spin_lock(&cifs_tcp_ses_lock);
 	if (tcon->ses->status != CifsGood ||
-	    (tcon->tidStatus != CifsNew &&
-	    tcon->tidStatus != CifsNeedTcon)) {
+	    (tcon->status != TID_NEW &&
+	    tcon->status != TID_NEED_TCON)) {
 		spin_unlock(&cifs_tcp_ses_lock);
 		return 0;
 	}
-	tcon->tidStatus = CifsInTcon;
+	tcon->status = TID_IN_TCON;
 	spin_unlock(&cifs_tcp_ses_lock);
 
 	tree = kzalloc(MAX_TREE_SIZE, GFP_KERNEL);
@@ -4532,13 +4532,13 @@  int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon, const stru
 
 	if (rc) {
 		spin_lock(&cifs_tcp_ses_lock);
-		if (tcon->tidStatus == CifsInTcon)
-			tcon->tidStatus = CifsNeedTcon;
+		if (tcon->status == TID_IN_TCON)
+			tcon->status = TID_NEED_TCON;
 		spin_unlock(&cifs_tcp_ses_lock);
 	} else {
 		spin_lock(&cifs_tcp_ses_lock);
-		if (tcon->tidStatus == CifsInTcon)
-			tcon->tidStatus = CifsGood;
+		if (tcon->status == TID_IN_TCON)
+			tcon->status = TID_GOOD;
 		spin_unlock(&cifs_tcp_ses_lock);
 		tcon->need_reconnect = false;
 	}
@@ -4554,24 +4554,24 @@  int cifs_tree_connect(const unsigned int xid, struct cifs_tcon *tcon, const stru
 	/* only send once per connect */
 	spin_lock(&cifs_tcp_ses_lock);
 	if (tcon->ses->status != CifsGood ||
-	    (tcon->tidStatus != CifsNew &&
-	    tcon->tidStatus != CifsNeedTcon)) {
+	    (tcon->status != TID_NEW &&
+	    tcon->status != TID_NEED_TCON)) {
 		spin_unlock(&cifs_tcp_ses_lock);
 		return 0;
 	}
-	tcon->tidStatus = CifsInTcon;
+	tcon->status = TID_IN_TCON;
 	spin_unlock(&cifs_tcp_ses_lock);
 
 	rc = ops->tree_connect(xid, tcon->ses, tcon->treeName, tcon, nlsc);
 	if (rc) {
 		spin_lock(&cifs_tcp_ses_lock);
-		if (tcon->tidStatus == CifsInTcon)
-			tcon->tidStatus = CifsNeedTcon;
+		if (tcon->status == TID_IN_TCON)
+			tcon->status = TID_NEED_TCON;
 		spin_unlock(&cifs_tcp_ses_lock);
 	} else {
 		spin_lock(&cifs_tcp_ses_lock);
-		if (tcon->tidStatus == CifsInTcon)
-			tcon->tidStatus = CifsGood;
+		if (tcon->status == TID_IN_TCON)
+			tcon->status = TID_GOOD;
 		spin_unlock(&cifs_tcp_ses_lock);
 		tcon->need_reconnect = false;
 	}
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 56598f7dbe00..afaf59c22193 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -116,7 +116,7 @@  tconInfoAlloc(void)
 	}
 
 	atomic_inc(&tconInfoAllocCount);
-	ret_buf->tidStatus = CifsNew;
+	ret_buf->status = TID_NEW;
 	++ret_buf->tc_count;
 	INIT_LIST_HEAD(&ret_buf->openFileList);
 	INIT_LIST_HEAD(&ret_buf->tcon_list);
diff --git a/fs/cifs/smb2pdu.c b/fs/cifs/smb2pdu.c
index 54b554c7aee8..1b7ad0c09566 100644
--- a/fs/cifs/smb2pdu.c
+++ b/fs/cifs/smb2pdu.c
@@ -163,7 +163,7 @@  smb2_reconnect(__le16 smb2_command, struct cifs_tcon *tcon,
 		return 0;
 
 	spin_lock(&cifs_tcp_ses_lock);
-	if (tcon->tidStatus == CifsExiting) {
+	if (tcon->status == TID_EXITING) {
 		/*
 		 * only tree disconnect, open, and write,
 		 * (and ulogoff which does not have tcon)
@@ -3860,7 +3860,7 @@  void smb2_reconnect_server(struct work_struct *work)
 		goto done;
 	}
 
-	tcon->tidStatus = CifsGood;
+	tcon->status = TID_GOOD;
 	tcon->retry = false;
 	tcon->need_reconnect = false;
 
-- 
2.32.0