Re: [PATCH 01/14] nfsd: Add layouts checking for state resources

["J. Bruce Fields" <bfields@fieldses.org>]

On Thu, Jul 16, 2015 at 10:30:37AM +0800, Kinglong Mee wrote:
> On 7/15/2015 23:03, J. Bruce Fields wrote:
> > On Mon, Jul 13, 2015 at 05:29:01PM +0800, Kinglong Mee wrote:
> >> Layout is a state resource, nfsd should check it too.
> >>
> >> Signed-off-by: Kinglong Mee <kinglongmee@gmail.com>
> >> ---
> >>  fs/nfsd/nfs4state.c | 6 ++++--
> >>  1 file changed, 4 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> >> index 61dfb33..e0a4556 100644
> >> --- a/fs/nfsd/nfs4state.c
> >> +++ b/fs/nfsd/nfs4state.c
> >> @@ -2241,6 +2241,7 @@ static bool client_has_state(struct nfs4_client *clp)
> >>  	 * Also note we should probably be using this in 4.0 case too.
> >>  	 */
> >>  	return !list_empty(&clp->cl_openowners)
> >> +		|| !list_empty(&clp->cl_lo_states)
> >>  		|| !list_empty(&clp->cl_delegations)
> >>  		|| !list_empty(&clp->cl_sessions);
> >>  }
> >> @@ -4257,8 +4258,9 @@ nfsd4_renew(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
> >>  		goto out;
> >>  	clp = cstate->clp;
> >>  	status = nfserr_cb_path_down;
> >> -	if (!list_empty(&clp->cl_delegations)
> >> -			&& clp->cl_cb_state != NFSD4_CB_UP)
> >> +	if ((!list_empty(&clp->cl_delegations)
> >> +	     || !list_empty(&clp->cl_lo_states))
> > 
> > nfsd4_renew is NFSv4.0 only, so I don't think this part is necessary.
> > 
> > That said, it's harmless--why don't we just call client_has_state() here
> > instead?
> 
> I don't think so.
> 
> NFSv4.0's callback path is used for delegation recalling only,
> without delegation, callback path is useless.
> 
> Also, describing in rfc3530,
> 
>    When the client holds delegations, it needs to use RENEW to detect
>    when the server has determined that the callback path is down.  When
>    the server has made such a determination, only the RENEW operation
>    will renew the lease on delegations.  If the server determines the
>    callback path is down, it returns NFS4ERR_CB_PATH_DOWN.  Even though
>    it returns NFS4ERR_CB_PATH_DOWN, the server MUST renew the lease on
>    the record locks and share reservations that the client has
>    established on the server.
> 
> I will drop the second modify in version 2.

Oh, you're right, the cl_openowners check would be wrong for the renew
case.

Applying v2, thanks.--b.

> 
> thanks,
> Kinglong Mee