NFSD: recall write delegation on GETATTR conflict
This patch series adds the recall of write delegation when there is
conflict with a GETATTR and a counter in /proc/net/rpc/nfsd to keep
count of this recall.
Changes from v1:
- add comment for nfsd4_deleg_getattr_conflict
- only wait 30ms for delegation to be returned before returing
NFS4ERR_DELAY
- fix test robot undeclared NFSD_STATS_WDELEG_GETATTR error
If the GETATTR request on a file that has write delegation in effect
and the request attributes include the change info and size attribute
then the write delegation is recalled. If the delegation is returned
within 30ms then the GETATTR is serviced as normal otherwise the
NFS4ERR_DELAY error is returned for the GETATTR.
Signed-off-by: Dai Ngo <[email protected]>
---
fs/nfsd/nfs4state.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
fs/nfsd/nfs4xdr.c | 5 +++++
fs/nfsd/state.h | 3 +++
3 files changed, 58 insertions(+)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index b90b74a5e66e..7826483e8421 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -8353,3 +8353,53 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
{
get_stateid(cstate, &u->write.wr_stateid);
}
+
+/**
+ * nfsd4_deleg_getattr_conflict - Trigger recall if GETATTR causes conflict
+ * @rqstp: RPC transaction context
+ * @inode: file to be checked for a conflict
+ *
+ * This function is called when there is a conflict between a write
+ * delegation and a change/size GETATR from another client. The server
+ * must either use the CB_GETATTR to get the current values of the
+ * attributes from the client that hold the delegation or recall the
+ * delegation before replying to the GETATTR. See RFC 8881 section
+ * 18.7.4.
+ *
+ * Returns 0 if there is no conflict; otherwise an nfs_stat
+ * code is returned.
+ */
+__be32
+nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode)
+{
+ __be32 status;
+ struct file_lock_context *ctx;
+ struct file_lock *fl;
+ struct nfs4_delegation *dp;
+
+ ctx = locks_inode_context(inode);
+ if (!ctx)
+ return 0;
+ spin_lock(&ctx->flc_lock);
+ list_for_each_entry(fl, &ctx->flc_lease, fl_list) {
+ if (fl->fl_flags == FL_LAYOUT ||
+ fl->fl_lmops != &nfsd_lease_mng_ops)
+ continue;
+ if (fl->fl_type == F_WRLCK) {
+ dp = fl->fl_owner;
+ if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
+ spin_unlock(&ctx->flc_lock);
+ return 0;
+ }
+ spin_unlock(&ctx->flc_lock);
+ status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
+ if (status != nfserr_jukebox ||
+ !nfsd_wait_for_delegreturn(rqstp, inode))
+ return status;
+ return 0;
+ }
+ break;
+ }
+ spin_unlock(&ctx->flc_lock);
+ return 0;
+}
diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
index b83954fc57e3..4590b893dbc8 100644
--- a/fs/nfsd/nfs4xdr.c
+++ b/fs/nfsd/nfs4xdr.c
@@ -2970,6 +2970,11 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
if (status)
goto out;
}
+ if (bmval0 & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
+ status = nfsd4_deleg_getattr_conflict(rqstp, d_inode(dentry));
+ if (status)
+ goto out;
+ }
err = vfs_getattr(&path, &stat,
STATX_BASIC_STATS | STATX_BTIME | STATX_CHANGE_COOKIE,
diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
index d49d3060ed4f..cbddcf484dba 100644
--- a/fs/nfsd/state.h
+++ b/fs/nfsd/state.h
@@ -732,4 +732,7 @@ static inline bool try_to_expire_client(struct nfs4_client *clp)
cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
return clp->cl_state == NFSD4_EXPIRABLE;
}
+
+extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp,
+ struct inode *inode);
#endif /* NFSD4_STATE_H */
--
2.9.5
Add counter to keep track of how many times write delegations are
recalled due to conflict with GETATTR.
Signed-off-by: Dai Ngo <[email protected]>
---
fs/nfsd/nfs4state.c | 1 +
fs/nfsd/stats.c | 2 ++
fs/nfsd/stats.h | 7 +++++++
3 files changed, 10 insertions(+)
diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 7826483e8421..829c68841d7d 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -8392,6 +8392,7 @@ nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode)
return 0;
}
spin_unlock(&ctx->flc_lock);
+ nfsd_stats_wdeleg_getattr_inc();
status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
if (status != nfserr_jukebox ||
!nfsd_wait_for_delegreturn(rqstp, inode))
diff --git a/fs/nfsd/stats.c b/fs/nfsd/stats.c
index 777e24e5da33..63797635e1c3 100644
--- a/fs/nfsd/stats.c
+++ b/fs/nfsd/stats.c
@@ -65,6 +65,8 @@ static int nfsd_show(struct seq_file *seq, void *v)
seq_printf(seq, " %lld",
percpu_counter_sum_positive(&nfsdstats.counter[NFSD_STATS_NFS4_OP(i)]));
}
+ seq_printf(seq, "\nwdeleg_getattr %lld",
+ percpu_counter_sum_positive(&nfsdstats.counter[NFSD_STATS_WDELEG_GETATTR]));
seq_putc(seq, '\n');
#endif
diff --git a/fs/nfsd/stats.h b/fs/nfsd/stats.h
index 9b43dc3d9991..cf5524e7ca06 100644
--- a/fs/nfsd/stats.h
+++ b/fs/nfsd/stats.h
@@ -22,6 +22,7 @@ enum {
NFSD_STATS_FIRST_NFS4_OP, /* count of individual nfsv4 operations */
NFSD_STATS_LAST_NFS4_OP = NFSD_STATS_FIRST_NFS4_OP + LAST_NFS4_OP,
#define NFSD_STATS_NFS4_OP(op) (NFSD_STATS_FIRST_NFS4_OP + (op))
+ NFSD_STATS_WDELEG_GETATTR, /* count of getattr conflict with wdeleg */
#endif
NFSD_STATS_COUNTERS_NUM
};
@@ -93,4 +94,10 @@ static inline void nfsd_stats_drc_mem_usage_sub(struct nfsd_net *nn, s64 amount)
percpu_counter_sub(&nn->counter[NFSD_NET_DRC_MEM_USAGE], amount);
}
+#ifdef CONFIG_NFSD_V4
+static inline void nfsd_stats_wdeleg_getattr_inc(void)
+{
+ percpu_counter_inc(&nfsdstats.counter[NFSD_STATS_WDELEG_GETATTR]);
+}
+#endif
#endif /* _NFSD_STATS_H */
--
2.9.5
On Mon, 2023-05-29 at 23:52 -0700, Dai Ngo wrote:
> If the GETATTR request on a file that has write delegation in effect
> and the request attributes include the change info and size attribute
> then the write delegation is recalled. If the delegation is returned
> within 30ms then the GETATTR is serviced as normal otherwise the
> NFS4ERR_DELAY error is returned for the GETATTR.
>
> Signed-off-by: Dai Ngo <[email protected]>
> ---
> fs/nfsd/nfs4state.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> fs/nfsd/nfs4xdr.c | 5 +++++
> fs/nfsd/state.h | 3 +++
> 3 files changed, 58 insertions(+)
>
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index b90b74a5e66e..7826483e8421 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -8353,3 +8353,53 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
> {
> get_stateid(cstate, &u->write.wr_stateid);
> }
> +
> +/**
> + * nfsd4_deleg_getattr_conflict - Trigger recall if GETATTR causes conflict
> + * @rqstp: RPC transaction context
> + * @inode: file to be checked for a conflict
> + *
> + * This function is called when there is a conflict between a write
> + * delegation and a change/size GETATR from another client. The server
> + * must either use the CB_GETATTR to get the current values of the
> + * attributes from the client that hold the delegation or recall the
> + * delegation before replying to the GETATTR. See RFC 8881 section
> + * 18.7.4.
> + *
> + * Returns 0 if there is no conflict; otherwise an nfs_stat
> + * code is returned.
> + */
> +__be32
> +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode)
> +{
> + __be32 status;
> + struct file_lock_context *ctx;
> + struct file_lock *fl;
> + struct nfs4_delegation *dp;
> +
> + ctx = locks_inode_context(inode);
> + if (!ctx)
> + return 0;
> + spin_lock(&ctx->flc_lock);
> + list_for_each_entry(fl, &ctx->flc_lease, fl_list) {
> + if (fl->fl_flags == FL_LAYOUT ||
> + fl->fl_lmops != &nfsd_lease_mng_ops)
> + continue;
> + if (fl->fl_type == F_WRLCK) {
> + dp = fl->fl_owner;
> + if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
> + spin_unlock(&ctx->flc_lock);
> + return 0;
> + }
> + spin_unlock(&ctx->flc_lock);
> + status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
> + if (status != nfserr_jukebox ||
> + !nfsd_wait_for_delegreturn(rqstp, inode))
> + return status;
> + return 0;
> + }
> + break;
> + }
> + spin_unlock(&ctx->flc_lock);
> + return 0;
> +}
If there is a lease held by a userland program (e.g. Samba), why don't
you want to break it here? Shouldn't it also be broken in this case?
I think this logic may be wrong. ISTM that you want to basically always
call nfsd_open_break_lease, unless it's a delegation held by the same
client.
> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> index b83954fc57e3..4590b893dbc8 100644
> --- a/fs/nfsd/nfs4xdr.c
> +++ b/fs/nfsd/nfs4xdr.c
> @@ -2970,6 +2970,11 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
> if (status)
> goto out;
> }
> + if (bmval0 & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
> + status = nfsd4_deleg_getattr_conflict(rqstp, d_inode(dentry));
> + if (status)
> + goto out;
> + }
>
> err = vfs_getattr(&path, &stat,
> STATX_BASIC_STATS | STATX_BTIME | STATX_CHANGE_COOKIE,
> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> index d49d3060ed4f..cbddcf484dba 100644
> --- a/fs/nfsd/state.h
> +++ b/fs/nfsd/state.h
> @@ -732,4 +732,7 @@ static inline bool try_to_expire_client(struct nfs4_client *clp)
> cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
> return clp->cl_state == NFSD4_EXPIRABLE;
> }
> +
> +extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp,
> + struct inode *inode);
> #endif /* NFSD4_STATE_H */
--
Jeff Layton <[email protected]>
On 5/30/23 3:29 AM, Jeff Layton wrote:
> On Mon, 2023-05-29 at 23:52 -0700, Dai Ngo wrote:
>> If the GETATTR request on a file that has write delegation in effect
>> and the request attributes include the change info and size attribute
>> then the write delegation is recalled. If the delegation is returned
>> within 30ms then the GETATTR is serviced as normal otherwise the
>> NFS4ERR_DELAY error is returned for the GETATTR.
>>
>> Signed-off-by: Dai Ngo <[email protected]>
>> ---
>> fs/nfsd/nfs4state.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
>> fs/nfsd/nfs4xdr.c | 5 +++++
>> fs/nfsd/state.h | 3 +++
>> 3 files changed, 58 insertions(+)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index b90b74a5e66e..7826483e8421 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -8353,3 +8353,53 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
>> {
>> get_stateid(cstate, &u->write.wr_stateid);
>> }
>> +
>> +/**
>> + * nfsd4_deleg_getattr_conflict - Trigger recall if GETATTR causes conflict
>> + * @rqstp: RPC transaction context
>> + * @inode: file to be checked for a conflict
>> + *
>> + * This function is called when there is a conflict between a write
>> + * delegation and a change/size GETATR from another client. The server
>> + * must either use the CB_GETATTR to get the current values of the
>> + * attributes from the client that hold the delegation or recall the
>> + * delegation before replying to the GETATTR. See RFC 8881 section
>> + * 18.7.4.
>> + *
>> + * Returns 0 if there is no conflict; otherwise an nfs_stat
>> + * code is returned.
>> + */
>> +__be32
>> +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode)
>> +{
>> + __be32 status;
>> + struct file_lock_context *ctx;
>> + struct file_lock *fl;
>> + struct nfs4_delegation *dp;
>> +
>> + ctx = locks_inode_context(inode);
>> + if (!ctx)
>> + return 0;
>> + spin_lock(&ctx->flc_lock);
>> + list_for_each_entry(fl, &ctx->flc_lease, fl_list) {
>> + if (fl->fl_flags == FL_LAYOUT ||
>> + fl->fl_lmops != &nfsd_lease_mng_ops)
>> + continue;
>> + if (fl->fl_type == F_WRLCK) {
>> + dp = fl->fl_owner;
>> + if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
>> + spin_unlock(&ctx->flc_lock);
>> + return 0;
>> + }
>> + spin_unlock(&ctx->flc_lock);
>> + status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
>> + if (status != nfserr_jukebox ||
>> + !nfsd_wait_for_delegreturn(rqstp, inode))
>> + return status;
>> + return 0;
>> + }
>> + break;
>> + }
>> + spin_unlock(&ctx->flc_lock);
>> + return 0;
>> +}
>
> If there is a lease held by a userland program (e.g. Samba), why don't
> you want to break it here? Shouldn't it also be broken in this case?
okay, I will make the change to also break non-nfs lease with F_WRLCK.
>
> I think this logic may be wrong. ISTM that you want to basically always
> call nfsd_open_break_lease, unless it's a delegation held by the same
> client.
I don't think we need to break any lease with F_RDLCK.
-Dai
>
>
>> diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
>> index b83954fc57e3..4590b893dbc8 100644
>> --- a/fs/nfsd/nfs4xdr.c
>> +++ b/fs/nfsd/nfs4xdr.c
>> @@ -2970,6 +2970,11 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
>> if (status)
>> goto out;
>> }
>> + if (bmval0 & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
>> + status = nfsd4_deleg_getattr_conflict(rqstp, d_inode(dentry));
>> + if (status)
>> + goto out;
>> + }
>>
>> err = vfs_getattr(&path, &stat,
>> STATX_BASIC_STATS | STATX_BTIME | STATX_CHANGE_COOKIE,
>> diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
>> index d49d3060ed4f..cbddcf484dba 100644
>> --- a/fs/nfsd/state.h
>> +++ b/fs/nfsd/state.h
>> @@ -732,4 +732,7 @@ static inline bool try_to_expire_client(struct nfs4_client *clp)
>> cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
>> return clp->cl_state == NFSD4_EXPIRABLE;
>> }
>> +
>> +extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp,
>> + struct inode *inode);
>> #endif /* NFSD4_STATE_H */
On Tue, 2023-05-30 at 15:04 -0700, [email protected] wrote:
> On 5/30/23 3:29 AM, Jeff Layton wrote:
> > On Mon, 2023-05-29 at 23:52 -0700, Dai Ngo wrote:
> > > If the GETATTR request on a file that has write delegation in effect
> > > and the request attributes include the change info and size attribute
> > > then the write delegation is recalled. If the delegation is returned
> > > within 30ms then the GETATTR is serviced as normal otherwise the
> > > NFS4ERR_DELAY error is returned for the GETATTR.
> > >
> > > Signed-off-by: Dai Ngo <[email protected]>
> > > ---
> > > fs/nfsd/nfs4state.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > fs/nfsd/nfs4xdr.c | 5 +++++
> > > fs/nfsd/state.h | 3 +++
> > > 3 files changed, 58 insertions(+)
> > >
> > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > index b90b74a5e66e..7826483e8421 100644
> > > --- a/fs/nfsd/nfs4state.c
> > > +++ b/fs/nfsd/nfs4state.c
> > > @@ -8353,3 +8353,53 @@ nfsd4_get_writestateid(struct nfsd4_compound_state *cstate,
> > > {
> > > get_stateid(cstate, &u->write.wr_stateid);
> > > }
> > > +
> > > +/**
> > > + * nfsd4_deleg_getattr_conflict - Trigger recall if GETATTR causes conflict
> > > + * @rqstp: RPC transaction context
> > > + * @inode: file to be checked for a conflict
> > > + *
> > > + * This function is called when there is a conflict between a write
> > > + * delegation and a change/size GETATR from another client. The server
> > > + * must either use the CB_GETATTR to get the current values of the
> > > + * attributes from the client that hold the delegation or recall the
> > > + * delegation before replying to the GETATTR. See RFC 8881 section
> > > + * 18.7.4.
> > > + *
> > > + * Returns 0 if there is no conflict; otherwise an nfs_stat
> > > + * code is returned.
> > > + */
> > > +__be32
> > > +nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp, struct inode *inode)
> > > +{
> > > + __be32 status;
> > > + struct file_lock_context *ctx;
> > > + struct file_lock *fl;
> > > + struct nfs4_delegation *dp;
> > > +
> > > + ctx = locks_inode_context(inode);
> > > + if (!ctx)
> > > + return 0;
> > > + spin_lock(&ctx->flc_lock);
> > > + list_for_each_entry(fl, &ctx->flc_lease, fl_list) {
> > > + if (fl->fl_flags == FL_LAYOUT ||
> > > + fl->fl_lmops != &nfsd_lease_mng_ops)
> > > + continue;
> > > + if (fl->fl_type == F_WRLCK) {
> > > + dp = fl->fl_owner;
> > > + if (dp->dl_recall.cb_clp == *(rqstp->rq_lease_breaker)) {
> > > + spin_unlock(&ctx->flc_lock);
> > > + return 0;
> > > + }
> > > + spin_unlock(&ctx->flc_lock);
> > > + status = nfserrno(nfsd_open_break_lease(inode, NFSD_MAY_READ));
> > > + if (status != nfserr_jukebox ||
> > > + !nfsd_wait_for_delegreturn(rqstp, inode))
> > > + return status;
> > > + return 0;
> > > + }
> > > + break;
> > > + }
> > > + spin_unlock(&ctx->flc_lock);
> > > + return 0;
> > > +}
> >
> > If there is a lease held by a userland program (e.g. Samba), why don't
> > you want to break it here? Shouldn't it also be broken in this case?
>
> okay, I will make the change to also break non-nfs lease with F_WRLCK.
>
Sounds good.
> >
> > I think this logic may be wrong. ISTM that you want to basically always
> > call nfsd_open_break_lease, unless it's a delegation held by the same
> > client.
>
> I don't think we need to break any lease with F_RDLCK.
>
Correct. To be clear: break any write lease unless it's a write
delegation held by the same client.
>
> >
> >
> > > diff --git a/fs/nfsd/nfs4xdr.c b/fs/nfsd/nfs4xdr.c
> > > index b83954fc57e3..4590b893dbc8 100644
> > > --- a/fs/nfsd/nfs4xdr.c
> > > +++ b/fs/nfsd/nfs4xdr.c
> > > @@ -2970,6 +2970,11 @@ nfsd4_encode_fattr(struct xdr_stream *xdr, struct svc_fh *fhp,
> > > if (status)
> > > goto out;
> > > }
> > > + if (bmval0 & (FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE)) {
> > > + status = nfsd4_deleg_getattr_conflict(rqstp, d_inode(dentry));
> > > + if (status)
> > > + goto out;
> > > + }
> > >
> > > err = vfs_getattr(&path, &stat,
> > > STATX_BASIC_STATS | STATX_BTIME | STATX_CHANGE_COOKIE,
> > > diff --git a/fs/nfsd/state.h b/fs/nfsd/state.h
> > > index d49d3060ed4f..cbddcf484dba 100644
> > > --- a/fs/nfsd/state.h
> > > +++ b/fs/nfsd/state.h
> > > @@ -732,4 +732,7 @@ static inline bool try_to_expire_client(struct nfs4_client *clp)
> > > cmpxchg(&clp->cl_state, NFSD4_COURTESY, NFSD4_EXPIRABLE);
> > > return clp->cl_state == NFSD4_EXPIRABLE;
> > > }
> > > +
> > > +extern __be32 nfsd4_deleg_getattr_conflict(struct svc_rqst *rqstp,
> > > + struct inode *inode);
> > > #endif /* NFSD4_STATE_H */
--
Jeff Layton <[email protected]>