From: Olga Kornievskaia <[email protected]>
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 <[email protected]>
---
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_opendata *data,
return status;
}
if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
+ struct nfs_fh *fh = &o_res->fh;
+
nfs4_sequence_free_slot(&o_res->seq_res);
- nfs4_proc_getattr(server, &o_res->fh, o_res->f_attr, NULL);
+ if (o_arg->claim == NFS4_OPEN_CLAIM_FH)
+ fh = NFS_FH(d_inode(data->dentry));
+ nfs4_proc_getattr(server, fh, o_res->f_attr, NULL);
}
return 0;
}
--
2.39.1
On 13 Jul 2023, at 15:54, Olga Kornievskaia wrote:
> From: Olga Kornievskaia <[email protected]>
>
> 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 <[email protected]>
> ---
> 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_opendata *data,
> return status;
> }
> if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
> + struct nfs_fh *fh = &o_res->fh;
> +
> nfs4_sequence_free_slot(&o_res->seq_res);
> - nfs4_proc_getattr(server, &o_res->fh, o_res->f_attr, NULL);
> + if (o_arg->claim == NFS4_OPEN_CLAIM_FH)
> + fh = 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.
Reviewed-by: Benjamin Coddington <[email protected]>
Ben
On 13 Jul 2023, at 17:08, Benjamin Coddington wrote:
> On 13 Jul 2023, at 15:54, Olga Kornievskaia wrote:
>
>> From: Olga Kornievskaia <[email protected]>
>>
>> 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 <[email protected]>
>> ---
>> 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_opendata *data,
>> return status;
>> }
>> if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
>> + struct nfs_fh *fh = &o_res->fh;
>> +
>> nfs4_sequence_free_slot(&o_res->seq_res);
>> - nfs4_proc_getattr(server, &o_res->fh, o_res->f_attr, NULL);
>> + if (o_arg->claim == NFS4_OPEN_CLAIM_FH)
>> + fh = 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.
>
> Reviewed-by: Benjamin Coddington <[email protected]>
.. and should we handle the other OPEN_CLAIM cases that use the filehandle
as well?
Ben
On Thu, Jul 13, 2023 at 5:09 PM Benjamin Coddington <[email protected]> wrote:
>
> On 13 Jul 2023, at 15:54, Olga Kornievskaia wrote:
>
> > From: Olga Kornievskaia <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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_opendata *data,
> > return status;
> > }
> > if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
> > + struct nfs_fh *fh = &o_res->fh;
> > +
> > nfs4_sequence_free_slot(&o_res->seq_res);
> > - nfs4_proc_getattr(server, &o_res->fh, o_res->f_attr, NULL);
> > + if (o_arg->claim == NFS4_OPEN_CLAIM_FH)
> > + fh = 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:
Jul 13 17:25:32 localhost kernel: ------------[ cut here ]------------
Jul 13 17:25:32 localhost kernel: WARNING: CPU: 0 PID: 5458 at
fs/nfs/nfs4xdr.c:978 encode_string+0x4c/0x68 [nfsv4]
Jul 13 17:25:32 localhost kernel: Modules linked in: rpcsec_gss_krb5
auth_rpcgss nfsv4 dns_resolver nfs lockd grace uinput nft_fib_inet
nft_fib_ipv4 nft_fib_ipv6 nft_fib nft_reject_inet nf_reject_ipv4
nf_reject_ipv6 nft_reject nft_ct nft_chain_nat nf_nat nf_conntrack
nf_defrag_ipv6 nf_defrag_ipv4 rfkill ip_set nf_tables nfnetlink qrtr
vsock_loopback vmw_vsock_virtio_transport_common
vmw_vsock_vmci_transport vsock sunrpc vfat fat vmw_vmci xfs libcrc32c
vmwgfx drm_ttm_helper ttm drm_kms_helper nvme crct10dif_ce sr_mod
cdrom sg syscopyarea nvme_core sysfillrect sysimgblt ghash_ce sha2_ce
e1000e drm sha256_arm64 sha1_ce nvme_common fuse
Jul 13 17:25:32 localhost kernel: CPU: 0 PID: 5458 Comm: cat Kdump:
loaded Not tainted 6.4.0-rc7+ #11
Jul 13 17:25:32 localhost kernel: Hardware name: VMware, Inc.
VMware20,1/VBSA, BIOS VMW201.00V.20904234.BA64.2212051119 12/05/2022
Jul 13 17:25:32 localhost kernel: pstate: 21400005 (nzCv daif +PAN
-UAO -TCO +DIT -SSBS BTYPE=--)
Jul 13 17:25:32 localhost kernel: pc : encode_string+0x4c/0x68 [nfsv4]
Jul 13 17:25:32 localhost kernel: lr : encode_string+0x30/0x68 [nfsv4]
Jul 13 17:25:32 localhost kernel: sp : ffff80000ccab400
Jul 13 17:25:32 localhost kernel: x29: ffff80000ccab400 x28:
ffff00008cadf008 x27: ffff0000866f4e30
Jul 13 17:25:32 localhost kernel: x26: 0000000000440000 x25:
0000000000000000 x24: ffff80000949f008
Jul 13 17:25:32 localhost kernel: x23: ffff800001200c30 x22:
ffff00008a4ef830 x21: ffff00008cadf018
Jul 13 17:25:32 localhost kernel: x20: ffff00008cadf01a x19:
0000000000000858 x18: 0000000000000000
Jul 13 17:25:32 localhost kernel: x17: 0000000000000000 x16:
0000000000000000 x15: 0000000000000000
Jul 13 17:25:32 localhost kernel: x14: ffffffffffffffff x13:
0000000000000007 x12: 93449b0e64a317a5
Jul 13 17:25:32 localhost kernel: x11: 00000006e6fbcafb x10:
0000000000000e10 x9 : ffff800001212040
Jul 13 17:25:32 localhost kernel: x8 : ffff00008a4ef8e0 x7 :
0000000000000007 x6 : 93449b0e64a317a5
Jul 13 17:25:32 localhost kernel: x5 : ffff00008824b08c x4 :
ffff000082196008 x3 : ffff80000ccab4c0
Jul 13 17:25:32 localhost kernel: x2 : 000000000000021c x1 :
000000000000085c x0 : 0000000000000000
Jul 13 17:25:32 localhost kernel: Call trace:
Jul 13 17:25:32 localhost kernel: encode_string+0x4c/0x68 [nfsv4]
Jul 13 17:25:32 localhost kernel: nfs4_xdr_enc_getattr+0xb0/0x108 [nfsv4]
Jul 13 17:25:32 localhost kernel: rpcauth_wrap_req_encode+0x20/0x38 [sunrpc]
Jul 13 17:25:32 localhost kernel: rpcauth_wrap_req+0x24/0x38 [sunrpc]
Jul 13 17:25:32 localhost kernel: call_encode+0x180/0x258 [sunrpc]
Jul 13 17:25:32 localhost kernel: __rpc_execute+0xb8/0x3e0 [sunrpc]
Jul 13 17:25:32 localhost kernel: rpc_execute+0x160/0x1d8 [sunrpc]
Jul 13 17:25:32 localhost kernel: rpc_run_task+0x148/0x1f8 [sunrpc]
Jul 13 17:25:32 localhost kernel: nfs4_do_call_sync+0x78/0xc8 [nfsv4]
Jul 13 17:25:32 localhost kernel: _nfs4_proc_getattr+0xe8/0x120 [nfsv4]
Jul 13 17:25:32 localhost kernel: nfs4_proc_getattr+0x7c/0x140 [nfsv4]
Jul 13 17:25:32 localhost kernel: _nfs4_open_and_get_state+0x1d8/0x460 [nfsv4]
Jul 13 17:25:32 localhost kernel: _nfs4_do_open.isra.0+0x140/0x408 [nfsv4]
Jul 13 17:25:32 localhost kernel: nfs4_do_open+0x9c/0x238 [nfsv4]
Jul 13 17:25:32 localhost kernel: nfs4_atomic_open+0x100/0x118 [nfsv4]
Jul 13 17:25:32 localhost kernel: nfs4_file_open+0x11c/0x240 [nfsv4]
Jul 13 17:25:32 localhost kernel: do_dentry_open+0x140/0x490
Jul 13 17:25:32 localhost kernel: vfs_open+0x30/0x38
Jul 13 17:25:32 localhost kernel: do_open+0x14c/0x360
Jul 13 17:25:32 localhost kernel: path_openat+0x104/0x250
Jul 13 17:25:32 localhost kernel: do_filp_open+0x84/0x138
Jul 13 17:25:32 localhost kernel: do_sys_openat2+0xb4/0x170
Jul 13 17:25:32 localhost kernel: __arm64_sys_openat+0x64/0xb0
Jul 13 17:25:32 localhost kernel: invoke_syscall.constprop.0+0x7c/0xd0
Jul 13 17:25:32 localhost kernel: el0_svc_common.constprop.0+0x13c/0x158
Jul 13 17:25:32 localhost kernel: do_el0_svc+0x38/0xa8
Jul 13 17:25:32 localhost kernel: el0_svc+0x3c/0x198
Jul 13 17:25:32 localhost kernel: el0t_64_sync_handler+0xb4/0x130
Jul 13 17:25:32 localhost kernel: el0t_64_sync+0x17c/0x180
Jul 13 17:25:32 localhost kernel: ---[ end trace 0000000000000000 ]---
> Reviewed-by: Benjamin Coddington <[email protected]>
>
> Ben
>
On Thu, Jul 13, 2023 at 5:16 PM Benjamin Coddington <[email protected]> wrote:
>
> On 13 Jul 2023, at 17:08, Benjamin Coddington wrote:
>
> > On 13 Jul 2023, at 15:54, Olga Kornievskaia wrote:
> >
> >> From: Olga Kornievskaia <[email protected]>
> >>
> >> 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 <[email protected]>
> >> ---
> >> 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_opendata *data,
> >> return status;
> >> }
> >> if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
> >> + struct nfs_fh *fh = &o_res->fh;
> >> +
> >> nfs4_sequence_free_slot(&o_res->seq_res);
> >> - nfs4_proc_getattr(server, &o_res->fh, o_res->f_attr, NULL);
> >> + if (o_arg->claim == NFS4_OPEN_CLAIM_FH)
> >> + fh = 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.
> >
> > Reviewed-by: Benjamin Coddington <[email protected]>
>
> .. and should we handle the other OPEN_CLAIM cases that use the filehandle
> as well?
I was pondering about that but I think the other OPEN CLAIM that use
the FH directly are recovery opens. I'm not sure if they need this.
>
> Ben
>
On 13 Jul 2023, at 17:27, Olga Kornievskaia wrote:
> On Thu, Jul 13, 2023 at 5:09 PM Benjamin Coddington <[email protected]> wrote:
>>
>> On 13 Jul 2023, at 15:54, Olga Kornievskaia wrote:
>>
>>> From: Olga Kornievskaia <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> 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_opendata *data,
>>> return status;
>>> }
>>> if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
>>> + struct nfs_fh *fh = &o_res->fh;
>>> +
>>> nfs4_sequence_free_slot(&o_res->seq_res);
>>> - nfs4_proc_getattr(server, &o_res->fh, o_res->f_attr, NULL);
>>> + if (o_arg->claim == NFS4_OPEN_CLAIM_FH)
>>> + fh = 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 cleaned
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 suspect we'd have the o_res.fh from nfs4_opendata_get_inode(). Out of
time, will check back in tomorrow.
Ben
On Thu, Jul 13, 2023 at 6:13 PM Benjamin Coddington <[email protected]> wrote:
>
> On 13 Jul 2023, at 17:27, Olga Kornievskaia wrote:
>
> > On Thu, Jul 13, 2023 at 5:09 PM Benjamin Coddington <[email protected]> wrote:
> >>
> >> On 13 Jul 2023, at 15:54, Olga Kornievskaia wrote:
> >>
> >>> From: Olga Kornievskaia <[email protected]>
> >>>
> >>> 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 <[email protected]>
> >>> ---
> >>> 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_opendata *data,
> >>> return status;
> >>> }
> >>> if (!(o_res->f_attr->valid & NFS_ATTR_FATTR)) {
> >>> + struct nfs_fh *fh = &o_res->fh;
> >>> +
> >>> nfs4_sequence_free_slot(&o_res->seq_res);
> >>> - nfs4_proc_getattr(server, &o_res->fh, o_res->f_attr, NULL);
> >>> + if (o_arg->claim == NFS4_OPEN_CLAIM_FH)
> >>> + fh = 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 cleaned
> 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
>
On 14 Jul 2023, at 9:57, Olga Kornievskaia wrote:
>
> I guess I don't understand what's the problem with the current
> approach which is simple.
No problem - I'm just bad at code review.
I was trying to get my head around why the o_arg->fh couldn't be used, and
then wondering why you were hitting that warning.
Then I worried that the opendata was already freed at the point the getattr
was trying to use it again, and because its a codepath I can't exercise
easily my normal approach of just throwing in debug statements didn't work -
but I think the opendata is still there because _nfs4_do_open has it pinned.
Ben