| Message ID | 1684366690-28029-3-git-send-email-dai.ngo@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | NFSD: add support for NFSv4 write delegation | expand |
> 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
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 > >
> 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
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 > >
> 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
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:
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(-)