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;
From:   Benjamin Coddington <bcodding@redhat.com>
To:     Olga Kornievskaia <olga.kornievskaia@gmail.com>
Cc:     trond.myklebust@hammerspace.com, anna.schumaker@netapp.com,
        linux-nfs@vger.kernel.org
Subject: Re: [PATCH 1/1] NFSv4.1: fix zero value filehandle in post open
 getattr
Date:   Thu, 13 Jul 2023 17:16:49 -0400
Message-ID: <2C013A73-79B2-446E-885B-33EF05916CCA@redhat.com>
In-Reply-To: <A0663B9C-F005-4089-ABD6-542F77EE43ED@redhat.com>
References: <20230713195416.30414-1-olga.kornievskaia@gmail.com>
 <A0663B9C-F005-4089-ABD6-542F77EE43ED@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On 13 Jul 2023, at 17:08, Benjamin Coddington 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_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 <bcodding@redhat.com>

.. and should we handle the other OPEN_CLAIM cases that use the filehandle
as well?

Ben