All of lore.kernel.org
 help / color / mirror / Atom feed
From: Trond Myklebust <trondmy@hammerspace.com>
To: "chuck.lever@oracle.com" <chuck.lever@oracle.com>
Cc: "linux-nfs@vger.kernel.org" <linux-nfs@vger.kernel.org>
Subject: Re: [PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr()
Date: Sat, 17 Feb 2024 18:16:27 +0000	[thread overview]
Message-ID: <7b51a5725d82ac9ec8f62bd664004d29a121ba43.camel@hammerspace.com> (raw)
In-Reply-To: <ZdDnZmQ5_rAUm6fl@manet.1015granger.net>

On Sat, 2024-02-17 at 12:05 -0500, Chuck Lever wrote:
> On Fri, Feb 16, 2024 at 06:57:16PM +0000, Trond Myklebust wrote:
> > On Fri, 2024-02-16 at 18:25 +0000, Chuck Lever III wrote:
> > > 
> > > 
> > > > On Feb 16, 2024, at 1:19 PM, Chuck Lever III
> > > > <chuck.lever@oracle.com> wrote:
> > > > 
> > > > 
> > > > 
> > > > > On Feb 16, 2024, at 1:18 PM, Trond Myklebust
> > > > > <trondmy@hammerspace.com> wrote:
> > > > > 
> > > > > On Fri, 2024-02-16 at 08:33 -0500, Chuck Lever wrote:
> > > > > > On Thu, Feb 15, 2024 at 08:24:50PM -0500,
> > > > > > trondmy@kernel.org wrote:
> > > > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > > > > 
> > > > > > > Commit bb4d53d66e4b broke the NFSv3 pre/post op
> > > > > > > attributes
> > > > > > > behaviour
> > > > > > > when doing a SETATTR rpc call by stripping out the calls
> > > > > > > to
> > > > > > > fh_fill_pre_attrs() and fh_fill_post_attrs().
> > > > > > 
> > > > > > Can you give more detail about what broke?
> > > > > 
> > > > > Without the calls to fh_fill_pre_attrs() and
> > > > > fh_fill_post_attrs(), we
> > > > > don't store any pre/post op attributes and we can't return
> > > > > any
> > > > > such
> > > > > attributes to the NFSv3 client.
> > > > 
> > > > I get that. Why does that matter?
> > > 
> > > Or, to be a little less terse... clients rely on the pre/post
> > > op attributes around a SETATTR, I guess, but I don't see why.
> > > I'm missing some context.
> > 
> >    1. SETATTR is not atomic, and is not implemented as being atomic
> > in
> >       Linux. It is perfectly possible for, say, the file to get
> >       truncated, but for the other attribute changes to get dropped
> > on
> >       the floor. NFSv4 communicates that information via the
> > bitmap.
> >       NFSv3 does it using the pre/post attributes.
> >    2. When doing a guarded SETATTR, if the server returns
> >       NFS3ERR_NOT_SYNC, the client may want to update its cached
> > ctime
> >       and resend.
> 
> All granted, but I'm still not clear. Let me ask this a different
> way.
> 
> As far as I can tell, it's always been optional for an NFSv3 server
> to include pre- and post-op attributes in wcc_data. Both the
> pre_op_attr and post_op_attr XDR types start with an
> "attribute_follows" discriminator. Therefore clients cannot rely on
> receiving those attributes.
> 
> The patch description says that "Commit bb4d53d66e4b broke the NFSv3
>                                                      ^^^^^
> pre/post op attributes ...". And I think you also used the word
> "nasty" in an earlier email. So what is broken if the server /never/
> returns those attributes? What are the application-visible effects
> of the server behavior change in bb4d53d66e4b ?
> 
> I don't have a problem reverting that part of bb4d53d66e4b, but
> "broke" is doing some heavy lifting here. I want to understand why
> we need to revert. Because it seems to me the server's current NFSv3
> SETATTR implementation is spec-compliant. As far as I can tell,
> bb4d53d66e4b might result in a little more network traffic in some
> cases, but it won't impact interoperability or outcome.
> 

As I said above, you broke the NFSv3 client's ability to determine
whether or not the SETATTR was a failure, success or a partial success.
That's not heavy lifting, it is a fact.

The function nfsd_setattr() uses two different calls to notify_change()
in order to perform its function. Either one of them can return an
error. Either one of them can fail, and the way that the client finds
out whether or not the operation was a partial success is by examining
the pre/post op attributes (NFSv3) or the returned bitmap (NFSv4).

The patch does not try to fix NFSv4, since AFAICS, that code has always
been broken.

However, the NFSv3 code was not broken prior to bb4d53d66e4b. It was
correctly returning pre/post op attributes that reflected the
success/failure of the setattr operation. That is therefore a
regression.
Furthermore, it is a totally unnecessary regression. The whole point of
SETATTR is to change the value of the attributes of the exact same file
for which the pre/post op attributes are being retrieved. There is no
extra disk access required to retrieve those attributes, nor should
there be any other overhead other than copying them into the buffer.

> Do you mean that you want to restore the previous, more optimized,
> server behavior to return pre- and post-op attributes when they are
> available? And if so, what is the application-visible benefit?

Application correctness: the ability to see that your file got
truncated despite the RPC call returning an error.

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com



  reply	other threads:[~2024-02-17 18:16 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-02-16  1:24 [PATCH v2 0/2] Fix NFSv3 SETATTR behaviours trondmy
2024-02-16  1:24 ` [PATCH v2 1/2] nfsd: Fix a regression in nfsd_setattr() trondmy
2024-02-16  1:24   ` [PATCH v2 2/2] nfsd: Fix NFSv3 atomicity bugs " trondmy
2024-02-18 14:24     ` Jeff Layton
2024-02-18 22:01     ` NeilBrown
2024-02-16 13:33   ` [PATCH v2 1/2] nfsd: Fix a regression " Chuck Lever
2024-02-16 18:18     ` Trond Myklebust
2024-02-16 18:19       ` Chuck Lever III
2024-02-16 18:25         ` Chuck Lever III
2024-02-16 18:57           ` Trond Myklebust
2024-02-17 17:05             ` Chuck Lever
2024-02-17 18:16               ` Trond Myklebust [this message]
2024-02-18 14:21                 ` Jeff Layton
2024-02-18 21:57   ` NeilBrown
2024-02-19  1:33     ` Trond Myklebust
2024-02-20 15:28 ` [PATCH v2 0/2] Fix NFSv3 SETATTR behaviours Chuck Lever

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7b51a5725d82ac9ec8f62bd664004d29a121ba43.camel@hammerspace.com \
    --to=trondmy@hammerspace.com \
    --cc=chuck.lever@oracle.com \
    --cc=linux-nfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.