[PATCH] cifs/smb3: directory sync should not return an error

[PATCH] cifs/smb3: directory sync should not return an error

[Steve French]

[-- Attachment #1: Type: text/plain, Size: 200 bytes --]

As with NFS, which ignores sync on directory handles,
fsync on a directory handle is a noop for CIFS/SMB3.
Do not return an error on it.  It breaks some database
apps otherwise.



-- 
Thanks,

Steve

[-- Attachment #2: 0001-smb3-directory-sync-should-not-return-an-error.patch --]
[-- Type: text/x-patch, Size: 1477 bytes --]

From 6112a4967573f9a347f7abc02e80423851b73737 Mon Sep 17 00:00:00 2001
From: Steve French <smfrench@gmail.com>
Date: Thu, 10 May 2018 10:59:37 -0500
Subject: [PATCH] smb3: directory sync should not return an error

As with NFS, which ignores sync on directory handles,
fsync on a directory handle is a noop for CIFS/SMB3.
Do not return an error on it.  It breaks some database
apps otherwise.

Signed-off-by: Steve French <smfrench@gmail.com>
CC: Stable <stable@vger.kernel.org>
---
 fs/cifs/cifsfs.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
index ed8e181927d6..8e41186d9923 100644
--- a/fs/cifs/cifsfs.c
+++ b/fs/cifs/cifsfs.c
@@ -1049,6 +1049,18 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
 	return rc;
 }
 
+/*
+ * Directory operations under CIFS/SMB2/SMB3 are synchronous, so fsync()
+ * is a dummy operation.
+ */
+int cifs_dir_fsync(struct file *file, loff_t start, loff_t end, int datasync)
+{
+	cifs_dbg(FYI, "Sync directory - name: %pD datasync: 0x%x\n",
+		 file, datasync);
+
+	return 0;
+}
+
 static ssize_t cifs_copy_file_range(struct file *src_file, loff_t off,
 				struct file *dst_file, loff_t destoff,
 				size_t len, unsigned int flags)
@@ -1183,6 +1195,7 @@ const struct file_operations cifs_dir_ops = {
 	.copy_file_range = cifs_copy_file_range,
 	.clone_file_range = cifs_clone_file_range,
 	.llseek = generic_file_llseek,
+	.fsync = cifs_dir_fsync,
 };
 
 static void
-- 
2.17.0

Re: [PATCH] cifs/smb3: directory sync should not return an error

[Jeremy Allison]

On Thu, May 10, 2018 at 11:04:14AM -0500, Steve French via samba-technical wrote:
> As with NFS, which ignores sync on directory handles,
> fsync on a directory handle is a noop for CIFS/SMB3.
> Do not return an error on it.  It breaks some database
> apps otherwise.

Thanks for this. I now have an smbtorture test that
shows we handle flush on directories incorrectly.

Patch to follow.

Jeremy.

> -- 
> Thanks,
> 
> Steve

> From 6112a4967573f9a347f7abc02e80423851b73737 Mon Sep 17 00:00:00 2001
> From: Steve French <smfrench@gmail.com>
> Date: Thu, 10 May 2018 10:59:37 -0500
> Subject: [PATCH] smb3: directory sync should not return an error
> 
> As with NFS, which ignores sync on directory handles,
> fsync on a directory handle is a noop for CIFS/SMB3.
> Do not return an error on it.  It breaks some database
> apps otherwise.
> 
> Signed-off-by: Steve French <smfrench@gmail.com>
> CC: Stable <stable@vger.kernel.org>
> ---
>  fs/cifs/cifsfs.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/fs/cifs/cifsfs.c b/fs/cifs/cifsfs.c
> index ed8e181927d6..8e41186d9923 100644
> --- a/fs/cifs/cifsfs.c
> +++ b/fs/cifs/cifsfs.c
> @@ -1049,6 +1049,18 @@ ssize_t cifs_file_copychunk_range(unsigned int xid,
>  	return rc;
>  }
>  
> +/*
> + * Directory operations under CIFS/SMB2/SMB3 are synchronous, so fsync()
> + * is a dummy operation.
> + */
> +int cifs_dir_fsync(struct file *file, loff_t start, loff_t end, int datasync)
> +{
> +	cifs_dbg(FYI, "Sync directory - name: %pD datasync: 0x%x\n",
> +		 file, datasync);
> +
> +	return 0;
> +}
> +
>  static ssize_t cifs_copy_file_range(struct file *src_file, loff_t off,
>  				struct file *dst_file, loff_t destoff,
>  				size_t len, unsigned int flags)
> @@ -1183,6 +1195,7 @@ const struct file_operations cifs_dir_ops = {
>  	.copy_file_range = cifs_copy_file_range,
>  	.clone_file_range = cifs_clone_file_range,
>  	.llseek = generic_file_llseek,
> +	.fsync = cifs_dir_fsync,
>  };
>  
>  static void
> -- 
> 2.17.0
> 

Re: [PATCH] cifs/smb3: directory sync should not return an error

[Pavel Shilovsky]

2018-05-10 9:04 GMT-07:00 Steve French via samba-technical
<samba-technical@lists.samba.org>:
> As with NFS, which ignores sync on directory handles,
> fsync on a directory handle is a noop for CIFS/SMB3.
> Do not return an error on it.  It breaks some database
> apps otherwise.

Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>

Best regards,
Pavel Shilovsky

Re: [PATCH] cifs/smb3: directory sync should not return an error

[Jeremy Allison]

On Thu, May 10, 2018 at 10:11:43AM -0700, Pavel Shilovsky via samba-technical wrote:
> 2018-05-10 9:04 GMT-07:00 Steve French via samba-technical
> <samba-technical@lists.samba.org>:
> > As with NFS, which ignores sync on directory handles,
> > fsync on a directory handle is a noop for CIFS/SMB3.
> > Do not return an error on it.  It breaks some database
> > apps otherwise.
> 
> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>

NAK on this patch. It's due to a specific Samba server
bug, which I've just fixed.

Look at the man page for fsync() on directory handles
- in SMB2+ as well this is a useful thing for an
application to do.

The broken Samba server returns NT_STATUS_ACCESS_DENIED
here. What you need to do is test the popular servers
(Windows, NetApp, EMC, Samba, OSX) and see which of
them are broken (not Windows obviously) and if so
what errors they return.

Best case scenario it's just Samba that was broken,
so check for the specific NT_STATUS_ACCESS_DENIED
error and ignore, otherwise return the error to
the caller - they *NEED* it :-).

Jeremy.

Re: [PATCH] cifs/smb3: directory sync should not return an error

[Steve French]

It wasn't sent to Samba - the error was returned by the VFS before it
comes to cifs.ko because we don't implement this call.    NFS
(correctly presumably) implements the syscall by ignoring it
(returning 0 - rather than the default which is an error), because
once an entry in the directory is created it is in the namespace, and
they are never cached on the client.

On Thu, May 10, 2018 at 1:48 PM, Jeremy Allison <jra@samba.org> wrote:
> On Thu, May 10, 2018 at 10:11:43AM -0700, Pavel Shilovsky via samba-technical wrote:
>> 2018-05-10 9:04 GMT-07:00 Steve French via samba-technical
>> <samba-technical@lists.samba.org>:
>> > As with NFS, which ignores sync on directory handles,
>> > fsync on a directory handle is a noop for CIFS/SMB3.
>> > Do not return an error on it.  It breaks some database
>> > apps otherwise.
>>
>> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>
> NAK on this patch. It's due to a specific Samba server
> bug, which I've just fixed.
>
> Look at the man page for fsync() on directory handles
> - in SMB2+ as well this is a useful thing for an
> application to do.
>
> The broken Samba server returns NT_STATUS_ACCESS_DENIED
> here. What you need to do is test the popular servers
> (Windows, NetApp, EMC, Samba, OSX) and see which of
> them are broken (not Windows obviously) and if so
> what errors they return.
>
> Best case scenario it's just Samba that was broken,
> so check for the specific NT_STATUS_ACCESS_DENIED
> error and ignore, otherwise return the error to
> the caller - they *NEED* it :-).
>
> Jeremy.



-- 
Thanks,

Steve

Re: [PATCH] cifs/smb3: directory sync should not return an error

[ronnie sahlberg]

Reviewed-by: Ronnie Sahlberg <lsahlber@redhat.com>

On Fri, May 11, 2018 at 2:04 AM, Steve French via samba-technical
<samba-technical@lists.samba.org> wrote:
> As with NFS, which ignores sync on directory handles,
> fsync on a directory handle is a noop for CIFS/SMB3.
> Do not return an error on it.  It breaks some database
> apps otherwise.
>
>
>
> --
> Thanks,
>
> Steve

Re: [PATCH] cifs/smb3: directory sync should not return an error

[Steve French]

checking various Linux file systems - looks like most of those I
checked end up with the default "EINVAL" so basically not a very
useful call to check the return code on - safer to ignore.   If others
prefer, I don't mind sending the call over the wire and ignoring the
return code - but presumably some server is going to error unusually
if we pass it over the wire and expect particularly return codes (and
thus break apps that check for rc==EINVAL or rc==0).

On Thu, May 10, 2018 at 3:28 PM, Steve French <smfrench@gmail.com> wrote:
> It wasn't sent to Samba - the error was returned by the VFS before it
> comes to cifs.ko because we don't implement this call.    NFS
> (correctly presumably) implements the syscall by ignoring it
> (returning 0 - rather than the default which is an error), because
> once an entry in the directory is created it is in the namespace, and
> they are never cached on the client.
>
> On Thu, May 10, 2018 at 1:48 PM, Jeremy Allison <jra@samba.org> wrote:
>> On Thu, May 10, 2018 at 10:11:43AM -0700, Pavel Shilovsky via samba-technical wrote:
>>> 2018-05-10 9:04 GMT-07:00 Steve French via samba-technical
>>> <samba-technical@lists.samba.org>:
>>> > As with NFS, which ignores sync on directory handles,
>>> > fsync on a directory handle is a noop for CIFS/SMB3.
>>> > Do not return an error on it.  It breaks some database
>>> > apps otherwise.
>>>
>>> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>>
>> NAK on this patch. It's due to a specific Samba server
>> bug, which I've just fixed.
>>
>> Look at the man page for fsync() on directory handles
>> - in SMB2+ as well this is a useful thing for an
>> application to do.
>>
>> The broken Samba server returns NT_STATUS_ACCESS_DENIED
>> here. What you need to do is test the popular servers
>> (Windows, NetApp, EMC, Samba, OSX) and see which of
>> them are broken (not Windows obviously) and if so
>> what errors they return.
>>
>> Best case scenario it's just Samba that was broken,
>> so check for the specific NT_STATUS_ACCESS_DENIED
>> error and ignore, otherwise return the error to
>> the caller - they *NEED* it :-).
>>
>> Jeremy.
>
>
>
> --
> Thanks,
>
> Steve



-- 
Thanks,

Steve

Re: [PATCH] cifs/smb3: directory sync should not return an error

[ronnie sahlberg]

SMB2 FLUSH ?

MS-SMB2.pdf  is pretty clear that FLUSH can only be used on files or pipes.
If we start using it for directory handles we would need some
clarifications about this use in the spec.

I would wait until MS-SMB2 is updated before we start sending FLUSH on
directory handles.


On Fri, May 11, 2018 at 7:56 AM, Steve French via samba-technical
<samba-technical@lists.samba.org> wrote:
> checking various Linux file systems - looks like most of those I
> checked end up with the default "EINVAL" so basically not a very
> useful call to check the return code on - safer to ignore.   If others
> prefer, I don't mind sending the call over the wire and ignoring the
> return code - but presumably some server is going to error unusually
> if we pass it over the wire and expect particularly return codes (and
> thus break apps that check for rc==EINVAL or rc==0).
>
> On Thu, May 10, 2018 at 3:28 PM, Steve French <smfrench@gmail.com> wrote:
>> It wasn't sent to Samba - the error was returned by the VFS before it
>> comes to cifs.ko because we don't implement this call.    NFS
>> (correctly presumably) implements the syscall by ignoring it
>> (returning 0 - rather than the default which is an error), because
>> once an entry in the directory is created it is in the namespace, and
>> they are never cached on the client.
>>
>> On Thu, May 10, 2018 at 1:48 PM, Jeremy Allison <jra@samba.org> wrote:
>>> On Thu, May 10, 2018 at 10:11:43AM -0700, Pavel Shilovsky via samba-technical wrote:
>>>> 2018-05-10 9:04 GMT-07:00 Steve French via samba-technical
>>>> <samba-technical@lists.samba.org>:
>>>> > As with NFS, which ignores sync on directory handles,
>>>> > fsync on a directory handle is a noop for CIFS/SMB3.
>>>> > Do not return an error on it.  It breaks some database
>>>> > apps otherwise.
>>>>
>>>> Reviewed-by: Pavel Shilovsky <pshilov@microsoft.com>
>>>
>>> NAK on this patch. It's due to a specific Samba server
>>> bug, which I've just fixed.
>>>
>>> Look at the man page for fsync() on directory handles
>>> - in SMB2+ as well this is a useful thing for an
>>> application to do.
>>>
>>> The broken Samba server returns NT_STATUS_ACCESS_DENIED
>>> here. What you need to do is test the popular servers
>>> (Windows, NetApp, EMC, Samba, OSX) and see which of
>>> them are broken (not Windows obviously) and if so
>>> what errors they return.
>>>
>>> Best case scenario it's just Samba that was broken,
>>> so check for the specific NT_STATUS_ACCESS_DENIED
>>> error and ignore, otherwise return the error to
>>> the caller - they *NEED* it :-).
>>>
>>> Jeremy.
>>
>>
>>
>> --
>> Thanks,
>>
>> Steve
>
>
>
> --
> Thanks,
>
> Steve
>

Re: [PATCH] cifs/smb3: directory sync should not return an error

[Jeremy Allison]

On Fri, May 11, 2018 at 08:08:46AM +1000, ronnie sahlberg wrote:
> SMB2 FLUSH ?
> 
> MS-SMB2.pdf  is pretty clear that FLUSH can only be used on files or pipes.
> If we start using it for directory handles we would need some
> clarifications about this use in the spec.

Yes. MS-SMB2 is also wrong :-).

I have test code that proves FLUSH works against any directory
handle opened with FILE_ADD|DIRECTORY_ADD access mask granted.

(Steve thought this might be special cased to just the root
directory handle on a share, this turns out not to be the
case - any directory handle with the required access works
OK).

> I would wait until MS-SMB2 is updated before we start sending FLUSH on
> directory handles.

We need to deal with the protocol as it really is,
not as the documentation would like it to be :-).

Jeremy.

Re: [PATCH] cifs/smb3: directory sync should not return an error

[Steve French]

On Thu, May 10, 2018 at 5:12 PM, Jeremy Allison <jra@samba.org> wrote:
> On Fri, May 11, 2018 at 08:08:46AM +1000, ronnie sahlberg wrote:
>> SMB2 FLUSH ?
>>
>> MS-SMB2.pdf  is pretty clear that FLUSH can only be used on files or pipes.
>> If we start using it for directory handles we would need some
>> clarifications about this use in the spec.
>
> Yes. MS-SMB2 is also wrong :-).
>
> I have test code that proves FLUSH works against any directory
> handle opened with FILE_ADD|DIRECTORY_ADD access mask granted.
>
> (Steve thought this might be special cased to just the root
> directory handle on a share, this turns out not to be the
> case - any directory handle with the required access works
> OK).
>
>> I would wait until MS-SMB2 is updated before we start sending FLUSH on
>> directory handles.
>
> We need to deal with the protocol as it really is,
> not as the documentation would like it to be :-).

Current behavior seems to be that (for SMB2/SMB3 as with NFS)
servers are not expected to cache file creates.   If we send a flush over
the wire without a lot more testing we could break even more apps - unless
we simply send the request and ignore the return code which I would prefer
not to do until we get feedback from more servers and clarification from
MS-SMB2).  What we don't want to do is pass EINVAL back which breaks some.

Ronnie said it well:
" If/once ms-smb2.pdf is updated to describe the semantics for flush
on a directory, then we can think about using flush here. Not before.
Otherwise we just revert back to chasing implementation specific
behavior" (as we did with SMB1)

(so fix the current behavior - then think about whether we can safely
send this as a flush if there are any valid cases which MS-SMB2
exposes in the future).

-- 
Thanks,

Steve

Re: [PATCH] cifs/smb3: directory sync should not return an error

[Jeremy Allison]

On Thu, May 10, 2018 at 05:25:55PM -0500, Steve French wrote:
> 
> Current behavior seems to be that (for SMB2/SMB3 as with NFS)
> servers are not expected to cache file creates.   If we send a flush over
> the wire without a lot more testing we could break even more apps - unless
> we simply send the request and ignore the return code which I would prefer
> not to do until we get feedback from more servers and clarification from
> MS-SMB2).  What we don't want to do is pass EINVAL back which breaks some.
> 
> Ronnie said it well:
> " If/once ms-smb2.pdf is updated to describe the semantics for flush
> on a directory, then we can think about using flush here. Not before.
> Otherwise we just revert back to chasing implementation specific
> behavior" (as we did with SMB1)
> 
> (so fix the current behavior - then think about whether we can safely
> send this as a flush if there are any valid cases which MS-SMB2
> exposes in the future).

In the meantime I'm going to fix the smbd server to act
the same way that Windows Does (TM). That's what real
clients expect :-).