Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
From:   "NeilBrown" <neilb@suse.de>
To:     "Jeff Layton" <jlayton@kernel.org>
Cc:     "Chuck Lever" <chuck.lever@oracle.com>, linux-nfs@vger.kernel.org,
        "Olga Kornievskaia" <kolga@netapp.com>,
        "Dai Ngo" <Dai.Ngo@oracle.com>, "Tom Talpey" <tom@talpey.com>
Subject: Re: [PATCH 0/9 v3] support admin-revocation of v4 state
In-reply-to: <9e18d992cdccf56d26482adc8c1c4a6012c68f8e.camel@kernel.org>
References: <20231117022121.23310-1-neilb@suse.de>,
 <9e18d992cdccf56d26482adc8c1c4a6012c68f8e.camel@kernel.org>
Date:   Mon, 20 Nov 2023 10:21:49 +1100
Message-id: <170043610989.19300.5572465091839523991@noble.neil.brown.name>
Precedence: bulk

On Fri, 17 Nov 2023, Jeff Layton wrote:
> On Fri, 2023-11-17 at 13:18 +1100, NeilBrown wrote:
> > This set adds a prequel series to my previous posting which addresses
> > some locking problems around ->sc_type and splits that field into
> > to separate fields: sc_type and sc_status.
> > The recovation code is modified to accomodate these changed.
> > 
> > Thanks
> > NeilBrown
> > 
> >  [PATCH 1/9] nfsd: hold ->cl_lock for hash_delegation_locked()
> >  [PATCH 2/9] nfsd: avoid race after unhash_delegation_locked()
> >  [PATCH 3/9] nfsd: split sc_status out of sc_type
> >  [PATCH 4/9] nfsd: prepare for supporting admin-revocation of state
> >  [PATCH 5/9] nfsd: allow admin-revoked state to appear in
> >  [PATCH 6/9] nfsd: allow admin-revoked NFSv4.0 state to be freed.
> >  [PATCH 7/9] nfsd: allow lock state ids to be revoked and then freed
> >  [PATCH 8/9] nfsd: allow open state ids to be revoked and then freed
> >  [PATCH 9/9] nfsd: allow delegation state ids to be revoked and then
> > 
> 
> Nice set. I like this overall. One (other) question: do we need to add
> handling for revoking layout stateids as well?

I guess so... I don't give much thought to layout stateids.
They are used in LAYOUTGET LAYOUTCOMMIT LAYOUTRETURN.
(it's seems odd that they aren't used in READ/WRITE....)

I guess we need to drop the ref on ->ls_file and maybe unlock the vfs
lease....

I wonder if I should just use ->sc_free for that.

Maybe ->sc_free could take a 'revoke_only' arg which causes it to free
resources but not free the stateid itself.
Maybe that would make the code cleaner.

NeilBrown


> 
> I'll plan to pull this into my local branch for some testing over the
> weekend.
> -- 
> Jeff Layton <jlayton@kernel.org>
>