On Tue, Jul 18, 2023 at 04:38:08PM +1000, NeilBrown wrote:
> When a sequence of numbers are needed for internal-use only, an enum is
> typically best. The sequence will inevitably need to be changed one
> day, and having an enum means the developer doesn't need to think about
> renumbering after insertion or deletion. The patch will be easier to
> review.
Last sentence needs to define the antecedant of "The patch".
> Signed-off-by: NeilBrown <neilb@xxxxxxx>
> ---
> include/linux/sunrpc/cache.h | 11 +++++++----
> include/linux/sunrpc/svc.h | 34 ++++++++++++++++++++--------------
> include/linux/sunrpc/svc_xprt.h | 39 +++++++++++++++++++++------------------
> include/linux/sunrpc/svcauth.h | 29 ++++++++++++++++-------------
> include/linux/sunrpc/xprtsock.h | 25 +++++++++++++------------
> 5 files changed, 77 insertions(+), 61 deletions(-)
>
> diff --git a/include/linux/sunrpc/cache.h b/include/linux/sunrpc/cache.h
> index 518bd28f5ab8..3cc4f4f0c764 100644
> --- a/include/linux/sunrpc/cache.h
> +++ b/include/linux/sunrpc/cache.h
> @@ -56,10 +56,13 @@ struct cache_head {
> struct kref ref;
> unsigned long flags;
> };
> -#define CACHE_VALID 0 /* Entry contains valid data */
> -#define CACHE_NEGATIVE 1 /* Negative entry - there is no match for the key */
> -#define CACHE_PENDING 2 /* An upcall has been sent but no reply received yet*/
> -#define CACHE_CLEANED 3 /* Entry has been cleaned from cache */
> +/* cache_head.flags */
> +enum {
> + CACHE_VALID, /* Entry contains valid data */
> + CACHE_NEGATIVE, /* Negative entry - there is no match for the key */
> + CACHE_PENDING, /* An upcall has been sent but no reply received yet*/
> + CACHE_CLEANED, /* Entry has been cleaned from cache */
> +};
Weird comment of the day: Please use a double-tab before the comments
to leave room for larger flag names in the future.
> #define CACHE_NEW_EXPIRY 120 /* keep new things pending confirmation for 120 seconds */
>
> diff --git a/include/linux/sunrpc/svc.h b/include/linux/sunrpc/svc.h
> index f3df7f963653..83f31a09c853 100644
> --- a/include/linux/sunrpc/svc.h
> +++ b/include/linux/sunrpc/svc.h
> @@ -31,7 +31,7 @@
> * node traffic on multi-node NUMA NFS servers.
> */
> struct svc_pool {
> - unsigned int sp_id; /* pool id; also node id on NUMA */
> + unsigned int sp_id; /* pool id; also node id on NUMA */
> spinlock_t sp_lock; /* protects all fields */
> struct list_head sp_sockets; /* pending sockets */
> unsigned int sp_nrthreads; /* # of threads in pool */
> @@ -44,12 +44,15 @@ struct svc_pool {
> struct percpu_counter sp_threads_starved;
> struct percpu_counter sp_threads_no_work;
>
> -#define SP_TASK_PENDING (0) /* still work to do even if no
> - * xprt is queued. */
> -#define SP_CONGESTED (1)
> unsigned long sp_flags;
> } ____cacheline_aligned_in_smp;
>
> +/* bits for sp_flags */
> +enum {
> + SP_TASK_PENDING, /* still work to do even if no xprt is queued */
> + SP_CONGESTED, /* all threads are busy, none idle */
> +};
> +
> /*
> * RPC service.
> *
> @@ -232,16 +235,6 @@ struct svc_rqst {
> u32 rq_proc; /* procedure number */
> u32 rq_prot; /* IP protocol */
> int rq_cachetype; /* catering to nfsd */
> -#define RQ_SECURE (0) /* secure port */
> -#define RQ_LOCAL (1) /* local request */
> -#define RQ_USEDEFERRAL (2) /* use deferral */
> -#define RQ_DROPME (3) /* drop current reply */
> -#define RQ_SPLICE_OK (4) /* turned off in gss privacy
> - * to prevent encrypting page
> - * cache pages */
> -#define RQ_VICTIM (5) /* about to be shut down */
> -#define RQ_BUSY (6) /* request is busy */
> -#define RQ_DATA (7) /* request has data */
> unsigned long rq_flags; /* flags field */
> ktime_t rq_qtime; /* enqueue time */
>
> @@ -272,6 +265,19 @@ struct svc_rqst {
> void ** rq_lease_breaker; /* The v4 client breaking a lease */
> };
>
> +/* bits for rq_flags */
> +enum {
> + RQ_SECURE, /* secure port */
> + RQ_LOCAL, /* local request */
> + RQ_USEDEFERRAL, /* use deferral */
> + RQ_DROPME, /* drop current reply */
> + RQ_SPLICE_OK, /* turned off in gss privacy to prevent encrypting page
> + * cache pages */
> + RQ_VICTIM, /* about to be shut down */
> + RQ_BUSY, /* request is busy */
> + RQ_DATA, /* request has data */
> +};
> +
Also here -- two tab stops instead of one.
> #define SVC_NET(rqst) (rqst->rq_xprt ? rqst->rq_xprt->xpt_net : rqst->rq_bc_net)
>
> /*
> diff --git a/include/linux/sunrpc/svc_xprt.h b/include/linux/sunrpc/svc_xprt.h
> index a6b12631db21..af383d0349b3 100644
> --- a/include/linux/sunrpc/svc_xprt.h
> +++ b/include/linux/sunrpc/svc_xprt.h
> @@ -56,26 +56,9 @@ struct svc_xprt {
> struct list_head xpt_list;
> struct list_head xpt_ready;
> unsigned long xpt_flags;
> -#define XPT_BUSY 0 /* enqueued/receiving */
> -#define XPT_CONN 1 /* conn pending */
> -#define XPT_CLOSE 2 /* dead or dying */
> -#define XPT_DATA 3 /* data pending */
> -#define XPT_TEMP 4 /* connected transport */
> -#define XPT_DEAD 6 /* transport closed */
> -#define XPT_CHNGBUF 7 /* need to change snd/rcv buf sizes */
> -#define XPT_DEFERRED 8 /* deferred request pending */
> -#define XPT_OLD 9 /* used for xprt aging mark+sweep */
> -#define XPT_LISTENER 10 /* listening endpoint */
> -#define XPT_CACHE_AUTH 11 /* cache auth info */
> -#define XPT_LOCAL 12 /* connection from loopback interface */
> -#define XPT_KILL_TEMP 13 /* call xpo_kill_temp_xprt before closing */
> -#define XPT_CONG_CTRL 14 /* has congestion control */
> -#define XPT_HANDSHAKE 15 /* xprt requests a handshake */
> -#define XPT_TLS_SESSION 16 /* transport-layer security established */
> -#define XPT_PEER_AUTH 17 /* peer has been authenticated */
>
> struct svc_serv *xpt_server; /* service for transport */
> - atomic_t xpt_reserved; /* space on outq that is rsvd */
> + atomic_t xpt_reserved; /* space on outq that is rsvd */
> atomic_t xpt_nr_rqsts; /* Number of requests */
> struct mutex xpt_mutex; /* to serialize sending data */
> spinlock_t xpt_lock; /* protects sk_deferred
> @@ -96,6 +79,26 @@ struct svc_xprt {
> struct rpc_xprt *xpt_bc_xprt; /* NFSv4.1 backchannel */
> struct rpc_xprt_switch *xpt_bc_xps; /* NFSv4.1 backchannel */
> };
> +/* flag bits for xpt_flags */
> +enum {
> + XPT_BUSY, /* enqueued/receiving */
> + XPT_CONN, /* conn pending */
> + XPT_CLOSE, /* dead or dying */
> + XPT_DATA, /* data pending */
> + XPT_TEMP, /* connected transport */
> + XPT_DEAD, /* transport closed */
> + XPT_CHNGBUF, /* need to change snd/rcv buf sizes */
> + XPT_DEFERRED, /* deferred request pending */
> + XPT_OLD, /* used for xprt aging mark+sweep */
> + XPT_LISTENER, /* listening endpoint */
> + XPT_CACHE_AUTH, /* cache auth info */
> + XPT_LOCAL, /* connection from loopback interface */
> + XPT_KILL_TEMP, /* call xpo_kill_temp_xprt before closing */
> + XPT_CONG_CTRL, /* has congestion control */
> + XPT_HANDSHAKE, /* xprt requests a handshake */
> + XPT_TLS_SESSION, /* transport-layer security established */
> + XPT_PEER_AUTH, /* peer has been authenticated */
> +};
>
> static inline void unregister_xpt_user(struct svc_xprt *xpt, struct svc_xpt_user *u)
> {
> diff --git a/include/linux/sunrpc/svcauth.h b/include/linux/sunrpc/svcauth.h
> index 6d9cc9080aca..8d1d0d0721d2 100644
> --- a/include/linux/sunrpc/svcauth.h
> +++ b/include/linux/sunrpc/svcauth.h
> @@ -133,19 +133,22 @@ struct auth_ops {
> int (*set_client)(struct svc_rqst *rq);
> };
>
> -#define SVC_GARBAGE 1
> -#define SVC_SYSERR 2
> -#define SVC_VALID 3
> -#define SVC_NEGATIVE 4
> -#define SVC_OK 5
> -#define SVC_DROP 6
> -#define SVC_CLOSE 7 /* Like SVC_DROP, but request is definitely
> - * lost so if there is a tcp connection, it
> - * should be closed
> - */
> -#define SVC_DENIED 8
> -#define SVC_PENDING 9
> -#define SVC_COMPLETE 10
> +/*return values for svc functions that analyse request */
> +enum {
> + SVC_GARBAGE,
> + SVC_SYSERR,
> + SVC_VALID,
> + SVC_NEGATIVE,
> + SVC_OK,
> + SVC_DROP,
> + SVC_CLOSE, /* Like SVC_DROP, but request is definitely
> + * lost so if there is a tcp connection, it
> + * should be closed
> + */
> + SVC_DENIED,
> + SVC_PENDING,
> + SVC_COMPLETE,
> +};
>
> struct svc_xprt;
>
> diff --git a/include/linux/sunrpc/xprtsock.h b/include/linux/sunrpc/xprtsock.h
> index 700a1e6c047c..1ed2f446010b 100644
> --- a/include/linux/sunrpc/xprtsock.h
> +++ b/include/linux/sunrpc/xprtsock.h
> @@ -81,17 +81,18 @@ struct sock_xprt {
> };
>
> /*
> - * TCP RPC flags
> + * TCP RPC flags in ->sock_state
> */
> -#define XPRT_SOCK_CONNECTING 1U
> -#define XPRT_SOCK_DATA_READY (2)
> -#define XPRT_SOCK_UPD_TIMEOUT (3)
> -#define XPRT_SOCK_WAKE_ERROR (4)
> -#define XPRT_SOCK_WAKE_WRITE (5)
> -#define XPRT_SOCK_WAKE_PENDING (6)
> -#define XPRT_SOCK_WAKE_DISCONNECT (7)
> -#define XPRT_SOCK_CONNECT_SENT (8)
> -#define XPRT_SOCK_NOSPACE (9)
> -#define XPRT_SOCK_IGNORE_RECV (10)
> -
> +enum {
> + XPRT_SOCK_CONNECTING,
> + XPRT_SOCK_DATA_READY,
> + XPRT_SOCK_UPD_TIMEOUT,
> + XPRT_SOCK_WAKE_ERROR,
> + XPRT_SOCK_WAKE_WRITE,
> + XPRT_SOCK_WAKE_PENDING,
> + XPRT_SOCK_WAKE_DISCONNECT,
> + XPRT_SOCK_CONNECT_SENT,
> + XPRT_SOCK_NOSPACE,
> + XPRT_SOCK_IGNORE_RECV,
> +};
> #endif /* _LINUX_SUNRPC_XPRTSOCK_H */
>
>
Let's not change client-side code in this patch. Please split this
hunk out and send it to Trond and Anna separately.
