From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id AB2B5C7EE23 for ; Tue, 23 May 2023 10:02:45 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 466A76B0074; Tue, 23 May 2023 06:02:45 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4167F6B0075; Tue, 23 May 2023 06:02:45 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 305AB900002; Tue, 23 May 2023 06:02:45 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0015.hostedemail.com [216.40.44.15]) by kanga.kvack.org (Postfix) with ESMTP id 1FD026B0074 for ; Tue, 23 May 2023 06:02:45 -0400 (EDT) Received: from smtpin09.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay05.hostedemail.com (Postfix) with ESMTP id E42D8406B9 for ; Tue, 23 May 2023 10:02:44 +0000 (UTC) X-FDA: 80821080648.09.5BDAC2F Received: from smtp-out2.suse.de (smtp-out2.suse.de [195.135.220.29]) by imf17.hostedemail.com (Postfix) with ESMTP id B9C384000A for ; Tue, 23 May 2023 10:02:42 +0000 (UTC) Authentication-Results: imf17.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=UQkTsQu7; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=CWIwKCxO; spf=pass (imf17.hostedemail.com: domain of jack@suse.cz designates 195.135.220.29 as permitted sender) smtp.mailfrom=jack@suse.cz; dmarc=none ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1684836163; a=rsa-sha256; cv=none; b=2uX/VQ6nozGCnZ6Ow509M4R5XXlYViZW3ZrE1o6K0tKOR155zxEEEFsHZHQm6YDlUpyxeP 5yjb3omrR6rmk6u6leHsBPumIlEfKj4OhXe2bcoiYTp7q01Mzc32bJvLz64pavkrx7EZQg s80kx2z+BWmxgGbAWNHP29yVsiQxkao= ARC-Authentication-Results: i=1; imf17.hostedemail.com; dkim=pass header.d=suse.cz header.s=susede2_rsa header.b=UQkTsQu7; dkim=pass header.d=suse.cz header.s=susede2_ed25519 header.b=CWIwKCxO; spf=pass (imf17.hostedemail.com: domain of jack@suse.cz designates 195.135.220.29 as permitted sender) smtp.mailfrom=jack@suse.cz; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1684836163; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=6C4JQXjcp8+xqNPx3JTb2B5ixIfG+Z8YxMqNpQGherU=; b=DytgzA2fwsW7XDbd8tw0sTAZhCBRZBynacqIEUJ94oUB3aRBbgF+yGaaZK6iF9IA91BhEp cPOVvAuzHYwdn9zEP85G06BMLrvkcePXLEcaq8bSF10t5Lz+QbNQM6pkILyg+7eq5LNFg4 JnH/eSvP96MXmiKNE64ieL7fXXA/ctY= Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by smtp-out2.suse.de (Postfix) with ESMTPS id 32D0220355; Tue, 23 May 2023 10:02:41 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_rsa; t=1684836161; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=6C4JQXjcp8+xqNPx3JTb2B5ixIfG+Z8YxMqNpQGherU=; b=UQkTsQu7xiV+E65Co+o0xcYshZRBebuGbcp2/gMLqn5xrzkHKwvzV9/ONgdLnAYLtBPkKF dhKDn+4ZKn3e+TbHGUeHJBICVIw+CqNPwJv1lWdX04/xr72IEdvgCfA6o5SacXlcP/D24K tFU7nzMS3GX2ymWgl+8LyFvxgVi//p8= DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=suse.cz; s=susede2_ed25519; t=1684836161; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=6C4JQXjcp8+xqNPx3JTb2B5ixIfG+Z8YxMqNpQGherU=; b=CWIwKCxOLkfZhMbk+fg/CJ+su0xIgrFAvWIdMkADnj0GFoRZfq8sleU0kJ0ZOu0qudjyKd cbc8DuFaflpRWaAQ== Received: from imap2.suse-dmz.suse.de (imap2.suse-dmz.suse.de [192.168.254.74]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-521) server-digest SHA512) (No client certificate requested) by imap2.suse-dmz.suse.de (Postfix) with ESMTPS id 1C66813588; Tue, 23 May 2023 10:02:41 +0000 (UTC) Received: from dovecot-director2.suse.de ([192.168.254.65]) by imap2.suse-dmz.suse.de with ESMTPSA id xK3iBkGPbGR4ZQAAMHmgww (envelope-from ); Tue, 23 May 2023 10:02:41 +0000 Received: by quack3.suse.cz (Postfix, from userid 1000) id 9D000A075D; Tue, 23 May 2023 12:02:40 +0200 (CEST) Date: Tue, 23 May 2023 12:02:40 +0200 From: Jan Kara To: Jeff Layton Cc: Alexander Viro , Christian Brauner , "Darrick J. Wong" , Hugh Dickins , Andrew Morton , Dave Chinner , Chuck Lever , Jan Kara , Amir Goldstein , David Howells , Neil Brown , Matthew Wilcox , Andreas Dilger , Theodore T'so , Chris Mason , Josef Bacik , David Sterba , Namjae Jeon , Steve French , Sergey Senozhatsky , Tom Talpey , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-xfs@vger.kernel.org, linux-btrfs@vger.kernel.org, linux-ext4@vger.kernel.org, linux-mm@kvack.org, linux-nfs@vger.kernel.org, linux-cifs@vger.kernel.org Subject: Re: [PATCH v4 2/9] fs: add infrastructure for multigrain inode i_m/ctime Message-ID: <20230523100240.mgeu4y46friv7hau@quack3> References: <20230518114742.128950-1-jlayton@kernel.org> <20230518114742.128950-3-jlayton@kernel.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20230518114742.128950-3-jlayton@kernel.org> X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: B9C384000A X-Stat-Signature: ucpq4ampe179w73cqbj6kyy6h1k6w3up X-Rspam-User: X-HE-Tag: 1684836162-991136 X-HE-Meta: U2FsdGVkX19bbVTYlt3HuyBXSFknmjc2NqphmyRcDRGyHjtAWQzQvNdOeYQBemIhxUddjlFkl8sPmVVaoVziNUgKWgC/6VU6G9S0g0n0VuuFPFOkcMuWSCYM1ltC4esVWf7LC/DbGcxw16KPQy/XKxIOf94//DZPh+UmIxGEeq2VrQWQN7gHvGQXyicHtvTN5+7t5JxtSO8Lfpdesfxw+hmVIo+RzgeEUdKuq0lU+bJIls4RpZKHdgiv/DcgNtGm2xNe1royK6BsDrYC39gxltofynGfmSZhH4jAgyViSzx5W5XSTJLdmOYPpKIymPqAzuCYpWJuPIrnQZmLnXtMyz38zxF/xn909bGBs97uJdpIOBE2rQpeVh88cV+yMQ3mFscayLr10z1UcaINFHmXvX05c/cnbxclPQKHUa/gfFNAuQn+ZusuhsvjWKiApcXj+NjVZ9QTroaWc4QwHeWqHCAwUCLY7NsNEJBsi0mTybA/5MSaNqCXvXDoWLZte/h23we+ZQTDWUOIYaMzfAqLqvpBN0ukXf5XelNPWCkgXaZY/IqvfuyaEdBeNr/EgxGmSrsofyPSZbUvaF9gMTk/7yZFHX2M0OMiU8IWJfW2suMExsB3HrdetOFqbol0UThC9TTizGjo/Y/R9Mdkv5gPzTTDk9Neg1eLGfdu+wl6hjG3czhMTKLyso8pVDFfiKmxlkRgl6nX3qRbjWLPTolI/JS2PwvLsowgGtwq11tyWegLvvToWsoZrC//EW1OnX0gdbMtnFkixQocSarjjbJ3iWbPdMK8z8+fu5Hu+kSwxvMZG7eqHN1WOnH3St6r2TxU2TrUuHrGD6yEguLkiORPaH21RAPrCTWY+I5gnnCVDmi6yreI/h/oenqyCDKs2TafeUfstx2rcPsPBN1kZ34ACBvU+A93ssjhlDYjpvZVIJn9It8YuOJf2+Xxk88jHRGR3nCbTJTGrpVD1B04524 iD4kEiZO wGkqWyHBHxz+u51RATskJoWWyOh+ry21zzWKkcyaG0By0uIT3NV+k7THtTsbZj6M0leVlCEz1YeKLanvVl7vXBkTlN/cRzhxtUu30dXcSkfJTG+6Z7GfGC6sNrXp0DBNZMgMJN47CrT3cz9oM7gVw718ca0eH3xm8fzxo93pKYIAFEcXwNSxukwwAMZTcbfshPG/Y8DE2Q9nTF3tCxgqzDGSCZMf7P6ampebsR39GnvtKfBSCxDT/2gifTuuiea2/pDOhYOi+wXqcsARhTSj7Ck/HkujAiEUT9fViSeb6zvF48CYM23kjTcmo3XHNJjXdv1gEO1iukfVCyTsNZ6CD/r5P3amxpIhgfvZc+wgyuk5SW1M= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Thu 18-05-23 07:47:35, Jeff Layton wrote: > The VFS always uses coarse-grained timestamp updates for filling out the > ctime and mtime after a change. This has the benefit of allowing > filesystems to optimize away a lot metadata updates, down to around 1 > per jiffy, even when a file is under heavy writes. > > Unfortunately, this has always been an issue when we're exporting via > NFSv3, which relies on timestamps to validate caches. Even with NFSv4, a > lot of exported filesystems don't properly support a change attribute > and are subject to the same problems with timestamp granularity. Other > applications have similar issues (e.g backup applications). > > Switching to always using fine-grained timestamps would improve the > situation, but that becomes rather expensive, as the underlying > filesystem will have to log a lot more metadata updates. > > What we need is a way to only use fine-grained timestamps when they are > being actively queried. > > The kernel always stores normalized ctime values, so only the first 30 > bits of the tv_nsec field are ever used. Whenever the mtime changes, the > ctime must also change. > > Use the 31st bit of the ctime tv_nsec field to indicate that something > has queried the inode for the i_mtime or i_ctime. When this flag is set, > on the next timestamp update, the kernel can fetch a fine-grained > timestamp instead of the usual coarse-grained one. > > This patch adds the infrastructure this scheme. Filesytems can opt > into it by setting the FS_MULTIGRAIN_TS flag in the fstype. > > Later patches will convert individual filesystems over to use it. > > Signed-off-by: Jeff Layton So there are two things I dislike about this series because I think they are fragile: 1) If we have a filesystem supporting multigrain ts and someone accidentally directly uses the value of inode->i_ctime, he can get bogus value (with QUERIED flag). This mistake is very easy to do. So I think we should rename i_ctime to something like __i_ctime and always use accessor function for it. 2) As I already commented in a previous version of the series, the scheme with just one flag for both ctime and mtime and flag getting cleared in current_time() relies on the fact that filesystems always do an equivalent of: inode->i_mtime = inode->i_ctime = current_time(); Otherwise we can do coarse grained update where we should have done a fine grained one. Filesystems often update timestamps like this but not universally. Grepping shows some instances where only inode->i_mtime is set from current_time() e.g. in autofs or bfs. Again a mistake that is rather easy to make and results in subtle issues. I think this would be also nicely solved by renaming i_ctime to __i_ctime and using a function to set ctime. Mtime could then be updated with inode->i_mtime = ctime_peek(). I understand this is quite some churn but a very mechanical one that could be just done with Coccinelle and a few manual fixups. So IMHO it is worth the more robust result. Some more nits below. > +/** > + * current_mg_time - Return FS time (possibly fine-grained) > + * @inode: inode. > + * > + * Return the current time truncated to the time granularity supported by > + * the fs, as suitable for a ctime/mtime change. If the ctime is flagged > + * as having been QUERIED, get a fine-grained timestamp. > + */ The comment should also mention that QUERIED flag is cleared from the ctime. > +static struct timespec64 current_mg_time(struct inode *inode) > +{ > + struct timespec64 now; > + atomic_long_t *pnsec = (atomic_long_t *)&inode->i_ctime.tv_nsec; > + long nsec = atomic_long_fetch_andnot(I_CTIME_QUERIED, pnsec); > + > + if (nsec & I_CTIME_QUERIED) { > + ktime_get_real_ts64(&now); > + } else { > + struct timespec64 ctime; > + > + ktime_get_coarse_real_ts64(&now); > + > + /* > + * If we've recently fetched a fine-grained timestamp > + * then the coarse-grained one may still be earlier than the > + * existing one. Just keep the existing ctime if so. > + */ > + ctime = ctime_peek(inode); > + if (timespec64_compare(&ctime, &now) > 0) > + now = ctime; > + } > + > + return now; > +} > + ... > +/** > + * ctime_nsec_peek - peek at (but don't query) the ctime tv_nsec field > + * @inode: inode to fetch the ctime from > + * > + * Grab the current ctime tv_nsec field from the inode, mask off the > + * I_CTIME_QUERIED flag and return it. This is mostly intended for use by > + * internal consumers of the ctime that aren't concerned with ensuring a > + * fine-grained update on the next change (e.g. when preparing to store > + * the value in the backing store for later retrieval). > + * > + * This is safe to call regardless of whether the underlying filesystem > + * is using multigrain timestamps. > + */ > +static inline long ctime_nsec_peek(const struct inode *inode) > +{ > + return inode->i_ctime.tv_nsec &~ I_CTIME_QUERIED; This is somewhat unusual spacing. I'd use: inode->i_ctime.tv_nsec & ~I_CTIME_QUERIED > +} > + > +/** > + * ctime_peek - peek at (but don't query) the ctime > + * @inode: inode to fetch the ctime from > + * > + * Grab the current ctime from the inode, sans I_CTIME_QUERIED flag. For > + * use by internal consumers that don't require a fine-grained update on > + * the next change. > + * > + * This is safe to call regardless of whether the underlying filesystem > + * is using multigrain timestamps. > + */ > +static inline struct timespec64 ctime_peek(const struct inode *inode) > +{ > + struct timespec64 ctime; > + > + ctime.tv_sec = inode->i_ctime.tv_sec; > + ctime.tv_nsec = ctime_nsec_peek(inode); > + > + return ctime; > +} Given this is in a header that gets included in a lot of places, maybe we should call it like inode_ctime_peek() or inode_ctime_get() to reduce chances of a name clash? Honza -- Jan Kara SUSE Labs, CR