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;
MIME-Version: 1.0
References: <20230713195416.30414-1-olga.kornievskaia@gmail.com>
 <A0663B9C-F005-4089-ABD6-542F77EE43ED@redhat.com> <CAN-5tyGa1dV1A5RgZsMCQzFHDV63=LDJq0DTpg8aJ=UCO+k+Og@mail.gmail.com>
 <A05D90E0-6D74-4948-B948-852B1448DD3C@redhat.com>
In-Reply-To: <A05D90E0-6D74-4948-B948-852B1448DD3C@redhat.com>
From:   Olga Kornievskaia <olga.kornievskaia@gmail.com>
Date:   Fri, 14 Jul 2023 09:57:05 -0400
Message-ID: <CAN-5tyGiiSr+jkh=WL_C4=wBAfZUiQZVsJoVxn0Sxg=yK+TA9g@mail.gmail.com>
Subject: Re: [PATCH 1/1] NFSv4.1: fix zero value filehandle in post open getattr
To:     Benjamin Coddington <bcodding@redhat.com>
Cc:     trond.myklebust@hammerspace.com, anna.schumaker@netapp.com,
        linux-nfs@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Thu, Jul 13, 2023 at 6:13=E2=80=AFPM Benjamin Coddington <bcodding@redha=
t.com> wrote:
>
> On 13 Jul 2023, at 17:27, Olga Kornievskaia wrote:
>
> > On Thu, Jul 13, 2023 at 5:09=E2=80=AFPM Benjamin Coddington <bcodding@r=
edhat.com> wrote:
> >>
> >> On 13 Jul 2023, at 15:54, Olga Kornievskaia wrote:
> >>
> >>> From: Olga Kornievskaia <kolga@netapp.com>
> >>>
> >>> Currently, if the OPEN compound experiencing an error and needs to
> >>> get the file attributes separately, it will send a stand alone
> >>> GETATTR but it would use the filehandle from the results of
> >>> the OPEN compound. In case of the CLAIM_FH OPEN, nfs_openres's fh
> >>> is zero value. That generate a GETATTR that's sent with a zero
> >>> value filehandle, and results in the server returning an error.
> >>>
> >>> Instead, for the CLAIM_FH OPEN, take the filehandle that was used
> >>> in the PUTFH of the OPEN compound.
> >>>
> >>> Signed-off-by: Olga Kornievskaia <kolga@netapp.com>
> >>> ---
> >>>  fs/nfs/nfs4proc.c | 6 +++++-
> >>>  1 file changed, 5 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
> >>> index 8edc610dc1d3..0b1b49f01c5b 100644
> >>> --- a/fs/nfs/nfs4proc.c
> >>> +++ b/fs/nfs/nfs4proc.c
> >>> @@ -2703,8 +2703,12 @@ static int _nfs4_proc_open(struct nfs4_opendat=
a *data,
> >>>                       return status;
> >>>       }
> >>>       if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
> >>> +             struct nfs_fh *fh =3D &o_res->fh;
> >>> +
> >>>               nfs4_sequence_free_slot(&o_res->seq_res);
> >>> -             nfs4_proc_getattr(server, &o_res->fh, o_res->f_attr, NU=
LL);
> >>> +             if (o_arg->claim =3D=3D NFS4_OPEN_CLAIM_FH)
> >>> +                     fh =3D NFS_FH(d_inode(data->dentry));
> >>> +             nfs4_proc_getattr(server, fh, o_res->f_attr, NULL);
> >>>       }
> >>>       return 0;
> >>>  }
> >>
> >> Looks good, but why not just use o_arg->fh?  Maybe also an opportunity
> >> to fix the whitespace damage a few lines before this hunk too.
> >>
> >
> > I did try it first. I had 2 problems with it. First of o_arg->fh is a
> > "const struct" so it wouldn't allow me to be assigned without casting.
> > Ok so perhaps it's not a big deal that we are going against the
> > "const". Second of all, when I did do that, it produced the following
> > warning and so I thought perhaps I should really use the original fh
> > instead of what's in the args:
>
> Oh maybe because this is the error path and nfs4_opendata is getting clea=
ned
> up in nfs4_open_release()?  The comments in nfs4_open_release are a bit
> confusing, but I think for the cases where we need to re-use the opendata=
 we
> are doing a kref_get on it.  Maybe we need a kref_get on the opendata for
> this case?

I guess I don't understand what's the problem with the current
approach which is simple.

> .. I suspect we'd have the o_res.fh from nfs4_opendata_get_inode().  Out =
of
> time, will check back in tomorrow.
>
> Ben
>