Bug 14241 - "Invalid file handle" error on opening files using vfs_glusterfs
Summary: "Invalid file handle" error on opening files using vfs_glusterfs
Status: NEW
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules (show other bugs)
Version: unspecified
Hardware: x64 Linux
: P5 major (vote)
Target Milestone: ---
Assignee: Samba QA Contact
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on: 14060
Blocks:
  Show dependency treegraph
 
Reported: 2020-01-17 11:19 UTC by Anoop C S
Modified: 2020-01-23 09:40 UTC (History)
2 users (show)

See Also:


Attachments
patch-for-v4-12 (1.56 KB, patch)
2020-01-17 11:24 UTC, Anoop C S
gd: review+
slow: review+
anoopcs: ci-passed+
Details
SMB_VFS_FCNTL in vfs_glusterfs (1.55 KB, patch)
2020-01-20 11:28 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 Anoop C S 2020-01-17 11:19:42 UTC
Windows explorer displays error dialogue box(Invalid file handle) while trying to open file/s from a GlusterFS volume shared using vfs_glusterfs module within Samba.

Additional info:
The following commit changed the way in which O_NONBLOCK is added/removed from file status flags during open.

https://git.samba.org/?p=samba.git;a=commit;h=ef582ffcf3a220b73f678d9bce0fd37800f76c54
smbd: Always open files with O_NONBLOCK

Since O_NONBLOCK is now internally getting added unconditionally, storage backed by GlusterFS volumes using vfs_glusterfs produces *Invalid file handle* error while opening files(I tested from Windows). set_blocking() from open_file() is now being invoked due to the presence of O_NONBLOCK flag and fails at fcntl() call operating on fsp->fh->fd which is obviously bad and returns EBADF.
Comment 1 Anoop C S 2020-01-17 11:24:20 UTC
Created attachment 15742 [details]
patch-for-v4-12

Adding the patch as discussed offline.
Comment 2 Anoop C S 2020-01-17 14:35:51 UTC
Comment on attachment 15742 [details]
patch-for-v4-12

Pipeline: https://gitlab.com/samba-team/devel/samba/pipelines/110183222
Comment 3 Ralph Böhme 2020-01-17 14:51:36 UTC
Comment on attachment 15742 [details]
patch-for-v4-12

Lgtm.
Comment 4 Guenther Deschner 2020-01-17 15:41:47 UTC
Comment on attachment 15742 [details]
patch-for-v4-12

LGTM
Comment 5 David Disseldorp 2020-01-17 16:43:21 UTC
IIUC, this is also going to affect other VFS modules which provide a dummy fd, e.g. vfs_ceph. I think a proper fix here would be to move the fcntl call into the VFS. Or am I missing something?
Comment 6 Jeremy Allison 2020-01-17 18:00:39 UTC
(In reply to David Disseldorp from comment #5)

The fcntl call did get moved into the VFS layer in master as commit 
0abd1189a60eea4501b5279ebc4bff2b5689f888.

I think this is a back-port for 4.12 which can't change the VFS interface (correct me if I'm wrong please Anoop).
Comment 7 Jeremy Allison 2020-01-17 18:07:37 UTC
Oh, this went into master too. Anoop, why can't you do this via adding a gluster-specific SMB_VFS_FCNTL() call that got added in master ? I can see why this is done for the back-port but in master we should have a method of doing it the clean way David mentioned.
Comment 8 Anoop C S 2020-01-19 03:40:37 UTC
(In reply to Jeremy Allison from comment #7)
Yes, that is the plan. But as of now GlusterFS doesn't have an API implementation to support SMB_VFS_FCNTL() interface in Samba. I made the proposal(https://lists.gluster.org/pipermail/gluster-devel/2019-October/056629.html) sometime back but didn't get much interest at first pass.

Due to time constraints this bug is being raised to get the issue fixed with upcoming(soon approaching) v4.12.
Comment 9 Jeremy Allison 2020-01-19 05:22:47 UTC
You don't need to have an interface for this inside glusterfs, just filter out the O_NONBLOCK setting on a gluster fd handle.

It's only being done for Linux-specific-fd lease code calls anyway, so just add an implementation in glusterfs and ignore the call if it's setting or removing O_NONBLOCK.

I understand you might need this for a released version, but not in master. In master you should be doing it right - that's why you added the SMB_VFS_FCNTL call in the first place.
Comment 10 Anoop C S 2020-01-20 11:28:18 UTC
Created attachment 15745 [details]
SMB_VFS_FCNTL in vfs_glusterfs

Hm..patch itself is already in master. I never noticed it being merged.

Shall we leave as it is for v4.12 to get branched from master(till tomorrow)? I am attaching a new patch for SMB_VFS_FCNTL implementation in vfs_glusterfs. Please have a look at it.

Let me know your thoughts..
Comment 11 David Disseldorp 2020-01-21 00:49:19 UTC
(In reply to Anoop C S from comment #10)
> Created attachment 15745 [details]
> SMB_VFS_FCNTL in vfs_glusterfs
> 
> Hm..patch itself is already in master. I never noticed it being merged.
> 
> Shall we leave as it is for v4.12 to get branched from master(till
> tomorrow)? I am attaching a new patch for SMB_VFS_FCNTL implementation in
> vfs_glusterfs. Please have a look at it.
> 
> Let me know your thoughts..

Oops, only just saw this, sorry. I've submitted vfs_ceph and vfs_glusterfs fcntl handlers via https://gitlab.com/samba-team/samba/merge_requests/1076 .
I think it's probably worth reverting the fake fd change c9adf47a.
Comment 12 Jeremy Allison 2020-01-21 01:36:22 UTC
Yep. Can you add the revert into the gitlab merge request please ?
Comment 13 Jeremy Allison 2020-01-21 01:37:42 UTC
Anoop, can you confirm David's patch fixes this bug for you ?

Thanks,

Jeremy.
Comment 14 Anoop C S 2020-01-21 02:43:11 UTC
(In reply to Jeremy Allison from comment #13)
Patch from https://gitlab.com/samba-team/samba/merge_requests/1076 also works fine.
Comment 15 Jeremy Allison 2020-01-22 21:32:27 UTC
Comment on attachment 15745 [details]
SMB_VFS_FCNTL in vfs_glusterfs

Isn't this one already in 4.12rc1 ? I think you only need the fd-hack for 4.11 and prior releases.
Comment 16 Anoop C S 2020-01-23 09:40:04 UTC
(In reply to Jeremy Allison from comment #15)
Yes. David added SMB_VFS_FCNTL() implementation in vfs_glusterfs and is part of v4.12rc1.

I will attach fake-fd fix backport for v4.11 and v4.10 after fixes for bug #14060 lands in those branches.