On 13 Jul 2023, at 17:27, Olga Kornievskaia wrote:
> On Thu, Jul 13, 2023 at 5:09 PM Benjamin Coddington <bcodding@xxxxxxxxxx> wrote:
>>
>> On 13 Jul 2023, at 15:54, Olga Kornievskaia wrote:
>>
>>> From: Olga Kornievskaia <kolga@xxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxx>
>>> ---
>>> 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
