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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 5/18/23 10:16 AM, Chuck Lever III wrote:

On May 18, 2023, at 1:11 PM, Dai Ngo <dai.ngo@xxxxxxxxxx> wrote:


On 5/18/23 6:23 AM, Chuck Lever III wrote:
On May 17, 2023, at 7:38 PM, Dai Ngo <dai.ngo@xxxxxxxxxx> 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@xxxxxxxxxx>
---
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





[Index of Archives]     [Linux Ext4 Filesystem]     [Union Filesystem]     [Filesystem Testing]     [Ceph Users]     [Ecryptfs]     [NTFS 3]     [AutoFS]     [Kernel Newbies]     [Share Photos]     [Security]     [Netfilter]     [Bugtraq]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux Cachefs]     [Reiser Filesystem]     [Linux RAID]     [NTFS 3]     [Samba]     [Device Mapper]     [CEPH Development]

  Powered by Linux