vfs_acl_xattr and Linux memory fragmentation
Uri Simchoni
uri at samba.org
Thu Apr 13 10:08:42 UTC 2017
On 04/13/2017 11:53 AM, Volker Lendecke wrote:
> On Thu, Apr 13, 2017 at 11:40:49AM +0300, Uri Simchoni wrote:
>> My thinking was that trying 2K and 8K could very well lead to
>> accomplishing the task with way more than 2 system calls. There's always
>> the option of passing 0 as size to get actual size, then call again with
>> the buffer - two system calls.
>>
>> But I can add those steps too - I don't have any field-gathered
>> information to support this or that strategy, they all have their
>> strengths and weaknesses (for example, maybe there are less objects in
>> the 8K pool, so it's likelier to be depleted with many concurrent
>> users). Ultimately, it's a kernel issue and has been fixed in the kernel
>> (don't know specifics of distros).
>>
>> What I think we should keep is that the initial allocation is 1K,
>> because that preserves the user-mode behavior for the majority of cases.
>
> Then what about starting with 4k, covering 99.9%. If that fails we're
> in the slow path. There we could easily take 2 more syscalls, one to
> get the real size. This is racy, because between getting the size and
> doing the real getxattr call the size could increase, but that should
> be rare enough.
>
> Volker
>
OK, see attached.
My concern with this was that it allocates 4K instead of 1K in smbd, but
since this memory is freed by the caller, I suppose that should have
little impact on performance / heap fragmentation / etc.
Thanks,
Uri
-------------- next part --------------
From 1adf66fb1c4b8621388593edc393fee2f867f723 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Sun, 9 Apr 2017 00:20:40 +0300
Subject: [PATCH v2 1/4] selftest: test fetching a large ACL from vfs_acl_xattr
Add a test that fetches an ACL whose size is larger than 4K.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12737
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/script/tests/test_large_acl.sh | 59 ++++++++++++++++++++++++++++++++++
source3/selftest/tests.py | 1 +
2 files changed, 60 insertions(+)
create mode 100755 source3/script/tests/test_large_acl.sh
diff --git a/source3/script/tests/test_large_acl.sh b/source3/script/tests/test_large_acl.sh
new file mode 100755
index 0000000..9b6901f
--- /dev/null
+++ b/source3/script/tests/test_large_acl.sh
@@ -0,0 +1,59 @@
+#!/bin/bash
+#
+# Blackbox test for fetching a large ACL
+#
+
+if [ $# -lt 5 ]; then
+cat <<EOF
+Usage: $0 SERVER USERNAME PASSWORD SMBCLIENT SMBCACLS PARAMS
+EOF
+exit 1;
+fi
+
+SERVER=${1}
+USERNAME=${2}
+PASSWORD=${3}
+SMBCLIENT=${4}
+SMBCACLS=${5}
+shift 5
+ADDARGS="$*"
+SMBCLIENT="$VALGRIND ${SMBCLIENT} ${ADDARGS}"
+SMBCACLS="$VALGRIND ${SMBCACLS} ${ADDARGS}"
+
+incdir=`dirname $0`/../../../testprogs/blackbox
+. $incdir/subunit.sh
+
+# build a file to work with
+build_files()
+{
+ touch large_acl
+ $SMBCLIENT //$SERVER/acl_xattr_ign_sysacl_windows -U $USERNAME%$PASSWORD -c 'put large_acl' > /dev/null 2>&1
+ rm -rf large_acl > /dev/null
+}
+
+cleanup()
+{
+ $SMBCLIENT //$SERVER/acl_xattr_ign_sysacl_windows -U $USERNAME%$PASSWORD -c 'rm large_acl' > /dev/null 2>&1
+}
+
+build_files
+
+test_large_acl()
+{
+ #An ACL with 200 entries, ~7K
+ new_acl=$(seq 1001 1200 | sed -r -e '1 i\D:(A;;0x001f01ff;;;WD)' -e 's/(.*)/(A;;0x001f01ff;;;S-1-5-21-11111111-22222222-33333333-\1)/' | tr -d '\n')
+ $SMBCACLS //$SERVER/acl_xattr_ign_sysacl_windows -U $USERNAME%$PASSWORD --sddl -S $new_acl large_acl
+ actual_acl=$($SMBCACLS //$SERVER/acl_xattr_ign_sysacl_windows -U $USERNAME%$PASSWORD --sddl --numeric large_acl 2>/dev/null | sed -rn 's/.*(D:.*)/\1/p' | tr -d '\n')
+ if [ ! "$new_acl" = "$actual_acl" ] ; then
+ echo -e "expected:\n$new_acl\nactual:\n$actual_acl\n"
+ return 1
+ fi
+}
+
+failed=0
+
+testit "able to retrieve a large ACL if VFS supports it" test_large_acl || failed=`expr $failed + 1`
+
+cleanup
+
+exit $failed
diff --git a/source3/selftest/tests.py b/source3/selftest/tests.py
index d0e2ae6..3959439 100755
--- a/source3/selftest/tests.py
+++ b/source3/selftest/tests.py
@@ -228,6 +228,7 @@ for env in ["fileserver"]:
plantestsuite("samba3.blackbox.inherit_owner.default(%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'tmp', '0', '0', '-m', 'NT1'])
plantestsuite("samba3.blackbox.inherit_owner.full (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'inherit_owner', '1', '1', '-m', 'NT1'])
plantestsuite("samba3.blackbox.inherit_owner.unix (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_inherit_owner.sh"), '$SERVER', '$USERNAME', '$PASSWORD', '$PREFIX', smbclient3, smbcacls, 'inherit_owner_u', '0', '1', '-m', 'NT1'])
+ plantestsuite("samba3.blackbox.large_acl (%s)" % env, env, [os.path.join(samba3srcdir, "script/tests/test_large_acl.sh"), '$SERVER', '$USERNAME', '$PASSWORD', smbclient3, smbcacls])
#
# tar command tests
--
2.9.3
From 38db4fd79210611a0e8a3ec75a8a142592733960 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 13 Apr 2017 12:50:47 +0300
Subject: [PATCH v2 2/4] vfs_xattr_tdb: handle case of zero size.
With getxattr(), passing a zero buffer size is a
way of obtaining actual xattr size.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12737
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/modules/vfs_xattr_tdb.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/source3/modules/vfs_xattr_tdb.c b/source3/modules/vfs_xattr_tdb.c
index b32fbc1..58acf44 100644
--- a/source3/modules/vfs_xattr_tdb.c
+++ b/source3/modules/vfs_xattr_tdb.c
@@ -85,6 +85,12 @@ static ssize_t xattr_tdb_getxattr(struct vfs_handle_struct *handle,
TALLOC_FREE(frame);
return -1;
}
+
+ if (size == 0) {
+ TALLOC_FREE(frame);
+ return xattr_size;
+ }
+
if (blob.length > size) {
TALLOC_FREE(frame);
errno = ERANGE;
@@ -125,6 +131,12 @@ static ssize_t xattr_tdb_fgetxattr(struct vfs_handle_struct *handle,
TALLOC_FREE(frame);
return -1;
}
+
+ if (size == 0) {
+ TALLOC_FREE(frame);
+ return xattr_size;
+ }
+
if (blob.length > size) {
TALLOC_FREE(frame);
errno = ERANGE;
--
2.9.3
From c07e3f869588305ec83f1c74b382bcf775137760 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Sun, 9 Apr 2017 00:40:44 +0300
Subject: [PATCH v2 3/4] vfs_acl_xattr: factor out fetching of an extended
attribute
Pure refactoring - add a function that fetches an extended attribute
based on either the file descriptor or the file name.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12737
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/modules/vfs_acl_xattr.c | 44 +++++++++++++++++++++++++++++------------
1 file changed, 31 insertions(+), 13 deletions(-)
diff --git a/source3/modules/vfs_acl_xattr.c b/source3/modules/vfs_acl_xattr.c
index e1f90ff..85b9a0d 100644
--- a/source3/modules/vfs_acl_xattr.c
+++ b/source3/modules/vfs_acl_xattr.c
@@ -37,6 +37,35 @@
Pull a security descriptor into a DATA_BLOB from a xattr.
*******************************************************************/
+static ssize_t getxattr_do(vfs_handle_struct *handle,
+ files_struct *fsp,
+ const struct smb_filename *smb_fname,
+ const char *xattr_name,
+ uint8_t *val,
+ size_t size)
+{
+ ssize_t sizeret;
+ int saved_errno = 0;
+
+ become_root();
+ if (fsp && fsp->fh->fd != -1) {
+ sizeret = SMB_VFS_FGETXATTR(fsp, xattr_name, val, size);
+ } else {
+ sizeret = SMB_VFS_GETXATTR(handle->conn, smb_fname->base_name,
+ XATTR_NTACL_NAME, val, size);
+ }
+ if (sizeret == -1) {
+ saved_errno = errno;
+ }
+ unbecome_root();
+
+ if (saved_errno != 0) {
+ errno = saved_errno;
+ }
+
+ return sizeret;
+}
+
static NTSTATUS get_acl_blob(TALLOC_CTX *ctx,
vfs_handle_struct *handle,
files_struct *fsp,
@@ -47,7 +76,6 @@ static NTSTATUS get_acl_blob(TALLOC_CTX *ctx,
uint8_t *val = NULL;
uint8_t *tmp;
ssize_t sizeret;
- int saved_errno = 0;
ZERO_STRUCTP(pblob);
@@ -60,21 +88,11 @@ static NTSTATUS get_acl_blob(TALLOC_CTX *ctx,
}
val = tmp;
- become_root();
- if (fsp && fsp->fh->fd != -1) {
- sizeret = SMB_VFS_FGETXATTR(fsp, XATTR_NTACL_NAME, val, size);
- } else {
- sizeret = SMB_VFS_GETXATTR(handle->conn, smb_fname->base_name,
- XATTR_NTACL_NAME, val, size);
- }
- if (sizeret == -1) {
- saved_errno = errno;
- }
- unbecome_root();
+ sizeret =
+ getxattr_do(handle, fsp, smb_fname, XATTR_NTACL_NAME, val, size);
/* Max ACL size is 65536 bytes. */
if (sizeret == -1) {
- errno = saved_errno;
if ((errno == ERANGE) && (size != 65536)) {
/* Too small, try again. */
size = 65536;
--
2.9.3
From a3fcf24c9cfe59b0cff4a6200f7fb1513289a092 Mon Sep 17 00:00:00 2001
From: Uri Simchoni <uri at samba.org>
Date: Thu, 13 Apr 2017 12:44:58 +0300
Subject: [PATCH v2 4/4] vfs_acl_xattr: avoid needlessly supplying a large
buffer to getxattr()
When obtaining the security descriptor via getxattr(), first try
optimistically to supply a buffer of 4K, and if that turns out
to be too small, determine the correct buffer size.
The previous behavior of falling back to a 64K buffer encountered
problem with Linux prior to version 3.6, due to pyisical memory
fragmentation. With those kernels, as long as the buffer is 8K or
smaller, getting the xattr is much less prone to failure due to
memory fragmentation.
BUG: https://bugzilla.samba.org/show_bug.cgi?id=12737
Signed-off-by: Uri Simchoni <uri at samba.org>
---
source3/modules/vfs_acl_xattr.c | 44 ++++++++++++++++++++++++++++-------------
1 file changed, 30 insertions(+), 14 deletions(-)
diff --git a/source3/modules/vfs_acl_xattr.c b/source3/modules/vfs_acl_xattr.c
index 85b9a0d..421860b 100644
--- a/source3/modules/vfs_acl_xattr.c
+++ b/source3/modules/vfs_acl_xattr.c
@@ -72,7 +72,7 @@ static NTSTATUS get_acl_blob(TALLOC_CTX *ctx,
const struct smb_filename *smb_fname,
DATA_BLOB *pblob)
{
- size_t size = 1024;
+ size_t size = 4096;
uint8_t *val = NULL;
uint8_t *tmp;
ssize_t sizeret;
@@ -91,22 +91,38 @@ static NTSTATUS get_acl_blob(TALLOC_CTX *ctx,
sizeret =
getxattr_do(handle, fsp, smb_fname, XATTR_NTACL_NAME, val, size);
- /* Max ACL size is 65536 bytes. */
- if (sizeret == -1) {
- if ((errno == ERANGE) && (size != 65536)) {
- /* Too small, try again. */
- size = 65536;
- goto again;
- }
+ if (sizeret >= 0) {
+ pblob->data = val;
+ pblob->length = sizeret;
+ return NT_STATUS_OK;
+ }
- /* Real error - exit here. */
- TALLOC_FREE(val);
- return map_nt_error_from_unix(errno);
+ if (errno != ERANGE) {
+ goto err;
}
- pblob->data = val;
- pblob->length = sizeret;
- return NT_STATUS_OK;
+ /* Too small, try again. */
+ sizeret =
+ getxattr_do(handle, fsp, smb_fname, XATTR_NTACL_NAME, NULL, 0);
+ if (sizeret < 0) {
+ goto err;
+ }
+
+ if (size < sizeret) {
+ size = sizeret;
+ }
+
+ if (size > 65536) {
+ /* Max ACL size is 65536 bytes. */
+ errno = ERANGE;
+ goto err;
+ }
+
+ goto again;
+ err:
+ /* Real error - exit here. */
+ TALLOC_FREE(val);
+ return map_nt_error_from_unix(errno);
}
/*******************************************************************
--
2.9.3
More information about the samba-technical
mailing list