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 |
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
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
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
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 > >
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 --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)