Message-ID: <20220713101754.50ad9bde@redhat.com>
Date:   Wed, 13 Jul 2022 10:17:54 +0200
From:   Igor Mammedov <imammedo@...hat.com>
To:     Bruce Fields <bfields@...ldses.org>
Cc:     Jeff Layton <jlayton@...hat.com>,
        Chuck Lever III <chuck.lever@...cle.com>,
        Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
        Ondrej Valousek <ondrej.valousek.xm@...esas.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [GIT PULL] nfsd changes for 5.18

On Tue, 12 Jul 2022 07:42:11 -0400
Bruce Fields <bfields@...ldses.org> wrote:

> On Tue, Jul 12, 2022 at 10:27:46AM +0200, Igor Mammedov wrote:
> > On Mon, 11 Jul 2022 14:19:41 -0400
> > Bruce Fields <bfields@...ldses.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).  
> 
> Hang on, now--our current server completely fails to decode any RPC
> including a SETATTR that attempts to set TIME_CREATE, which means it
> isn't able to return a useful error or tell the client which attribute
> was the problem.
> 
> It's not too surprising that that would cause a problem for a client.
> 
> But failures to set supported attributes are completely normal, and if
> mounts are failing completely because of that, something is really very
> wrong with the client.

returning unsupported attribute error might work, but as Chuck mentioned
we do kind of support TIME_CREATE for some requests so client might be
confused when server itself sends this attribute while errors out when
client tries to send it.
What I'm saying if we are to try returning error in this case
it should be tested with variety of clients before committing
to this approach. (meanwhile decoding and ignoring attribute
with Chuck's patch fixes immediate issue).

> Could you first retest with a server that's patched to at least decode
> the attribute correctly?  I suspect that may be enough.  If not, then
it does work with fixed decoding path:
 (i.e. patched with https://lore.kernel.org/lkml/A4F0C111-B2EB-4325-AC6A-4A80BD19DA43@oracle.com/T/)

> the client in question has a more interesting problem on its hands.
> 
> --b.
> 

Powered by blists - more mailing lists