[PATCH 0/3] nfsd update mtime/ctime on CLONE/COPY with delegated attritutes

[PATCH 0/3] nfsd update mtime/ctime on CLONE/COPY with delegated attritutes

[Olga Kornievskaia]

generic/407 is failing in presence of delegated attributes and this patch series 
explores the solution where the CLONE compound returns updated mtime/ctime
in the GETATTR op in the compound.

It's broken into several pieces as I consider asynchronous copy update the trickest
because the copy thread currently have no dentry to call notify_change() on. And
if we are going to keep a copy of it while the copy is running we probably need to
take a reference on it (which I attempt to do).

An outstanding question I have is whether or not a CLONE/COPY operation is supposed to
also update the atime on the source file? But also atime for the dest file as well?
I'm updating both ctime and mtime for the destination (though Jeff only suggested
mtime and perhaps I'm missing something in updating ctime as well but I thought it 
was appropriate). 

COPY was tested by modifying the linux client to send a GETATTR in the COPY compound.
Whether or not the client should add the GETATTR to the COPY compound is yet another
question I have but I guess it would be for Trond.

Olga Kornievskaia (3):
  nfsd: update mtime/ctime on CLONE in presense of delegated attributes
  nfsd: update mtime/ctime on synchronous COPY with delegated attributes
  nfsd: update mtime/ctime on asynchronous COPY with delegated
    attributes

 fs/nfsd/nfs4proc.c | 38 +++++++++++++++++++++++++++++++++++++-
 fs/nfsd/xdr4.h     |  1 +
 2 files changed, 38 insertions(+), 1 deletion(-)

-- 
2.52.0

[PATCH 1/3] nfsd: update mtime/ctime on CLONE in presense of delegated attributes

[Olga Kornievskaia]

When delegated attributes are given on open the file is opened with NOCMTIME
and modifying operations do not update mtime/ctime as to not get out-of-sync
with the client's delegated view. However, for CLONE operation, the server
should update its view of mtime/ctime and reflect that in any GETATTR queries.

Fixes: e5e9b24ab8fa ("nfsd: freeze c/mtime updates with outstanding WRITE_ATTRS delegation")
Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
---
 fs/nfsd/nfs4proc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 99b44b6ec056..fb891e35ebe9 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -1396,6 +1396,17 @@ nfsd4_verify_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	goto out;
 }
 
+static void nfsd_update_cmtime_attr(struct dentry *dentry)
+{
+	struct iattr attr = {
+		.ia_valid = ATTR_CTIME | ATTR_MTIME,
+	};
+
+	inode_lock(d_inode(dentry));
+	notify_change(&nop_mnt_idmap, dentry, &attr, NULL);
+	inode_unlock(d_inode(dentry));
+}
+
 static __be32
 nfsd4_clone(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		union nfsd4_op_u *u)
@@ -1413,6 +1424,9 @@ nfsd4_clone(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 			dst, clone->cl_dst_pos, clone->cl_count,
 			EX_ISSYNC(cstate->current_fh.fh_export));
 
+	if ((READ_ONCE(dst->nf_file->f_mode) & FMODE_NOCMTIME) != 0)
+		nfsd_update_cmtime_attr(cstate->current_fh.fh_dentry);
+
 	nfsd_file_put(dst);
 	nfsd_file_put(src);
 out:
-- 
2.52.0

Re: [PATCH 1/3] nfsd: update mtime/ctime on CLONE in presense of delegated attributes

[Chuck Lever]

On Tue, Apr 7, 2026, at 7:50 PM, Olga Kornievskaia wrote:
> When delegated attributes are given on open the file is opened with NOCMTIME
> and modifying operations do not update mtime/ctime as to not get out-of-sync
> with the client's delegated view. However, for CLONE operation, the server
> should update its view of mtime/ctime and reflect that in any GETATTR queries.
>
> Fixes: e5e9b24ab8fa ("nfsd: freeze c/mtime updates with outstanding 
> WRITE_ATTRS delegation")
> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> ---
>  fs/nfsd/nfs4proc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 99b44b6ec056..fb891e35ebe9 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -1396,6 +1396,17 @@ nfsd4_verify_copy(struct svc_rqst *rqstp, struct 
> nfsd4_compound_state *cstate,
>  	goto out;
>  }
> 
> +static void nfsd_update_cmtime_attr(struct dentry *dentry)
> +{
> +	struct iattr attr = {
> +		.ia_valid = ATTR_CTIME | ATTR_MTIME,
> +	};
> +
> +	inode_lock(d_inode(dentry));
> +	notify_change(&nop_mnt_idmap, dentry, &attr, NULL);
> +	inode_unlock(d_inode(dentry));
> +}

I think there is an active delegation here. Without ATTR_DELEG,
notify_change() calls try_break_deleg(), which will return
-EWOULDBLOCK, causing the setattr to be silently skipped.
Wouldn't it also initiate a CB_RECALL as well?


> @@ -1413,6 +1424,9 @@ nfsd4_clone(struct svc_rqst *rqstp, struct 
> nfsd4_compound_state *cstate,
>  			dst, clone->cl_dst_pos, clone->cl_count,
>  			EX_ISSYNC(cstate->current_fh.fh_export));
> 
> +	if ((READ_ONCE(dst->nf_file->f_mode) & FMODE_NOCMTIME) != 0)
> +		nfsd_update_cmtime_attr(cstate->current_fh.fh_dentry);
> +
>  	nfsd_file_put(dst);
>  	nfsd_file_put(src);
>  out:

Should this check whether nfsd4_clone_file_range() succeeded before
updating timestamps?  If the clone fails, the file content is not
modified, so a timestamp update would be incorrect.


-- 
Chuck Lever

Re: [PATCH 1/3] nfsd: update mtime/ctime on CLONE in presense of delegated attributes

[Olga Kornievskaia]

On Wed, Apr 8, 2026 at 9:53 AM Chuck Lever <cel@kernel.org> wrote:
>
>
> On Tue, Apr 7, 2026, at 7:50 PM, Olga Kornievskaia wrote:
> > When delegated attributes are given on open the file is opened with NOCMTIME
> > and modifying operations do not update mtime/ctime as to not get out-of-sync
> > with the client's delegated view. However, for CLONE operation, the server
> > should update its view of mtime/ctime and reflect that in any GETATTR queries.
> >
> > Fixes: e5e9b24ab8fa ("nfsd: freeze c/mtime updates with outstanding
> > WRITE_ATTRS delegation")
> > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> > ---
> >  fs/nfsd/nfs4proc.c | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> >
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 99b44b6ec056..fb891e35ebe9 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -1396,6 +1396,17 @@ nfsd4_verify_copy(struct svc_rqst *rqstp, struct
> > nfsd4_compound_state *cstate,
> >       goto out;
> >  }
> >
> > +static void nfsd_update_cmtime_attr(struct dentry *dentry)
> > +{
> > +     struct iattr attr = {
> > +             .ia_valid = ATTR_CTIME | ATTR_MTIME,
> > +     };
> > +
> > +     inode_lock(d_inode(dentry));
> > +     notify_change(&nop_mnt_idmap, dentry, &attr, NULL);
> > +     inode_unlock(d_inode(dentry));
> > +}
>
> I think there is an active delegation here. Without ATTR_DELEG,
> notify_change() calls try_break_deleg(), which will return
> -EWOULDBLOCK, causing the setattr to be silently skipped.
> Wouldn't it also initiate a CB_RECALL as well?

Destination file is opened for write (and has a write (attribute)
delegation) so there shouldn't be any conflicts. I don't see any
delegation recall triggered while testing CLONE. However, looking at
the code I now see that it does make sense to set ATTR_DELEG so that
the code under "if (!(ia_valid & ATTR_DELEG))" is not evaluated.

> > @@ -1413,6 +1424,9 @@ nfsd4_clone(struct svc_rqst *rqstp, struct
> > nfsd4_compound_state *cstate,
> >                       dst, clone->cl_dst_pos, clone->cl_count,
> >                       EX_ISSYNC(cstate->current_fh.fh_export));
> >
> > +     if ((READ_ONCE(dst->nf_file->f_mode) & FMODE_NOCMTIME) != 0)
> > +             nfsd_update_cmtime_attr(cstate->current_fh.fh_dentry);
> > +
> >       nfsd_file_put(dst);
> >       nfsd_file_put(src);
> >  out:
>
> Should this check whether nfsd4_clone_file_range() succeeded before
> updating timestamps?  If the clone fails, the file content is not
> modified, so a timestamp update would be incorrect.

I thought about doing it only if (!status) but didn't know if there
were some errors where the file was modified but an error occurred.
Sounds like a v2 is needed when it's only done for success.

> --
> Chuck Lever
>

[PATCH 2/3] nfsd: update mtime/ctime on synchronous COPY with delegated attributes

[Olga Kornievskaia]

COPY should update destination file's mtime/ctime upon completion.

Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
---
 fs/nfsd/nfs4proc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index fb891e35ebe9..04d8d0d1ca7d 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -2211,6 +2211,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	} else {
 		status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
 				       copy->nf_dst->nf_file, true);
+		if ((READ_ONCE(copy->nf_dst->nf_file->f_mode) &
+			       FMODE_NOCMTIME) != 0)
+			nfsd_update_cmtime_attr(cstate->current_fh.fh_dentry);
 	}
 out:
 	trace_nfsd_copy_done(copy, status);
-- 
2.52.0

Re: [PATCH 2/3] nfsd: update mtime/ctime on synchronous COPY with delegated attributes

[Chuck Lever]

On Tue, Apr 7, 2026, at 7:50 PM, Olga Kornievskaia wrote:
> COPY should update destination file's mtime/ctime upon completion.
>
> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>

Should 2/3 also carry a Fixes: tag?


> ---
>  fs/nfsd/nfs4proc.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index fb891e35ebe9..04d8d0d1ca7d 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c
> @@ -2211,6 +2211,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct 
> nfsd4_compound_state *cstate,
>  	} else {
>  		status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
>  				       copy->nf_dst->nf_file, true);
> +		if ((READ_ONCE(copy->nf_dst->nf_file->f_mode) &
> +			       FMODE_NOCMTIME) != 0)
> +			nfsd_update_cmtime_attr(cstate->current_fh.fh_dentry);
>  	}
>  out:
>  	trace_nfsd_copy_done(copy, status);
> -- 
> 2.52.0

-- 
Chuck Lever

Re: [PATCH 2/3] nfsd: update mtime/ctime on synchronous COPY with delegated attributes

[Olga Kornievskaia]

On Wed, Apr 8, 2026 at 9:59 AM Chuck Lever <cel@kernel.org> wrote:
>
>
>
> On Tue, Apr 7, 2026, at 7:50 PM, Olga Kornievskaia wrote:
> > COPY should update destination file's mtime/ctime upon completion.
> >
> > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
>
> Should 2/3 also carry a Fixes: tag?

My bad. I guess it should be the same as the first patch. But I now
also wonder if the update should be done based on the status of
nfsd4_do_copy? But this is where I think nfsd4_do_copy can return say
ENOSPC but it would have modified the file as well. So I'm not clear
if we need to special case return values?

> > ---
> >  fs/nfsd/nfs4proc.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index fb891e35ebe9..04d8d0d1ca7d 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
> > @@ -2211,6 +2211,9 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
> > nfsd4_compound_state *cstate,
> >       } else {
> >               status = nfsd4_do_copy(copy, copy->nf_src->nf_file,
> >                                      copy->nf_dst->nf_file, true);
> > +             if ((READ_ONCE(copy->nf_dst->nf_file->f_mode) &
> > +                            FMODE_NOCMTIME) != 0)
> > +                     nfsd_update_cmtime_attr(cstate->current_fh.fh_dentry);
> >       }
> >  out:
> >       trace_nfsd_copy_done(copy, status);
> > --
> > 2.52.0
>
> --
> Chuck Lever
>

Re: [PATCH 2/3] nfsd: update mtime/ctime on synchronous COPY with delegated attributes

[Chuck Lever]

On Wed, Apr 8, 2026, at 11:24 AM, Olga Kornievskaia wrote:
> On Wed, Apr 8, 2026 at 9:59 AM Chuck Lever <cel@kernel.org> wrote:
>>
>> On Tue, Apr 7, 2026, at 7:50 PM, Olga Kornievskaia wrote:
>> > COPY should update destination file's mtime/ctime upon completion.
>> >
>> > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
>>
>> Should 2/3 also carry a Fixes: tag?
>
> My bad. I guess it should be the same as the first patch. But I now
> also wonder if the update should be done based on the status of
> nfsd4_do_copy? But this is where I think nfsd4_do_copy can return say
> ENOSPC but it would have modified the file as well. So I'm not clear
> if we need to special case return values?

Thinking aloud:

If you updated the time stamps in _nfsd_copy_file_range() wouldn't
that take care of both the sync and async cases, and you could
determine precisely whether a data change had been made to the
destination file?


-- 
Chuck Lever

[PATCH 3/3] nfsd: update mtime/ctime on asynchronous COPY with delegated attributes

[Olga Kornievskaia]

Asynchronous COPY should update destination file's mtime/ctime upon
completion of copy work (not COPY compound processing).

Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
---
 fs/nfsd/nfs4proc.c | 21 ++++++++++++++++++++-
 fs/nfsd/xdr4.h     |  1 +
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
index 04d8d0d1ca7d..d858a5b58a24 100644
--- a/fs/nfsd/nfs4proc.c
+++ b/fs/nfsd/nfs4proc.c
@@ -2134,6 +2134,16 @@ static int nfsd4_do_async_copy(void *data)
 	trace_nfsd_copy_async_done(copy);
 	nfsd4_send_cb_offload(copy);
 	atomic_dec(&copy->cp_nn->pending_async_copies);
+	/* choosing to check for existence of set dentry pointer to indicate
+	 * that we need to update the attributes and do a dput because the
+	 * file flag could be cleared by a DELEGRETURN and then we'd lose
+	 * that copy was started with file opened with NOCMTIME and we gotten
+	 * a reference on the dentry.
+	 */
+	if (copy->d_dst) {
+		nfsd_update_cmtime_attr(copy->d_dst);
+		dput(copy->d_dst);
+	}
 	return 0;
 }
 
@@ -2193,6 +2203,11 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 		memcpy(&result->cb_stateid, &copy->cp_stateid.cs_stid,
 			sizeof(result->cb_stateid));
 		dup_copy_fields(copy, async_copy);
+		if ((READ_ONCE(copy->nf_dst->nf_file->f_mode) &
+			       FMODE_NOCMTIME) != 0) {
+			async_copy->d_dst = cstate->current_fh.fh_dentry;
+			dget(cstate->current_fh.fh_dentry);
+		}
 		memcpy(async_copy->cp_cb_offload.co_referring_sessionid.data,
 		       cstate->session->se_sessionid.data,
 		       NFS4_MAX_SESSIONID_LEN);
@@ -2220,8 +2235,12 @@ nfsd4_copy(struct svc_rqst *rqstp, struct nfsd4_compound_state *cstate,
 	release_copy_files(copy);
 	return status;
 out_dec_async_copy_err:
-	if (async_copy)
+	if (async_copy) {
 		atomic_dec(&nn->pending_async_copies);
+		if ((READ_ONCE(copy->nf_dst->nf_file->f_mode) &
+			       FMODE_NOCMTIME) != 0)
+			dput(cstate->current_fh.fh_dentry);
+	}
 out_err:
 	if (nfsd4_ssc_is_inter(copy)) {
 		/*
diff --git a/fs/nfsd/xdr4.h b/fs/nfsd/xdr4.h
index 417e9ad9fbb3..ddd6e005d3cf 100644
--- a/fs/nfsd/xdr4.h
+++ b/fs/nfsd/xdr4.h
@@ -752,6 +752,7 @@ struct nfsd4_copy {
 
 	struct nfsd_file        *nf_src;
 	struct nfsd_file        *nf_dst;
+	struct dentry		*d_dst;
 
 	copy_stateid_t		cp_stateid;
 
-- 
2.52.0

Re: [PATCH 3/3] nfsd: update mtime/ctime on asynchronous COPY with delegated attributes

[Chuck Lever]

On Tue, Apr 7, 2026, at 7:50 PM, Olga Kornievskaia wrote:
> Asynchronous COPY should update destination file's mtime/ctime upon
> completion of copy work (not COPY compound processing).
>
> Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> ---
>  fs/nfsd/nfs4proc.c | 21 ++++++++++++++++++++-
>  fs/nfsd/xdr4.h     |  1 +
>  2 files changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> index 04d8d0d1ca7d..d858a5b58a24 100644
> --- a/fs/nfsd/nfs4proc.c
> +++ b/fs/nfsd/nfs4proc.c

> @@ -2134,6 +2134,16 @@ static int nfsd4_do_async_copy(void *data)
>  	nfsd4_send_cb_offload(copy);
>  	atomic_dec(&copy->cp_nn->pending_async_copies);
> +	/* choosing to check for existence of set dentry pointer to indicate
> +	 * that we need to update the attributes and do a dput because the
> +	 * file flag could be cleared by a DELEGRETURN and then we'd lose
> +	 * that copy was started with file opened with NOCMTIME and we gotten
> +	 * a reference on the dentry.
> +	 */

Nit: "we gotten" might instead be "we had gotten" or "we got".
And let's stick with "/*" on a separate line to introduce
comment blocks in NFSD.


> +	if (copy->d_dst) {
> +		nfsd_update_cmtime_attr(copy->d_dst);
> +		dput(copy->d_dst);
> +	}

Jeff earlier suggested that the timestamp update should happen
before sending CB_OFFLOAD, so that a client issuing GETATTR in
response to the callback sees the updated timestamps. Should
nfsd_update_cmtime_attr be moved above nfsd4_send_cb_offload?


> @@ -2193,6 +2203,11 @@ nfsd4_copy(struct svc_rqst *rqstp, struct 
> nfsd4_compound_state *cstate,
>  		memcpy(&result->cb_stateid, &copy->cp_stateid.cs_stid,
>  			sizeof(result->cb_stateid));
>  		dup_copy_fields(copy, async_copy);
> +		if ((READ_ONCE(copy->nf_dst->nf_file->f_mode) &
> +			       FMODE_NOCMTIME) != 0) {
> +			async_copy->d_dst = cstate->current_fh.fh_dentry;
> +			dget(cstate->current_fh.fh_dentry);
> +		}
>  		memcpy(async_copy->cp_cb_offload.co_referring_sessionid.data,
>  		       cstate->session->se_sessionid.data,
>  		       NFS4_MAX_SESSIONID_LEN);

The error path checks FMODE_NOCMTIME to decide whether to dput,
but three gotos reach out_dec_async_copy_err before the dget:

nfsd4_copy() {
    ...
    if (atomic_inc_return(...) > sp_nrthreads)
        goto out_dec_async_copy_err;    /* before dget */
    async_copy->cp_src = kmalloc_obj(...);
    if (!async_copy->cp_src)
        goto out_dec_async_copy_err;    /* before dget */
    if (!nfs4_init_copy_state(nn, copy))
        goto out_dec_async_copy_err;    /* before dget */
    ...
    dget(cstate->current_fh.fh_dentry); /* dget is here */
    ...
    if (IS_ERR(async_copy->copy_task))
        goto out_dec_async_copy_err;    /* after dget */
}

If FMODE_NOCMTIME happens to be set when one of the early gotos
fires, this calls dput without a matching dget, resulting in a
dentry refcount underflow.

Additionally, the comment in nfsd4_do_async_copy explains that
FMODE_NOCMTIME can be cleared by a concurrent DELEGRETURN, which
is why the thread checks copy->d_dst instead. The same reasoning
applies here: a concurrent DELEGRETURN between the dget and a
kthread_create failure would cause the error path to skip the
dput, leaking the dentry reference.

Would it be simpler to check async_copy->d_dst here instead of
re-reading FMODE_NOCMTIME? That field is NULL before the dget
and non-NULL after, which handles both cases correctly:

    if (async_copy->d_dst)
        dput(async_copy->d_dst);


-- 
Chuck Lever

Re: [PATCH 3/3] nfsd: update mtime/ctime on asynchronous COPY with delegated attributes

[Olga Kornievskaia]

On Wed, Apr 8, 2026 at 10:10 AM Chuck Lever <cel@kernel.org> wrote:
>
>
>
> On Tue, Apr 7, 2026, at 7:50 PM, Olga Kornievskaia wrote:
> > Asynchronous COPY should update destination file's mtime/ctime upon
> > completion of copy work (not COPY compound processing).
> >
> > Signed-off-by: Olga Kornievskaia <okorniev@redhat.com>
> > ---
> >  fs/nfsd/nfs4proc.c | 21 ++++++++++++++++++++-
> >  fs/nfsd/xdr4.h     |  1 +
> >  2 files changed, 21 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/nfsd/nfs4proc.c b/fs/nfsd/nfs4proc.c
> > index 04d8d0d1ca7d..d858a5b58a24 100644
> > --- a/fs/nfsd/nfs4proc.c
> > +++ b/fs/nfsd/nfs4proc.c
>
> > @@ -2134,6 +2134,16 @@ static int nfsd4_do_async_copy(void *data)
> >       nfsd4_send_cb_offload(copy);
> >       atomic_dec(&copy->cp_nn->pending_async_copies);
> > +     /* choosing to check for existence of set dentry pointer to indicate
> > +      * that we need to update the attributes and do a dput because the
> > +      * file flag could be cleared by a DELEGRETURN and then we'd lose
> > +      * that copy was started with file opened with NOCMTIME and we gotten
> > +      * a reference on the dentry.
> > +      */
>
> Nit: "we gotten" might instead be "we had gotten" or "we got".
> And let's stick with "/*" on a separate line to introduce
> comment blocks in NFSD.

Will fix. Thanks.

>
> > +     if (copy->d_dst) {
> > +             nfsd_update_cmtime_attr(copy->d_dst);
> > +             dput(copy->d_dst);
> > +     }
>
> Jeff earlier suggested that the timestamp update should happen
> before sending CB_OFFLOAD, so that a client issuing GETATTR in
> response to the callback sees the updated timestamps. Should
> nfsd_update_cmtime_attr be moved above nfsd4_send_cb_offload?

I'll move it up. I thought that since nfsd4_send_cb_offload just
schedules the CB_OFFLOAD and returns, then placement of
nfsd_update_cmtime_attr isn't so critical.

> > @@ -2193,6 +2203,11 @@ nfsd4_copy(struct svc_rqst *rqstp, struct
> > nfsd4_compound_state *cstate,
> >               memcpy(&result->cb_stateid, &copy->cp_stateid.cs_stid,
> >                       sizeof(result->cb_stateid));
> >               dup_copy_fields(copy, async_copy);
> > +             if ((READ_ONCE(copy->nf_dst->nf_file->f_mode) &
> > +                            FMODE_NOCMTIME) != 0) {
> > +                     async_copy->d_dst = cstate->current_fh.fh_dentry;
> > +                     dget(cstate->current_fh.fh_dentry);
> > +             }
> >               memcpy(async_copy->cp_cb_offload.co_referring_sessionid.data,
> >                      cstate->session->se_sessionid.data,
> >                      NFS4_MAX_SESSIONID_LEN);
>
> The error path checks FMODE_NOCMTIME to decide whether to dput,
> but three gotos reach out_dec_async_copy_err before the dget:
>
> nfsd4_copy() {
>     ...
>     if (atomic_inc_return(...) > sp_nrthreads)
>         goto out_dec_async_copy_err;    /* before dget */
>     async_copy->cp_src = kmalloc_obj(...);
>     if (!async_copy->cp_src)
>         goto out_dec_async_copy_err;    /* before dget */
>     if (!nfs4_init_copy_state(nn, copy))
>         goto out_dec_async_copy_err;    /* before dget */
>     ...
>     dget(cstate->current_fh.fh_dentry); /* dget is here */
>     ...
>     if (IS_ERR(async_copy->copy_task))
>         goto out_dec_async_copy_err;    /* after dget */
> }
>
> If FMODE_NOCMTIME happens to be set when one of the early gotos
> fires, this calls dput without a matching dget, resulting in a
> dentry refcount underflow.

I will attempt to correct my error handling dput() in v2! Thanks.

> Additionally, the comment in nfsd4_do_async_copy explains that
> FMODE_NOCMTIME can be cleared by a concurrent DELEGRETURN, which
> is why the thread checks copy->d_dst instead. The same reasoning
> applies here: a concurrent DELEGRETURN between the dget and a
> kthread_create failure would cause the error path to skip the
> dput, leaking the dentry reference.
>
> Would it be simpler to check async_copy->d_dst here instead of
> re-reading FMODE_NOCMTIME? That field is NULL before the dget
> and non-NULL after, which handles both cases correctly:
>
>     if (async_copy->d_dst)
>         dput(async_copy->d_dst);

Will fix. Thanks.

>
>
> --
> Chuck Lever
>

Re: [PATCH 0/3] nfsd update mtime/ctime on CLONE/COPY with delegated attritutes

[Jeff Layton]

On Tue, 2026-04-07 at 19:50 -0400, Olga Kornievskaia wrote:
> generic/407 is failing in presence of delegated attributes and this patch series 
> explores the solution where the CLONE compound returns updated mtime/ctime
> in the GETATTR op in the compound.
> 
> It's broken into several pieces as I consider asynchronous copy update the trickest
> because the copy thread currently have no dentry to call notify_change() on. And
> if we are going to keep a copy of it while the copy is running we probably need to
> take a reference on it (which I attempt to do).
> 
> An outstanding question I have is whether or not a CLONE/COPY operation is supposed to
> also update the atime on the source file? But also atime for the dest file as well?
> I'm updating both ctime and mtime for the destination (though Jeff only suggested
> mtime and perhaps I'm missing something in updating ctime as well but I thought it 
> was appropriate). 
> 

CLONE/COPY aren't defined by POSIX so I'm not sure we have clear
guidance about the atime on the source or destination. Within Linux,
COPY is implemented using reads and writes though, so I'd expect that
the source file has its atime updated, and the destination gets a new
c/time. I don't see why it'd need to read the destination file though,
so I wouldn't expect an atime update there.

> COPY was tested by modifying the linux client to send a GETATTR in the COPY compound.
> Whether or not the client should add the GETATTR to the COPY compound is yet another
> question I have but I guess it would be for Trond.
> 
> Olga Kornievskaia (3):
>   nfsd: update mtime/ctime on CLONE in presense of delegated attributes
>   nfsd: update mtime/ctime on synchronous COPY with delegated attributes
>   nfsd: update mtime/ctime on asynchronous COPY with delegated
>     attributes
> 
>  fs/nfsd/nfs4proc.c | 38 +++++++++++++++++++++++++++++++++++++-
>  fs/nfsd/xdr4.h     |  1 +
>  2 files changed, 38 insertions(+), 1 deletion(-)

-- 
Jeff Layton <jlayton@kernel.org>

Re: [PATCH 0/3] nfsd update mtime/ctime on CLONE/COPY with delegated attritutes

[Jeff Layton]

On Tue, 2026-04-07 at 19:50 -0400, Olga Kornievskaia wrote:
> generic/407 is failing in presence of delegated attributes and this patch series 
> explores the solution where the CLONE compound returns updated mtime/ctime
> in the GETATTR op in the compound.
> 
> It's broken into several pieces as I consider asynchronous copy update the trickest
> because the copy thread currently have no dentry to call notify_change() on. And
> if we are going to keep a copy of it while the copy is running we probably need to
> take a reference on it (which I attempt to do).
> 
> An outstanding question I have is whether or not a CLONE/COPY operation is supposed to
> also update the atime on the source file? But also atime for the dest file as well?
> I'm updating both ctime and mtime for the destination (though Jeff only suggested
> mtime and perhaps I'm missing something in updating ctime as well but I thought it 
> was appropriate). 
> 
> COPY was tested by modifying the linux client to send a GETATTR in the COPY compound.
> Whether or not the client should add the GETATTR to the COPY compound is yet another
> question I have but I guess it would be for Trond.
> 
> Olga Kornievskaia (3):
>   nfsd: update mtime/ctime on CLONE in presense of delegated attributes
>   nfsd: update mtime/ctime on synchronous COPY with delegated attributes
>   nfsd: update mtime/ctime on asynchronous COPY with delegated
>     attributes
> 
>  fs/nfsd/nfs4proc.c | 38 +++++++++++++++++++++++++++++++++++++-
>  fs/nfsd/xdr4.h     |  1 +
>  2 files changed, 38 insertions(+), 1 deletion(-)

This all looks good to me. Thanks for fixing this up, Olga!

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