| Message ID | CAH2r5mt74Z7fGKu2esBk+sJzR=Qt=xRJ-oGgWztUcRDdXKD7ig@mail.gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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).
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 :-). -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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