From: Trond Myklebust <[email protected]>
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 <[email protected]>
---
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
Thanks! Testing....
--b.
On Wed, Apr 14, 2021 at 10:31:38AM -0400, [email protected] wrote:
> From: Trond Myklebust <[email protected]>
>
> 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 <[email protected]>
> ---
> 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
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, [email protected] wrote:
> > From: Trond Myklebust <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > 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
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, [email protected] wrote:
> > > From: Trond Myklebust <[email protected]>
> > >
> > > 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 <[email protected]>
> > > ---
> > > 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
[email protected]
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, [email protected] wrote:
> > > > From: Trond Myklebust <[email protected]>
> > > >
> > > > 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 <[email protected]>
> > > > ---
> > > > 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
> [email protected]
>
>
On Wed, Apr 14, 2021 at 01:28:25PM -0400, [email protected] 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, [email protected] wrote:
> > > > > From: Trond Myklebust <[email protected]>
> > > > >
> > > > > 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 <[email protected]>
> > > > > ---
> > > > > 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
> > [email protected]
> >
> >