Bug 14060 - smbd fails to open kernel-oplocked files with smb1
Summary: smbd fails to open kernel-oplocked files with smb1
Status: RESOLVED FIXED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Anoop C S
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks: 14241
  Show dependency treegraph
 
Reported: 2019-07-30 11:13 UTC by Volker Lendecke
Modified: 2020-11-12 10:00 UTC (History)
5 users (show)

See Also:


Attachments
Patch for 4.11 (15.05 KB, patch)
2019-08-02 08:07 UTC, Volker Lendecke
jra: review+
Details
Patch for 4.10 (14.41 KB, patch)
2019-08-02 08:08 UTC, Volker Lendecke
jra: review+
Details
Patch for 4.9 (14.39 KB, patch)
2019-08-02 08:09 UTC, Volker Lendecke
jra: review+
Details
vfs_glusterfs temp fix for master (2.01 KB, patch)
2019-08-27 10:59 UTC, Anoop C S
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Volker Lendecke 2019-07-30 11:13:03 UTC

    
Comment 1 Volker Lendecke 2019-08-02 08:07:35 UTC
Created attachment 15367 [details]
Patch for 4.11

https://gitlab.com/samba-team/devel/samba/pipelines/74335669
Comment 2 Volker Lendecke 2019-08-02 08:08:21 UTC
Created attachment 15368 [details]
Patch for 4.10

https://gitlab.com/samba-team/devel/samba/pipelines/74335691
Comment 3 Volker Lendecke 2019-08-02 08:09:23 UTC
Created attachment 15369 [details]
Patch for 4.9

https://gitlab.com/samba-team/devel/samba/pipelines/74335906
Comment 4 Jeremy Allison 2019-08-02 21:39:13 UTC
Re-assigning to Karolin for inclusion in 4.11.RCnext, 4.10.next, 4.9.next
Comment 5 Anoop C S 2019-08-05 14:27:41 UTC
(In reply to Jeremy Allison from comment #4)
We have an issue with above patches where file open resulting in "Invalid file handle" error on Windows. See the following thread on samba-technical:

https://lists.samba.org/archive/samba-technical/2019-August/134039.html
Comment 6 Jeremy Allison 2019-08-05 16:41:25 UTC
(In reply to Anoop C S from comment #5)

Hi Anoop, can you fix this by removing the O_NONBLOCK
from the open call inside vfs_glusterfs ?

Adding the O_NONBLOCK is correct for the local filesystem
side of things, so if glusterfs needs something different
it'll need to cope.

Adding a test environment for gluster into Samba would
also be a really good way to ensure we don't cause
problems like this in the future - but currently
our base test environment is the local filesystem,
which I think is the correct decision.
Comment 7 Ralph Böhme 2019-08-05 17:52:18 UTC
(In reply to Jeremy Allison from comment #6)
I don't think that helps with the problem. We're making a call to fcntl() via set_blocking() called from open_file() which of course fails with fake fds returned from vfs_glusterfs open.

Lacking aSMB_VFS_FCNTL() we may want to somehow funnel this through SMB_VFS_FSCTL().
Comment 8 Jeremy Allison 2019-08-05 18:00:07 UTC
Oh, I see the problem now.

We're opening with O_NONBLOCK (which is OK for gluster), then we run into:

                if (local_flags & O_NONBLOCK) {
                        /*
                         * GPFS can return ETIMEDOUT for pread on
                         * nonblocking file descriptors when files
                         * migrated to tape need to be recalled. I
                         * could imagine this happens elsewhere
                         * too. With blocking file descriptors this
                         * does not happen.
                         */
                        ret = set_blocking(fsp->fh->fd, true);
                        if (ret == -1) {
                                status = map_nt_error_from_unix(errno);
                                DBG_WARNING("Could not set fd to blocking: "
                                            "%s\n", strerror(errno));
                                fd_close(fsp);
                                return status;
                        }
                }

where fsp->fh->fd is a fake gluster-created fd.

Yes, ultimately we need to add SMB_VFS_FCNTL() for operation on fd's returned from the VFS.

This isn't really a problem with the original patch, it's a more generic issue with how we're handling fd's returned from the VFS and making sure we only vector through the VFS for this.

A test infrastructure for gluster/ceph that is integrated with Samba would help find these things before they go it.
Comment 9 Jeremy Allison 2019-08-05 18:07:47 UTC
In the meantime, vfs_gluster can fix this by creating a local fd using pipe(), and then returning one end of this from:

vfs_gluster_open(), instead of 13371337.

Does this also cause a problem for ceph ?

We might want to fix this in master by adding SMB_VFS_FCNTL(), and leave the local kernel oplock bug alone in 4.11 and below until we get the fixed VFS in 4.12.
Comment 10 Anoop C S 2019-08-06 06:00:20 UTC
(In reply to Jeremy Allison from comment #9)
I will try suggested changes and confirm whether it can fix the issue for now.
Comment 11 Anoop C S 2019-08-27 10:59:19 UTC
Created attachment 15436 [details]
vfs_glusterfs temp fix for master

Ok. After offline discussions I have come up with the attached patch which fixes the issue. patch is also available at https://lists.samba.org/archive/samba-technical/2019-August/134128.html
Comment 12 Jeremy Allison 2019-08-27 23:22:07 UTC
(In reply to Anoop C S from comment #11)

Hi Anoop - that patch looks good to me (once you remove the //FIXME comment :-).

Can you get Guenther to +1 it and we'll push to master.

Can you log a separate bug for this (and make it depend on this one) so we can
track the backports separately.

Thanks,

Jeremy.
Comment 13 Anoop C S 2019-08-28 05:45:46 UTC
(In reply to Jeremy Allison from comment #12)
Wait.. I am confused on our way forward w.r.t this bug. Based on your comment #9 it was my impression that we implement SMB_VFS_FCNTL in future and fix it with 4.12 leaving current stable branches with original bug. Following that vfs_glusterfs fix was prepared out of your suggestion as a temporary code change in just master.

What did I get wrong here?
Comment 14 Jeremy Allison 2020-01-22 21:25:23 UTC
I think 4.12 was forked from master and contains the SMB_VFS_FCNTL() call, so we don't need it for 4.12.

For 4.11 and below you can add the "dummy-fd" fix. Does that answer the question Anoop ?

Jeremy.
Comment 15 Anoop C S 2020-01-23 09:35:52 UTC
(In reply to Jeremy Allison from comment #14)
We don't need fake-fd fix in v4.11 and below because the original fixes for this bug are only present in v4.12 and master. I could see the backport patches for v4.11, v4.10 and v4.9 as attachments and reviewed but not pulled in yet.

I will attach fake-fd fix backports in bug #14241 as soon as original fixes for this bug gets into v4.11 and v4.10.
Comment 16 Jeremy Allison 2020-01-23 19:26:10 UTC
OK, that works. Thanks for your help !
Comment 17 Anoop C S 2020-10-19 08:01:11 UTC
@Volker,

Given that v4.10 is discontinued and v4.11 is in "security fixes only" mode, shall we close this bug as RESOLVED as actual vfs_glusterfs fix has been merged and present from v4.12.
Comment 18 Anoop C S 2020-11-12 10:00:20 UTC
Closing as we have required patches in current stable release versions(v4.12 and above).