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 92854C77B75 for ; Fri, 21 Apr 2023 10:48:03 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 2B56F6B0072; Fri, 21 Apr 2023 06:48:03 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 23D466B0074; Fri, 21 Apr 2023 06:48:03 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 12C826B0075; Fri, 21 Apr 2023 06:48:03 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0010.hostedemail.com [216.40.44.10]) by kanga.kvack.org (Postfix) with ESMTP id 028936B0072 for ; Fri, 21 Apr 2023 06:48:03 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id D3F9F120676 for ; Fri, 21 Apr 2023 10:48:02 +0000 (UTC) X-FDA: 80705073204.18.AB81D5E Received: from dfw.source.kernel.org (dfw.source.kernel.org [139.178.84.217]) by imf20.hostedemail.com (Postfix) with ESMTP id 195141C000F for ; Fri, 21 Apr 2023 10:47:59 +0000 (UTC) Authentication-Results: imf20.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Hjutz1fv; spf=pass (imf20.hostedemail.com: domain of jlayton@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=jlayton@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1682074080; 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:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=9ig1G3snkksicJsGjEgSk5CSrNqkU8xAMVaTUJN5b3Q=; b=JUiCrsdalQgRuwh0S0VSQsD0Xi8IMwAa4ikEZuCprL5Lr59XaTP0VrrW9hLqGcDqBS/8/h YJ4ci9cHIwHk4OS7tTbl9nwb8fNVeu5iHqmvSq73IrZL+LtO+vLFoUeLmaO3XRD9wN1PRB xwbTavtSAGG3LYuPdty3YvmVAdSjD8c= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=pass header.d=kernel.org header.s=k20201202 header.b=Hjutz1fv; spf=pass (imf20.hostedemail.com: domain of jlayton@kernel.org designates 139.178.84.217 as permitted sender) smtp.mailfrom=jlayton@kernel.org; dmarc=pass (policy=none) header.from=kernel.org ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1682074080; a=rsa-sha256; cv=none; b=e/9hY05jQrPCh8lx7dm2U0/Ox7n3zvSJQT5eSGPFoYWHkjPVWEaYcuvCln3TtmSEaiykti mE/QXk46zSOUqcQ/oW3k0sokHAKBU/XZToRiLNRyVfdHuH1r4YNaoLG0zltptpGtN8Bn58 HazofuzghB/VHZ7TylZeKWLdgnFLLkA= Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 135CB60CEB; Fri, 21 Apr 2023 10:47:59 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 66A37C433EF; Fri, 21 Apr 2023 10:47:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1682074078; bh=YMiCd2tn+Kwhq5vt740oxHkL6CrclEtNSKXXgkMDZzc=; h=Subject:From:To:Cc:Date:In-Reply-To:References:From; b=Hjutz1fvbTbSug9eVUquviL4tAoNlkEh+D8UwRz2EmqxPmD/qIE/1sOTRzmn5mwqx mKM95MWbE5ujOBQHDUryrtj3aqFhc0Q0SATadEGGrZRLjIxBV8nq0lrqpmhmgCTUKh lndHzOw7gLoe3S5beyCwDRSRjL1vX/WylSDNOcfAjn6RKpLfuCoNsC6AMTVf+ZImBs M3cix5G9qaqYVwm89y3tY5ZNdrS3+cNddU1PF9z56nFky4dcJSc1cQmV7aUBD+FE+2 Vp7JlPZBRoPkjedQHc7virsAH4OI+StJ+5EdISxvirGszqGIL3Gz4r8XpZo0rZk+Jg biHjSlsBXy5yw== Message-ID: Subject: Re: [RFC PATCH 1/3] fs: add infrastructure for opportunistic high-res ctime/mtime updates From: Jeff Layton To: Jan Kara Cc: Alexander Viro , Christian Brauner , "Darrick J. Wong" , Hugh Dickins , Andrew Morton , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-xfs@vger.kernel.org, linux-mm@kvack.org, Dave Chinner Date: Fri, 21 Apr 2023 06:47:55 -0400 In-Reply-To: <20230421101331.dlxom6b5e7yds5tn@quack3> References: <20230411142708.62475-1-jlayton@kernel.org> <20230411142708.62475-2-jlayton@kernel.org> <20230421101331.dlxom6b5e7yds5tn@quack3> Content-Type: text/plain; charset="ISO-8859-15" Content-Transfer-Encoding: quoted-printable User-Agent: Evolution 3.48.0 (3.48.0-1.fc38) MIME-Version: 1.0 X-Rspam-User: X-Rspamd-Server: rspam03 X-Stat-Signature: 64czwzxqqw8h1bibhsn779iwi7e3n198 X-Rspamd-Queue-Id: 195141C000F X-HE-Tag: 1682074079-439250 X-HE-Meta: U2FsdGVkX18xA8+2mixECQehtmMlJfy4wM8zQbOjdi3HctbZL/PcWVP1JdpZ+Jh22GvwEQ77iSXsedPBrhmZQcpzxyry6B6bOcQo1Zh5uXzu+3r1JLRwecvEdFT5bxh1IYc4gipD8g8Id13zF1aCM2eUjUumLWlQtQ0xR682K+qT5sktPQGUUZSsB8YRS2f83wNMI/pMogbJnrt3eVqJ2zpp5ygif6/sSBIOnlAVCyyPAPSBqHMWkfg8gNVB8Qc8CYskutJPA+99ifNdChxuDI0ie/tD+tbYZQqirzrw5GZayzQbX1ns/8IO6Ww/j6azOgx51GfUJrSPWciOf5s70e86s0LiWhz65I4tLvfL+5QEmI90MGBArdcXkGNILXJ/FELVT1JRpziqF6COXwNqvcZfdaBx+tGsMcMhqetYMSwl3X8fMD2vb343dIIikTowAq5N3hW5owJPge0HCiGQppqUsFbn4YRHntEJfIE5yAwHP227J8DUGkic8hbF946SqwJ9OBPeioHC+SFrJCSbXLwwWwLMKveZ6eW9h9U7+TP30lmQKWP6D6toZnzOzYIHmdacSuqXUlJcJ2AraQilPuQ0Gf+9g1vhyDxLWfCO62PDD53fZPDUbv+DofzFmxk9PWuSp8i4bReX6JpRspS6E7XcAp6U3164JT7IUfvb+Pdyi/f9aM/ZTp94OLEh3ONS0fQXzB72rvivKIoFsSG94/LUj70Ys2LnCPxmg6ovjNX6EkghhfAurN/FfOJEgJOaxbyCDrGxTC3x4M5Z115Inf9DqsAm7TqebTEnAGvQYlMJMiceBUL+PVGN8lQSrQQkYcZMdaZLp5fBOtB9mz2nsSUwicJsnQDUDecM288hPVZejs6OtWojD83+tt96dz8CFrFDGB9LYBFMXamJULK7ntupZTbi+IJKyRAQkyBIylPPuP6PqDG9V/zXO0Bc4SeQc91yfmRiTiucOtxeUgy MvtO9aEN o+pewISToehevoH7zIFPs3htiBN5C8KRqoMj2kUCkTzvOcNOzQZbU8n3NxBmqpmqcB6zIh4ao6bDjS9BXPp9NquuPa4Hi0UHnQOEv1zM25N8HiKLGDhGXrWt+en3FvKM4ylDn8gd9zR0RMsxDL+Yg6A5Q7JQwt0i9o6j/XHgLi7yB31WVGmJYv/yvHaQ4ASTiT+316N3MffvqYfk8IUTtN1GYaQSSCrB+2Wpe/7kAIQXv9Dq1mnvsdXC+kBNYQRrTwntBj2QHJ0XFgAWDxTcOGGq+2LTLB9j4tPZXT5r38DNCDkQzdk8+5Bw7U4ZqqmoGbCCMjnuC7tgawCOCLz/1tuBl754X7gYkxReWNI3SxQ5aqe60X/fstylqxLqC4/JAPncnDJ9dfQJiqbJXGLXnxvKYCR/2i1mwKKWz 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 Fri, 2023-04-21 at 12:13 +0200, Jan Kara wrote: > On Tue 11-04-23 10:27:06, Jeff Layton wrote: > > The VFS always uses coarse-grained timestamp updates for filling out th= e > > ctime and mtime after a change. This has the benefit of allowing > > filesystems to optimize away metadata updates. > >=20 > > 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 problem of timestamp granularity. Other > > applications have similar issues (e.g backup applications). > >=20 > > Switching to always using high resolution timestamps would improve the > > situation for NFS, but that becomes rather expensive, as we'd have to > > log a lot more metadata updates. > >=20 > > This patch grabs a new i_state bit to use as a flag that filesystems ca= n > > set in their getattr routine to indicate that the mtime or ctime was > > queried since it was last updated. > >=20 > > It then adds a new current_cmtime function that acts like the > > current_time helper, but will conditionally grab high-res timestamps > > when the i_state flag is set in the inode. > >=20 > > This allows NFS and other applications to reap the benefits of high-res > > ctime and mtime timestamps, but at a substantially lower cost than > > fetching them every time. > >=20 > > Cc: Dave Chinner > > Signed-off-by: Jeff Layton > > --- > > fs/inode.c | 40 ++++++++++++++++++++++++++++++++++++++-- > > fs/stat.c | 10 ++++++++++ > > include/linux/fs.h | 5 ++++- > > 3 files changed, 52 insertions(+), 3 deletions(-) > >=20 > > diff --git a/fs/inode.c b/fs/inode.c > > index 4558dc2f1355..3630f67fd042 100644 > > --- a/fs/inode.c > > +++ b/fs/inode.c > > @@ -2062,6 +2062,42 @@ static int __file_update_time(struct file *file,= struct timespec64 *now, > > return ret; > > } > > =20 > > +/** > > + * current_cmtime - Return FS time (possibly high-res) > > + * @inode: inode. > > + * > > + * Return the current time truncated to the time granularity supported= by > > + * the fs, as suitable for a ctime or mtime change. If something recen= tly > > + * fetched the ctime or mtime out of the inode via getattr, then get a > > + * high-resolution timestamp. > > + * > > + * Note that inode and inode->sb cannot be NULL. > > + * Otherwise, the function warns and returns coarse time without trunc= ation. > > + */ > > +struct timespec64 current_cmtime(struct inode *inode) > > +{ > > + struct timespec64 now; > > + > > + if (unlikely(!inode->i_sb)) { >=20 > I don't think we can have inodes without a superblock. Did you ever hit > this? >=20 No, I copied this from current_time. I've already removed this in my working branch. We can probably remove it from current_time too. > > + WARN(1, "%s() called with uninitialized super_block in the inode", _= _func__); > > + ktime_get_coarse_real_ts64(&now); > > + return now; > > + } > > + > > + /* Do a lockless check for the flag before taking the spinlock */ > > + if (READ_ONCE(inode->i_state) & I_CMTIME_QUERIED) { > > + ktime_get_real_ts64(&now); > > + spin_lock(&inode->i_lock); > > + inode->i_state &=3D ~I_CMTIME_QUERIED; >=20 > Isn't this a bit fragile? If someone does: >=20 > inode->i_mtime =3D current_cmtime(inode); > inode->i_ctime =3D current_cmtime(inode); >=20 > the ctime update will be coarse although it should be fine-grained. >=20 It is a bit. We'll need for users to do something like: inode->i_mtime =3D inode->i_ctime =3D current_ctime(inode); Fortunately, most do this already. > > + spin_unlock(&inode->i_lock); > > + } else { > > + ktime_get_coarse_real_ts64(&now); > > + } > > + > > + return timestamp_truncate(now, inode); >=20 > I'm a bit confused here. Isn't the point of this series also to give NFS > finer grained granularity time stamps than what the filesystem is possibl= y > able to store on disk? >=20 No. We actually don't want to hand out timestamps more granular than the underlying filesystem can support, as we'd end up having to invalidate caches for all of those inodes once the server rebooted and the unrecordable bits get zeroed out. The main idea here is to just ensure that we use fine-grained timestamps when someone has queried the mtime or ctime since the last time it was updated. > Hmm, checking XFS it sets 1 ns granularity (as well as tmpfs) so for thes= e > using the coarser timers indeed gives a performance benefit. And probably > you've decided not implement the "better NFS support with coarse grained > timestamps" yet. >=20 Yep. The coarse grained timestamps are a _good_ thing for most filesystems as they allow you to skip a lot of metadata updates. My hope is that this will end up being like the i_version changes such that the extra fine-grained updates should be relatively rare and should (hopefully!) not cause noticeable performance blips. We'll see! > > +} > > +EXPORT_SYMBOL(current_cmtime); > > + > > /** > > * file_update_time - update mtime and ctime time > > * @file: file accessed > > @@ -2080,7 +2116,7 @@ int file_update_time(struct file *file) > > { > > int ret; > > struct inode *inode =3D file_inode(file); > > - struct timespec64 now =3D current_time(inode); > > + struct timespec64 now =3D current_cmtime(inode); > > =20 > > ret =3D inode_needs_update_time(inode, &now); > > if (ret <=3D 0) > > @@ -2109,7 +2145,7 @@ static int file_modified_flags(struct file *file,= int flags) > > { > > int ret; > > struct inode *inode =3D file_inode(file); > > - struct timespec64 now =3D current_time(inode); > > + struct timespec64 now =3D current_cmtime(inode); > > =20 > > /* > > * Clear the security bits if the process is not being run by root. > > diff --git a/fs/stat.c b/fs/stat.c > > index 7c238da22ef0..d8b80a2e36b7 100644 > > --- a/fs/stat.c > > +++ b/fs/stat.c > > @@ -64,6 +64,16 @@ void generic_fillattr(struct mnt_idmap *idmap, struc= t inode *inode, > > } > > EXPORT_SYMBOL(generic_fillattr); > > =20 > > +void fill_cmtime_and_mark(struct inode *inode, struct kstat *stat) > > +{ > > + spin_lock(&inode->i_lock); > > + inode->i_state |=3D I_CMTIME_QUERIED; > > + stat->ctime =3D inode->i_ctime; > > + stat->mtime =3D inode->i_mtime; > > + spin_unlock(&inode->i_lock); > > +} > > +EXPORT_SYMBOL(fill_cmtime_and_mark); >=20 > The name could be better here :). Maybe stat_fill_cmtime_and_mark()? >=20 > =09 I have a quite different set that I've been working on that I'll (hopefully!) post soon. That one uses the least significant bit of the tv_nsec field as the QUERIED flag instead of the spinlock. Still cleaning up the set and need to test it some more though, so it's not quite ready to post. Stay tuned! Thanks for the review!=20 --=20 Jeff Layton