From: Omar Polo <op@omarpolo.com>
Subject: Re: fix pack cache size scaling for low ulimit -n
To: Stefan Sperling <stsp@stsp.name>
Cc: gameoftrees@openbsd.org
Date: Mon, 31 Oct 2022 19:05:51 +0100

On 2022/10/31 18:35:55 +0100, Stefan Sperling <stsp@stsp.name> wrote:
> Landry noticed "too many open files" errors during 'got checkout' on arm64.
> 
> I eventually figured out that commit 0ae84acc1f0a0584e4f26ccbe029c895043b3abe
> broke down-scaling of the pack file cache for low ulimit -n situations.
> Since then, the pack_fd array has been fully populated with open file handles.
> The current logic is such that we do not use all of those files if ulimit -n
> is low. But the unused files should not even be opened in the first place.
> 
> The patch below fixes this issue, and adds a test case which triggers the
> problem seen by Landry.
> 
> ok?

ok

a comment below

> [...]
> blob - 3ceb6774f7bce8a411b960566268aea4c45554cc
> blob + fa93789a82f9ac4e822dda641bca1e8841f33829
> --- regress/cmdline/checkout.sh
> +++ regress/cmdline/checkout.sh
> @@ -889,6 +889,51 @@ test_parseargs "$@"
>  	test_done "$testroot" 0
>  }
>  
> +test_checkout_ulimit_n() {
> +	local testroot=`test_init checkout_ulimit_n`
> +
> +	echo -n "Checked out refs/heads/master: " >> $testroot/stdout.expected
> +	git_show_head $testroot/repo >> $testroot/stdout.expected
> +	printf "\nNow shut up and hack\n" >> $testroot/stdout.expected
> +
> +	# Drastically reduce the number of files we are allowed to use.
> +	# This tests our down-scaling of caches which store open file handles.
> +	# Checkout should still work; if it does not, then either there is
> +	# a bug or the fixed limit used by this test case is no longer valid
> +	# and must be raised.
> +	ulimit -n 20
> +
> +	got checkout -q $testroot/repo $testroot/wt > $testroot/stdout

I'd run this in a subshell to avoid changing the current ulimit

+	(ulimit -n && got checkout ...)

it shouldn't matter, `got checkout' should work with ulimit -n 20
anyway, but it could have interactions with tests ran after it

> +	ret=$?
> [...]
> +	test_done "$testroot" "$ret"
> +}
> +
> +

double empty line

>  test_parseargs "$@"
>  run_test test_checkout_basic
>  run_test test_checkout_dir_exists
> @@ -904,3 +949,4 @@ run_test test_checkout_umask
>  run_test test_checkout_repo_with_unknown_extension
>  run_test test_checkout_quiet
>  run_test test_checkout_umask
> +run_test test_checkout_ulimit_n