Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::3:3 as permitted sender) client-ip=2620:137:e000::3:3;
Date:   Fri, 27 Oct 2023 19:34:24 -0400
From:   Chuck Lever <chuck.lever@oracle.com>
To:     NeilBrown <neilb@suse.de>
Cc:     Jeff Layton <jlayton@kernel.org>, 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/6] support admin-revocation of v4 state
Message-ID: <ZTxJAElWAMiGtjF6@tissot.1015granger.net>
References: <20231027015613.26247-1-neilb@suse.de>
 <ZTvaxWodP4rKFkrm@tissot.1015granger.net>
 <169844461904.20306.4942454840443131694@noble.neil.brown.name>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <169844461904.20306.4942454840443131694@noble.neil.brown.name>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?QYtwEz2grOcZTSp/Fa+YHPDJUsYXLta6DkKo4v1qe7L1lhLX6SxVSUeuZNN0?=
 =?us-ascii?Q?YgPp8NL6KuqvfILIzPbpKwjVpMTJCJkdqxSybM7qLMhDXru+1lRQW/n53SjF?=
 =?us-ascii?Q?hHCw4CRhIqS4jHaTTjKOAMDPF4ZzIktYP0i3gaC37D0zWVc1JGExkk1rM0fq?=
 =?us-ascii?Q?Eb6LqzHm/FbFC+k+4gz6mWox9E6R/LS3egFNjyBy/EpLeqjqIBOajpbcpc2V?=
 =?us-ascii?Q?VqSkEe1noYufleSQimMay/mvOskrpEt4Ty5QftfVQB2jbAK3BBbmGRtmho7e?=
 =?us-ascii?Q?FcO5WeHqwPX1pw84dLExh883bqQ7NtHVGvScFLMsvj3Wp21c1lxAKvqHgU4t?=
 =?us-ascii?Q?FJ3JqRwk05D9IUJU19xq7h6sixcoh46ltvCWCBcO2CesPcmL+rnAwoOXawQ7?=
 =?us-ascii?Q?geu5KzalsvmCm/Wc73Zc+C6sx09qIkyyfZHwiz8fUwnaRuMzIblrzsF4zb6y?=
 =?us-ascii?Q?Z7+m2BeG7q6MgKqtW/6yDAD7FT/Y57jJRktv5zLjpGYo+MTN4UTGWp4LYrVf?=
 =?us-ascii?Q?z2bhGkWJU7RVmRkXB4JP47+52JWLxPHXwFsJPH+uiF3pRsLIPtIenbCiNQ/G?=
 =?us-ascii?Q?/g2s26tvRp7GHrd8EchK1fogoGIUgEoxOraC0wWzD6spfT40UHLnqZKqOpAM?=
 =?us-ascii?Q?BZxSNNxVqSUP9+YS4qCnnt7i/0RpESQApCfAe78JgRWAqkcSePQj6IGAKO6p?=
 =?us-ascii?Q?N4rFYDSr2lVq582Bul+UJq2WZz9beLD1MtLBj8nw5FY8YbEee9zEbSPzTPcO?=
 =?us-ascii?Q?inxE70Mt/lJ+2qi8TXX/tR/lO3iZz3sT/WzHCwVsskyiUKmz37jwn+2XDKdu?=
 =?us-ascii?Q?QQwKJaJ06x9LmT7fcCbESVqGS0EC8GpBN+Un5xJO5LMGvYsPCZLa/JiCwW33?=
 =?us-ascii?Q?O+TgAIOGiDhT5oS63pdCtRZ3hfnKYD2CtK/4DH6QaDoP+WnfUJVleiYXg1a9?=
 =?us-ascii?Q?keYnK+m9wnhlMnbFVuiRafLHyBDei2DAFwhK47YXjWYArMPG+AHR/YpF2ViT?=
 =?us-ascii?Q?QqvHpIFunOVVwUoZcixZgLZpWQ/mvRGT0yP+y+KQ6elqDEv9lnh/sJ5xqZXh?=
 =?us-ascii?Q?tWg8rtDtkYyXmPFhWZZ4bjLMsvGMMxlFhgqV6vOney1bi6uZnb9P2BQepiyK?=
 =?us-ascii?Q?3jj6wAQb4nxzSTEtg/iCOSM1oI3lQzuzNRzO5xCAOfIerfyajGGRWvqtxGVf?=
 =?us-ascii?Q?2R9nAOSsAYELNC3mJxi4uAmrJ90fS9FZTt1wJboD09ZQShX3wEclWljIqCQz?=
 =?us-ascii?Q?YPI/tJ6yjvYdBVFyI0wxgWHXf5BeQ86UWP4/b8bf5vLbv0NLT+guUgOuOhIj?=
 =?us-ascii?Q?37WxUfbK8ar3UAkkUKaaBdF/FpiiRV4CSVGsdtGtB5XzM3XKjmD6lE+iaXPE?=
 =?us-ascii?Q?Gi+iFW9QUwG4d+Av1opY1HI1S5g1Zh8Lbfp5sGkP0e6WpRXAT0lNA4hrt4Uk?=
 =?us-ascii?Q?QzxZ50/wz9qVyT0ICtrHBXNF1nIYkaWkkSiL9kMEnc5Hyo/pL3rutwgIZW3l?=
 =?us-ascii?Q?tVwntY5kbdrw5OEsd4a5+fSKe/qGvzgV/q0cAYV9q3kJlQTPsd5Zv92AkqXQ?=
 =?us-ascii?Q?eotUkcpJs3cDOgRpMktkgfqUqPPVjEKz1eFcPcLg?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 6CRTWsjvju5eEN+gKnUrgxMLBJvXk4P/dAxhxTrM9oCW2ufc12kW/DBY5geDdUgrDVh6aCMUdRGapo86otOBDOEBbF7AbRsip62KtWIyUr+oyR0mvf3GmKIjBB1JepQRyQYIj1wPikLojeePsuYIf8xUenD7yHSOv9OWNdC9pGEiyjjnSE/LaR7pk2wjk13pRKo/TQKXKgLLFXzdImmdm0QtnQ3YAept/bL9BQFllDrPDoWjFMM1PtQQ5fpXi2kr2DxFmcUCK88QprUA7ChCQ+QL3FhbOUvZ2W+bN1jEFuJDdmMSCx0s85mQecYGCaAoyhS6PPHW7pUIjLIPOgQQvNKKp0tBBrERUQWB/oB73Lb+zcXmgjY6WYTZefqpDmJwHdmJeydYimHPX7oGoM5ftys4lJaowCarEJldbQtlgcELdpJJnyKJuc+TrPNgjtivqrbtvwaUogjaS1+BdhxbPueF7kpz045v54RG/ZADkgVR8Yu7vdnODZxvZPErWco13rBmBACgybkav/aCdDHwnjmqH0BL2OtZRLkGb+GQh35WsGo/YUZxPGHnhvrAfzIBFdByTuC5FQl91g50oGRO0ODLIZCXbc+0VC9kkAcB0wBN+2x0xjvO1MQEumC8Z3Wyz8GKRr/92SEzZ7/ucCIPUYT0tHPtmKAPet7TjYrPop4lHaAsVFPXZU4IgQkAifQd+yq94bLGBjNr6069baN7Qfdbew8T5oEoVDImjOmcIW5lLgx/gUO39FyzHVhIl6IbgYRjgOQx8xFMbFSFYDB2d9Zle3vBoJo/YAHRAaGDNXKgTvU07wt16dtxbNHGZPc7AZv38k6yu6S94FJz7A32fEMIBuRt+Le3Gl2KMYYH3dfleDbCeooTEL3d7R3pbuIw
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 8a3f8945-01f7-448f-4a43-08dbd74542e9
X-MS-Exchange-CrossTenant-AuthSource: DS7PR10MB5134.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Oct 2023 23:34:28.1761
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: GQinFDXmrLszqbJYsIRwvmLninoo3Wmi/AR+1jL6Y8Pu82PW7Y3N5ecOrlvzgyvXsQBX4mT0gAZqPZDbIraYnQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR10MB7035
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.987,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2023-10-27_22,2023-10-27_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 suspectscore=0 mlxscore=0 adultscore=0
 phishscore=0 spamscore=0 mlxlogscore=523 bulkscore=0 malwarescore=0
 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.12.0-2310240000
 definitions=main-2310270202
X-Proofpoint-ORIG-GUID: c5AkhBFIvqcbi6C4E9DdDIKZwGOqXfA0
X-Proofpoint-GUID: c5AkhBFIvqcbi6C4E9DdDIKZwGOqXfA0
X-Spam-Status: No, score=-0.8 required=5.0 tests=DKIM_SIGNED,DKIM_VALID,
	DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,
	SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no
	version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lipwig.vger.email
Precedence: bulk
List-ID: <linux-nfs.vger.kernel.org>
X-Mailing-List: linux-nfs@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (lipwig.vger.email [0.0.0.0]); Fri, 27 Oct 2023 16:37:50 -0700 (PDT)

On Sat, Oct 28, 2023 at 09:10:19AM +1100, NeilBrown wrote:
> On Sat, 28 Oct 2023, Chuck Lever wrote:
> > On Fri, Oct 27, 2023 at 12:45:28PM +1100, NeilBrown wrote:
> > > This is a revised version of a patch set I sent over a year ago.
> > > It now supports v4.0 and has had more testing.
> > > 
> > > There are cirsumstances where an admin might need to unmount a
> > > filesystem that is NFS-exported and in active use, but does not want to
> > > stop the NFS server completely.  These are certainly unusual
> > > circumstance and doing this might negatively impact any clients acting
> > > on the filesystem, but the admin should be able to do this.
> > > 
> > > Currently this is quite possible for for NFSv3.  Unexporting the
> > > filesystem will ensure no new opens happen, and writing the path name to
> > > /proc/fs/nfsd/unlock_filesystem will ensure anly NLM locks held in the
> > > filesystem are released so that NFSD no longer prevents the filesystem
> > > from being unlocked.
> > > 
> > > It is not currently possible for NFSv4.  Writing to unlock_filesystem
> > > does not affect NFSv4, which is arguably a bug.  This series fixes the bug.
> > 
> > I agree that this is a good thing to do.
> > 
> > However, I'd like to migrate the "unlock_filesystem" functionality
> > to the nfsd netlink protocol first rather than adding this support
> > to /proc/fs/nfsd/. I don't believe that would be a difficult pre-
> > requisite to get through.
> > 
> > Does that seem sensible?
> 
> Not to me.
> 
> This is not new functionality - it is a fix for existing functionality
> which incorrectly ignores NFSv4.

It's arguable whether this counts as new functionality. I can see
both sides of that argument. For now let's agree to call it a fix,
and let's say it would therefore be appropriate for, say, an -rc
pull request (although I think it will land in nfsd-next for v6.8).

So, on first read through these patches, for some reason I had the
impression that you were adding another file under /proc/fs/nfsd.
That's what I would like to avoid at this point.

Changing the values that can appear in one of these files is an
area that is somewhat more grey. I'm willing to be flexible about
that for now.


> When you say "migrate" I hope you mean to add the "unlock_filesystem"
> functionality to netlink, but not remove it from /proc/fs/nfsd for
> several years at least.  I certainly wouldn't want to wait several
> (more) years for this to land.

Naturally. We are playing by the usual rules about kernel-user space
API deprecation. But there's always a fine line about whether an
about-to-be-deprecated API should get new functionality or even bug
fixes.


> However it lands, the interface that it used for NFSv3 should be the
> same as the interface that is used for NFSv4 and I think
> /proc/fs/nfsd/unlock_filesystem should be one such interface until (if
> ever) we discard the /proc/fs/nfsd filesystem.

Fair enough. It wasn't clear to me before that the new state types
described below will not be exposed through the existing
unlock_filesystem interface.

If Jeff agrees, I can pull this into v6.8.


> > > For NFSv4.1 and later code is straight forward.  We add new state types
> > > for admin-revoked state (open, lock, deleg) and change the type of any
> > > state on a filesystem - inavlidating any access and closing files as we
> > > go.  While there are any revoked states we report this to the client in
> > > the response to SEQUENCE requests, and it will check and free any states
> > > that need to be freed.
> > > 
> > > For NFSv4.0 it isn't quite so easy as there is no mechanism for the
> > > client to explicitly acknowledged admin-revoked states.  The approach
> > > this patchset takes is to discard NFSv4.0 admin-revoked states one
> > > lease-time after they were revoked, or immediately for a state that the
> > > client tryies to use and gets an "ADMIN_REVOKED" error for.  If the
> > > filestystem has been unmounted (as expected), the client will see STATE
> > > errors before it has a chance to see ADMIN_REVOKED errors, so most often
> > > the timeout will be how states are discarded.
> > > 
> > > NeilBrown
> > > 
> > >  [PATCH 1/6] nfsd: prepare for supporting admin-revocation of state
> > >  [PATCH 2/6] nfsd: allow admin-revoked state to appear in
> > >  [PATCH 3/6] nfsd: allow admin-revoked NFSv4.0 state to be freed.
> > >  [PATCH 4/6] nfsd: allow lock state ids to be revoked and then freed
> > >  [PATCH 5/6] nfsd: allow open state ids to be revoked and then freed
> > >  [PATCH 6/6] nfsd: allow delegation state ids to be revoked and then
> > > 
> > 
> > -- 
> > Chuck Lever
> > 
> 

-- 
Chuck Lever