[PATCH 1/2] NFSv42: Copy offload should update the file size when appropriate

[PATCH 1/2] NFSv42: Copy offload should update the file size when appropriate

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

If the result of a copy offload or clone operation is to grow the
destination file size, then we should update it. The reason is that when
a client holds a delegation, it is authoritative for the file size.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/nfs42proc.c | 41 ++++++++++++++++++++++++++++++++---------
 1 file changed, 32 insertions(+), 9 deletions(-)

diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
index 8d64eb953347..3875120ef3ef 100644
--- a/fs/nfs/nfs42proc.c
+++ b/fs/nfs/nfs42proc.c
@@ -269,6 +269,33 @@ static int process_copy_commit(struct file *dst, loff_t pos_dst,
 	return status;
 }
 
+/**
+ * nfs42_copy_dest_done - perform inode cache updates after clone/copy offload
+ * @inode: pointer to destination inode
+ * @pos: destination offset
+ * @len: copy length
+ *
+ * Punch a hole in the inode page cache, so that the NFS client will
+ * know to retrieve new data.
+ * Update the file size if necessary, and then mark the inode as having
+ * invalid cached values for change attribute, ctime, mtime and space used.
+ */
+static void nfs42_copy_dest_done(struct inode *inode, loff_t pos, loff_t len)
+{
+	loff_t newsize = pos + len;
+	loff_t end = newsize - 1;
+
+	truncate_pagecache_range(inode, pos, end);
+	spin_lock(&inode->i_lock);
+	if (newsize > i_size_read(inode))
+		i_size_write(inode, newsize);
+	nfs_set_cache_invalid(inode, NFS_INO_INVALID_CHANGE |
+					     NFS_INO_INVALID_CTIME |
+					     NFS_INO_INVALID_MTIME |
+					     NFS_INO_INVALID_BLOCKS);
+	spin_unlock(&inode->i_lock);
+}
+
 static ssize_t _nfs42_proc_copy(struct file *src,
 				struct nfs_lock_context *src_lock,
 				struct file *dst,
@@ -362,14 +389,8 @@ static ssize_t _nfs42_proc_copy(struct file *src,
 			goto out;
 	}
 
-	truncate_pagecache_range(dst_inode, pos_dst,
-				 pos_dst + res->write_res.count);
-	spin_lock(&dst_inode->i_lock);
-	nfs_set_cache_invalid(
-		dst_inode, NFS_INO_REVAL_PAGECACHE | NFS_INO_REVAL_FORCED |
-				   NFS_INO_INVALID_SIZE | NFS_INO_INVALID_ATTR |
-				   NFS_INO_INVALID_DATA);
-	spin_unlock(&dst_inode->i_lock);
+	nfs42_copy_dest_done(dst_inode, pos_dst, res->write_res.count);
+
 	spin_lock(&src_inode->i_lock);
 	nfs_set_cache_invalid(src_inode, NFS_INO_REVAL_PAGECACHE |
 						 NFS_INO_REVAL_FORCED |
@@ -1055,8 +1076,10 @@ static int _nfs42_proc_clone(struct rpc_message *msg, struct file *src_f,
 
 	status = nfs4_call_sync(server->client, server, msg,
 				&args.seq_args, &res.seq_res, 0);
-	if (status == 0)
+	if (status == 0) {
+		nfs42_copy_dest_done(dst_inode, dst_offset, count);
 		status = nfs_post_op_update_inode(dst_inode, res.dst_fattr);
+	}
 
 	kfree(res.dst_fattr);
 	return status;
-- 
2.30.2

[PATCH 2/2] NFSv42: Don't force attribute revalidation of the copy offload source

[trondmy]

From: Trond Myklebust <trond.myklebust@hammerspace.com>

When a copy offload is performed, we do not expect the source file to
change other than perhaps to see the atime be updated.

Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
---
 fs/nfs/nfs42proc.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
index 3875120ef3ef..a24349512ffe 100644
--- a/fs/nfs/nfs42proc.c
+++ b/fs/nfs/nfs42proc.c
@@ -390,12 +390,7 @@ static ssize_t _nfs42_proc_copy(struct file *src,
 	}
 
 	nfs42_copy_dest_done(dst_inode, pos_dst, res->write_res.count);
-
-	spin_lock(&src_inode->i_lock);
-	nfs_set_cache_invalid(src_inode, NFS_INO_REVAL_PAGECACHE |
-						 NFS_INO_REVAL_FORCED |
-						 NFS_INO_INVALID_ATIME);
-	spin_unlock(&src_inode->i_lock);
+	nfs_invalidate_atime(src_inode);
 	status = res->write_res.count;
 out:
 	if (args->sync)
-- 
2.30.2

Re: [PATCH 2/2] NFSv42: Don't force attribute revalidation of the copy offload source

[J. Bruce Fields]

Thanks!  Testing....

--b.

On Wed, Apr 14, 2021 at 10:31:38AM -0400, trondmy@kernel.org wrote:
> From: Trond Myklebust <trond.myklebust@hammerspace.com>
> 
> When a copy offload is performed, we do not expect the source file to
> change other than perhaps to see the atime be updated.
> 
> Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> ---
>  fs/nfs/nfs42proc.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> index 3875120ef3ef..a24349512ffe 100644
> --- a/fs/nfs/nfs42proc.c
> +++ b/fs/nfs/nfs42proc.c
> @@ -390,12 +390,7 @@ static ssize_t _nfs42_proc_copy(struct file *src,
>  	}
>  
>  	nfs42_copy_dest_done(dst_inode, pos_dst, res->write_res.count);
> -
> -	spin_lock(&src_inode->i_lock);
> -	nfs_set_cache_invalid(src_inode, NFS_INO_REVAL_PAGECACHE |
> -						 NFS_INO_REVAL_FORCED |
> -						 NFS_INO_INVALID_ATIME);
> -	spin_unlock(&src_inode->i_lock);
> +	nfs_invalidate_atime(src_inode);
>  	status = res->write_res.count;
>  out:
>  	if (args->sync)
> -- 
> 2.30.2

Re: [PATCH 2/2] NFSv42: Don't force attribute revalidation of the copy offload source

[J. Bruce Fields]

On Wed, Apr 14, 2021 at 10:40:33AM -0400, bfields wrote:
> Thanks!  Testing....

... and, test results back with these two patches applied, looks good!

--b.

> 
> --b.
> 
> On Wed, Apr 14, 2021 at 10:31:38AM -0400, trondmy@kernel.org wrote:
> > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > 
> > When a copy offload is performed, we do not expect the source file to
> > change other than perhaps to see the atime be updated.
> > 
> > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > ---
> >  fs/nfs/nfs42proc.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> > index 3875120ef3ef..a24349512ffe 100644
> > --- a/fs/nfs/nfs42proc.c
> > +++ b/fs/nfs/nfs42proc.c
> > @@ -390,12 +390,7 @@ static ssize_t _nfs42_proc_copy(struct file *src,
> >  	}
> >  
> >  	nfs42_copy_dest_done(dst_inode, pos_dst, res->write_res.count);
> > -
> > -	spin_lock(&src_inode->i_lock);
> > -	nfs_set_cache_invalid(src_inode, NFS_INO_REVAL_PAGECACHE |
> > -						 NFS_INO_REVAL_FORCED |
> > -						 NFS_INO_INVALID_ATIME);
> > -	spin_unlock(&src_inode->i_lock);
> > +	nfs_invalidate_atime(src_inode);
> >  	status = res->write_res.count;
> >  out:
> >  	if (args->sync)
> > -- 
> > 2.30.2

Re: [PATCH 2/2] NFSv42: Don't force attribute revalidation of the copy offload source

[Trond Myklebust]

On Wed, 2021-04-14 at 13:05 -0400, J. Bruce Fields wrote:
> On Wed, Apr 14, 2021 at 10:40:33AM -0400, bfields wrote:
> > Thanks!  Testing....
> 
> ... and, test results back with these two patches applied, looks
> good!

So, just out of curiosity. With which backing filesystem were you
testing? If it was XFS, then note that you may have been testing the
CLONE functionality instead of copy offload. As you saw, I added the
same fix for both clone and copy offload because they have the same
requirements for how the page and attribute caches are handled, so the
end result should be the same. However the unpatched code is very
different for the two methods, and clone may have been missing more
functionality than the actual copy-offload was.

> 
> --b.
> 
> > 
> > --b.
> > 
> > On Wed, Apr 14, 2021 at 10:31:38AM -0400, trondmy@kernel.org wrote:
> > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > 
> > > When a copy offload is performed, we do not expect the source
> > > file to
> > > change other than perhaps to see the atime be updated.
> > > 
> > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > ---
> > >  fs/nfs/nfs42proc.c | 7 +------
> > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > > 
> > > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> > > index 3875120ef3ef..a24349512ffe 100644
> > > --- a/fs/nfs/nfs42proc.c
> > > +++ b/fs/nfs/nfs42proc.c
> > > @@ -390,12 +390,7 @@ static ssize_t _nfs42_proc_copy(struct file
> > > *src,
> > >         }
> > >  
> > >         nfs42_copy_dest_done(dst_inode, pos_dst, res-
> > > >write_res.count);
> > > -
> > > -       spin_lock(&src_inode->i_lock);
> > > -       nfs_set_cache_invalid(src_inode, NFS_INO_REVAL_PAGECACHE
> > > |
> > > -                                               
> > > NFS_INO_REVAL_FORCED |
> > > -                                               
> > > NFS_INO_INVALID_ATIME);
> > > -       spin_unlock(&src_inode->i_lock);
> > > +       nfs_invalidate_atime(src_inode);
> > >         status = res->write_res.count;
> > >  out:
> > >         if (args->sync)
> > > -- 
> > > 2.30.2

-- 
Trond Myklebust
Linux NFS client maintainer, Hammerspace
trond.myklebust@hammerspace.com

Re: [PATCH 2/2] NFSv42: Don't force attribute revalidation of the copy offload source

[bfields]

On Wed, Apr 14, 2021 at 05:23:53PM +0000, Trond Myklebust wrote:
> On Wed, 2021-04-14 at 13:05 -0400, J. Bruce Fields wrote:
> > On Wed, Apr 14, 2021 at 10:40:33AM -0400, bfields wrote:
> > > Thanks!  Testing....
> > 
> > ... and, test results back with these two patches applied, looks
> > good!
> 
> So, just out of curiosity. With which backing filesystem were you
> testing? If it was XFS, then note that you may have been testing the
> CLONE functionality instead of copy offload. As you saw, I added the
> same fix for both clone and copy offload because they have the same
> requirements for how the page and attribute caches are handled, so the
> end result should be the same. However the unpatched code is very
> different for the two methods, and clone may have been missing more
> functionality than the actual copy-offload was.

I think it was actually xfs, but I did look at a trace and my memory is
it was trying a CLONE and falling back on COPY.  I'll double check....

--b.

> 
> > 
> > --b.
> > 
> > > 
> > > --b.
> > > 
> > > On Wed, Apr 14, 2021 at 10:31:38AM -0400, trondmy@kernel.org wrote:
> > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > 
> > > > When a copy offload is performed, we do not expect the source
> > > > file to
> > > > change other than perhaps to see the atime be updated.
> > > > 
> > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > ---
> > > >  fs/nfs/nfs42proc.c | 7 +------
> > > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > > > 
> > > > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> > > > index 3875120ef3ef..a24349512ffe 100644
> > > > --- a/fs/nfs/nfs42proc.c
> > > > +++ b/fs/nfs/nfs42proc.c
> > > > @@ -390,12 +390,7 @@ static ssize_t _nfs42_proc_copy(struct file
> > > > *src,
> > > >         }
> > > >  
> > > >         nfs42_copy_dest_done(dst_inode, pos_dst, res-
> > > > >write_res.count);
> > > > -
> > > > -       spin_lock(&src_inode->i_lock);
> > > > -       nfs_set_cache_invalid(src_inode, NFS_INO_REVAL_PAGECACHE
> > > > |
> > > > -                                               
> > > > NFS_INO_REVAL_FORCED |
> > > > -                                               
> > > > NFS_INO_INVALID_ATIME);
> > > > -       spin_unlock(&src_inode->i_lock);
> > > > +       nfs_invalidate_atime(src_inode);
> > > >         status = res->write_res.count;
> > > >  out:
> > > >         if (args->sync)
> > > > -- 
> > > > 2.30.2
> 
> -- 
> Trond Myklebust
> Linux NFS client maintainer, Hammerspace
> trond.myklebust@hammerspace.com
> 
> 

Re: [PATCH 2/2] NFSv42: Don't force attribute revalidation of the copy offload source

[bfields]

On Wed, Apr 14, 2021 at 01:28:25PM -0400, bfields@fieldses.org wrote:
> On Wed, Apr 14, 2021 at 05:23:53PM +0000, Trond Myklebust wrote:
> > On Wed, 2021-04-14 at 13:05 -0400, J. Bruce Fields wrote:
> > > On Wed, Apr 14, 2021 at 10:40:33AM -0400, bfields wrote:
> > > > Thanks!  Testing....
> > > 
> > > ... and, test results back with these two patches applied, looks
> > > good!
> > 
> > So, just out of curiosity. With which backing filesystem were you
> > testing? If it was XFS, then note that you may have been testing the
> > CLONE functionality instead of copy offload. As you saw, I added the
> > same fix for both clone and copy offload because they have the same
> > requirements for how the page and attribute caches are handled, so the
> > end result should be the same. However the unpatched code is very
> > different for the two methods, and clone may have been missing more
> > functionality than the actual copy-offload was.
> 
> I think it was actually xfs, but I did look at a trace and my memory is
> it was trying a CLONE and falling back on COPY.  I'll double check....

Yes, confirmed.  At least on my test server's distro (Fedora 28) xfs
doesn't seem to be enabling it by default.  I recreated the filesystems
with "mkfs.xfs -f -mreflink=1 /dev/vdf" and generic/430 is still passing
with your patches.

--b.


> 
> --b.
> 
> > 
> > > 
> > > --b.
> > > 
> > > > 
> > > > --b.
> > > > 
> > > > On Wed, Apr 14, 2021 at 10:31:38AM -0400, trondmy@kernel.org wrote:
> > > > > From: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > > 
> > > > > When a copy offload is performed, we do not expect the source
> > > > > file to
> > > > > change other than perhaps to see the atime be updated.
> > > > > 
> > > > > Signed-off-by: Trond Myklebust <trond.myklebust@hammerspace.com>
> > > > > ---
> > > > >  fs/nfs/nfs42proc.c | 7 +------
> > > > >  1 file changed, 1 insertion(+), 6 deletions(-)
> > > > > 
> > > > > diff --git a/fs/nfs/nfs42proc.c b/fs/nfs/nfs42proc.c
> > > > > index 3875120ef3ef..a24349512ffe 100644
> > > > > --- a/fs/nfs/nfs42proc.c
> > > > > +++ b/fs/nfs/nfs42proc.c
> > > > > @@ -390,12 +390,7 @@ static ssize_t _nfs42_proc_copy(struct file
> > > > > *src,
> > > > >         }
> > > > >  
> > > > >         nfs42_copy_dest_done(dst_inode, pos_dst, res-
> > > > > >write_res.count);
> > > > > -
> > > > > -       spin_lock(&src_inode->i_lock);
> > > > > -       nfs_set_cache_invalid(src_inode, NFS_INO_REVAL_PAGECACHE
> > > > > |
> > > > > -                                               
> > > > > NFS_INO_REVAL_FORCED |
> > > > > -                                               
> > > > > NFS_INO_INVALID_ATIME);
> > > > > -       spin_unlock(&src_inode->i_lock);
> > > > > +       nfs_invalidate_atime(src_inode);
> > > > >         status = res->write_res.count;
> > > > >  out:
> > > > >         if (args->sync)
> > > > > -- 
> > > > > 2.30.2
> > 
> > -- 
> > Trond Myklebust
> > Linux NFS client maintainer, Hammerspace
> > trond.myklebust@hammerspace.com
> > 
> >