[PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

classic Classic list List threaded Threaded
16 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Ralph Böhme-2
Hi!

Attached is a patchset for vfs_fruit that fixes bug #12427.

This addresses that essentially any internal metadata backend other then the
default was broken at one place or another.

Most part of the patchset is a proper refactoring of all VFS functions
implemented in vfs_fruit to switch between the configured backend. I haven't yet
taken the full step to fully modularize the backends, but this could be done
later.

At then end, we get to fully test all possible backend configs in autobuild as
well as a few additional tests for issues I found.

I had to disable and later reenabled vfs.fruit tests in the middle of the
patchset in order to stay git-bisect clean. It wasn't possible to refactor the
module without breaking something at some point, so I chose this approach.

I dare not ask, but can anyone please review this one and push if ok! I owe you
a beverage of your choice at SambaXP! Thanks!

Cheerio!
-slow

master-bug12427-fruit.patch (232K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Ralph Böhme-2
On Thu, Jan 12, 2017 at 04:08:16PM +0100, Ralph Böhme wrote:
> I dare not ask, but can anyone please review this one and push if ok! I owe you
> a beverage of your choice at SambaXP! Thanks!

is this one on anyones list? I'd like to make backports for 4.5 and 4.6 once
this is in master, it would be great to get this into in time for 4.6.0.

Thanks!

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Uri Simchoni-4
On 01/19/2017 10:11 AM, Ralph Böhme wrote:

> On Thu, Jan 12, 2017 at 04:08:16PM +0100, Ralph Böhme wrote:
>> I dare not ask, but can anyone please review this one and push if ok! I owe you
>> a beverage of your choice at SambaXP! Thanks!
>
> is this one on anyones list? I'd like to make backports for 4.5 and 4.6 once
> this is in master, it would be great to get this into in time for 4.6.0.
>
> Thanks!
>
> Cheerio!
> -slow
>
I'll do it.
Thanks,
Uri.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Ralph Böhme-2
On Thu, Jan 19, 2017 at 10:47:50AM +0200, Uri Simchoni wrote:

> On 01/19/2017 10:11 AM, Ralph Böhme wrote:
> > On Thu, Jan 12, 2017 at 04:08:16PM +0100, Ralph Böhme wrote:
> >> I dare not ask, but can anyone please review this one and push if ok! I owe you
> >> a beverage of your choice at SambaXP! Thanks!
> >
> > is this one on anyones list? I'd like to make backports for 4.5 and 4.6 once
> > this is in master, it would be great to get this into in time for 4.6.0.
> >
> > Thanks!
> >
> > Cheerio!
> > -slow
> >
> I'll do it.

awesome, thanks!

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Uri Simchoni-4
In reply to this post by Ralph Böhme-2
So far I got up to 35/56, although the catia patches deserve another
look. I have some minor comments, see below.
Will be looking at the rest, but it would take a few more days to get
back to - hopefully next weekend.

Thanks,
Uri.
On 01/12/2017 05:08 PM, Ralph Böhme wrote:

> Hi!
>
> Attached is a patchset for vfs_fruit that fixes bug #12427.
>
> This addresses that essentially any internal metadata backend other then the
> default was broken at one place or another.
>
> Most part of the patchset is a proper refactoring of all VFS functions
> implemented in vfs_fruit to switch between the configured backend. I haven't yet
> taken the full step to fully modularize the backends, but this could be done
> later.
>
> At then end, we get to fully test all possible backend configs in autobuild as
> well as a few additional tests for issues I found.
>
> I had to disable and later reenabled vfs.fruit tests in the middle of the
> patchset in order to stay git-bisect clean. It wasn't possible to refactor the
> module without breaking something at some point, so I chose this approach.
>
> I dare not ask, but can anyone please review this one and push if ok! I owe you
> a beverage of your choice at SambaXP! Thanks!
>
> Cheerio!
> -slow
>

[10/56] -
On error, shouldn't we be calling SMB_VFS_NEXT_CLOSE instead of
SMB_VFS_CLOSE? The choice between SMB_VFS_XXX and SMB_VFS_NEXT_XXX is
never an easy one. However, in the case of
close-from-within-failed-open, it's the file we've just opened, and
higher layers may have not seen the open return.

I also don't understand the BUGBUGBUG comment - fd_close_posix() *is*
called if it's a regular file and we get to the bottom of the vfs stack.

[13/56] -
That doesn't seem right... if we update btime out of the Mac metadata,
we do it no matter where the metadata is stored. One way is to call
fruit_open_meta(), read the metadata via fruit_pread_meta(), and parse
the metadata.

BTW, I was a bit puzzled by the use of update_btime() while stat'ing a
stream. What is the meaning of stat'ing a stream? stat is a UNIX concept
and a stream is a Windows (Mac too...) concept. Are there any "rules" of
what should a stat of a stream return?

[15/56] -
Seems like readdir_attr_meta_finderi_stream() would work for netatalk
too, since we know how to open and read the meta stream no matter where
it's being stored. Is the separate netatalk implementation a performance
optimization?

[17/56]

> +static int fruit_unlink_rsrc_xattr(vfs_handle_struct *handle,
> +                                  const struct smb_filename *smb_fname,
> +                                  bool force_unlink)
> +{
> +       /* Nothing to do here, removing the file will remove the xattr */
About this comment - If I understand correctly, we do nothing because
OS-X don't delete the resource.

[26/56]
in fruit_streaminfo_meta_netatalk():
> +       if (is_fi_empty) {
> +               return NT_STATUS_OK;
> +       }
> +
I think this early return doesn't let us remove the :$DATA "stream".

in fruit_streaminfo_rsrc_xattr():
Seems like the code assumes that someone already added the resource
stream, and we just have to filter it if it's empty. This might be the
case if we have streams_xattr below, but:
1. Streams_xattr could prvide us with a wrongly-named stream, especially
if streams_xattr:prefix is set
2. We may have another stream engine which doesn't tough extended
attributes.

[28/56]

> +static int fruit_ftruncate_rsrc_xattr(struct vfs_handle_struct *handle,
> +                                     struct files_struct *fsp,
> +                                     off_t offset)
> +{
> +       if (offset == 0) {
> +               return SMB_VFS_FREMOVEXATTR(fsp, AFPRESOURCE_EA_NETATALK);
> +       }
> +
> +       return SMB_VFS_NEXT_FTRUNCATE(handle, fsp, offset);
> +}
> +
For safety against future mistakes, and also for readability, I would
enclose the _NEXT call with #ifdef HAVE_ATTROPEN.


[30/56] and [31/56]:
Doing path translation inside a handle-based function raises some
warning flags inside my head. True, it's been that way before. But now,
other users of vfs_catia (assuming they exist :)) pay the toll as well.
The file name gets analyze on every pread().

Maybe you can cache the path during open to avoid this? Even as a
follow-up patch set...

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Ralph Böhme-2
Hi Uri,

thanks a lot for looking into this! Comments below:

On Sun, Jan 29, 2017 at 09:13:41PM +0200, Uri Simchoni wrote:

> So far I got up to 35/56, although the catia patches deserve another
> look. I have some minor comments, see below.
> Will be looking at the rest, but it would take a few more days to get
> back to - hopefully next weekend.
>
> [10/56] -
> On error, shouldn't we be calling SMB_VFS_NEXT_CLOSE instead of
> SMB_VFS_CLOSE? The choice between SMB_VFS_XXX and SMB_VFS_NEXT_XXX is
> never an easy one. However, in the case of
> close-from-within-failed-open, it's the file we've just opened, and
> higher layers may have not seen the open return.
agreed, fixed.

> I also don't understand the BUGBUGBUG comment - fd_close_posix() *is*
> called if it's a regular file and we get to the bottom of the vfs stack.

Well, that's essentially c/p from vfs_streams_xattr. :/ That's probably a
historic artefact, removing it...

> [13/56] -
> That doesn't seem right... if we update btime out of the Mac metadata,
> we do it no matter where the metadata is stored.

Not quite. :) In all cases execept the Netatalk case the btime will also be set
and fetched by the client (!) via the usual SMB protocol semantics and Samba FSA
processing. But that's not the case with Netatalk. There the primary source for
bime is the Netatalk metadata xattr, updated in fruit_ntimes() and queried here
in update_btime().

> One way is to call
> fruit_open_meta(), read the metadata via fruit_pread_meta(), and parse
> the metadata.
>
> BTW, I was a bit puzzled by the use of update_btime() while stat'ing a
> stream. What is the meaning of stat'ing a stream?

just like a stat on the file: get the file metadata. The VFS backends then
implement this somehow, iirc streas_xattr stats the basefile while streams_depot
stats the actual stream file in the "depot".

> stat is a UNIX concept and a stream is a Windows (Mac too...) concept. Are
> there any "rules" of what should a stat of a stream return?

Afacit this stuff is somewhat underspecified is the MS protocol specs. There are
also no tests for this in the MS protocol test suite. I asked them for a pointer
to stream related tests while attending MS-IOLab and it turned out there aren't
any.

> [15/56] -
> Seems like readdir_attr_meta_finderi_stream() would work for netatalk
> too, since we know how to open and read the meta stream no matter where
> it's being stored. Is the separate netatalk implementation a performance
> optimization?

yes. Going through create_file sucks, but there's no other way from
readdir_attr_meta_finderi_stream().

> [17/56]
>
> > +static int fruit_unlink_rsrc_xattr(vfs_handle_struct *handle,
> > +                                  const struct smb_filename *smb_fname,
> > +                                  bool force_unlink)
> > +{
> > +       /* Nothing to do here, removing the file will remove the xattr */
>
> About this comment - If I understand correctly, we do nothing because
> OS-X don't delete the resource.
This comments describes OS X SMB *server* behaviour. That one ignores unlink
requests on the resource fork (no idea at which layer, may just be the OS X SMB
server FSA-layer that exposes this behaviour).

> [26/56]
> in fruit_streaminfo_meta_netatalk():
> > +       if (is_fi_empty) {
> > +               return NT_STATUS_OK;
> > +       }
> > +
>
> I think this early return doesn't let us remove the :$DATA "stream".

good catch! Fixed in the updated patchset.

> in fruit_streaminfo_rsrc_xattr():
> Seems like the code assumes that someone already added the resource
> stream, and we just have to filter it if it's empty.

This function is only called for the case when fruit:resource=xattr which
only works on Solaris, as it uses the Solaris xattr API (which supports xattrs
of arbitrary size and the POSIX file handle base API) and it is indeed
incomplete. Thanks for spotting this.

I added the code to handle fruit:resource=xattr on Solaris to the initial
vfs_fruit version, but it is long a time ago since I last tested it.

It was broken in several places before this patchset and the patchset improves
on this in some places but not all.

To be honest, I'm not aware of anyone using fruit:resource=xattr (on a Solaris
(or derived) system), so lacking time to properly implement and test it I see
two options:

- mark it as incomplete and broken (via a patch for the manpage on top)

- remove it

What do you think?

> This might be the case if we have streams_xattr below, but:
>
> 1. Streams_xattr could prvide us with a wrongly-named stream, especially
> if streams_xattr:prefix is set
> 2. We may have another stream engine which doesn't tough extended
> attributes.
>
> [28/56]
> > +static int fruit_ftruncate_rsrc_xattr(struct vfs_handle_struct *handle,
> > +                                     struct files_struct *fsp,
> > +                                     off_t offset)
> > +{
> > +       if (offset == 0) {
> > +               return SMB_VFS_FREMOVEXATTR(fsp, AFPRESOURCE_EA_NETATALK);
> > +       }
> > +
> > +       return SMB_VFS_NEXT_FTRUNCATE(handle, fsp, offset);
> > +}
> > +
> For safety against future mistakes, and also for readability, I would
> enclose the _NEXT call with #ifdef HAVE_ATTROPEN.
How did you spot that one?! :) Fixed in updated patchset.

> [30/56] and [31/56]:
> Doing path translation inside a handle-based function raises some
> warning flags inside my head. True, it's been that way before. But now,
> other users of vfs_catia (assuming they exist :)) pay the toll as well.

And they get the fix is well -- which this is all about. Not doing the
translation on the names attached to handles is just a bug.

> The file name gets analyze on every pread().
>
> Maybe you can cache the path during open to avoid this? Even as a
> follow-up patch set...

Yup. It's probably simple, just attach the path as an fsp extension, but I don't
have the time atm, so I'd prefer postponing this.

Updated patchset, also rebased on current master, attached.

Cheerio!
-slow

fruit-2017-01-31.patch (233K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Uri Simchoni-4
Continuing from 36/56 with some minor comments, then responding to last
email. Everything else LGTM.

Thanks for bearing with me,
Uri.

[39/56]

> -static int fruit_fstat_rsrc(vfs_handle_struct *handle, files_struct *fsp,
> +static int fruit_fstat_meta(vfs_handle_struct *handle,
> +                           files_struct *fsp,
>                             SMB_STRUCT_STAT *sbuf)
>  {
> -       struct fruit_config_data *config;
> -       struct adouble *ad = (struct adouble *)VFS_FETCH_FSP_EXTENSION(
> -               handle, fsp);
> +       struct fio *fio = (struct fio *)VFS_FETCH_FSP_EXTENSION(handle, fsp);
> +       int ret;
>  
> -       DEBUG(10, ("fruit_fstat_rsrc called for %s\n",
> -                  smb_fname_str_dbg(fsp->base_fsp->fsp_name)));
> +       if (fio == NULL) {
> +               return SMB_VFS_NEXT_FSTAT(handle, fsp, sbuf);
> +       }

That (fio not being NULL) is already checked in fruit_fstat(). Maybe for
clarity and efficiency it would be better to pass fio to
fruit_fstat_{rsrc,meta}()

[44/56], [45/56]
> +[vfs_fruit_metadata_stream]
> +       path = $shrdir
> +       vfs objects = fruit streams_xattr acl_xattr

Shouldn't we have xattr_tdb below acl_xattr, to be env-agnostic?

>          plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/vfs_fruit -U$USERNAME%$PASSWORD --option=torture:localdir=$SELFTEST_PREFIX/nt4_dc/share')
> +        plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/vfs_fruit_metadata_stream -U$USERNAME%$PASSWORD')

I didn't understand why localdir is not set - that skips tests, but I
couldn't figure out why they should be skipped.

[50/56]
> +               return false; \

Should be "ret = false" ?


... And responses to previous patches:

On 01/31/2017 01:56 PM, Ralph Böhme wrote:

> Hi Uri,
>
> thanks a lot for looking into this! Comments below:
>
> On Sun, Jan 29, 2017 at 09:13:41PM +0200, Uri Simchoni wrote:
>> So far I got up to 35/56, although the catia patches deserve another
>> look. I have some minor comments, see below.
>> Will be looking at the rest, but it would take a few more days to get
>> back to - hopefully next weekend.
>>
>> [10/56] -
>> On error, shouldn't we be calling SMB_VFS_NEXT_CLOSE instead of
>> SMB_VFS_CLOSE? The choice between SMB_VFS_XXX and SMB_VFS_NEXT_XXX is
>> never an easy one. However, in the case of
>> close-from-within-failed-open, it's the file we've just opened, and
>> higher layers may have not seen the open return.
>
> agreed, fixed.
>
I still don't see the fix in last patch set

>> [13/56] -
>> That doesn't seem right... if we update btime out of the Mac metadata,
>> we do it no matter where the metadata is stored.
>
> Not quite. :) In all cases execept the Netatalk case the btime will also be set
> and fetched by the client (!) via the usual SMB protocol semantics and Samba FSA
> processing. But that's not the case with Netatalk. There the primary source for
> bime is the Netatalk metadata xattr, updated in fruit_ntimes() and queried here
> in update_btime().
>
I still don't understand :)

- How does the client (mac or otherwise) know if the backend is netatalk
or something different?
- I understand that for Mac clients, it's not necessary to
update_btime() at all, since the client would read the metadata stream
and figure this by itself. So, if I understand correctly, what
update_btime() does is take care of things for non-Mac clients.

>
>> [17/56]
>>
>>> +static int fruit_unlink_rsrc_xattr(vfs_handle_struct *handle,
>>> +                                  const struct smb_filename *smb_fname,
>>> +                                  bool force_unlink)
>>> +{
>>> +       /* Nothing to do here, removing the file will remove the xattr */
>>
>> About this comment - If I understand correctly, we do nothing because
>> OS-X don't delete the resource.
>
> This comments describes OS X SMB *server* behaviour. That one ignores unlink
> requests on the resource fork (no idea at which layer, may just be the OS X SMB
> server FSA-layer that exposes this behaviour).
>
What I understood from that comment was that we do nothing *because* the
"stream" (implemented by xattr) is removed with the file, whereas the
real reason is that we act like an OS-X server.

>> [26/56]
>> in fruit_streaminfo_meta_netatalk():
>>> +       if (is_fi_empty) {
>>> +               return NT_STATUS_OK;
>>> +       }
>>> +
>>
>> I think this early return doesn't let us remove the :$DATA "stream".
>
> good catch! Fixed in the updated patchset.
>

OK

>> in fruit_streaminfo_rsrc_xattr():
>> Seems like the code assumes that someone already added the resource
>> stream, and we just have to filter it if it's empty.
>
> This function is only called for the case when fruit:resource=xattr which
> only works on Solaris, as it uses the Solaris xattr API (which supports xattrs
> of arbitrary size and the POSIX file handle base API) and it is indeed
> incomplete. Thanks for spotting this.
>
> I added the code to handle fruit:resource=xattr on Solaris to the initial
> vfs_fruit version, but it is long a time ago since I last tested it.
>
> It was broken in several places before this patchset and the patchset improves
> on this in some places but not all.
>
> To be honest, I'm not aware of anyone using fruit:resource=xattr (on a Solaris
> (or derived) system), so lacking time to properly implement and test it I see
> two options:
>
> - mark it as incomplete and broken (via a patch for the manpage on top)
>
> - remove it
>
> What do you think?
>
Come the use case, and the resources to test it, this can be easily
fixed if necessary. The benefits of resource:xattr are obvious.

>> This might be the case if we have streams_xattr below, but:
>>
>> 1. Streams_xattr could prvide us with a wrongly-named stream, especially
>> if streams_xattr:prefix is set
>> 2. We may have another stream engine which doesn't tough extended
>> attributes.
>>
>> [28/56]
>>> +static int fruit_ftruncate_rsrc_xattr(struct vfs_handle_struct *handle,
>>> +                                     struct files_struct *fsp,
>>> +                                     off_t offset)
>>> +{
>>> +       if (offset == 0) {
>>> +               return SMB_VFS_FREMOVEXATTR(fsp, AFPRESOURCE_EA_NETATALK);
>>> +       }
>>> +
>>> +       return SMB_VFS_NEXT_FTRUNCATE(handle, fsp, offset);
>>> +}
>>> +
>> For safety against future mistakes, and also for readability, I would
>> enclose the _NEXT call with #ifdef HAVE_ATTROPEN.
>
> How did you spot that one?! :) Fixed in updated patchset.
>
OK

>> [30/56] and [31/56]:
>> Doing path translation inside a handle-based function raises some
>> warning flags inside my head. True, it's been that way before. But now,
>> other users of vfs_catia (assuming they exist :)) pay the toll as well.
>
> And they get the fix is well -- which this is all about. Not doing the
> translation on the names attached to handles is just a bug.
>
...not necessarily - handle-based APIs usually don't need paths - the
handle embodies the object. The cost of translating a path for each read
block might not be negligible. So this change could affect performance
for current catia users which don't use fruit.

An easy alternative is to have those added functions depend on a
configuration var.

Do we have vfs_catia stakeholders that can comment on this?

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Ralph Böhme-2
On Mon, Feb 06, 2017 at 07:35:27AM +0200, Uri Simchoni wrote:
> Continuing from 36/56 with some minor comments, then responding to last
> email. Everything else LGTM.
>
> Thanks for bearing with me,
> Uri.

thanks for taking the stab! :)

Just a quick update, still working through your review feedback, also adding
caching to catia handle based ops.

I also have two fixups that I will squash into the apporpiate patch, attaching
them here so you can take a look before I squash them.

Cheerio!
-slow

fruit-fixup2.patch (2K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Uri Simchoni-4
On 02/08/2017 10:41 AM, Ralph Böhme wrote:

> On Mon, Feb 06, 2017 at 07:35:27AM +0200, Uri Simchoni wrote:
>> Continuing from 36/56 with some minor comments, then responding to last
>> email. Everything else LGTM.
>>
>> Thanks for bearing with me,
>> Uri.
>
> thanks for taking the stab! :)
>
> Just a quick update, still working through your review feedback, also adding
> caching to catia handle based ops.
>
> I also have two fixups that I will squash into the apporpiate patch, attaching
> them here so you can take a look before I squash them.
>
> Cheerio!
> -slow
>
Those look reasonable... Sorry to have missed the second one.

Thanks,
Uri.

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Ralph Böhme-2
In reply to this post by Ralph Böhme-2
On Wed, Feb 08, 2017 at 09:41:41AM +0100, Ralph Böhme wrote:

> On Mon, Feb 06, 2017 at 07:35:27AM +0200, Uri Simchoni wrote:
> > Continuing from 36/56 with some minor comments, then responding to last
> > email. Everything else LGTM.
> >
> > Thanks for bearing with me,
> > Uri.
>
> thanks for taking the stab! :)
>
> Just a quick update, still working through your review feedback, also adding
> caching to catia handle based ops.

ok, so after struggling with getting the recursive VFS calls and fsp recursion
right, here's a catia name translation caching for handle based ops that finally
passes make test, that I understand and can explain. :)

I've been banging my head against this on Wednesday together with metze and we
couldn't figure it out. Finally came up with something under the shower this
morning (no, really), so metze if you have the time, please take a look at the
catia caching patch.

Full branch:
<https://git.samba.org/?p=slow/samba.git;a=shortlog;h=refs/heads/fruit-metadata>

This is the catia patch:
<https://git.samba.org/?p=slow/samba.git;a=commitdiff;h=0e36321c5b2d9d8f7582941b557217d528390894>

Once I have metze's blessing on this one, I'll send out a fresh patchset to Uri
for review, alongside a diff showing what changed since the previous iteration.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Ralph Böhme-2

On Sat, Feb 11, 2017 at 09:56:39PM +0100, Ralph Böhme wrote:

> On Wed, Feb 08, 2017 at 09:41:41AM +0100, Ralph Böhme wrote:
> > On Mon, Feb 06, 2017 at 07:35:27AM +0200, Uri Simchoni wrote:
> > > Continuing from 36/56 with some minor comments, then responding to last
> > > email. Everything else LGTM.
> > >
> > > Thanks for bearing with me,
> > > Uri.
> >
> > thanks for taking the stab! :)
> >
> > Just a quick update, still working through your review feedback, also adding
> > caching to catia handle based ops.
>
> ok, so after struggling with getting the recursive VFS calls and fsp recursion
> right, here's a catia name translation caching for handle based ops that finally
> passes make test, that I understand and can explain. :)
>
> I've been banging my head against this on Wednesday together with metze and we
> couldn't figure it out. Finally came up with something under the shower this
> morning (no, really), so metze if you have the time, please take a look at the
> catia caching patch.
>
> Full branch:
> <https://git.samba.org/?p=slow/samba.git;a=shortlog;h=refs/heads/fruit-metadata>
>
> This is the catia patch:
> <https://git.samba.org/?p=slow/samba.git;a=commitdiff;h=0e36321c5b2d9d8f7582941b557217d528390894>
>
> Once I have metze's blessing on this one, I'll send out a fresh patchset to Uri
> for review, alongside a diff showing what changed since the previous iteration.

updated branch:
<https://git.samba.org/?p=slow/samba.git;a=shortlog;h=refs/heads/fruit-metadata>

catia name translation cache code is new, still need metze to take a look at it:
<https://goo.gl/aVWwqc>

Looks like the cache is effective. I added some basic cache usage metrics to the
code and then copied a bunch of directories:

(gdb) p cc_stats
$1 = {created_fsp_ext = 996, created_tmp = 37, cache_hits = 13396}

I'm not planning to ship the stats code.

Cheerio!
-slow

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Ralph Böhme-2
In reply to this post by Uri Simchoni-4
Hi Uri,

sorry for the delay.

So here's an updated patchset with the changes described below as well as the
catia translation cache discussed.

The catia translation cache patch already carries mine and metzes signed-off so
it would be good to go, but you're very welcome to take a look as well.

Commits are slightly reordered, moving all non-fruit VFS module changes to the
front of the patchset.

I've attached a simple diff showing just the differences in vfs_fruit.c between
the last patchset and the new one.

Additional changes:

- fixed error handling in fruit_open_meta_stream (see fruit-changes.diff)

- extended existing test in commit "s4/torture: vfs_fruit: add stream with
  illegal ntfs characters to copyile test" to test for something I came across
  when developing the catia caching

- new commit "vfs_streams_xattr: call SMB_VFS_OPEN with smb_fname_base" fixes a
  bug in this module

- new commit "vfs_streams_xattr: use SMB_VFS_NEXT_OPEN and CLOSE", there's no
  need for recursive VFS open calls

On Mon, Feb 06, 2017 at 07:35:27AM +0200, Uri Simchoni wrote:
> Continuing from 36/56 with some minor comments, then responding to last
> email. Everything else LGTM.
>
> Thanks for bearing with me,
> Uri.

thanks for taking the stab! :)

> [39/56]
> > -static int fruit_fstat_rsrc(vfs_handle_struct *handle, files_struct *fsp,
> > +static int fruit_fstat_meta(vfs_handle_struct *handle,
> > +                           files_struct *fsp,
> >                             SMB_STRUCT_STAT *sbuf)
> >  {
> > -       struct fruit_config_data *config;
> > -       struct adouble *ad = (struct adouble *)VFS_FETCH_FSP_EXTENSION(
> > -               handle, fsp);
> > +       struct fio *fio = (struct fio *)VFS_FETCH_FSP_EXTENSION(handle, fsp);
> > +       int ret;
> >  
> > -       DEBUG(10, ("fruit_fstat_rsrc called for %s\n",
> > -                  smb_fname_str_dbg(fsp->base_fsp->fsp_name)));
> > +       if (fio == NULL) {
> > +               return SMB_VFS_NEXT_FSTAT(handle, fsp, sbuf);
> > +       }
>
> That (fio not being NULL) is already checked in fruit_fstat(). Maybe for
> clarity and efficiency it would be better to pass fio to
> fruit_fstat_{rsrc,meta}()
Done.

> [44/56], [45/56]
> > +[vfs_fruit_metadata_stream]
> > +       path = $shrdir
> > +       vfs objects = fruit streams_xattr acl_xattr
>
> Shouldn't we have xattr_tdb below acl_xattr, to be env-agnostic?

Well, the existing vfs_fruit share didn't use xattr_tdb and I just copied. There
are actually insane tests that use vfs_fruit that make direct filesystem access
to the --option=torture:localdir provided path making direct xattr calls
(test_read_netatalk_metadata).

In theory the new shares vfs_fruit_metadata_stream and vfs_fruit_stream_depot
could use xattr_tdb as they don't use such hack. I just tried it but got some
test failures and I really, really would prefer not to debug issues caused by
xattr_tdb this time.

>
> >          plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/vfs_fruit -U$USERNAME%$PASSWORD --option=torture:localdir=$SELFTEST_PREFIX/nt4_dc/share')
> > +        plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/vfs_fruit_metadata_stream -U$USERNAME%$PASSWORD')
>
> I didn't understand why localdir is not set - that skips tests, but I
> couldn't figure out why they should be skipped.
>
> [50/56]
> > +               return false; \
>
> Should be "ret = false" ?
Oh, good catch, thanks!

> ... And responses to previous patches:
>
> On 01/31/2017 01:56 PM, Ralph Böhme wrote:
> > Hi Uri,
> >
> > thanks a lot for looking into this! Comments below:
> >
> > On Sun, Jan 29, 2017 at 09:13:41PM +0200, Uri Simchoni wrote:
> >> So far I got up to 35/56, although the catia patches deserve another
> >> look. I have some minor comments, see below.
> >> Will be looking at the rest, but it would take a few more days to get
> >> back to - hopefully next weekend.
> >>
> >> [10/56] -
> >> On error, shouldn't we be calling SMB_VFS_NEXT_CLOSE instead of
> >> SMB_VFS_CLOSE? The choice between SMB_VFS_XXX and SMB_VFS_NEXT_XXX is
> >> never an easy one. However, in the case of
> >> close-from-within-failed-open, it's the file we've just opened, and
> >> higher layers may have not seen the open return.
> >
> > agreed, fixed.
> >
> I still don't see the fix in last patch set
uh, sorry, no idea how that one got lost. :/ Fixed.

I've changed a few additional places to consistenly call the NEXT function and
not recurse in the VFS.

> >> [13/56] -
> >> That doesn't seem right... if we update btime out of the Mac metadata,
> >> we do it no matter where the metadata is stored.
> >
> > Not quite. :) In all cases execept the Netatalk case the btime will also be set
> > and fetched by the client (!) via the usual SMB protocol semantics and Samba FSA
> > processing. But that's not the case with Netatalk. There the primary source for
> > bime is the Netatalk metadata xattr, updated in fruit_ntimes() and queried here
> > in update_btime().
> >
> I still don't understand :)
>
> - How does the client (mac or otherwise) know if the backend is netatalk
> or something different?
it doesn't.

> - I understand that for Mac clients, it's not necessary to
> update_btime() at all, since the client would read the metadata stream
> and figure this by itself. So, if I understand correctly, what
> update_btime() does is take care of things for non-Mac clients.

This is for the case where you have both Mac clients connecting over
AFP/Netatalk *and* over SMB/Samba. update_btime() together with fruit_ntimes()
ensures that the btime is consistently read and written from the Netatalk
metadata xattr.

> >> [17/56]
> >>
> >>> +static int fruit_unlink_rsrc_xattr(vfs_handle_struct *handle,
> >>> +                                  const struct smb_filename *smb_fname,
> >>> +                                  bool force_unlink)
> >>> +{
> >>> +       /* Nothing to do here, removing the file will remove the xattr */
> >>
> >> About this comment - If I understand correctly, we do nothing because
> >> OS-X don't delete the resource.
> >
> > This comments describes OS X SMB *server* behaviour. That one ignores unlink
> > requests on the resource fork (no idea at which layer, may just be the OS X SMB
> > server FSA-layer that exposes this behaviour).
> >
> What I understood from that comment was that we do nothing *because* the
> "stream" (implemented by xattr) is removed with the file, whereas the
> real reason is that we act like an OS-X server.
Ah, *now* I see what you mean. :) You're right, I was mixing up
comments. Attached patchset has an updated comment for this function.

> >> [30/56] and [31/56]:
> >> Doing path translation inside a handle-based function raises some
> >> warning flags inside my head. True, it's been that way before. But now,
> >> other users of vfs_catia (assuming they exist :)) pay the toll as well.
> >
> > And they get the fix is well -- which this is all about. Not doing the
> > translation on the names attached to handles is just a bug.
> >
> ...not necessarily - handle-based APIs usually don't need paths - the
> handle embodies the object. The cost of translating a path for each read
> block might not be negligible. So this change could affect performance
> for current catia users which don't use fruit.
>
> An easy alternative is to have those added functions depend on a
> configuration var.
>
> Do we have vfs_catia stakeholders that can comment on this?
Attached patchset adds translated name caching via FSP extension.

Cheerio!
-slow

fruit-2017-02-22.patch (248K) Download Attachment
fruit-changes.diff (4K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Uri Simchoni-4
Everything RB+ me, including the additional 2-patch set for bug 12565
and the 1-patch for bug 12526, except:

1. The catia caching patch needs some 80-col fixes (catia_fset_nt_acl()
really stands out...)

2. A peculiar thing about catia - if this needs fixing maybe it should
go on a follow-up patch because some of this is before the current patch
- the catia xattr handling seems a bit inconsistent:

catia_fgetxattr / catia_fsetxattr map both path and attr name
catia_fremovexattr maps only the path
catia_flistxattr / catia_listxattr map the path but do not reverse-map
the EA names
catia_getxattr / catia_removexattr / catia_setxattr map only the attr name

Is mapping EA names (which are not file names) in the scope of vfs_catia
at all?

Also, the catia readdir doesn't reverse-map the entries (pre-patch), so
I'm really getting confused now...

Thanks,
Uri.

On 02/22/2017 01:51 PM, Ralph Böhme wrote:

> Hi Uri,
>
> sorry for the delay.
>
> So here's an updated patchset with the changes described below as well as the
> catia translation cache discussed.
>
> The catia translation cache patch already carries mine and metzes signed-off so
> it would be good to go, but you're very welcome to take a look as well.
>
> Commits are slightly reordered, moving all non-fruit VFS module changes to the
> front of the patchset.
>
> I've attached a simple diff showing just the differences in vfs_fruit.c between
> the last patchset and the new one.
>
> Additional changes:
>
> - fixed error handling in fruit_open_meta_stream (see fruit-changes.diff)
>
> - extended existing test in commit "s4/torture: vfs_fruit: add stream with
>   illegal ntfs characters to copyile test" to test for something I came across
>   when developing the catia caching
>
> - new commit "vfs_streams_xattr: call SMB_VFS_OPEN with smb_fname_base" fixes a
>   bug in this module
>
> - new commit "vfs_streams_xattr: use SMB_VFS_NEXT_OPEN and CLOSE", there's no
>   need for recursive VFS open calls
>
> On Mon, Feb 06, 2017 at 07:35:27AM +0200, Uri Simchoni wrote:
>> Continuing from 36/56 with some minor comments, then responding to last
>> email. Everything else LGTM.
>>
>> Thanks for bearing with me,
>> Uri.
>
> thanks for taking the stab! :)
>
>> [39/56]
>>> -static int fruit_fstat_rsrc(vfs_handle_struct *handle, files_struct *fsp,
>>> +static int fruit_fstat_meta(vfs_handle_struct *handle,
>>> +                           files_struct *fsp,
>>>                             SMB_STRUCT_STAT *sbuf)
>>>  {
>>> -       struct fruit_config_data *config;
>>> -       struct adouble *ad = (struct adouble *)VFS_FETCH_FSP_EXTENSION(
>>> -               handle, fsp);
>>> +       struct fio *fio = (struct fio *)VFS_FETCH_FSP_EXTENSION(handle, fsp);
>>> +       int ret;
>>>  
>>> -       DEBUG(10, ("fruit_fstat_rsrc called for %s\n",
>>> -                  smb_fname_str_dbg(fsp->base_fsp->fsp_name)));
>>> +       if (fio == NULL) {
>>> +               return SMB_VFS_NEXT_FSTAT(handle, fsp, sbuf);
>>> +       }
>>
>> That (fio not being NULL) is already checked in fruit_fstat(). Maybe for
>> clarity and efficiency it would be better to pass fio to
>> fruit_fstat_{rsrc,meta}()
>
> Done.
>
>> [44/56], [45/56]
>>> +[vfs_fruit_metadata_stream]
>>> +       path = $shrdir
>>> +       vfs objects = fruit streams_xattr acl_xattr
>>
>> Shouldn't we have xattr_tdb below acl_xattr, to be env-agnostic?
>
> Well, the existing vfs_fruit share didn't use xattr_tdb and I just copied. There
> are actually insane tests that use vfs_fruit that make direct filesystem access
> to the --option=torture:localdir provided path making direct xattr calls
> (test_read_netatalk_metadata).
>
> In theory the new shares vfs_fruit_metadata_stream and vfs_fruit_stream_depot
> could use xattr_tdb as they don't use such hack. I just tried it but got some
> test failures and I really, really would prefer not to debug issues caused by
> xattr_tdb this time.
>
>>
>>>          plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/vfs_fruit -U$USERNAME%$PASSWORD --option=torture:localdir=$SELFTEST_PREFIX/nt4_dc/share')
>>> +        plansmbtorture4testsuite(t, "nt4_dc", '//$SERVER_IP/vfs_fruit_metadata_stream -U$USERNAME%$PASSWORD')
>>
>> I didn't understand why localdir is not set - that skips tests, but I
>> couldn't figure out why they should be skipped.
>>
>> [50/56]
>>> +               return false; \
>>
>> Should be "ret = false" ?
>
> Oh, good catch, thanks!
>
>> ... And responses to previous patches:
>>
>> On 01/31/2017 01:56 PM, Ralph Böhme wrote:
>>> Hi Uri,
>>>
>>> thanks a lot for looking into this! Comments below:
>>>
>>> On Sun, Jan 29, 2017 at 09:13:41PM +0200, Uri Simchoni wrote:
>>>> So far I got up to 35/56, although the catia patches deserve another
>>>> look. I have some minor comments, see below.
>>>> Will be looking at the rest, but it would take a few more days to get
>>>> back to - hopefully next weekend.
>>>>
>>>> [10/56] -
>>>> On error, shouldn't we be calling SMB_VFS_NEXT_CLOSE instead of
>>>> SMB_VFS_CLOSE? The choice between SMB_VFS_XXX and SMB_VFS_NEXT_XXX is
>>>> never an easy one. However, in the case of
>>>> close-from-within-failed-open, it's the file we've just opened, and
>>>> higher layers may have not seen the open return.
>>>
>>> agreed, fixed.
>>>
>> I still don't see the fix in last patch set
>
> uh, sorry, no idea how that one got lost. :/ Fixed.
>
> I've changed a few additional places to consistenly call the NEXT function and
> not recurse in the VFS.
>
>>>> [13/56] -
>>>> That doesn't seem right... if we update btime out of the Mac metadata,
>>>> we do it no matter where the metadata is stored.
>>>
>>> Not quite. :) In all cases execept the Netatalk case the btime will also be set
>>> and fetched by the client (!) via the usual SMB protocol semantics and Samba FSA
>>> processing. But that's not the case with Netatalk. There the primary source for
>>> bime is the Netatalk metadata xattr, updated in fruit_ntimes() and queried here
>>> in update_btime().
>>>
>> I still don't understand :)
>>
>> - How does the client (mac or otherwise) know if the backend is netatalk
>> or something different?
>
> it doesn't.
>
>> - I understand that for Mac clients, it's not necessary to
>> update_btime() at all, since the client would read the metadata stream
>> and figure this by itself. So, if I understand correctly, what
>> update_btime() does is take care of things for non-Mac clients.
>
> This is for the case where you have both Mac clients connecting over
> AFP/Netatalk *and* over SMB/Samba. update_btime() together with fruit_ntimes()
> ensures that the btime is consistently read and written from the Netatalk
> metadata xattr.
>
>>>> [17/56]
>>>>
>>>>> +static int fruit_unlink_rsrc_xattr(vfs_handle_struct *handle,
>>>>> +                                  const struct smb_filename *smb_fname,
>>>>> +                                  bool force_unlink)
>>>>> +{
>>>>> +       /* Nothing to do here, removing the file will remove the xattr */
>>>>
>>>> About this comment - If I understand correctly, we do nothing because
>>>> OS-X don't delete the resource.
>>>
>>> This comments describes OS X SMB *server* behaviour. That one ignores unlink
>>> requests on the resource fork (no idea at which layer, may just be the OS X SMB
>>> server FSA-layer that exposes this behaviour).
>>>
>> What I understood from that comment was that we do nothing *because* the
>> "stream" (implemented by xattr) is removed with the file, whereas the
>> real reason is that we act like an OS-X server.
>
> Ah, *now* I see what you mean. :) You're right, I was mixing up
> comments. Attached patchset has an updated comment for this function.
>
>>>> [30/56] and [31/56]:
>>>> Doing path translation inside a handle-based function raises some
>>>> warning flags inside my head. True, it's been that way before. But now,
>>>> other users of vfs_catia (assuming they exist :)) pay the toll as well.
>>>
>>> And they get the fix is well -- which this is all about. Not doing the
>>> translation on the names attached to handles is just a bug.
>>>
>> ...not necessarily - handle-based APIs usually don't need paths - the
>> handle embodies the object. The cost of translating a path for each read
>> block might not be negligible. So this change could affect performance
>> for current catia users which don't use fruit.
>>
>> An easy alternative is to have those added functions depend on a
>> configuration var.
>>
>> Do we have vfs_catia stakeholders that can comment on this?
>
> Attached patchset adds translated name caching via FSP extension.
>
> Cheerio!
> -slow
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Ralph Böhme-2
On Sun, Feb 26, 2017 at 12:54:47AM +0200, Uri Simchoni wrote:
> Everything RB+ me, including the additional 2-patch set for bug 12565
> and the 1-patch for bug 12526, except:
>
> 1. The catia caching patch needs some 80-col fixes (catia_fset_nt_acl()
> really stands out...)

fixed.

> 2. A peculiar thing about catia - if this needs fixing maybe it should
> go on a follow-up patch because some of this is before the current patch
> - the catia xattr handling seems a bit inconsistent:

I know, the xattr/stream name mapping is a little awkward and probably only
works correctly with vfs_streams_xattr, but not with streams_depot.

xattr names are mapped in catia in the getters and setters, but stream names are
mapped in catia_streaminfo(), because the former was already done in catia when
I started to build on-top a stream/xattr mapping that work with vfs_streams_xattr.

The better approach would be to map all stream names in fsps and smb_fnames, but
as that wasn't done before, I just fixed the existing code up to a point to make
my usecase work.

> catia_fgetxattr / catia_fsetxattr map both path and attr name

Those are new in the patchset and for now probably should just do the same as
the corresponding pathname based ops, that is *not* map the fsp name, only the
xattr name.

Fixed in attached patchset.

> catia_fremovexattr maps only the path

Same, fixed to not map the fsp name but map the xattr name.

> catia_flistxattr / catia_listxattr map the path but do not reverse-map
> the EA names

The mapping of the xattr names is done at the stream layer in
catia_streaminfo(). This is good enough to make the stream/xattr mapping work
with vfs_streams_xattr, but it's also the point where it'll break with
streams_depot. As it's only an issue when setting fruit:encoding=native and I
don't see that makes any sense with streams_depot, it's probably not as bad as
it seems.

> catia_getxattr / catia_removexattr / catia_setxattr map only the attr name

Yup, that was the state of the module when I started patching stuff in order to
get a working stream name -> xattr name mapping.

> Is mapping EA names (which are not file names) in the scope of vfs_catia
> at all?

I gues a better solution would be mapping stream names in fsps and smb_fnames
instead, but that would be quite some work.

> Also, the catia readdir doesn't reverse-map the entries (pre-patch), so
> I'm really getting confused now...

??? There's no catia_readdir(), are you referring to catia_translate_name()
which is called via SMB_VFS_TRANSLATE_NAME() from vfs_readdirname() ?

Attached is the updated patchset with the changes discussed above.

-Ralph

fruit-2017-02-28.patch (248K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Uri Simchoni-4
Pushed!

[A comment inside]
Thanks,
Uri.

On 02/28/2017 01:22 PM, Ralph Böhme wrote:
(snip)

>> Also, the catia readdir doesn't reverse-map the entries (pre-patch), so
>> I'm really getting confused now...
>
> ??? There's no catia_readdir(), are you referring to catia_translate_name()
> which is called via SMB_VFS_TRANSLATE_NAME() from vfs_readdirname() ?
>

I confused READDIR_ATTR with READDIR, sorry..

> Attached is the updated patchset with the changes discussed above.
>
> -Ralph
>


Reply | Threaded
Open this post in threaded view
|

Re: [PATCH] Small (*cough*) patchset for vfs_fruit bug #12427

Ralph Böhme-2
On Thu, Mar 02, 2017 at 12:56:57AM +0200, Uri Simchoni wrote:
> Pushed!

thanks a lot for helping me getting this in. Much appreciated!

Cheerio!
-slow