From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mail-oa1-f54.google.com (mail-oa1-f54.google.com [209.85.160.54]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6F5D2401A36 for ; Fri, 27 Mar 2026 18:22:45 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.85.160.54 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774635769; cv=none; b=FEdP8LeWlRF3atEiI0hTY6Lu8cGpukzB15SA4QdcpsFRN+SSheYIFvJ0PeGIWam5qsnHqDc4yFHW/OttGtgitSWg01Zg2k68y+P210WpOt7cahOphxj9vXSpoApDkKN7lPbl4DSe6decEERAgIgZdDVHQ+6SkDTeWES0D3VWzkk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1774635769; c=relaxed/simple; bh=Ce+JeBdKPngkz03e9C0BLp1VEvPo2cdoJJHu0LbAVaY=; h=From:Date:To:Cc:Subject:Message-ID:References:MIME-Version: Content-Type:Content-Disposition:In-Reply-To; b=EQo4zfuBd2d49U9hliizoCr/11opo2Wkeisj4VSlsJMFAJ42qo81b1nM4lo3uvfmPj9PqIjMMMl7FXBdyhKbLtkn9glVYEyLshr/3l3jssYydfOgqWtohb4hfLi32WOQAhAGRuqH+bpNas/5BuKTs6cwhvKNmivnXQDmWSx9hgE= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com; spf=pass smtp.mailfrom=gmail.com; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b=aiedcPOe; arc=none smtp.client-ip=209.85.160.54 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=gmail.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="aiedcPOe" Received: by mail-oa1-f54.google.com with SMTP id 586e51a60fabf-40ea36b56b7so1945983fac.3 for ; Fri, 27 Mar 2026 11:22:45 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20251104; t=1774635764; x=1775240564; darn=vger.kernel.org; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:date:from:from:to :cc:subject:date:message-id:reply-to; bh=T7n7hKZ8FOeCaaapHn5s1aqBX5jq6R1SR85ZlaoOO7A=; b=aiedcPOe2YF4iGhN1HF0Itb1a4cmgma4ge/1M6gKa9mlxRrhG3IxrsUnbxXd3mdBc5 WY5rSOFuaMwyuo1GlbXCmZTXTVSn/Q/faRIP8k1/R1BaTKUG/l5DkHi+4PrzJDIcUlb9 GKjHpQtjuO9bdvVl2jSNED6yMwviBX1nXgYkevlhyD8tr8JfmYuMcWnwWtbw5CXiXSjj 9dpTW7S2kZoYUP8ltCBsqEu9ln6mDm76xLQuxyGERq2oPV+IvmB8nQBEPg7CLnUqYjrY IksorMUoJqK8biQvA6a4UIITNzSVhGjmNgiF5tG5CjirX8VUAZyweFQsVNhTyW4CZi8C Femw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1774635764; x=1775240564; h=in-reply-to:content-transfer-encoding:content-disposition :mime-version:references:message-id:subject:cc:to:date:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=T7n7hKZ8FOeCaaapHn5s1aqBX5jq6R1SR85ZlaoOO7A=; b=p3aRLU5Y48M0ELkYX/yJC/teso2/WqQVbTHFj4t899aLv25YVZMtfDfsQ2LuRy0c/e S+6y6w4EEEFdDTaV5clGU5u2cx5eDg7YmG6rpPovx1Bq+EgioZCaPDWMY7bjOZ43g0/e DYXDV4E9GYCGgFNc8nsU0iBpKoBhM/sz9Tr38uxQ+quSrnc3l69SX7ExmO2RhGwXN6Tj DhVChGAg/j3eibNAIGbxmjs5ma5GzcRGQ+vqw84sMXYCypN0dI9ayXFCBQkMtmafD0tQ RzjVjjWKuExGalo7HAfsdjS28/yNRjo5caxbU/1Q8Q+y2CLigsPlpJ8CMKk+dO3p11k1 nK3w== X-Forwarded-Encrypted: i=1; AJvYcCVAkhBwa9/pr35WTheoyUC5Jb8pef3l7Pqk/4W5EpuQRFo8m9+698fzVjwetSl3nM87Vjv82DsLeCK6C40=@vger.kernel.org X-Gm-Message-State: AOJu0YyIuiSrBvcs6PzkFkDuKPaE18qHYJx45WBhcGndRbpklQz5igGM IQSvjeS5YkjCFCSgWMp17e+0fegnfcIlerWvXQEvDJ18w39iUlY8i4aD X-Gm-Gg: ATEYQzwkRgU+chZEDW1wasG4tenk50jcfSkK1c2ZBg5rkJvJDVywYt4xJQcZDkq3KLh 6TEh3jCEv1ToyyEebgG/VTmqLv+qOfAaFA+ZyzUDrdTjpreD0HyR4+wuwLmReCLvMWyWVyYUEyS L6FEKsEuEsbrOBtnncn2P3jUvqhjfWm+6DsjS09BGqDNyiRM2XycEnGTn94zam1e9qUzqJOPm3p Bkub9uOjint/y3OiCPclvlgqtb5UBPKxJSE9XyPWMtC3ldeX83ctRoPgszwf3Po1F3tM0zkGKWq BnNQCa/HLteIdsczoMT3CC/Ad6TZVJyQGnlfAfGtW72RAdFwW6dbDWG7dE6LIyRRMbd8t6p1yUr UdPTBPj9Wx1P0bio/vrLqZFzKI68FGJkvzCFvJMayNtWbdaoXsh7LvDJwZnUz37WeEYVl2FNPXR bLIgqtboWpri+LMHdmfCjWopNYTlESWx6DY4yxrwXb9iFhmNEuwGrQoaN6Z00/UFpR4tXHopo4T Y1slCl6bw== X-Received: by 2002:a05:6870:b620:b0:40e:e0b2:e375 with SMTP id 586e51a60fabf-41cec2ca743mr1462914fac.31.1774635764042; Fri, 27 Mar 2026 11:22:44 -0700 (PDT) Received: from localhost (c-174-160-87-152.hsd1.ca.comcast.net. [174.160.87.152]) by smtp.gmail.com with ESMTPSA id 586e51a60fabf-41d049523a6sm50122fac.6.2026.03.27.11.22.41 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 27 Mar 2026 11:22:42 -0700 (PDT) From: Thomas Haynes X-Google-Original-From: Thomas Haynes Date: Fri, 27 Mar 2026 11:22:39 -0700 To: Jeff Layton Cc: Olga Kornievskaia , Thomas Haynes , Trond Myklebust , Anna Schumaker , linux-nfs@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2 2/2] nfs: update inode ctime after removexattr operation Message-ID: References: <20260324-nfs-7-1-v2-0-d110da3c0036@kernel.org> <20260324-nfs-7-1-v2-2-d110da3c0036@kernel.org> <284ca17e74af8c4f5942b2952f2bf75490dd17c0.camel@kernel.org> <80b423c66dba84b46be1084307d2c66b935065bc.camel@kernel.org> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <80b423c66dba84b46be1084307d2c66b935065bc.camel@kernel.org> On Fri, Mar 27, 2026 at 12:59:54PM -0800, Jeff Layton wrote: > On Fri, 2026-03-27 at 12:20 -0400, Olga Kornievskaia wrote: > > On Fri, Mar 27, 2026 at 11:50 AM Jeff Layton wrote: > > > > > > On Fri, 2026-03-27 at 11:11 -0400, Olga Kornievskaia wrote: > > > > On Tue, Mar 24, 2026 at 1:32 PM Jeff Layton wrote: > > > > > > > > > > xfstest generic/728 fails with delegated timestamps. The client does a > > > > > removexattr and then a stat to test the ctime, which doesn't change. The > > > > > stat() doesn't trigger a GETATTR because of the delegated timestamps, so > > > > > it relies on the cached ctime, which is wrong. > > > > > > > > > > The setxattr compound has a trailing GETATTR, which ensures that its > > > > > ctime gets updated. Follow the same strategy with removexattr. > > > > > > > > This approach relies on the fact that the server the serves delegated > > > > attributes would update change_attr on operations which might now > > > > necessarily happen (ie, linux server does not update change_attribute > > > > on writes or clone). I propose an alternative fix for the failing > > > > generic/728. > > > > > > > > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c > > > > index 7b3ca68fb4bb..ede1835a45b3 100644 > > > > --- a/fs/nfs/nfs42proc.c > > > > +++ b/fs/nfs/nfs42proc.c > > > > @@ -1389,7 +1389,13 @@ static int _nfs42_proc_removexattr(struct inode > > > > *inode, const char *name) > > > > &res.seq_res, 1); > > > > trace_nfs4_removexattr(inode, name, ret); > > > > if (!ret) > > > > - nfs4_update_changeattr(inode, &res.cinfo, timestamp, 0); > > > > + if (nfs_have_delegated_attributes(inode)) { > > > > + nfs_update_delegated_mtime(inode); > > > > + spin_lock(&inode->i_lock); > > > > + nfs_set_cache_invalid(inode, NFS_INO_INVALID_BLOCKS); > > > > + spin_unlock(&inode->i_lock); > > > > + } else > > > > + nfs4_update_changeattr(inode, &res.cinfo, timestamp, 0); > > > > > > > > return ret; > > > > } > > > > > > > > > > What's the advantage of doing it this way? > > > > > > You just sent a REMOVEXATTR operation to the server that will change > > > the mtime there. The server has the most up-to-date version of the > > > mtime and ctime at that point. > > > > In presence of delegated attributes, Is the server required to update > > its mtime/ctime on an operation? As I mentioned, the linux server does > > not update its ctime/mtime for WRITE, CLONE, COPY. > > > > Is possible that > > some implementations might be different and also do not update the > > ctime/mtime on REMOVEXATTR? > > > > Therefore I was suggesting that the patch > > relies on the fact that it would receive an updated value. Of course > > perhaps all implementations are done the same as the linux server and > > my point is moot. I didn't see anything in the spec that clarifies > > what the server supposed to do (and client rely on). > > > > (cc'ing Tom) > > That is a very good point. > > My interpretation was that delegated timestamps generally covered > writes, but SETATTR style operations that do anything beyond only > changing the mtime can't be cached. > > We probably need some delstid spec clarification: for what operations > is the server required to disable timestamp updates when a write > delegation is outstanding? > > In the case of nfsd, we disable timestamp updates for WRITE/COPY/CLONE > but not SETATTR/SETXATTR/REMOVEXATTR. > > How does the Hammerspace anvil behave? Does it disable c/mtime updates > for writes when there is an outstanding timestamp delegation like we're > doing in nfsd? If so, does it do the same for > SETATTR/SETXATTR/REMOVEXATTR operations as well? Jeff, I think the right way to look at this is closer to how size is handled under delegation in RFC8881, rather than as a per-op rule. In our implementation, because we are acting as an MDS and data I/O goes to DSes, we already treat size as effectively delegated when a write layout is outstanding. The MDS does not maintain authoritative size locally in that case. We may refresh size/timestamps internally (e.g., on GETATTR by querying DSes), but we don’t treat that as overriding the delegated authority. For timestamps, our behavior is effectively the same model. When the client holds the relevant delegation, the server does not consider itself authoritative for ctime/mtime. If current values are needed, we can obtain them from the client (e.g., via CB_GETATTR), and the client must present the delegation stateid to demonstrate that authority. So the authority follows the delegation, not the specific operation. That said, I don’t think we’ve fully resolved the semantics for all metadata-style ops either. WRITE and SETATTR are clear in our model, but for things like CLONE/COPY/SETXATTR/REMOVEXATTR, we’ve likely been relying on assumptions rather than a fully consistent rule. I.e., CLONE and COPY we just pass through to the DS and we don't implement SETXATTR/REMOVEXATTR. So the spec question, as I see it, is not whether REMOVEXATTR (or any particular op) should update ctime/mtime, but whether delegated timestamps are meant to follow the same attribute-authority model as delegated size in RFC8881. If so, then we expect that the server should query the client via CB_GETATTR to return updated ctime/mtime after such operations while the delegation is outstanding. Thanks, Tom > > > > > > It's certainly possible that the REMOVEXATTR is the only change that > > > occurred. With what I'm proposing, we don't even need to do a SETATTR > > > at all if nothing else changed. With your version, you would. > > > > > > > > > > > > > Fixes: 3e1f02123fba ("NFSv4.2: add client side XDR handling for extended attributes") > > > > > Reported-by: Olga Kornievskaia > > > > > Signed-off-by: Jeff Layton > > > > > --- > > > > > fs/nfs/nfs42proc.c | 18 ++++++++++++++++-- > > > > > fs/nfs/nfs42xdr.c | 10 ++++++++-- > > > > > include/linux/nfs_xdr.h | 3 +++ > > > > > 3 files changed, 27 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c > > > > > index 7b3ca68fb4bb3bee293f8621e5ed5fa596f5da3f..7e5c1172fc11c9d5a55b3621977ac83bb98f7c20 100644 > > > > > --- a/fs/nfs/nfs42proc.c > > > > > +++ b/fs/nfs/nfs42proc.c > > > > > @@ -1372,11 +1372,15 @@ int nfs42_proc_clone(struct file *src_f, struct file *dst_f, > > > > > static int _nfs42_proc_removexattr(struct inode *inode, const char *name) > > > > > { > > > > > struct nfs_server *server = NFS_SERVER(inode); > > > > > + __u32 bitmask[NFS_BITMASK_SZ]; > > > > > struct nfs42_removexattrargs args = { > > > > > .fh = NFS_FH(inode), > > > > > + .bitmask = bitmask, > > > > > .xattr_name = name, > > > > > }; > > > > > - struct nfs42_removexattrres res; > > > > > + struct nfs42_removexattrres res = { > > > > > + .server = server, > > > > > + }; > > > > > struct rpc_message msg = { > > > > > .rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_REMOVEXATTR], > > > > > .rpc_argp = &args, > > > > > @@ -1385,12 +1389,22 @@ static int _nfs42_proc_removexattr(struct inode *inode, const char *name) > > > > > int ret; > > > > > unsigned long timestamp = jiffies; > > > > > > > > > > + res.fattr = nfs_alloc_fattr(); > > > > > + if (!res.fattr) > > > > > + return -ENOMEM; > > > > > + > > > > > + nfs4_bitmask_set(bitmask, server->cache_consistency_bitmask, > > > > > + inode, NFS_INO_INVALID_CHANGE); > > > > > + > > > > > ret = nfs4_call_sync(server->client, server, &msg, &args.seq_args, > > > > > &res.seq_res, 1); > > > > > trace_nfs4_removexattr(inode, name, ret); > > > > > - if (!ret) > > > > > + if (!ret) { > > > > > nfs4_update_changeattr(inode, &res.cinfo, timestamp, 0); > > > > > + ret = nfs_post_op_update_inode(inode, res.fattr); > > > > > + } > > > > > > > > > > + kfree(res.fattr); > > > > > return ret; > > > > > } > > > > > > > > > > diff --git a/fs/nfs/nfs42xdr.c b/fs/nfs/nfs42xdr.c > > > > > index 5c7452ce6e8ac94bd24bc3a33d4479d458a29907..ec105c62f721cfe01bfc60f5981396958084d627 100644 > > > > > --- a/fs/nfs/nfs42xdr.c > > > > > +++ b/fs/nfs/nfs42xdr.c > > > > > @@ -263,11 +263,13 @@ > > > > > #define NFS4_enc_removexattr_sz (compound_encode_hdr_maxsz + \ > > > > > encode_sequence_maxsz + \ > > > > > encode_putfh_maxsz + \ > > > > > - encode_removexattr_maxsz) > > > > > + encode_removexattr_maxsz + \ > > > > > + encode_getattr_maxsz) > > > > > #define NFS4_dec_removexattr_sz (compound_decode_hdr_maxsz + \ > > > > > decode_sequence_maxsz + \ > > > > > decode_putfh_maxsz + \ > > > > > - decode_removexattr_maxsz) > > > > > + decode_removexattr_maxsz + \ > > > > > + decode_getattr_maxsz) > > > > > > > > > > /* > > > > > * These values specify the maximum amount of data that is not > > > > > @@ -869,6 +871,7 @@ static void nfs4_xdr_enc_removexattr(struct rpc_rqst *req, > > > > > encode_sequence(xdr, &args->seq_args, &hdr); > > > > > encode_putfh(xdr, args->fh, &hdr); > > > > > encode_removexattr(xdr, args->xattr_name, &hdr); > > > > > + encode_getfattr(xdr, args->bitmask, &hdr); > > > > > encode_nops(&hdr); > > > > > } > > > > > > > > > > @@ -1818,6 +1821,9 @@ static int nfs4_xdr_dec_removexattr(struct rpc_rqst *req, > > > > > goto out; > > > > > > > > > > status = decode_removexattr(xdr, &res->cinfo); > > > > > + if (status) > > > > > + goto out; > > > > > + status = decode_getfattr(xdr, res->fattr, res->server); > > > > > out: > > > > > return status; > > > > > } > > > > > diff --git a/include/linux/nfs_xdr.h b/include/linux/nfs_xdr.h > > > > > index ff1f12aa73d27b6fd874baf7019dd03195fc36e5..fcbd21b5685f46136a210c8e11c20a54d6ed9dad 100644 > > > > > --- a/include/linux/nfs_xdr.h > > > > > +++ b/include/linux/nfs_xdr.h > > > > > @@ -1611,12 +1611,15 @@ struct nfs42_listxattrsres { > > > > > struct nfs42_removexattrargs { > > > > > struct nfs4_sequence_args seq_args; > > > > > struct nfs_fh *fh; > > > > > + const u32 *bitmask; > > > > > const char *xattr_name; > > > > > }; > > > > > > > > > > struct nfs42_removexattrres { > > > > > struct nfs4_sequence_res seq_res; > > > > > struct nfs4_change_info cinfo; > > > > > + struct nfs_fattr *fattr; > > > > > + const struct nfs_server *server; > > > > > }; > > > > > > > > > > #endif /* CONFIG_NFS_V4_2 */ > > > > > > > > > > -- > > > > > 2.53.0 > > > > > > > > > > > -- > > > Jeff Layton > > -- > Jeff Layton