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
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.
Created attachment 15264 [details] Updated patchset rebased on current master Updated patch that is rebased on current master.
Comment on attachment 15264 [details] Updated patchset rebased on current master LGTM
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.
Created attachment 15279 [details] patch from master for v4.9 and v4.10
Karolin, can you please add the patch to 4.10 and 4.9 ? Thanks!
(In reply to Guenther Deschner from comment #6) Pushed to autobuild-v4-{10,9}-test.
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?
(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?
(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.
Currently, the patch is included in the release branches. Should we revert it for now?
(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?
(In reply to Anoop C S from comment #12) If it doesn't break existing setups I'm fine.
(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
Ok, thanks for clarifying! :-) Pushed to both branches (with old commit messages, because the concern was raised after pushing). Closing out bug report. Thanks!