Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <a30414b7b720217082a8245d10e50e9c342ec8ad.camel@kernel.org>
Subject: Re: [man-pages RFC PATCH v4] statx, inode: document the new
 STATX_INO_VERSION field
From:   Jeff Layton <jlayton@kernel.org>
To:     Jan Kara <jack@suse.cz>
Cc:     Dave Chinner <david@fromorbit.com>, Theodore Ts'o <tytso@mit.edu>,
        NeilBrown <neilb@suse.de>,
        Trond Myklebust <trondmy@hammerspace.com>,
        "bfields@fieldses.org" <bfields@fieldses.org>,
        "zohar@linux.ibm.com" <zohar@linux.ibm.com>,
        "djwong@kernel.org" <djwong@kernel.org>,
        "brauner@kernel.org" <brauner@kernel.org>,
        "linux-xfs@vger.kernel.org" <linux-xfs@vger.kernel.org>,
        "linux-api@vger.kernel.org" <linux-api@vger.kernel.org>,
        "fweimer@redhat.com" <fweimer@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "chuck.lever@oracle.com" <chuck.lever@oracle.com>,
        "linux-man@vger.kernel.org" <linux-man@vger.kernel.org>,
        "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>,
        "linux-ext4@vger.kernel.org" <linux-ext4@vger.kernel.org>,
        "viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
        "xiubli@redhat.com" <xiubli@redhat.com>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "adilger.kernel@dilger.ca" <adilger.kernel@dilger.ca>,
        "lczerner@redhat.com" <lczerner@redhat.com>,
        "ceph-devel@vger.kernel.org" <ceph-devel@vger.kernel.org>,
        "linux-btrfs@vger.kernel.org" <linux-btrfs@vger.kernel.org>
Date:   Fri, 23 Sep 2022 06:19:31 -0400
In-Reply-To: <20220923095653.5c63i2jgv52j3zqp@quack3>
References: <24005713ad25370d64ab5bd0db0b2e4fcb902c1c.camel@kernel.org>
         <20220918235344.GH3600936@dread.disaster.area>
         <87fb43b117472c0a4c688c37a925ac51738c8826.camel@kernel.org>
         <20220920001645.GN3600936@dread.disaster.area>
         <5832424c328ea427b5c6ecdaa6dd53f3b99c20a0.camel@kernel.org>
         <20220921000032.GR3600936@dread.disaster.area>
         <93b6d9f7cf997245bb68409eeb195f9400e55cd0.camel@kernel.org>
         <20220921214124.GS3600936@dread.disaster.area>
         <e04e349170bc227b330556556d0592a53692b5b5.camel@kernel.org>
         <1ef261e3ff1fa7fcd0d75ed755931aacb8062de2.camel@kernel.org>
         <20220923095653.5c63i2jgv52j3zqp@quack3>
Content-Type: text/plain; charset="ISO-8859-15"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 (3.44.4-1.fc36) 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2022-09-23 at 11:56 +0200, Jan Kara wrote:
> On Thu 22-09-22 16:18:02, Jeff Layton wrote:
> > On Thu, 2022-09-22 at 06:18 -0400, Jeff Layton wrote:
> > > On Thu, 2022-09-22 at 07:41 +1000, Dave Chinner wrote:
> > > > e.g. The NFS server can track the i_version values when the NFSD
> > > > syncs/commits a given inode. The nfsd can sample i_version it when
> > > > calls ->commit_metadata or flushed data on the inode, and then when
> > > > it peeks at i_version when gathering post-op attrs (or any other
> > > > getattr op) it can decide that there is too much in-memory change
> > > > (e.g. 10,000 counts since last sync) and sync the inode.
> > > >=20
> > > > i.e. the NFS server can trivially cap the maximum number of
> > > > uncommitted NFS change attr bumps it allows to build up in memory.
> > > > At that point, the NFS server has a bound "maximum write count" tha=
t
> > > > can be used in conjunction with the xattr based crash counter to
> > > > determine how the change_attr is bumped by the crash counter.
> > >=20
> > > Well, not "trivially". This is the bit where we have to grow struct
> > > inode (or the fs-specific inode), as we'll need to know what the late=
st
> > > on-disk value is for the inode.
> > >=20
> > > I'm leaning toward doing this on the query side. Basically, when nfsd
> > > goes to query the i_version, it'll check the delta between the curren=
t
> > > version and the latest one on disk. If it's bigger than X then we'd j=
ust
> > > return NFS4ERR_DELAY to the client.
> > >=20
> > > If the delta is >X/2, maybe it can kick off a workqueue job or someth=
ing
> > > that calls write_inode with WB_SYNC_ALL to try to get the thing onto =
the
> > > platter ASAP.
> >=20
> > Still looking at this bit too. Probably we can just kick off a
> > WB_SYNC_NONE filemap_fdatawrite at that point and hope for the best?
>=20
> "Hope" is not a great assurance regarding data integrity ;)=A0

By "hoping for the best", I meant hoping that we never have to take the
the drastic action of returning NFS4ERR_DELAY on GETATTR operations. We
definitely don't want to jeopardize data integrity.=20

> Anyway, it
> depends on how you imagine the "i_version on disk" is going to be
> maintained. It could be maintained by NFSD inside commit_inode_metadata()=
 -
> fetch current i_version value before asking filesystem for the sync and b=
y the
> time commit_metadata() returns we know that value is on disk. If we detec=
t the
> current - on_disk is > X/2, we call commit_inode_metadata() and we are
> done. It is not even *that* expensive because usually filesystems optimiz=
e
> away unnecessary IO when the inode didn't change since last time it got
> synced.
>=20

At >X/2 we don't really want to start blocking or anything. I'd prefer
if we could kick off something in the background for this, but if it's
not too expensive then maybe just calling commit_inode_metadata
synchronously in this codepath is OK. Alternately, we could consider
doing that in a workqueue job too.

I need to do a bit more research here, but I think we have some options.
--=20
Jeff Layton <jlayton@kernel.org>