On Tue, Jul 18, 2023 at 09:37:58AM -0400, Chuck Lever wrote:
> 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".
I've changed the last sentence in the description to "Such patches
will be easier ..."
I've applied 1/5 through 5/5, with a few cosmetic changes, to the
SUNRPC threads topic branch. 6/6 needed more work:
I've split this into one patch for each new enum.
The XPT_ flags change needed TRACE_DEFINE_ENUM macros to make
show_svc_xprt_flags() work properly. Added.
The new enum for SVC_GARBAGE and friends... those aren't bit flags.
So I've made that a named enum so that it can be used for type-
checking function return values properly.
I dropped the hunk that changes XPRT_SOCK_CONNECTING and friends.
That should be submitted separately to Anna and Trond.
All this will appear in the nfsd repo later today.
> > 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.
>
--
Chuck Lever
