Bug 14010 - Unable to create or rename file/directory inside shares configured with vfs_glusterfs_fuse module
Unable to create or rename file/directory inside shares configured with vfs_g...
Status: RESOLVED FIXED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: VFS Modules
4.10.5
All Linux
: P5 major
: ---
Assigned To: Karolin Seeger
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-25 11:41 UTC by Anoop C S
Modified: 2019-08-09 08:04 UTC (History)
3 users (show)

See Also:


Attachments
Patchset for master to fix the issue. (3.42 KB, patch)
2019-06-25 17:25 UTC, Michael Adam
no flags Details
Updated patchset rebased on current master (4.18 KB, patch)
2019-06-26 10:44 UTC, Michael Adam
gd: review+
Details
patch from master for v4.9 and v4.10 (4.73 KB, patch)
2019-07-02 14:07 UTC, Guenther Deschner
obnox: review+
metze: review-
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Anoop C S 2019-06-25 11:41:15 UTC
Fairly recent VFS module named "vfs_glusterfs_fuse", which operates out of a FUSE mount of GlusterFS volumes, fails to perform creation or rename of file/directory anywhere inside the share with an error code NT_STATUS_ACCESS_DENIED.

Steps to reproduce:
1. Have a basic GlusterFS(v6.0 or above) volume mounted on a system.
2. Configure a share with path pointing to the mount point from step 1.
3. Add glusterfs_fuse module as follows:
   vfs objects = glusterfs_fuse
4. Try to create or rename a file/directory from Windows client or using `smbclient`

The above attempt fails with NT_STATUS_ACCESS_DENIED.

Additional information:
This regression was caused due to an error code conversion change[1] which replaced ENOENT with ESTALE for certain operations to trigger re-tries from kernel. getxattr() was among those operations with which get_real_filename is implemented in GlusterFS. On the other side, Samba on receiving ESTALE presents client with NT_STATUS_ACCESS_DENIED as it does not have a defined mapping for this particular unix error code.

Note(need for Samba fix):
Even with the patch[2] posted to handle this change in GlusterFS we will experience performance degradation for which a corresponding fix should go into both vfs_glusterfs and vfs_glusterfs_fuse modules so that we take advantage of get_real_filename logic in Samba. Refer commit message from [2] for details.

[1] http://git.gluster.org/cgit/glusterfs.git/commit/?id=59629f1da9dca670d5dcc6425f7f89b3e96b46bf 

[2] https://review.gluster.org/c/glusterfs/+/22925
Comment 1 Michael Adam 2019-06-25 17:25:07 UTC
Created attachment 15263 [details]
Patchset for master to fix the issue.

This is the patchset of two 1-line patches against vfs_glusterfs.c and vfs_glusterfs_fues.c, respectively. Kept the patches separate for easier backporting.
Comment 2 Michael Adam 2019-06-26 10:44:37 UTC
Created attachment 15264 [details]
Updated patchset rebased on current master

Updated patch that is rebased on current master.
Comment 3 Guenther Deschner 2019-06-27 16:01:02 UTC
Comment on attachment 15264 [details]
Updated patchset rebased on current master

LGTM
Comment 4 Michael Adam 2019-06-27 22:45:16 UTC
I put the master patches up as a MR on github:

https://gitlab.com/samba-team/samba/merge_requests/584

Slightly modified the commit messages.
Comment 5 Guenther Deschner 2019-07-02 14:07:59 UTC
Created attachment 15279 [details]
patch from master for v4.9 and v4.10
Comment 6 Guenther Deschner 2019-07-03 11:52:40 UTC
Karolin, can you please add the patch to 4.10 and 4.9 ? Thanks!
Comment 7 Karolin Seeger 2019-07-08 11:41:27 UTC
(In reply to Guenther Deschner from comment #6)
Pushed to autobuild-v4-{10,9}-test.
Comment 8 Stefan Metzmacher 2019-07-09 13:04:01 UTC
Comment on attachment 15279 [details]
patch from master for v4.9 and v4.10

> With this patch, Samba will not work correctly any more against
> very old gluster servers

I don't think it's acceptable to break old gluster servers?

Is it possible to find out the gluster version and adapt to it?
Comment 9 Stefan Metzmacher 2019-07-09 15:52:39 UTC
(In reply to Stefan Metzmacher from comment #8)

I also don't understand why old versions are broken with the patch
in use, can you explain that more detailed?
Comment 10 Anoop C S 2019-07-10 06:28:39 UTC
(In reply to Stefan Metzmacher from comment #9)
May be not true. Because xattr name which gluster uses for get_real_filename implementation is prefixed with "glusterfs.get_real_filename" and a query for this xattr on a directory always returned EOPNOTSUPP in its absence. Thus with or without this patch Samba will do a full_directory_scan on receiving EOPNOTSUPP from very old gluster servers.

I think commit message was prepared before realizing the fact that there is a difference in errno returned based on xattr namespaces as explained in man xattr(7). Since "glusterfs.get_real_filename" lies outside standard/defined namespaces, getxattr() will return EOPNOTSUPP(and not ENOATTR) if it is not set.
Comment 11 Karolin Seeger 2019-08-06 07:52:45 UTC
Currently, the patch is included in the release branches.
Should we revert it for now?
Comment 12 Anoop C S 2019-08-06 16:07:32 UTC
(In reply to Karolin Seeger from comment #11)
I don't think so.

I would say the concern from comment #8(and comment #9) was a confusion based on the last part of commit message, specifically the 2nd bullet point. I think my explanation via comment #10 is enough to clear things out.

@Michael,
Can you confirm?

@metze,
Can you please consider re-visiting your review?
Comment 13 Stefan Metzmacher 2019-08-06 17:16:31 UTC
(In reply to Anoop C S from comment #12)

If it doesn't break existing setups I'm fine.
Comment 14 Michael Adam 2019-08-08 09:56:29 UTC
(In reply to Stefan Metzmacher from comment #13)
> (In reply to Anoop C S from comment #12)
> 
> If it doesn't break existing setups I'm fine.

True, I initially thought that it would break very very old (long unsopported) versions of gluster that hadn't any implementation of get-real-filename yet. But at closer inspection of th gluster code, I found out that this is not true. It does not break any older setup. And I updated the commit messages accordingly. So this statement is not present in the merged patches  any more.

Hope this makes it clear.

Thanks - Michael
Comment 15 Karolin Seeger 2019-08-09 08:04:41 UTC
Ok, thanks for clarifying! :-)

Pushed to both branches (with old commit messages, because the concern was raised after pushing).
Closing out bug report.

Thanks!