On Wed, Feb 14, 2024 at 10:22:05AM -0800, dai.ngo@xxxxxxxxxx wrote:
>
> On 2/14/24 10:14 AM, Chuck Lever wrote:
> > On Wed, Feb 14, 2024 at 10:10:50AM -0800, dai.ngo@xxxxxxxxxx wrote:
> > > On 2/14/24 6:50 AM, Chuck Lever wrote:
> > > > On Tue, Feb 13, 2024 at 09:47:27AM -0800, 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 request is handled as below:
> > > > >
> > > > > Server sends CB_GETATTR to client to get the latest change info and file
> > > > > size. If these values are the same as the server's cached values then
> > > > > the GETATTR proceeds as normal.
> > > > >
> > > > > If either the change info or file size is different from the server's
> > > > > cached values, or the file was already marked as modified, then:
> > > > >
> > > > > . update time_modify and time_metadata into file's metadata
> > > > > with current time
> > > > >
> > > > > . encode GETATTR as normal except the file size is encoded with
> > > > > the value returned from CB_GETATTR
> > > > >
> > > > > . mark the file as modified
> > > > >
> > > > > The CB_GETATTR is given 30ms to complte. If the CB_GETATTR fails for
> > > > > any reasons, the delegation is recalled and NFS4ERR_DELAY is returned
> > > > > for the GETATTR from the second client.
> > > > >
> > > > > Signed-off-by: Dai Ngo <dai.ngo@xxxxxxxxxx>
> > > > > ---
> > > > > fs/nfsd/nfs4state.c | 119 ++++++++++++++++++++++++++++++++++++++++----
> > > > > fs/nfsd/nfs4xdr.c | 10 +++-
> > > > > fs/nfsd/nfsd.h | 1 +
> > > > > fs/nfsd/state.h | 10 +++-
> > > > > 4 files changed, 127 insertions(+), 13 deletions(-)
> > > > >
> > > > > diff --git a/fs/nfsd/nfs4state.c b/fs/nfsd/nfs4state.c
> > > > > index d9260e77ef2d..87987515e03d 100644
> > > > > --- a/fs/nfsd/nfs4state.c
> > > > > +++ b/fs/nfsd/nfs4state.c
> > > > > @@ -127,6 +127,7 @@ static void free_session(struct nfsd4_session *);
> > > > > static const struct nfsd4_callback_ops nfsd4_cb_recall_ops;
> > > > > static const struct nfsd4_callback_ops nfsd4_cb_notify_lock_ops;
> > > > > +static const struct nfsd4_callback_ops nfsd4_cb_getattr_ops;
> > > > > static struct workqueue_struct *laundry_wq;
> > > > > @@ -1189,6 +1190,10 @@ alloc_init_deleg(struct nfs4_client *clp, struct nfs4_file *fp,
> > > > > dp->dl_recalled = false;
> > > > > nfsd4_init_cb(&dp->dl_recall, dp->dl_stid.sc_client,
> > > > > &nfsd4_cb_recall_ops, NFSPROC4_CLNT_CB_RECALL);
> > > > > + nfsd4_init_cb(&dp->dl_cb_fattr.ncf_getattr, dp->dl_stid.sc_client,
> > > > > + &nfsd4_cb_getattr_ops, NFSPROC4_CLNT_CB_GETATTR);
> > > > > + dp->dl_cb_fattr.ncf_file_modified = false;
> > > > > + dp->dl_cb_fattr.ncf_cb_bmap[0] = FATTR4_WORD0_CHANGE | FATTR4_WORD0_SIZE;
> > > > > get_nfs4_file(fp);
> > > > > dp->dl_stid.sc_file = fp;
> > > > > return dp;
> > > > > @@ -3044,11 +3049,59 @@ nfsd4_cb_recall_any_release(struct nfsd4_callback *cb)
> > > > > spin_unlock(&nn->client_lock);
> > > > > }
> > > > > +static int
> > > > > +nfsd4_cb_getattr_done(struct nfsd4_callback *cb, struct rpc_task *task)
> > > > > +{
> > > > > + struct nfs4_cb_fattr *ncf =
> > > > > + container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
> > > > > +
> > > > > + ncf->ncf_cb_status = task->tk_status;
> > > > > + switch (task->tk_status) {
> > > > > + case -NFS4ERR_DELAY:
> > > > > + rpc_delay(task, 2 * HZ);
> > > > > + return 0;
> > > > > + default:
> > > > > + return 1;
> > > > > + }
> > > > > +}
> > > > > +
> > > > > +static void
> > > > > +nfsd4_cb_getattr_release(struct nfsd4_callback *cb)
> > > > > +{
> > > > > + struct nfs4_cb_fattr *ncf =
> > > > > + container_of(cb, struct nfs4_cb_fattr, ncf_getattr);
> > > > > + struct nfs4_delegation *dp =
> > > > > + container_of(ncf, struct nfs4_delegation, dl_cb_fattr);
> > > > > +
> > > > > + nfs4_put_stid(&dp->dl_stid);
> > > > > + clear_bit(CB_GETATTR_BUSY, &ncf->ncf_cb_flags);
> > > > > + wake_up_bit(&ncf->ncf_cb_flags, CB_GETATTR_BUSY);
> > > > > +}
> > > > > +
> > > > What happens if the client responds after the GETATTR timer elapses?
> > > > Can nfsd4_cb_getattr_release over-write memory that is now being
> > > > used for something else?
> > > The refcount added in nfs4_cb_getattr keeps the delegation state valid
> > > until it's released here by nfs4_put_stid.
> > If the client never replies, does that pin the stateid?
>
> Won't RPC timeout on waiting for reply?
> This is the same behavior as when recalling a delegation,
> nfsd_break_deleg_cb and nfsd4_cb_recall_release.
Fair enough. Looking forward to v2.
--
Chuck Lever
