vfs_acl_xattr and Linux memory fragmentation
Jeremy Allison
jra at samba.org
Mon Apr 17 22:42:07 UTC 2017
On Thu, Apr 13, 2017 at 01:08:42PM +0300, Uri Simchoni via samba-technical wrote:
> 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.
LGTM. I was going to ask for 4k as well ! :-).
Jeremy.
> 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