[PATCH] cifs: Fix non-availability of dedup breaking generic/304

[PATCH] cifs: Fix non-availability of dedup breaking generic/304

[David Howells]

Deduplication isn't supported on cifs, but cifs doesn't reject it, instead
treating it as extent duplication/cloning.  This can cause generic/304 to go
silly and run for hours on end.

Fix cifs to indicate EOPNOTSUPP if REMAP_FILE_DEDUP is set in
->remap_file_range().

Note that it's unclear whether or not commit b073a08016a1 is meant to cause
cifs to return an error if REMAP_FILE_DEDUP.

Fixes: b073a08016a1 ("cifs: fix that return -EINVAL when do dedupe operation")
Suggested-by: Dave Chinner <david@fromorbit.com>
cc: Steve French <sfrench@samba.org>
cc: Xiaoli Feng <fengxiaoli0714@gmail.com>
cc: Shyam Prasad N <nspmangalore@gmail.com>
cc: Rohith Surabattula <rohiths.msft@gmail.com>
cc: Jeff Layton <jlayton@kernel.org>
cc: Darrick Wong <darrick.wong@oracle.com>
cc: fstests@vger.kernel.org
cc: linux-cifs@vger.kernel.org
cc: linux-fsdevel@vger.kernel.org
Link: https://lore.kernel.org/r/3876191.1701555260@warthog.procyon.org.uk/
---
 fs/smb/client/cifsfs.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
index 4d8927b57776..96a65cf9b5ec 100644
--- a/fs/smb/client/cifsfs.c
+++ b/fs/smb/client/cifsfs.c
@@ -1276,7 +1276,9 @@ static loff_t cifs_remap_file_range(struct file *src_file, loff_t off,
 	unsigned int xid;
 	int rc;
 
-	if (remap_flags & ~(REMAP_FILE_DEDUP | REMAP_FILE_ADVISORY))
+	if (remap_flags & REMAP_FILE_DEDUP)
+		return -EOPNOTSUPP;
+	if (remap_flags & ~REMAP_FILE_ADVISORY)
 		return -EINVAL;
 
 	cifs_dbg(FYI, "clone range\n");

Re: [PATCH] cifs: Fix non-availability of dedup breaking generic/304

[Steve French]

added missing Signed-off-by line (and also for the two previous
patches from David) and added Cc: stable and tentatively merged into
cifs-2.6.git for-next pending additional testing

On Mon, Dec 4, 2023 at 8:02 AM David Howells <dhowells@redhat.com> wrote:
>
> Deduplication isn't supported on cifs, but cifs doesn't reject it, instead
> treating it as extent duplication/cloning.  This can cause generic/304 to go
> silly and run for hours on end.
>
> Fix cifs to indicate EOPNOTSUPP if REMAP_FILE_DEDUP is set in
> ->remap_file_range().
>
> Note that it's unclear whether or not commit b073a08016a1 is meant to cause
> cifs to return an error if REMAP_FILE_DEDUP.
>
> Fixes: b073a08016a1 ("cifs: fix that return -EINVAL when do dedupe operation")
> Suggested-by: Dave Chinner <david@fromorbit.com>
> cc: Steve French <sfrench@samba.org>
> cc: Xiaoli Feng <fengxiaoli0714@gmail.com>
> cc: Shyam Prasad N <nspmangalore@gmail.com>
> cc: Rohith Surabattula <rohiths.msft@gmail.com>
> cc: Jeff Layton <jlayton@kernel.org>
> cc: Darrick Wong <darrick.wong@oracle.com>
> cc: fstests@vger.kernel.org
> cc: linux-cifs@vger.kernel.org
> cc: linux-fsdevel@vger.kernel.org
> Link: https://lore.kernel.org/r/3876191.1701555260@warthog.procyon.org.uk/
> ---
>  fs/smb/client/cifsfs.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> index 4d8927b57776..96a65cf9b5ec 100644
> --- a/fs/smb/client/cifsfs.c
> +++ b/fs/smb/client/cifsfs.c
> @@ -1276,7 +1276,9 @@ static loff_t cifs_remap_file_range(struct file *src_file, loff_t off,
>         unsigned int xid;
>         int rc;
>
> -       if (remap_flags & ~(REMAP_FILE_DEDUP | REMAP_FILE_ADVISORY))
> +       if (remap_flags & REMAP_FILE_DEDUP)
> +               return -EOPNOTSUPP;
> +       if (remap_flags & ~REMAP_FILE_ADVISORY)
>                 return -EINVAL;
>
>         cifs_dbg(FYI, "clone range\n");
>
>


-- 
Thanks,

Steve

Re: [PATCH] cifs: Fix non-availability of dedup breaking generic/304

[Dave Chinner]

On Mon, Dec 04, 2023 at 02:01:59PM +0000, David Howells wrote:
> Deduplication isn't supported on cifs, but cifs doesn't reject it, instead
> treating it as extent duplication/cloning.  This can cause generic/304 to go
> silly and run for hours on end.

This should also mention that it cloning rather than comparing data
can cause server-side data corruption in the destination file for
the benefit of anyone trying to track down weird data corruption
problems....

> Fix cifs to indicate EOPNOTSUPP if REMAP_FILE_DEDUP is set in
> ->remap_file_range().
> 
> Note that it's unclear whether or not commit b073a08016a1 is meant to cause
> cifs to return an error if REMAP_FILE_DEDUP.
>
> Fixes: b073a08016a1 ("cifs: fix that return -EINVAL when do dedupe operation")
> Suggested-by: Dave Chinner <david@fromorbit.com>
> cc: Steve French <sfrench@samba.org>
> cc: Xiaoli Feng <fengxiaoli0714@gmail.com>
> cc: Shyam Prasad N <nspmangalore@gmail.com>
> cc: Rohith Surabattula <rohiths.msft@gmail.com>
> cc: Jeff Layton <jlayton@kernel.org>
> cc: Darrick Wong <darrick.wong@oracle.com>
> cc: fstests@vger.kernel.org
> cc: linux-cifs@vger.kernel.org
> cc: linux-fsdevel@vger.kernel.org
> Link: https://lore.kernel.org/r/3876191.1701555260@warthog.procyon.org.uk/
> ---
>  fs/smb/client/cifsfs.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/smb/client/cifsfs.c b/fs/smb/client/cifsfs.c
> index 4d8927b57776..96a65cf9b5ec 100644
> --- a/fs/smb/client/cifsfs.c
> +++ b/fs/smb/client/cifsfs.c
> @@ -1276,7 +1276,9 @@ static loff_t cifs_remap_file_range(struct file *src_file, loff_t off,
>  	unsigned int xid;
>  	int rc;
>  
> -	if (remap_flags & ~(REMAP_FILE_DEDUP | REMAP_FILE_ADVISORY))
> +	if (remap_flags & REMAP_FILE_DEDUP)
> +		return -EOPNOTSUPP;
> +	if (remap_flags & ~REMAP_FILE_ADVISORY)
>  		return -EINVAL;
>  
>  	cifs_dbg(FYI, "clone range\n");

Apart from updating the commit message, the fix looks fine to me.

Reviewed-by: Dave Chinner <dchinner@redhat.com>
-- 
Dave Chinner
david@fromorbit.com

[PATCH] cifs: Fix non-availability of dedup breaking generic/304

[Steve French]

Running some tests with for-next (which includes the dedup patch) I do
see one additional test being skipped that passes on 6.7-rc4 (without
the dedup patch from David Howells):

     generic/374 0s ... [not run] Dedupe not supported by scratch
filesystem type: cifs

That may be ok, but seems strange - ie that the test 'works' in rc4
even without dedupe support.
The test is trying to check cross mount dedupe so probably good that
it now reports "not supported" properly.

Current for-next does pass test generic/075 which is good ...
-- 
Thanks,

Steve