From: NeilBrown <neilb@suse.com>
To: Jeff Layton <jlayton@kernel.org>, linux-fsdevel@vger.kernel.org
Date: Sat, 16 Dec 2017 15:17:05 +1100
Cc: linux-kernel@vger.kernel.org, hch@lst.de, neilb@suse.de,
        bfields@fieldses.org, amir73il@gmail.com, jack@suse.de,
        viro@zeniv.linux.org.uk
Subject: Re: [PATCH 01/19] fs: new API for handling inode->i_version
In-Reply-To: <1513211220.3498.79.camel@kernel.org>
References: <20171213142017.23653-1-jlayton@kernel.org> <20171213142017.23653-2-jlayton@kernel.org> <87indanv2m.fsf@notabene.neil.brown.name> <1513211220.3498.79.camel@kernel.org>
Message-ID: <87mv2jmhmm.fsf@notabene.neil.brown.name>
MIME-Version: 1.0
Content-Type: multipart/signed; boundary="=-=-=";
        micalg=pgp-sha256; protocol="application/pgp-signature"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 5315
Lines: 125

--=-=-=
Content-Type: text/plain
Content-Transfer-Encoding: quoted-printable

On Wed, Dec 13 2017, Jeff Layton wrote:

> On Thu, 2017-12-14 at 09:04 +1100, NeilBrown wrote:
>> On Wed, Dec 13 2017, Jeff Layton wrote:
>>=20
>> > +/*
>> > + * The change attribute (i_version) is mandated by NFSv4 and is mostl=
y for
>> > + * knfsd, but is also used for other purposes (e.g. IMA). The i_versi=
on must
>> > + * appear different to observers if there was a change to the inode's=
 data or
>> > + * metadata since it was last queried.
>> > + *
>> > + * It should be considered an opaque value by observers. If it remain=
s the same
>> > + * since it was last checked, then nothing has changed in the inode. =
If it's
>> > + * different then something has changed. Observers cannot infer anyth=
ing about
>> > + * the nature or magnitude of the changes from the value, only that t=
he inode
>> > + * has changed in some fashion.
>>=20
>> I agree that it "should be" considered opaque, but I have a suspicion
>> that NFSv4 doesn't consider it opaque.
>> There is something about write delegations and the server performing a
>> GETATTR callback to the delegated client so that it can answer GETATTR
>> from other clients without recalling the delegation.
>>=20
>> Specifically section "10.4.3 Handling of CB_GETATTR" of RFC5661 contains
>> the text:
>>=20
>>    o  The client will create a value greater than c that will be used
>>       for communicating that modified data is held at the client.  Let
>>       this value be represented by d.
>>=20
>> "c" here is a 'change' attribute.
>>=20
>> Then:
>>=20
>>    While the change attribute is opaque to the client in the sense that
>>    it has no idea what units of time, if any, the server is counting
>>    change with, it is not opaque in that the client has to treat it as
>>    an unsigned integer, and the server has to be able to see the results
>>    of the client's changes to that integer.  Therefore, the server MUST
>>    encode the change attribute in network order when sending it to the
>>    client.  The client MUST decode it from network order to its native
>>    order when receiving it, and the client MUST encode it in network
>>    order when sending it to the server.  For this reason, change is
>>    defined as an unsigned integer rather than an opaque array of bytes.
>>=20
>> This all suggests that nfsd needs to be certain that "incrementing" the
>> change id will produce a new changeid, which has not been used before,
>> and also suggests that nfsd needs to be able to control the changeid
>> stored after writes that result from a delegation being returned.
>>=20
>> I'd just like to say that this is one of the most annoying dumb features
>> of NFSv4, because it is trivial to fix and I suggested a fix before
>> NFSv4.0 was finalized.  Grumble.
>>=20
>> Otherwise the patch set looks good.  I haven't gone over the code
>> closely, the but approach is spot-on.
>
> I don't think we have to do that. There are really only two states with
> a client holding a write delegation, as far as the server is concerned.
> Either:
>
> a) the client has done no writes to the file, in which case it'll return
> the same i_version that the server has when issued a CB_GETATTR
>
> ...or...
>
> b) it has written to the file while holding the delegation, in which
> case it'll return a different CB_GETATTR to the server
>
> The simplest thing for the server to do is to just increment the change
> attribute _once_ when it gets back a CB_GETATTR with a different change
> attr than it has.
>
> That's sufficient to tell another client issuing a a GETATTR that the
> file has changed without needing to recall the delegation.
>
> Prior to the delegation being returned, the client will send at least
> one WRITE RPC, and that's enough to ensure that the the next stat will
> see the thing increase.

"increment" and "increase" are not words that mean anything for an
"opaque value".
NFSd is, presumably, an "observer" of i_version (as it isn't the
filesytem that controls it), so your text says it must treat i_version as
opaque.  That means it cannot detect an "increase" (only a change), and
it certainly cannot "increment" the value.

I think you need to allow observers to treat i_version as a 64 bit number
which will monotonically increase.  Any change to the file will result
in an increment of at least '1'.

Thanks,
NeilBrown

--=-=-=
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEEG8Yp69OQ2HB7X0l6Oeye3VZigbkFAlo0nkIACgkQOeye3VZi
gbk2fg/+PskPThNi2n1fBh/VrwLyUkMmuB12/2Y98/luQk6unXlhf+kc9D1EF7jM
QQhHVyCTHLNKn+hiClF2IuLMewpOisup0LCGcjebk99z7bsKmr0wzwvR8zF9KopK
fDs3NpyMZqmRQF4s9TI1ol5TKO+W5JxPbuazoMzRv/PA6EnC75krCy2gAP8BrgmB
3yKOU9kAOeVINp3ocy9tigW5Cq2BalcsoyeHx6KSxeXBqBEVF6nP3oVUr0MjrE2p
HHxHKjeavqMenqz8p4o3wCSWRVBNrAjfnTHMZRqXZ6qPpqs0ykFXCrP6/hbj+MeW
y2eKHDndzeFVXjV8Nz0/U7k4Hu7T8UbaXykQZztGQ3WLcr5Y2CN6mKUYELkf4f3n
5coNP8CbZeUa/YFG8R9SEd6Z3eImc+l2VMs42uW2T/6NPE5hC6dv2iIOAfdBh3yu
AghTxVyTJT8fXSRHf/+7CkOlM7z6roY9GkFW0UBRmHE4s5kmjtqXHCVST6t+MZag
5VtE5fGD8WVc6IL4SEKFZr3noqbcDUgXFe80qwotkBC9wTqRTTvJEEqmZFndm/+b
Wrd7ax/bUeLyVqGR6H/5NLWNFAM5jkvgKk7ZAfR7HqadLbXH1LnJudQ/zr36861y
kmQ/wsbbPeJ2ZKxutZxJLVRxsdoyKjH/TDKnVlh/eJ5e6tGDsD4=
=7evN
-----END PGP SIGNATURE-----
--=-=-=--