Bug 13119 - Return NT_STATUS_FILE_SYSTEM_LIMITATION stream IO runs into fs limit
Return NT_STATUS_FILE_SYSTEM_LIMITATION stream IO runs into fs limit
Status: ASSIGNED
Product: Samba 4.1 and newer
Classification: Unclassified
Component: File services
unspecified
All All
: P5 normal
: ---
Assigned To: Ralph Böhme
Samba QA Contact
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-11-05 09:07 UTC by Ralph Böhme
Modified: 2017-11-07 07:43 UTC (History)
1 user (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.