Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Chuck Lever III" <chuck.lever@oracle.com>
Cc:     "Jeff Layton" <jlayton@kernel.org>,
        "Linux NFS Mailing List" <linux-nfs@vger.kernel.org>,
        "Olga Kornievskaia" <kolga@netapp.com>,
        "Dai Ngo" <dai.ngo@oracle.com>, "Tom Talpey" <tom@talpey.com>
Subject: Re: [PATCH 1/6] nfsd: prepare for supporting admin-revocation of state
In-reply-to: <E8637B0D-B026-4835-A8D4-946B9542965B@oracle.com>
References: <20231101010049.27315-1-neilb@suse.de>,
 <20231101010049.27315-2-neilb@suse.de>,
 <E8637B0D-B026-4835-A8D4-946B9542965B@oracle.com>
Date:   Wed, 01 Nov 2023 13:49:52 +1100
Message-id: <169880699287.24305.9894523784673960041@noble.neil.brown.name>
Precedence: bulk

On Wed, 01 Nov 2023, Chuck Lever III wrote:
> 
> > On Oct 31, 2023, at 5:57 PM, NeilBrown <neilb@suse.de> wrote:
> > 
> > The NFSv4 protocol allows state to be revoked by the admin and has error
> > codes which allow this to be communicated to the client.
> > 
> > This patch
> > - introduces 3 new state-id types for revoked open, lock, and
> >   delegation state.  This requires the bitmask to be 'short',
> >   not 'char'
> > - reports NFS4ERR_ADMIN_REVOKED when these are accessed
> > - introduces a per-client counter of these states and returns
> >   SEQ4_STATUS_ADMIN_STATE_REVOKED when the counter is not zero.
> >   Decrement this when freeing any admin-revoked state.
> > - introduces stub code to find all interesting states for a given
> >   superblock so they can be revoked via the 'unlock_filesystem'
> >   file in /proc/fs/nfsd/
> >   No actual states are handled yet.
> > 
> > Signed-off-by: NeilBrown <neilb@suse.de>
> > ---
> > fs/nfsd/nfs4layouts.c |  2 +-
> > fs/nfsd/nfs4state.c   | 93 +++++++++++++++++++++++++++++++++++++++----
> > fs/nfsd/nfsctl.c      |  1 +
> > fs/nfsd/nfsd.h        |  1 +
> > fs/nfsd/state.h       | 35 +++++++++++-----
> > fs/nfsd/trace.h       |  8 +++-
> > 6 files changed, 120 insertions(+), 20 deletions(-)
> 
>  ....
> 
> > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > index f96eaa8e9413..3af5ab55c978 100644
> > --- a/fs/nfsd/state.h
> > +++ b/fs/nfsd/state.h
> > @@ -88,17 +88,23 @@ struct nfsd4_callback_ops {
> >  */
> > struct nfs4_stid {
> > refcount_t sc_count;
> > -#define NFS4_OPEN_STID 1
> > -#define NFS4_LOCK_STID 2
> > -#define NFS4_DELEG_STID 4
> > + struct list_head sc_cp_list;
> > + unsigned short sc_type;
> > +#define NFS4_OPEN_STID BIT(0)
> > +#define NFS4_LOCK_STID BIT(1)
> > +#define NFS4_DELEG_STID BIT(2)
> > /* For an open stateid kept around *only* to process close replays: */
> > -#define NFS4_CLOSED_STID 8
> > +#define NFS4_CLOSED_STID BIT(3)
> > /* For a deleg stateid kept around only to process free_stateid's: */
> > -#define NFS4_REVOKED_DELEG_STID 16
> > -#define NFS4_CLOSED_DELEG_STID 32
> > -#define NFS4_LAYOUT_STID 64
> > - struct list_head sc_cp_list;
> > - unsigned char sc_type;
> > +#define NFS4_REVOKED_DELEG_STID BIT(4)
> > +#define NFS4_CLOSED_DELEG_STID BIT(5)
> > +#define NFS4_LAYOUT_STID BIT(6)
> > +#define NFS4_ADMIN_REVOKED_STID BIT(7)
> > +#define NFS4_ADMIN_REVOKED_LOCK_STID BIT(8)
> > +#define NFS4_ADMIN_REVOKED_DELEG_STID BIT(9)
> > +#define NFS4_ALL_ADMIN_REVOKED_STIDS (NFS4_ADMIN_REVOKED_STID | \
> > +     NFS4_ADMIN_REVOKED_LOCK_STID | \
> > +     NFS4_ADMIN_REVOKED_DELEG_STID)
> > stateid_t sc_stateid;
> > spinlock_t sc_lock;
> > struct nfs4_client *sc_client;
> > 
> > diff --git a/fs/nfsd/trace.h b/fs/nfsd/trace.h
> > index fbc0ccb40424..e359d531402c 100644
> > --- a/fs/nfsd/trace.h
> > +++ b/fs/nfsd/trace.h
> > @@ -648,6 +648,9 @@ TRACE_DEFINE_ENUM(NFS4_CLOSED_STID);
> > TRACE_DEFINE_ENUM(NFS4_REVOKED_DELEG_STID);
> > TRACE_DEFINE_ENUM(NFS4_CLOSED_DELEG_STID);
> > TRACE_DEFINE_ENUM(NFS4_LAYOUT_STID);
> > +TRACE_DEFINE_ENUM(NFS4_ADMIN_REVOKED_STID);
> > +TRACE_DEFINE_ENUM(NFS4_ADMIN_REVOKED_LOCK_STID);
> > +TRACE_DEFINE_ENUM(NFS4_ADMIN_REVOKED_DELEG_STID);
> 
> This is a bug that pre-dates your change in this patch...
> 
> Since the NFS4_ flags are C macros and not enum symbols,
> TRACE_DEFINE_ENUM() is not necessary. All these can be
> removed, rather than adding three new ones.
> 
> I can fix this up when I apply the series, or if you
> happen to send a v3, you can fix it up first.

OK, thanks.  I guess this use of "ENUM" for things that aren't enums
should have been a red flags :-)

NeilBrown

> 
> 
> > #define show_stid_type(x) \
> > __print_flags(x, "|", \
> > @@ -657,7 +660,10 @@ TRACE_DEFINE_ENUM(NFS4_LAYOUT_STID);
> > { NFS4_CLOSED_STID, "CLOSED" }, \
> > { NFS4_REVOKED_DELEG_STID, "REVOKED" }, \
> > { NFS4_CLOSED_DELEG_STID, "CLOSED_DELEG" }, \
> > - { NFS4_LAYOUT_STID, "LAYOUT" })
> > + { NFS4_LAYOUT_STID, "LAYOUT" }, \
> > + { NFS4_ADMIN_REVOKED_STID, "ADMIN_REVOKED" }, \
> > + { NFS4_ADMIN_REVOKED_LOCK_STID, "ADMIN_REVOKED_LOCK" }, \
> > + { NFS4_ADMIN_REVOKED_DELEG_STID,"ADMIN_REVOKED_DELEG" })
> > 
> > DECLARE_EVENT_CLASS(nfsd_stid_class,
> > TP_PROTO(
> > -- 
> > 2.42.0
> > 
> 
> --
> Chuck Lever
> 
> 
>