Bug 13119 - Return NT_STATUS_FILE_SYSTEM_LIMITATION stream IO runs into fs limit
Summary: Return NT_STATUS_FILE_SYSTEM_LIMITATION stream IO runs into fs limit
Status: ASSIGNED
Alias: None
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services (show other bugs)
Version: unspecified
Hardware: All All
: P5 normal (vote)
Target Milestone: ---
Assignee: Ralph Böhme
QA Contact: Samba QA Contact
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-05 09:07 UTC by Ralph Böhme
Modified: 2019-03-04 15:00 UTC (History)
2 users (show)

See Also:


Attachments
WIP patch for master (1.37 KB, patch)
2017-11-06 10:15 UTC, Ralph Böhme
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Ralph Böhme 2017-11-05 09:07:10 UTC
Cf the discussion here:
https://lists.samba.org/archive/samba/2017-August/210145.html
Comment 1 Timur Bakeyev 2017-11-05 20:58:28 UTC
We have a "almost correct" patch for this issue, I'll try to clean it up a bit and post here. Requires code reviews still.
Comment 2 Timur Bakeyev 2017-11-05 21:01:57 UTC
Actually, you can take a peek here: https://github.com/freenas/samba/commit/f0a2dc4acc76bff4cf98639d12ebe3cc857574fa
Comment 3 Timur Bakeyev 2017-11-05 21:05:38 UTC
(In reply to Timur Bakeyev from comment #2)

Ok, this one solves origin of the problem, still need to address mapping of NO_MEMORY into NT_STATUS_FILE_SYSTEM_LIMITATION
Comment 4 Ralph Böhme 2017-11-06 10:14:52 UTC
(In reply to Timur Bakeyev from comment #2)
I skimmed your patch and afaict this removes the optimisation in get_ea_value() that avoids an xattr syscall to fetch the xattr size for the common case of small xattrs (less then 256 bytes).

There's also a lot of unrelated indentation changes and README.Coding issues.

I also wonder whether simply raising the limit in get_ea_value() would do the trick. Something like 64 MB should cover even the largest streams. Maybe also add a parametric  option that allows setting the limit to a configurable value (stored in a static variable to avoid expensive config parsing in every call to get_ea_value()).
Comment 5 Ralph Böhme 2017-11-06 10:15:48 UTC
Created attachment 13757 [details]
WIP patch for master

WIP patch that increases the max xattr size by adding a parametric option with a 64 MB default.
Comment 6 Ralph Böhme 2017-11-06 10:21:06 UTC
(In reply to Timur Bakeyev from comment #2)
Oh, I noticed another thing in your patch: you call talloc_stackframe() but don't use the returned frame but talloc_tos() instead.

There's no need to call talloc_stackframe() in order to be able to use talloc_tos(). It  just triggers unneeded expensive talloc allocation. Just allocate of talloc_tos() and explicitly free the allocated object.
Comment 7 Björn Jacke 2019-03-04 15:00:50 UTC
it might be the better approach to return NT_STATUS_OBJECT_NAME_COLLISION in case of overlage streams. This is also what a Windows servers return when there is no stream support like on vfat devices. The benefit with this is that windows explorer clients will then show the possiblity to skip writing the meta data only but copy the rest of the file. With NT_STATUS_FILE_SYSTEM_LIMITATION Windows explorer would only allow to skip copying the entire file.