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