Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 12 Jul 2022 10:27:46 +0200
From:   Igor Mammedov <imammedo@redhat.com>
To:     Bruce Fields <bfields@fieldses.org>
Cc:     Jeff Layton <jlayton@redhat.com>,
        Chuck Lever III <chuck.lever@oracle.com>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Ondrej Valousek <ondrej.valousek.xm@renesas.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [GIT PULL] nfsd changes for 5.18
Message-ID: <20220712102746.5404e88a@redhat.com>
In-Reply-To: <20220711181941.GC14184@fieldses.org>
References: <EF97E1F5-B70F-4F9F-AC6D-7B48336AE3E5@oracle.com>
        <20220710124344.36dfd857@redhat.com>
        <B62B3A57-A8F7-478B-BBAB-785D0C2EE51C@oracle.com>
        <5268baed1650b4cba32978ad32d14a5ef00539f2.camel@redhat.com>
        <20220711181941.GC14184@fieldses.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Mon, 11 Jul 2022 14:19:41 -0400
Bruce Fields <bfields@fieldses.org> wrote:

> On Mon, Jul 11, 2022 at 06:33:04AM -0400, Jeff Layton wrote:
> > On Sun, 2022-07-10 at 16:42 +0000, Chuck Lever III wrote:  
> > > > This patch regressed clients that support TIME_CREATE attribute.
> > > > Starting with this patch client might think that server supports
> > > > TIME_CREATE and start sending this attribute in its requests.  
> > > 
> > > Indeed, e377a3e698fb ("nfsd: Add support for the birth time
> > > attribute") does not include a change to nfsd4_decode_fattr4()
> > > that decodes the birth time attribute.
> > > 
> > > I don't immediately see another storage protocol stack in our
> > > kernel that supports a client setting the birth time, so NFSD
> > > might have to ignore the client-provided value.
> > >   
> > 
> > Cephfs allows this. My thinking at the time that I implemented it was
> > that it should be settable for backup purposes, but this was possibly a
> > mistake. On most filesystems, the btime seems to be equivalent to inode
> > creation time and is read-only.  
> 
> So supporting it as read-only seems reasonable.
> 
> Clearly, failing to decode the setattr attempt isn't the right way to do
> that.  I'm not sure what exactly it should be doing--some kind of
> permission error on any setattr containing TIME_CREATE?

erroring out on TIME_CREATE will break client that try to
set this attribute (legitimately). That's what by chance 
happening with current master (return error when TIME_CREATE
is present).

As long as server advertises support for TIME_CREATE
it should not error out when client sends it if spec permits
such use.

I think ignoring this attribute like Chuck has proposed
is acceptable (if one ignores archiving use case where
setting it makes sense).

Alternatively if folks inclined towards erroring out,
there should be a way to optout or optin from TIME_CREATE support,
to keep existing clients working + a sane error message so users
won't have to debug kernel to figure out what's wrong with
their setup.

> --b.
>