Re: [PATCH 3/3 v4] vfs_glusterfs: Samba VFS module for glusterfs

[Jeremy Allison]

On Fri, May 24, 2013 at 04:17:48PM -0700, Anand Avati wrote:
> On 5/24/13 5:18 AM, Simo wrote:
> >
> >By using an array you are forced to create this cleanup function,
> >however if you use a doubly linked list you could simply free the fs and
> >use a talloc destructor to perform the clean up. Have you though about
> >doing that ?
>
> I did not look around enough in the samba code to find a list
> template and did not want to write list primitives from scratch. Is
> there a specific list template in samba which you recommend?

Check out all uses of DLIST_XXX() functions (found in
lib/util/dlinklist.h.

> fallocate() support is currently under review in glusterfs (likely
> to be merged soon). This vfs_slow_fallocate() behavior is not a new
> issue as this has been how it worked with the FUSE mount as well.
> Once fallocate() support is merged, I will send a new patch to use
> the API call.

Having a native gluster fallocate will help a *LOT*.

> Does this flock() strictly have to be in a separate lock space from
> fcntl()? In Linux they are separete, but how does samba deal with
> other OSes where they compete in the same lockspace? In gluster we
> have a single lockspace. Will errno = ENOSYS make samba autodetect
> the lack of 'kernel share modes' support?

Yes.

> Gluster's ACL is byte-compatible with Linux's POSIX ACL format. The
> use of acl_copy_int() here is possibly dangerous on non-Linux
> servers (though in reality most of the OS's POSIX ACL format is byte
> compatible?). In that sense, maybe it makes sense to have a Gluster
> format to smb_acl_t converter implemented natively in this module
> (though it might look similar to smb_acl_to_internal).

Yes.

Jeremy.

[Simo]

On 05/24/2013 07:17 PM, Anand Avati wrote:
> On 5/24/13 5:18 AM, Simo wrote:
>> By using an array you are forced to create this cleanup function,
>> however if you use a doubly linked list you could simply free the fs and
>> use a talloc destructor to perform the clean up. Have you though about
>> doing that ?
>
> I did not look around enough in the samba code to find a list template
> and did not want to write list primitives from scratch. Is there a
> specific list template in samba which you recommend?

look for dlinklist.h

> I'm not sure I completely understand what a sub request is (or what an
> appropriate tevent function call is). I believe this call is required
> to implement AIO, which we plan to next (as mentioned in the TODO
> above). Can we leave this as-is for now as the code path is not
> executed? It will anyways get addressed during the AIO support.

Ok, maybe just add a comment then that this will be supported in the
near future.

>> Are you sure you do not want to implement this one ?
>> If you return ENOTSUP samba will use vfs_slow_fallocate() which will use
>> pwrite to send a ton of zeros to your filesystem. Is this the most
>> efficient way on gluster ?

>
> fallocate() support is currently under review in glusterfs (likely to
> be merged soon). This vfs_slow_fallocate() behavior is not a new issue
> as this has been how it worked with the FUSE mount as well. Once
> fallocate() support is merged, I will send a new patch to use the API
> call.
>

Ok, it will definitely make a big difference.

>>> +static int vfs_gluster_kernel_flock(struct vfs_handle_struct *handle,
>>> + files_struct *fsp, uint32 share_mode,
>>> + uint32_t access_mask)
>>> +{
>>> + return 0;
>>> +}
>>
>> Lieing about the success of locking is not really a good idea.
>> I think you should return -1 here and advice users of gluster vfs to set
>> "kernel share modes" to false instead.

>
> Does this flock() strictly have to be in a separate lock space from
> fcntl()? In Linux they are separete, but how does samba deal with
> other OSes where they compete in the same lockspace? In gluster we
> have a single lockspace. Will errno = ENOSYS make samba autodetect the
> lack of 'kernel share modes' support?

As far as I can see in the code the errno is ignored.
A -1 will cause a NT_STATUS_SHARING_VIOLATION

Maybe we should patch the vfs to check the errno value and aout disable
kernel share modes if ENOTSUP is returned ...

>
>> Are you sure you can always access 'trusted' attributes ?
>> IIRC trusted can only be accessed by root or a process with equivalent
>> capabilities, while samba performs file operations as the user, does
>> gluster do no checking on who is accessing the attributes ?
>> If gluster doesn't then a lot of the code in this module neeeds a ton of
>> access control applied on top.
>
> Ah, changed to "user." namespace. I have been testing as root and the
> issue hadn't shown up (yet).

Gluster makes this available on both trusted. and user. namespace ? I am
confused.

> Gluster's ACL is byte-compatible with Linux's POSIX ACL format. The
> use of acl_copy_int() here is possibly dangerous on non-Linux servers
> (though in reality most of the OS's POSIX ACL format is byte
> compatible?). In that sense, maybe it makes sense to have a Gluster
> format to smb_acl_t converter implemented natively in this module
> (though it might look similar to smb_acl_to_internal).
>

> This would settle the debate of exposing other module internals too.
> Thoughts?

Yes, this is probably best.

>> Also I am a bit concerned about how access control is performed when
>> using the glusterfs library directly.
>> Can you shed some light about that please ?
>>
>
> The Samba connects to gluster bricks as a trusted client (running in
> the same trusted hosts as the bricks, verified by host authentication
> by IP). This means the library performs getpid() etc. and presents
> those values as the identity in the RPC request. The gluster bricks
> perform access control by trusting the presented identity.
>
> Running Samba outside the gluster trusted storage pool requires some
> more auth work, and we do not plan to support that yet.

I am not sure I understand how this works, if samba is 'trusted' does it
mean samba is responsible to perform access control ? Because samba
doesn't, it defers access control to the kernel.

Does gluster use geteuid()/getegid()/getgroups() to find out what are
the current ids ?

Simo.