[PATCH v3 0/2] NFSD: add support for NFSv4 write delegation

[PATCH v3 0/2] NFSD: add support for NFSv4 write delegation

[Dai Ngo]

NFSD: add support for NFSv4 write delegation

The NFSv4 server currently supports read delegation using VFS lease
which is implemented using file_lock. 

This patch series add write delegation support for NFSv4 server by:

    . remove the check for F_WRLCK in generic_add_lease to allow
      file_lock to be used for write delegation.  

    . grant write delegation for OPEN with NFS4_SHARE_ACCESS_WRITE
      if there is no conflict with other OPENs.

Write delegation conflict with another OPEN, REMOVE, RENAME and SETATTR
are handled the same as read delegation using notify_change, try_break_deleg.

Changes since v1:

[PATCH 3/4] NFSD: add supports for CB_GETATTR callback
- remove WARN_ON_ONCE from encode_bitmap4
- replace decode_bitmap4 with xdr_stream_decode_uint32_array
- replace xdr_inline_decode and xdr_decode_hyper in decode_cb_getattr
   with xdr_stream_decode_u64. Also remove the un-needed likely().
- modify signature of encode_cb_getattr4args to take pointer to
   nfs4_cb_fattr
- replace decode_attr_length with xdr_stream_decode_u32
- rename decode_cb_getattr to decode_cb_fattr4
- fold the initialization of cb_cinfo and cb_fsize into decode_cb_fattr4
- rename ncf_cb_cinfo to ncf_cb_change to avoid confusion of cindo usage
  in fs/nfsd/nfs4xdr.c
- correct NFS4_dec_cb_getattr_sz and update size description

[PATCH 4/4] NFSD: handle GETATTR conflict with write delegation
- change nfs4_handle_wrdeleg_conflict returns __be32 to fix test robot
- change ncf_cb_cinfo to ncf_cb_change to avoid confusion of cindo usage
  in fs/nfsd/nfs4xdr.c

Changes since v2:

[PATCH 2/4] NFSD: enable support for write delegation
- rename 'deleg' to 'dl_type' in nfs4_set_delegation
- remove 'wdeleg' in nfs4_open_delegation

- drop [PATCH 3/4] NFSD: add supports for CB_GETATTR callback
  and [PATCH 4/4] NFSD: handle GETATTR conflict with write delegation
  for futher clarification of the benefits of using CB_GETATTR
  for handling GETATTR from the 2nd client

[PATCH v3 1/2] locks: allow support for write delegation

[Dai Ngo]

Remove the check for F_WRLCK in generic_add_lease to allow file_lock
to be used for write delegation.

First consumer is NFSD.

Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 fs/locks.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/fs/locks.c b/fs/locks.c
index df8b26a42524..08fb0b4fd4f8 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1729,13 +1729,6 @@ generic_add_lease(struct file *filp, long arg, struct file_lock **flp, void **pr
 	if (is_deleg && !inode_trylock(inode))
 		return -EAGAIN;
 
-	if (is_deleg && arg == F_WRLCK) {
-		/* Write delegations are not currently supported: */
-		inode_unlock(inode);
-		WARN_ON_ONCE(1);
-		return -EINVAL;
-	}
-
 	percpu_down_read(&file_rwsem);
 	spin_lock(&ctx->flc_lock);
 	time_out_leases(inode, &dispose);
-- 
2.9.5

[PATCH v3 2/2] NFSD: enable support for write delegation

[Dai Ngo]

This patch grants write delegation for OPEN with NFS4_SHARE_ACCESS_WRITE
if there is no conflict with other OPENs.

Write delegation conflict with another OPEN, REMOVE, RENAME and SETATTR
are handled the same as read delegation using notify_change,
try_break_deleg.

Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
---
 fs/nfsd/nfs4state.c | 24 ++++++++++++++++--------
 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
index 6e61fa3acaf1..09a9e16407f9 100644
--- a/fs/nfsd/nfs4state.c
+++ b/fs/nfsd/nfs4state.c
@@ -1144,7 +1144,7 @@ static void block_delegations(struct knfsd_fh *fh)
 
 static struct nfs4_delegation *
 alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
-		 struct nfs4_clnt_odstate *odstate)
+		struct nfs4_clnt_odstate *odstate, u32 dl_type)
 {
 	struct nfs4_delegation *dp;
 	long n;
@@ -1170,7 +1170,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
 	INIT_LIST_HEAD(&dp->dl_recall_lru);
 	dp->dl_clnt_odstate = odstate;
 	get_clnt_odstate(odstate);
-	dp->dl_type = NFS4_OPEN_DELEGATE_READ;
+	dp->dl_type = dl_type;
 	dp->dl_retries = 1;
 	dp->dl_recalled = false;
 	nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client,
@@ -5451,6 +5451,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 	struct nfs4_delegation *dp;
 	struct nfsd_file *nf;
 	struct file_lock *fl;
+	u32 deleg;
 
 	/*
 	 * The fi_had_conflict and nfs_get_existing_delegation checks
@@ -5460,7 +5461,13 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 	if (fp->fi_had_conflict)
 		return ERR_PTR(-EAGAIN);
 
-	nf = find_readable_file(fp);
+	if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
+		nf = find_writeable_file(fp);
+		deleg = NFS4_OPEN_DELEGATE_WRITE;
+	} else {
+		nf = find_readable_file(fp);
+		deleg = NFS4_OPEN_DELEGATE_READ;
+	}
 	if (!nf) {
 		/*
 		 * We probably could attempt another open and get a read
@@ -5491,11 +5498,11 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 		return ERR_PTR(status);
 
 	status = -ENOMEM;
-	dp = alloc_init_deleg(clp, fp, odstate);
+	dp = alloc_init_deleg(clp, fp, odstate, deleg);
 	if (!dp)
 		goto out_delegees;
 
-	fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ);
+	fl = nfs4_alloc_init_lease(dp, deleg);
 	if (!fl)
 		goto out_clnt_odstate;
 
@@ -5583,6 +5590,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 	struct svc_fh *parent = NULL;
 	int cb_up;
 	int status = 0;
+	u32 wdeleg = false;
 
 	cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client);
 	open->op_recall = 0;
@@ -5590,8 +5598,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 		case NFS4_OPEN_CLAIM_PREVIOUS:
 			if (!cb_up)
 				open->op_recall = 1;
-			if (open->op_delegate_type != NFS4_OPEN_DELEGATE_READ)
-				goto out_no_deleg;
 			break;
 		case NFS4_OPEN_CLAIM_NULL:
 			parent = currentfh;
@@ -5617,7 +5623,9 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
 	memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
 
 	trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
-	open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
+	wdeleg = open->op_share_access & NFS4_SHARE_ACCESS_WRITE;
+	open->op_delegate_type = wdeleg ?
+			NFS4_OPEN_DELEGATE_WRITE : NFS4_OPEN_DELEGATE_READ;
 	nfs4_put_stid(&dp->dl_stid);
 	return;
 out_no_deleg:
-- 
2.9.5

Re: [PATCH v3 2/2] NFSD: enable support for write delegation

[Chuck Lever III]

> On May 17, 2023, at 7:38 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
> 
> This patch grants write delegation for OPEN with NFS4_SHARE_ACCESS_WRITE
> if there is no conflict with other OPENs.
> 
> Write delegation conflict with another OPEN, REMOVE, RENAME and SETATTR
> are handled the same as read delegation using notify_change,
> try_break_deleg.

Very clean. A couple of suggestions, one is down below, and here is
the other:

I was thinking we should add one or two counters in fs/nfsd/stats.c
to track how often read and write delegations are offered, and
perhaps one to count the number of DELEGRETURN operations. What do
you think makes sense?


> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
> ---
> fs/nfsd/nfs4state.c | 24 ++++++++++++++++--------
> 1 file changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> index 6e61fa3acaf1..09a9e16407f9 100644
> --- a/fs/nfsd/nfs4state.c
> +++ b/fs/nfsd/nfs4state.c
> @@ -1144,7 +1144,7 @@ static void block_delegations(struct knfsd_fh *fh)
> 
> static struct nfs4_delegation *
> alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
> - struct nfs4_clnt_odstate *odstate)
> + struct nfs4_clnt_odstate *odstate, u32 dl_type)
> {
> struct nfs4_delegation *dp;
> long n;
> @@ -1170,7 +1170,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
> INIT_LIST_HEAD(&dp->dl_recall_lru);
> dp->dl_clnt_odstate = odstate;
> get_clnt_odstate(odstate);
> - dp->dl_type = NFS4_OPEN_DELEGATE_READ;
> + dp->dl_type = dl_type;
> dp->dl_retries = 1;
> dp->dl_recalled = false;
> nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client,
> @@ -5451,6 +5451,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> struct nfs4_delegation *dp;
> struct nfsd_file *nf;
> struct file_lock *fl;
> + u32 deleg;
> 
> /*
> * The fi_had_conflict and nfs_get_existing_delegation checks
> @@ -5460,7 +5461,13 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> if (fp->fi_had_conflict)
> return ERR_PTR(-EAGAIN);
> 
> - nf = find_readable_file(fp);
> + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
> + nf = find_writeable_file(fp);
> + deleg = NFS4_OPEN_DELEGATE_WRITE;
> + } else {
> + nf = find_readable_file(fp);
> + deleg = NFS4_OPEN_DELEGATE_READ;
> + }
> if (!nf) {
> /*
> * We probably could attempt another open and get a read
> @@ -5491,11 +5498,11 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> return ERR_PTR(status);
> 
> status = -ENOMEM;
> - dp = alloc_init_deleg(clp, fp, odstate);
> + dp = alloc_init_deleg(clp, fp, odstate, deleg);
> if (!dp)
> goto out_delegees;
> 
> - fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ);
> + fl = nfs4_alloc_init_lease(dp, deleg);
> if (!fl)
> goto out_clnt_odstate;
> 
> @@ -5583,6 +5590,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> struct svc_fh *parent = NULL;
> int cb_up;
> int status = 0;
> + u32 wdeleg = false;
> 
> cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client);
> open->op_recall = 0;
> @@ -5590,8 +5598,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> case NFS4_OPEN_CLAIM_PREVIOUS:
> if (!cb_up)
> open->op_recall = 1;
> - if (open->op_delegate_type != NFS4_OPEN_DELEGATE_READ)
> - goto out_no_deleg;
> break;
> case NFS4_OPEN_CLAIM_NULL:
> parent = currentfh;
> @@ -5617,7 +5623,9 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
> memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
> 
> trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);

I'd like you to add a trace_nfsd_deleg_write(), and invoke
it here instead of trace_nfsd_deleg_read when NFSD hands out
a write delegation.


> - open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
> + wdeleg = open->op_share_access & NFS4_SHARE_ACCESS_WRITE;
> + open->op_delegate_type = wdeleg ?
> + NFS4_OPEN_DELEGATE_WRITE : NFS4_OPEN_DELEGATE_READ;
> nfs4_put_stid(&dp->dl_stid);
> return;
> out_no_deleg:
> -- 
> 2.9.5
> 

--
Chuck Lever

Re: [PATCH v3 2/2] NFSD: enable support for write delegation

[dai.ngo]

On 5/18/23 6:23 AM, Chuck Lever III wrote:
>
>> On May 17, 2023, at 7:38 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>
>> This patch grants write delegation for OPEN with NFS4_SHARE_ACCESS_WRITE
>> if there is no conflict with other OPENs.
>>
>> Write delegation conflict with another OPEN, REMOVE, RENAME and SETATTR
>> are handled the same as read delegation using notify_change,
>> try_break_deleg.
> Very clean. A couple of suggestions, one is down below, and here is
> the other:
>
> I was thinking we should add one or two counters in fs/nfsd/stats.c
> to track how often read and write delegations are offered, and
> perhaps one to count the number of DELEGRETURN operations. What do
> you think makes sense?

I'm not sure what these counters will tell us, currently we already
has a counter for number of delegations handed out. I think a counter
on how often nfsd has to recall the write delegation due to GETATTR can
be useful to know whether we should implement CB_GETATTR.

>
>
>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>> ---
>> fs/nfsd/nfs4state.c | 24 ++++++++++++++++--------
>> 1 file changed, 16 insertions(+), 8 deletions(-)
>>
>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>> index 6e61fa3acaf1..09a9e16407f9 100644
>> --- a/fs/nfsd/nfs4state.c
>> +++ b/fs/nfsd/nfs4state.c
>> @@ -1144,7 +1144,7 @@ static void block_delegations(struct knfsd_fh *fh)
>>
>> static struct nfs4_delegation *
>> alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
>> - struct nfs4_clnt_odstate *odstate)
>> + struct nfs4_clnt_odstate *odstate, u32 dl_type)
>> {
>> struct nfs4_delegation *dp;
>> long n;
>> @@ -1170,7 +1170,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
>> INIT_LIST_HEAD(&dp->dl_recall_lru);
>> dp->dl_clnt_odstate = odstate;
>> get_clnt_odstate(odstate);
>> - dp->dl_type = NFS4_OPEN_DELEGATE_READ;
>> + dp->dl_type = dl_type;
>> dp->dl_retries = 1;
>> dp->dl_recalled = false;
>> nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client,
>> @@ -5451,6 +5451,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>> struct nfs4_delegation *dp;
>> struct nfsd_file *nf;
>> struct file_lock *fl;
>> + u32 deleg;
>>
>> /*
>> * The fi_had_conflict and nfs_get_existing_delegation checks
>> @@ -5460,7 +5461,13 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>> if (fp->fi_had_conflict)
>> return ERR_PTR(-EAGAIN);
>>
>> - nf = find_readable_file(fp);
>> + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
>> + nf = find_writeable_file(fp);
>> + deleg = NFS4_OPEN_DELEGATE_WRITE;
>> + } else {
>> + nf = find_readable_file(fp);
>> + deleg = NFS4_OPEN_DELEGATE_READ;
>> + }
>> if (!nf) {
>> /*
>> * We probably could attempt another open and get a read
>> @@ -5491,11 +5498,11 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>> return ERR_PTR(status);
>>
>> status = -ENOMEM;
>> - dp = alloc_init_deleg(clp, fp, odstate);
>> + dp = alloc_init_deleg(clp, fp, odstate, deleg);
>> if (!dp)
>> goto out_delegees;
>>
>> - fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ);
>> + fl = nfs4_alloc_init_lease(dp, deleg);
>> if (!fl)
>> goto out_clnt_odstate;
>>
>> @@ -5583,6 +5590,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>> struct svc_fh *parent = NULL;
>> int cb_up;
>> int status = 0;
>> + u32 wdeleg = false;
>>
>> cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client);
>> open->op_recall = 0;
>> @@ -5590,8 +5598,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>> case NFS4_OPEN_CLAIM_PREVIOUS:
>> if (!cb_up)
>> open->op_recall = 1;
>> - if (open->op_delegate_type != NFS4_OPEN_DELEGATE_READ)
>> - goto out_no_deleg;
>> break;
>> case NFS4_OPEN_CLAIM_NULL:
>> parent = currentfh;
>> @@ -5617,7 +5623,9 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>> memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
>>
>> trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
> I'd like you to add a trace_nfsd_deleg_write(), and invoke
> it here instead of trace_nfsd_deleg_read when NFSD hands out
> a write delegation.

Fix in v4.

-Dai

>
>
>> - open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
>> + wdeleg = open->op_share_access & NFS4_SHARE_ACCESS_WRITE;
>> + open->op_delegate_type = wdeleg ?
>> + NFS4_OPEN_DELEGATE_WRITE : NFS4_OPEN_DELEGATE_READ;
>> nfs4_put_stid(&dp->dl_stid);
>> return;
>> out_no_deleg:
>> -- 
>> 2.9.5
>>
> --
> Chuck Lever
>
>

Re: [PATCH v3 2/2] NFSD: enable support for write delegation

[Chuck Lever III]

> On May 18, 2023, at 1:11 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
> 
> 
> On 5/18/23 6:23 AM, Chuck Lever III wrote:
>> 
>>> On May 17, 2023, at 7:38 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>> 
>>> This patch grants write delegation for OPEN with NFS4_SHARE_ACCESS_WRITE
>>> if there is no conflict with other OPENs.
>>> 
>>> Write delegation conflict with another OPEN, REMOVE, RENAME and SETATTR
>>> are handled the same as read delegation using notify_change,
>>> try_break_deleg.
>> Very clean. A couple of suggestions, one is down below, and here is
>> the other:
>> 
>> I was thinking we should add one or two counters in fs/nfsd/stats.c
>> to track how often read and write delegations are offered, and
>> perhaps one to count the number of DELEGRETURN operations. What do
>> you think makes sense?
> 
> I'm not sure what these counters will tell us, currently we already
> has a counter for number of delegations handed out.

I haven't found that, where is it? Certainly, if NFSD already
has one, then no need to add more.

It would be nice one day, perhaps, to have a metric of how many
delegations a client holds. That's not for this series.


> I think a counter
> on how often nfsd has to recall the write delegation due to GETATTR can
> be useful to know whether we should implement CB_GETATTR.

I hesitated to mention that because I wonder if that's something
that would be interesting only for defending a design choice,
not for site-to-site tuning. In other words, after we plumb it
into NFSD, it will never actually be used after CB_GETATTR
support is added.

Do you believe it's something that administrators can use to
help balance or tune their workloads?


>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>> ---
>>> fs/nfsd/nfs4state.c | 24 ++++++++++++++++--------
>>> 1 file changed, 16 insertions(+), 8 deletions(-)
>>> 
>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>> index 6e61fa3acaf1..09a9e16407f9 100644
>>> --- a/fs/nfsd/nfs4state.c
>>> +++ b/fs/nfsd/nfs4state.c
>>> @@ -1144,7 +1144,7 @@ static void block_delegations(struct knfsd_fh *fh)
>>> 
>>> static struct nfs4_delegation *
>>> alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
>>> - struct nfs4_clnt_odstate *odstate)
>>> + struct nfs4_clnt_odstate *odstate, u32 dl_type)
>>> {
>>> struct nfs4_delegation *dp;
>>> long n;
>>> @@ -1170,7 +1170,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
>>> INIT_LIST_HEAD(&dp->dl_recall_lru);
>>> dp->dl_clnt_odstate = odstate;
>>> get_clnt_odstate(odstate);
>>> - dp->dl_type = NFS4_OPEN_DELEGATE_READ;
>>> + dp->dl_type = dl_type;
>>> dp->dl_retries = 1;
>>> dp->dl_recalled = false;
>>> nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client,
>>> @@ -5451,6 +5451,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>> struct nfs4_delegation *dp;
>>> struct nfsd_file *nf;
>>> struct file_lock *fl;
>>> + u32 deleg;
>>> 
>>> /*
>>> * The fi_had_conflict and nfs_get_existing_delegation checks
>>> @@ -5460,7 +5461,13 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>> if (fp->fi_had_conflict)
>>> return ERR_PTR(-EAGAIN);
>>> 
>>> - nf = find_readable_file(fp);
>>> + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
>>> + nf = find_writeable_file(fp);
>>> + deleg = NFS4_OPEN_DELEGATE_WRITE;
>>> + } else {
>>> + nf = find_readable_file(fp);
>>> + deleg = NFS4_OPEN_DELEGATE_READ;
>>> + }
>>> if (!nf) {
>>> /*
>>> * We probably could attempt another open and get a read
>>> @@ -5491,11 +5498,11 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>> return ERR_PTR(status);
>>> 
>>> status = -ENOMEM;
>>> - dp = alloc_init_deleg(clp, fp, odstate);
>>> + dp = alloc_init_deleg(clp, fp, odstate, deleg);
>>> if (!dp)
>>> goto out_delegees;
>>> 
>>> - fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ);
>>> + fl = nfs4_alloc_init_lease(dp, deleg);
>>> if (!fl)
>>> goto out_clnt_odstate;
>>> 
>>> @@ -5583,6 +5590,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>> struct svc_fh *parent = NULL;
>>> int cb_up;
>>> int status = 0;
>>> + u32 wdeleg = false;
>>> 
>>> cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client);
>>> open->op_recall = 0;
>>> @@ -5590,8 +5598,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>> case NFS4_OPEN_CLAIM_PREVIOUS:
>>> if (!cb_up)
>>> open->op_recall = 1;
>>> - if (open->op_delegate_type != NFS4_OPEN_DELEGATE_READ)
>>> - goto out_no_deleg;
>>> break;
>>> case NFS4_OPEN_CLAIM_NULL:
>>> parent = currentfh;
>>> @@ -5617,7 +5623,9 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>> memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
>>> 
>>> trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
>> I'd like you to add a trace_nfsd_deleg_write(), and invoke
>> it here instead of trace_nfsd_deleg_read when NFSD hands out
>> a write delegation.
> 
> Fix in v4.
> 
> -Dai
> 
>> 
>> 
>>> - open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
>>> + wdeleg = open->op_share_access & NFS4_SHARE_ACCESS_WRITE;
>>> + open->op_delegate_type = wdeleg ?
>>> + NFS4_OPEN_DELEGATE_WRITE : NFS4_OPEN_DELEGATE_READ;
>>> nfs4_put_stid(&dp->dl_stid);
>>> return;
>>> out_no_deleg:
>>> -- 
>>> 2.9.5
>>> 
>> --
>> Chuck Lever


--
Chuck Lever

Re: [PATCH v3 2/2] NFSD: enable support for write delegation

[dai.ngo]

On 5/18/23 10:16 AM, Chuck Lever III wrote:
>
>> On May 18, 2023, at 1:11 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>
>>
>> On 5/18/23 6:23 AM, Chuck Lever III wrote:
>>>> On May 17, 2023, at 7:38 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>>>
>>>> This patch grants write delegation for OPEN with NFS4_SHARE_ACCESS_WRITE
>>>> if there is no conflict with other OPENs.
>>>>
>>>> Write delegation conflict with another OPEN, REMOVE, RENAME and SETATTR
>>>> are handled the same as read delegation using notify_change,
>>>> try_break_deleg.
>>> Very clean. A couple of suggestions, one is down below, and here is
>>> the other:
>>>
>>> I was thinking we should add one or two counters in fs/nfsd/stats.c
>>> to track how often read and write delegations are offered, and
>>> perhaps one to count the number of DELEGRETURN operations. What do
>>> you think makes sense?
>> I'm not sure what these counters will tell us, currently we already
>> has a counter for number of delegations handed out.
> I haven't found that, where is it? Certainly, if NFSD already
> has one, then no need to add more.

num_delegations in nfs4state.c

>
> It would be nice one day, perhaps, to have a metric of how many
> delegations a client holds. That's not for this series.

okay.

>
>
>> I think a counter
>> on how often nfsd has to recall the write delegation due to GETATTR can
>> be useful to know whether we should implement CB_GETATTR.
> I hesitated to mention that because I wonder if that's something
> that would be interesting only for defending a design choice,
> not for site-to-site tuning. In other words, after we plumb it
> into NFSD, it will never actually be used after CB_GETATTR
> support is added.
>
> Do you believe it's something that administrators can use to
> help balance or tune their workloads?

You're right. That is just for ourselves to determine if CB_GETATTR
is needed.

-Dai

>
>
>>>> Signed-off-by: Dai Ngo <dai.ngo@oracle.com>
>>>> ---
>>>> fs/nfsd/nfs4state.c | 24 ++++++++++++++++--------
>>>> 1 file changed, 16 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
>>>> index 6e61fa3acaf1..09a9e16407f9 100644
>>>> --- a/fs/nfsd/nfs4state.c
>>>> +++ b/fs/nfsd/nfs4state.c
>>>> @@ -1144,7 +1144,7 @@ static void block_delegations(struct knfsd_fh *fh)
>>>>
>>>> static struct nfs4_delegation *
>>>> alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
>>>> - struct nfs4_clnt_odstate *odstate)
>>>> + struct nfs4_clnt_odstate *odstate, u32 dl_type)
>>>> {
>>>> struct nfs4_delegation *dp;
>>>> long n;
>>>> @@ -1170,7 +1170,7 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
>>>> INIT_LIST_HEAD(&dp->dl_recall_lru);
>>>> dp->dl_clnt_odstate = odstate;
>>>> get_clnt_odstate(odstate);
>>>> - dp->dl_type = NFS4_OPEN_DELEGATE_READ;
>>>> + dp->dl_type = dl_type;
>>>> dp->dl_retries = 1;
>>>> dp->dl_recalled = false;
>>>> nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client,
>>>> @@ -5451,6 +5451,7 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>>> struct nfs4_delegation *dp;
>>>> struct nfsd_file *nf;
>>>> struct file_lock *fl;
>>>> + u32 deleg;
>>>>
>>>> /*
>>>> * The fi_had_conflict and nfs_get_existing_delegation checks
>>>> @@ -5460,7 +5461,13 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>>> if (fp->fi_had_conflict)
>>>> return ERR_PTR(-EAGAIN);
>>>>
>>>> - nf = find_readable_file(fp);
>>>> + if (open->op_share_access & NFS4_SHARE_ACCESS_WRITE) {
>>>> + nf = find_writeable_file(fp);
>>>> + deleg = NFS4_OPEN_DELEGATE_WRITE;
>>>> + } else {
>>>> + nf = find_readable_file(fp);
>>>> + deleg = NFS4_OPEN_DELEGATE_READ;
>>>> + }
>>>> if (!nf) {
>>>> /*
>>>> * We probably could attempt another open and get a read
>>>> @@ -5491,11 +5498,11 @@ nfs4_set_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>>> return ERR_PTR(status);
>>>>
>>>> status = -ENOMEM;
>>>> - dp = alloc_init_deleg(clp, fp, odstate);
>>>> + dp = alloc_init_deleg(clp, fp, odstate, deleg);
>>>> if (!dp)
>>>> goto out_delegees;
>>>>
>>>> - fl = nfs4_alloc_init_lease(dp, NFS4_OPEN_DELEGATE_READ);
>>>> + fl = nfs4_alloc_init_lease(dp, deleg);
>>>> if (!fl)
>>>> goto out_clnt_odstate;
>>>>
>>>> @@ -5583,6 +5590,7 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>>> struct svc_fh *parent = NULL;
>>>> int cb_up;
>>>> int status = 0;
>>>> + u32 wdeleg = false;
>>>>
>>>> cb_up = nfsd4_cb_channel_good(oo->oo_owner.so_client);
>>>> open->op_recall = 0;
>>>> @@ -5590,8 +5598,6 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>>> case NFS4_OPEN_CLAIM_PREVIOUS:
>>>> if (!cb_up)
>>>> open->op_recall = 1;
>>>> - if (open->op_delegate_type != NFS4_OPEN_DELEGATE_READ)
>>>> - goto out_no_deleg;
>>>> break;
>>>> case NFS4_OPEN_CLAIM_NULL:
>>>> parent = currentfh;
>>>> @@ -5617,7 +5623,9 @@ nfs4_open_delegation(struct nfsd4_open *open, struct nfs4_ol_stateid *stp,
>>>> memcpy(&open->op_delegate_stateid, &dp->dl_stid.sc_stateid, sizeof(dp->dl_stid.sc_stateid));
>>>>
>>>> trace_nfsd_deleg_read(&dp->dl_stid.sc_stateid);
>>> I'd like you to add a trace_nfsd_deleg_write(), and invoke
>>> it here instead of trace_nfsd_deleg_read when NFSD hands out
>>> a write delegation.
>> Fix in v4.
>>
>> -Dai
>>
>>>
>>>> - open->op_delegate_type = NFS4_OPEN_DELEGATE_READ;
>>>> + wdeleg = open->op_share_access & NFS4_SHARE_ACCESS_WRITE;
>>>> + open->op_delegate_type = wdeleg ?
>>>> + NFS4_OPEN_DELEGATE_WRITE : NFS4_OPEN_DELEGATE_READ;
>>>> nfs4_put_stid(&dp->dl_stid);
>>>> return;
>>>> out_no_deleg:
>>>> -- 
>>>> 2.9.5
>>>>
>>> --
>>> Chuck Lever
>
> --
> Chuck Lever
>
>

Re: [PATCH v3 2/2] NFSD: enable support for write delegation

[Chuck Lever III]

> On May 18, 2023, at 2:01 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
> 
> 
> On 5/18/23 10:16 AM, Chuck Lever III wrote:
>> 
>>> On May 18, 2023, at 1:11 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>> 
>>> 
>>> On 5/18/23 6:23 AM, Chuck Lever III wrote:
>>>>> On May 17, 2023, at 7:38 PM, Dai Ngo <dai.ngo@oracle.com> wrote:
>>>>> 
>>>>> This patch grants write delegation for OPEN with NFS4_SHARE_ACCESS_WRITE
>>>>> if there is no conflict with other OPENs.
>>>>> 
>>>>> Write delegation conflict with another OPEN, REMOVE, RENAME and SETATTR
>>>>> are handled the same as read delegation using notify_change,
>>>>> try_break_deleg.
>>>> Very clean. A couple of suggestions, one is down below, and here is
>>>> the other:
>>>> 
>>>> I was thinking we should add one or two counters in fs/nfsd/stats.c
>>>> to track how often read and write delegations are offered, and
>>>> perhaps one to count the number of DELEGRETURN operations. What do
>>>> you think makes sense?
>>> I'm not sure what these counters will tell us, currently we already
>>> has a counter for number of delegations handed out.
>> I haven't found that, where is it? Certainly, if NFSD already
>> has one, then no need to add more.
> 
> num_delegations in nfs4state.c
> 
>> 
>> It would be nice one day, perhaps, to have a metric of how many
>> delegations a client holds. That's not for this series.
> 
> okay.
> 
>> 
>> 
>>> I think a counter
>>> on how often nfsd has to recall the write delegation due to GETATTR can
>>> be useful to know whether we should implement CB_GETATTR.
>> I hesitated to mention that because I wonder if that's something
>> that would be interesting only for defending a design choice,
>> not for site-to-site tuning. In other words, after we plumb it
>> into NFSD, it will never actually be used after CB_GETATTR
>> support is added.
>> 
>> Do you believe it's something that administrators can use to
>> help balance or tune their workloads?
> 
> You're right. That is just for ourselves to determine if CB_GETATTR
> is needed.

To be clear, such a counter, I agree, would be useful /to us/.
I'm just not sure how we could add something that would not
become part of the kernel/userspace API. Anyone have thoughts
about that?


--
Chuck Lever

Re: [PATCH v3 0/2] NFSD: add support for NFSv4 write delegation

[Jeff Layton]

On Wed, 2023-05-17 at 16:38 -0700, Dai Ngo wrote:
> NFSD: add support for NFSv4 write delegation
> 
> The NFSv4 server currently supports read delegation using VFS lease
> which is implemented using file_lock. 
> 
> This patch series add write delegation support for NFSv4 server by:
> 
>     . remove the check for F_WRLCK in generic_add_lease to allow
>       file_lock to be used for write delegation.  
> 
>     . grant write delegation for OPEN with NFS4_SHARE_ACCESS_WRITE
>       if there is no conflict with other OPENs.
> 
> Write delegation conflict with another OPEN, REMOVE, RENAME and SETATTR
> are handled the same as read delegation using notify_change, try_break_deleg.
> 
> Changes since v1:
> 
> [PATCH 3/4] NFSD: add supports for CB_GETATTR callback
> - remove WARN_ON_ONCE from encode_bitmap4
> - replace decode_bitmap4 with xdr_stream_decode_uint32_array
> - replace xdr_inline_decode and xdr_decode_hyper in decode_cb_getattr
>    with xdr_stream_decode_u64. Also remove the un-needed likely().
> - modify signature of encode_cb_getattr4args to take pointer to
>    nfs4_cb_fattr
> - replace decode_attr_length with xdr_stream_decode_u32
> - rename decode_cb_getattr to decode_cb_fattr4
> - fold the initialization of cb_cinfo and cb_fsize into decode_cb_fattr4
> - rename ncf_cb_cinfo to ncf_cb_change to avoid confusion of cindo usage
>   in fs/nfsd/nfs4xdr.c
> - correct NFS4_dec_cb_getattr_sz and update size description
> 
> [PATCH 4/4] NFSD: handle GETATTR conflict with write delegation
> - change nfs4_handle_wrdeleg_conflict returns __be32 to fix test robot
> - change ncf_cb_cinfo to ncf_cb_change to avoid confusion of cindo usage
>   in fs/nfsd/nfs4xdr.c
> 
> Changes since v2:
> 
> [PATCH 2/4] NFSD: enable support for write delegation
> - rename 'deleg' to 'dl_type' in nfs4_set_delegation
> - remove 'wdeleg' in nfs4_open_delegation
> 
> - drop [PATCH 3/4] NFSD: add supports for CB_GETATTR callback
>   and [PATCH 4/4] NFSD: handle GETATTR conflict with write delegation
>   for futher clarification of the benefits of using CB_GETATTR
>   for handling GETATTR from the 2nd client
> 

Pretty straightforward. Not as useful (IMO) without CB_GETATTR, since
even a stray 'ls -l' in the parent directory will cause the delegation
to be recalled, but it's a reasonable first step.

Reviewed-by: Jeff Layton <jlayton@kernel.org>

Re: [PATCH v3 0/2] NFSD: add support for NFSv4 write delegation

[dai.ngo]

On 5/18/23 6:51 AM, Jeff Layton wrote:
> On Wed, 2023-05-17 at 16:38 -0700, Dai Ngo wrote:
>> NFSD: add support for NFSv4 write delegation
>>
>> The NFSv4 server currently supports read delegation using VFS lease
>> which is implemented using file_lock.
>>
>> This patch series add write delegation support for NFSv4 server by:
>>
>>      . remove the check for F_WRLCK in generic_add_lease to allow
>>        file_lock to be used for write delegation.
>>
>>      . grant write delegation for OPEN with NFS4_SHARE_ACCESS_WRITE
>>        if there is no conflict with other OPENs.
>>
>> Write delegation conflict with another OPEN, REMOVE, RENAME and SETATTR
>> are handled the same as read delegation using notify_change, try_break_deleg.
>>
>> Changes since v1:
>>
>> [PATCH 3/4] NFSD: add supports for CB_GETATTR callback
>> - remove WARN_ON_ONCE from encode_bitmap4
>> - replace decode_bitmap4 with xdr_stream_decode_uint32_array
>> - replace xdr_inline_decode and xdr_decode_hyper in decode_cb_getattr
>>     with xdr_stream_decode_u64. Also remove the un-needed likely().
>> - modify signature of encode_cb_getattr4args to take pointer to
>>     nfs4_cb_fattr
>> - replace decode_attr_length with xdr_stream_decode_u32
>> - rename decode_cb_getattr to decode_cb_fattr4
>> - fold the initialization of cb_cinfo and cb_fsize into decode_cb_fattr4
>> - rename ncf_cb_cinfo to ncf_cb_change to avoid confusion of cindo usage
>>    in fs/nfsd/nfs4xdr.c
>> - correct NFS4_dec_cb_getattr_sz and update size description
>>
>> [PATCH 4/4] NFSD: handle GETATTR conflict with write delegation
>> - change nfs4_handle_wrdeleg_conflict returns __be32 to fix test robot
>> - change ncf_cb_cinfo to ncf_cb_change to avoid confusion of cindo usage
>>    in fs/nfsd/nfs4xdr.c
>>
>> Changes since v2:
>>
>> [PATCH 2/4] NFSD: enable support for write delegation
>> - rename 'deleg' to 'dl_type' in nfs4_set_delegation
>> - remove 'wdeleg' in nfs4_open_delegation
>>
>> - drop [PATCH 3/4] NFSD: add supports for CB_GETATTR callback
>>    and [PATCH 4/4] NFSD: handle GETATTR conflict with write delegation
>>    for futher clarification of the benefits of using CB_GETATTR
>>    for handling GETATTR from the 2nd client
>>
> Pretty straightforward. Not as useful (IMO) without CB_GETATTR, since
> even a stray 'ls -l' in the parent directory will cause the delegation
> to be recalled, but it's a reasonable first step.

Oh I forgot to leave to code to recall the write delegation in when
there is GETATTR. Will fix in v4.

Yes, let just run with this simple code for awhile to make sure nothing
break then we can add the CB_GETATTR later it it's needed.

-Dai

>
> Reviewed-by: Jeff Layton <jlayton@kernel.org>