diff mbox series

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

Message ID 20210414143138.15192-2-trondmy@kernel.org (mailing list archive)
State New, archived
Headers show
Series [1/2] NFSv42: Copy offload should update the file size when appropriate | expand

Commit Message

Trond Myklebust April 14, 2021, 2:31 p.m. UTC 
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(-)

Comments

J. Bruce Fields April 14, 2021, 2:40 p.m. UTC | #1 
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
J. Bruce Fields April 14, 2021, 5:05 p.m. UTC | #2 
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
Trond Myklebust April 14, 2021, 5:23 p.m. UTC | #3 
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
J. Bruce Fields April 14, 2021, 5:28 p.m. UTC | #4 
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
> 
>
J. Bruce Fields April 14, 2021, 8:15 p.m. UTC | #5 
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
> > 
> >
diff mbox series

Patch

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)