ufs: try to elide the interlock in ufs_itimes
ClosedPublic
Actions

Authored by mjg on Sep 10 2022, 9:33 PM.

Details

Reviewers

kib
mckusick

Commits

rG60457d1ea068: ufs: try to elide the interlock in ufs_itimes
rGca63cbbe9554: ufs: always call ufs_itimes on close
rG04f095a49165: ufs: try to elide the interlock in ufs_itimes
rGbe931de6c99f: ufs: always call ufs_itimes on close

Summary

Especially if the fs is mounted noatime none of the flags tend to be set.

While here dedup code with ufsfifo_close.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

mjg requested review of this revision.Sep 10 2022, 9:33 PM

mjg created this revision.

Then why checking for v_usecount > 1 at all? It can go to 1 at any time.

I'm guessing the original code was avoiding the call, expecting for it to happen in ufs_inactive later. This retains the ultimately racy nature, except avoids the interlock.

I agree, it probably was an attempt for micro-optimization, I do not see any other reason. Then, since inactivation is not guaranteed to occur now, while close is, I think the check for v_usecount should be removed.

mjg updated this revision to Diff 110485.Sep 12 2022, 11:29 PM

mjg retitled this revision from ufs: do a lockless usecount check in ufs_close to ufs: try to elide the interlock in ufs_itimes.

mjg edited the summary of this revision. (Show Details)

kib accepted this revision.Sep 13 2022, 10:17 AM

This revision is now accepted and ready to land.Sep 13 2022, 10:17 AM

With updates, looks good.

Closed by commit rGbe931de6c99f: ufs: always call ufs_itimes on close (authored by mjg). · Explain WhySep 14 2022, 6:13 PM

This revision was automatically updated to reflect the committed changes.

mjg added a commit: rGbe931de6c99f: ufs: always call ufs_itimes on close.

Herald added a subscriber: imp. · View Herald TranscriptSep 14 2022, 6:13 PM

mjg added a commit: rG04f095a49165: ufs: try to elide the interlock in ufs_itimes.Sep 14 2022, 6:13 PM

mjg added a commit: rGca63cbbe9554: ufs: always call ufs_itimes on close.Aug 24 2023, 11:25 PM

mjg added a commit: rG60457d1ea068: ufs: try to elide the interlock in ufs_itimes.Aug 24 2023, 11:31 PM

Revision Contents
Changeset List

Path

Size

sys/

ufs/

ufs_vnops.c

15 lines

Diff 110555

View Options

sys/ufs/ufs/ufs_vnops.c

Show All 28 Lines

# TODO:		# TODO:
# - roll back after errors or SIGINT		# - roll back after errors or SIGINT
# - created revs		# - created revs
# - main (for git arc stage)		# - main (for git arc stage)

warn()		warn()
{		{
echo "$(basename $0): $1" >&2		echo "$(basename "$0"): $1" >&2
}		}

err()		err()
{		{
warn "$1"		warn "$1"
exit 1		exit 1
}		}

▲ Show 20 Lines • Show All 100 Lines • ▼ Show 20 Lines
{		{
local diff		local diff

diff=$1		diff=$1
if ! expr "$diff" : 'D[1-9][0-9]*$' >/dev/null; then		if ! expr "$diff" : 'D[1-9][0-9]*$' >/dev/null; then
err "invalid diff ID $diff"		err "invalid diff ID $diff"
fi		fi

echo '{"names":["'$diff'"]}' \|		echo '{"names":["'"$diff"'"]}' \|
arc call-conduit -- phid.lookup \|		arc call-conduit -- phid.lookup \|
jq -r "select(.response != []) \| .response.${diff}.phid"		jq -r "select(.response != []) \| .response.${diff}.phid"
}		}

diff2status()		diff2status()
{		{
local diff tmp status summary		local diff tmp status summary

diff=$1		diff=$1
if ! expr "$diff" : 'D[1-9][0-9]*$' >/dev/null; then		if ! expr "$diff" : 'D[1-9][0-9]*$' >/dev/null; then
err "invalid diff ID $diff"		err "invalid diff ID $diff"
fi		fi

tmp=$(mktemp)		tmp=$(mktemp)
echo '{"names":["'$diff'"]}' \|		echo '{"names":["'"$diff"'"]}' \|
arc call-conduit -- phid.lookup > $tmp		arc call-conduit -- phid.lookup > "$tmp"
status=$(jq -r "select(.response != []) \| .response.${diff}.status" < $tmp)		status=$(jq -r "select(.response != []) \| .response.${diff}.status" < "$tmp")
summary=$(jq -r "select(.response != []) \|		summary=$(jq -r "select(.response != []) \|
.response.${diff}.fullName" < $tmp)		.response.${diff}.fullName" < "$tmp")
printf "%-14s %s\n" "${status}" "${summary}"		printf "%-14s %s\n" "${status}" "${summary}"
}		}

log2diff()		log2diff()
{		{
local diff		local diff

diff=$(git show -s --format=%B $commit \|		diff=$(git show -s --format=%B "$commit" \|
sed -nE '/^Differential Revision:[[:space:]]+(https:\/\/reviews.freebsd.org\/)?(D[0-9]+)$/{s//\2/;p;}')		sed -nE '/^Differential Revision:[[:space:]]+(https:\/\/reviews.freebsd.org\/)?(D[0-9]+)$/{s//\2/;p;}')
if [ -n "$diff" ] && [ $(echo "$diff" \| wc -l) -eq 1 ]; then		if [ -n "$diff" ] && [ "$(echo "$diff" \| wc -l)" -eq 1 ]; then
echo $diff		echo "$diff"
else		else
echo		echo
fi		fi
}		}

commit2diff()		commit2diff()
{		{
local commit diff title		local commit diff title

commit=$1		commit=$1

# First, look for a valid differential reference in the commit		# First, look for a valid differential reference in the commit
# log.		# log.
diff=$(log2diff $commit)		diff=$(log2diff "$commit")
if [ -n "$diff" ]; then		if [ -n "$diff" ]; then
echo $diff		echo "$diff"
return		return
fi		fi

# Second, search the open reviews returned by 'arc list' looking		# Second, search the open reviews returned by 'arc list' looking
# for a subject match.		# for a subject match.
title=$(git show -s --format=%s $commit)		title=$(git show -s --format=%s "$commit")
diff=$(arc list \| fgrep "$title" \| egrep -o 'D[1-9][0-9]*:' \| tr -d ':')		diff=$(arc list \| grep -F "$title" \| grep -E -o 'D[1-9][0-9]*:' \| tr -d ':')
if [ -z "$diff" ]; then		if [ -z "$diff" ]; then
err "could not find review for '${title}'"		err "could not find review for '${title}'"
elif [ $(echo "$diff" \| wc -l) -ne 1 ]; then		elif [ "$(echo "$diff" \| wc -l)" -ne 1 ]; then
err "found multiple reviews with the same title"		err "found multiple reviews with the same title"
fi		fi

echo $diff		echo "$diff"
}		}

create_one_review()		create_one_review()
{		{
local childphid commit doprompt msg parent parentphid reviewers		local childphid commit doprompt msg parent parentphid reviewers
local subscribers		local subscribers

commit=$1		commit=$1
reviewers=$2		reviewers=$2
subscribers=$3		subscribers=$3
parent=$4		parent=$4
doprompt=$5		doprompt=$5

if [ "$doprompt" ] && ! show_and_prompt $commit; then		if [ "$doprompt" ] && ! show_and_prompt "$commit"; then
return 1		return 1
fi		fi

git checkout -q $commit		git checkout -q "$commit"

msg=$(mktemp)		msg=$(mktemp)
git show -s --format='%B' $commit > $msg		git show -s --format='%B' "$commit" > "$msg"
printf "\nTest Plan:\n" >> $msg		printf "\nTest Plan:\n" >> "$msg"
printf "\nReviewers:\n" >> $msg		printf "\nReviewers:\n" >> "$msg"
printf "${reviewers}\n" >> $msg		printf "%s\n" "${reviewers}" >> "$msg"
printf "\nSubscribers:\n" >> $msg		printf "\nSubscribers:\n" >> "$msg"
printf "${subscribers}\n" >> $msg		printf "%s\n" "${subscribers}" >> "$msg"

yes \| env EDITOR=true \		yes \| env EDITOR=true \
arc diff --message-file $msg --never-apply-patches --create --allow-untracked $BROWSE HEAD~		arc diff --message-file "$msg" --never-apply-patches --create --allow-untracked $BROWSE HEAD~
[ $? -eq 0 ] \|\| err "could not create Phabricator diff"		[ $? -eq 0 ] \|\| err "could not create Phabricator diff"

if [ -n "$parent" ]; then		if [ -n "$parent" ]; then
diff=$(commit2diff $commit)		diff=$(commit2diff "$commit")
[ -n "$diff" ] \|\| err "failed to look up review ID for $commit"		[ -n "$diff" ] \|\| err "failed to look up review ID for $commit"

childphid=$(diff2phid $diff)		childphid=$(diff2phid "$diff")
parentphid=$(diff2phid $parent)		parentphid=$(diff2phid "$parent")
echo '{		echo '{
"objectIdentifier": "'${childphid}'",		"objectIdentifier": "'"${childphid}"'",
"transactions": [		"transactions": [
{		{
"type": "parents.add",		"type": "parents.add",
"value": ["'${parentphid}'"]		"value": ["'"${parentphid}"'"]
}		}
]}' \|		]}' \|
arc call-conduit -- differential.revision.edit >&3		arc call-conduit -- differential.revision.edit >&3
fi		fi
rm -f $msg		rm -f "$msg"
return 0		return 0
}		}

# Get a list of reviewers who accepted the specified diff.		# Get a list of reviewers who accepted the specified diff.
diff2reviewers()		diff2reviewers()
{		{
local diff phid reviewid userids		local diff reviewid userids

diff=$1		diff=$1
reviewid=$(diff2phid $diff)		reviewid=$(diff2phid "$diff")
userids=$( \		userids=$( \
echo '{		echo '{
"constraints": {"phids": ["'$reviewid'"]},		"constraints": {"phids": ["'"$reviewid"'"]},
"attachments": {"reviewers": true}		"attachments": {"reviewers": true}
}' \|		}' \|
arc call-conduit -- differential.revision.search \|		arc call-conduit -- differential.revision.search \|
jq '.response.data[0].attachments.reviewers.reviewers[] \| select(.status == "accepted").reviewerPHID')		jq '.response.data[0].attachments.reviewers.reviewers[] \| select(.status == "accepted").reviewerPHID')
if [ -n "$userids" ]; then		if [ -n "$userids" ]; then
echo '{		echo '{
"constraints": {"phids": ['$(echo -n $userids \| tr '[:space:]' ',')']}		"constraints": {"phids": ['"$(echo -n "$userids" \| tr '[:space:]' ',')"']}
}' \|		}' \|
arc call-conduit -- user.search \|		arc call-conduit -- user.search \|
jq -r '.response.data[].fields.username'		jq -r '.response.data[].fields.username'
fi		fi
}		}

prompt()		prompt()
{		{
local resp		local resp

if [ "$ASSUME_YES" ]; then		if [ "$ASSUME_YES" ]; then
return 1		return 1
fi		fi

printf "\nDoes this look OK? [y/N] "		printf "\nDoes this look OK? [y/N] "
read resp		read -r resp

case $resp in		case $resp in
[Yy])		[Yy])
return 0		return 0
;;		;;
*)		*)
return 1		return 1
;;		;;
esac		esac
}		}

show_and_prompt()		show_and_prompt()
{		{
local commit		local commit

commit=$1		commit=$1

git show $commit		git show "$commit"
prompt		prompt
}		}

save_head()		save_head()
{		{
local orig		local orig

if ! orig=$(git symbolic-ref --short -q HEAD); then		if ! orig=$(git symbolic-ref --short -q HEAD); then
orig=$(git show -s --pretty=%H HEAD)		orig=$(git show -s --pretty=%H HEAD)
fi		fi
SAVED_HEAD=$orig		SAVED_HEAD=$orig
}		}

restore_head()		restore_head()
{		{
if [ -n "$SAVED_HEAD" ]; then		if [ -n "$SAVED_HEAD" ]; then
git checkout -q $SAVED_HEAD		git checkout -q "$SAVED_HEAD"
SAVED_HEAD=		SAVED_HEAD=
fi		fi
}		}

build_commit_list()		build_commit_list()
{		{
local chash _commits commits		local chash _commits commits

for chash in $@; do		for chash in "$@"; do
_commits=$(git rev-parse "${chash}")		_commits=$(git rev-parse "${chash}")
if ! git cat-file -e "${chash}"'^{commit}' >/dev/null 2>&1; then		if ! git cat-file -e "${chash}"'^{commit}' >/dev/null 2>&1; then
_commits=$(git rev-list $_commits \| tail -r)		_commits=$(git rev-list "$_commits" \| tail -r)
fi		fi
[ -n "$_commits" ] \|\| err "invalid commit ID ${chash}"		[ -n "$_commits" ] \|\| err "invalid commit ID ${chash}"
commits="$commits $_commits"		commits="$commits $_commits"
done		done
echo $commits		echo "$commits"
}		}

gitarc::create()		gitarc::create()
{		{
local commit commits doprompt list o prev reviewers subscribers		local commit commits doprompt list o prev reviewers subscribers

list=		list=
if eval $(git config --bool --default false --get arc.list); then		if [ "$(git config --bool --default false --get arc.list)" != "false" ]; then
list=1		list=1
fi		fi
doprompt=1		doprompt=1
while getopts lr:s: o; do		while getopts lr:s: o; do
case "$o" in		case "$o" in
l)		l)
list=1		list=1
;;		;;
r)		r)
reviewers="$OPTARG"		reviewers="$OPTARG"
;;		;;
s)		s)
subscribers="$OPTARG"		subscribers="$OPTARG"
;;		;;
*)		*)
err_usage		err_usage
;;		;;
esac		esac
done		done
shift $((OPTIND-1))		shift $((OPTIND-1))

commits=$(build_commit_list $@)		commits=$(build_commit_list "$@")

if [ "$list" ]; then		if [ "$list" ]; then
for commit in ${commits}; do		for commit in ${commits}; do
git --no-pager show --oneline --no-patch $commit		git --no-pager show --oneline --no-patch "$commit"
done \| git_pager		done \| git_pager
if ! prompt; then		if ! prompt; then
return		return
fi		fi
doprompt=		doprompt=
fi		fi

save_head		save_head
prev=""		prev=""
for commit in ${commits}; do		for commit in ${commits}; do
if create_one_review "$commit" "$reviewers" "$subscribers" "$prev" \		if create_one_review "$commit" "$reviewers" "$subscribers" "$prev" \
"$doprompt"; then		"$doprompt"; then
prev=$(commit2diff "$commit")		prev=$(commit2diff "$commit")
else		else
prev=""		prev=""
fi		fi
done		done
restore_head		restore_head
}		}

gitarc::list()		gitarc::list()
{		{
local chash commit commits diff title		local chash commit commits diff title

commits=$(build_commit_list $@)		commits=$(build_commit_list "$@")

for commit in $commits; do		for commit in $commits; do
chash=$(git show -s --format='%C(auto)%h' $commit)		chash=$(git show -s --format='%C(auto)%h' "$commit")
echo -n "${chash} "		echo -n "${chash} "

diff=$(log2diff $commit)		diff=$(log2diff "$commit")
if [ -n "$diff" ]; then		if [ -n "$diff" ]; then
diff2status $diff		diff2status "$diff"
continue		continue
fi		fi

# This does not use commit2diff as it needs to handle errors		# This does not use commit2diff as it needs to handle errors
# differently and keep the entire status. The extra 'cat'		# differently and keep the entire status. The extra 'cat'
# after 'fgrep' avoids erroring due to -e.		# after 'fgrep' avoids erroring due to -e.
title=$(git show -s --format=%s $commit)		title=$(git show -s --format=%s "$commit")
diff=$(arc list \| fgrep "$title" \| cat)		diff=$(arc list \| grep -F "$title" \| cat)
if [ -z "$diff" ]; then		if [ -z "$diff" ]; then
echo "No Review : $title"		echo "No Review : $title"
elif [ $(echo "$diff" \| wc -l) -ne 1 ]; then		elif [ "$(echo "$diff" \| wc -l)" -ne 1 ]; then
echo -n "Ambiguous Reviews: "		echo -n "Ambiguous Reviews: "
echo "$diff" \| egrep -o 'D[1-9][0-9]*:' \| tr -d ':' \		echo "$diff" \| grep -E -o 'D[1-9][0-9]*:' \| tr -d ':' \
\| paste -sd ',' - \| sed 's/,/, /g'		\| paste -sd ',' - \| sed 's/,/, /g'
else		else
echo "$diff" \| sed -e 's/^[^ ]* *//'		echo "$diff" \| sed -e 's/^[^ ]* *//'
fi		fi
done		done
}		}

gitarc::patch()		gitarc::patch()
{		{
local rev		local rev

if [ $# -eq 0 ]; then		if [ $# -eq 0 ]; then
err_usage		err_usage
fi		fi

for rev in $@; do		for rev in "$@"; do
arc patch --skip-dependencies --nocommit --nobranch --force $rev		arc patch --skip-dependencies --nocommit --nobranch --force "$rev"
echo "Applying ${rev}..."		echo "Applying ${rev}..."
[ $? -eq 0 ] \|\| break		[ $? -eq 0 ] \|\| break
done		done
}		}

gitarc::stage()		gitarc::stage()
{		{
local author branch commit commits diff reviewers tmp		local author branch commit commits diff reviewers tmp

branch=main		branch=main
while getopts b: o; do		while getopts b: o; do
case "$o" in		case "$o" in
b)		b)
branch="$OPTARG"		branch="$OPTARG"
;;		;;
*)		*)
err_usage		err_usage
;;		;;
esac		esac
done		done
shift $((OPTIND-1))		shift $((OPTIND-1))

commits=$(build_commit_list $@)		commits=$(build_commit_list "$@")

if [ "$branch" = "main" ]; then		if [ "$branch" = "main" ]; then
git checkout -q main		git checkout -q main
else		else
git checkout -q -b ${branch} main		git checkout -q -b "${branch}" main
fi		fi

tmp=$(mktemp)		tmp=$(mktemp)
for commit in $commits; do		for commit in $commits; do
git show -s --format=%B $commit > $tmp		git show -s --format=%B "$commit" > "$tmp"
diff=$(arc list \| fgrep "$(git show -s --format=%s $commit)" \|		diff=$(arc list \| grep -F "$(git show -s --format=%s "$commit")" \|
egrep -o 'D[1-9][0-9]*:' \| tr -d ':')		grep -E -o 'D[1-9][0-9]*:' \| tr -d ':')
if [ -n "$diff" ]; then		if [ -n "$diff" ]; then
# XXX this leaves an extra newline in some cases.		# XXX this leaves an extra newline in some cases.
reviewers=$(diff2reviewers $diff \| sed '/^$/d' \| paste -sd ',' - \| sed 's/,/, /g')		reviewers=$(diff2reviewers "$diff" \| sed '/^$/d' \| paste -sd ',' - \| sed 's/,/, /g')
if [ -n "$reviewers" ]; then		if [ -n "$reviewers" ]; then
printf "Reviewed by:\t${reviewers}\n" >> $tmp		printf "Reviewed by:\t%s\n" "${reviewers}" >> "$tmp"
fi		fi
printf "Differential Revision:\thttps://reviews.freebsd.org/${diff}" >> $tmp		printf "Differential Revision:\thttps://reviews.freebsd.org/%s" "${diff}" >> "$tmp"
fi		fi
author=$(git show -s --format='%an <%ae>' ${commit})		author=$(git show -s --format='%an <%ae>' "${commit}")
if ! git cherry-pick --no-commit ${commit}; then		if ! git cherry-pick --no-commit "${commit}"; then
warn "Failed to apply $(git rev-parse --short ${commit}). Are you staging patches in the wrong order?"		warn "Failed to apply $(git rev-parse --short "${commit}"). Are you staging patches in the wrong order?"
git checkout -f		git checkout -f
break		break
fi		fi
git commit --edit --file $tmp --author "${author}"		git commit --edit --file "$tmp" --author "${author}"
done		done
}		}

gitarc::update()		gitarc::update()
{		{
local commit commits diff		local commit commits diff

commits=$(build_commit_list $@)		commits=$(build_commit_list "$@")
save_head		save_head
for commit in ${commits}; do		for commit in ${commits}; do
diff=$(commit2diff $commit)		diff=$(commit2diff "$commit")

if ! show_and_prompt $commit; then		if ! show_and_prompt "$commit"; then
break		break
fi		fi

git checkout -q $commit		git checkout -q "$commit"

# The linter is stupid and applies patches to the working copy.		# The linter is stupid and applies patches to the working copy.
# This would be tolerable if it didn't try to correct "misspelled" variable		# This would be tolerable if it didn't try to correct "misspelled" variable
# names.		# names.
arc diff --allow-untracked --never-apply-patches --update $diff HEAD~		arc diff --allow-untracked --never-apply-patches --update "$diff" HEAD~
done		done
restore_head		restore_head
}		}

set -e		set -e

ASSUME_YES=		ASSUME_YES=
if eval $(git config --bool --default false --get arc.assume-yes); then		if [ "$(git config --bool --default false --get arc.assume-yes)" != "false" ]; then
ASSUME_YES=1		ASSUME_YES=1
fi		fi

VERBOSE=		VERBOSE=
while getopts vy o; do		while getopts vy o; do
case "$o" in		case "$o" in
v)		v)
VERBOSE=1		VERBOSE=1
Show All 34 Lines	if [ $# -eq 0 ]; then
err_usage		err_usage
fi		fi

# Pull in some git helper functions.		# Pull in some git helper functions.
git_sh_setup=$(git --exec-path)/git-sh-setup		git_sh_setup=$(git --exec-path)/git-sh-setup
[ -f "$git_sh_setup" ] \|\| err "cannot find git-sh-setup"		[ -f "$git_sh_setup" ] \|\| err "cannot find git-sh-setup"
SUBDIRECTORY_OK=y		SUBDIRECTORY_OK=y
USAGE=		USAGE=
		# shellcheck disable=SC1090
. "$git_sh_setup"		. "$git_sh_setup"

# Bail if the working tree is unclean, except for "list" and "patch"		# Bail if the working tree is unclean, except for "list" and "patch"
# operations.		# operations.
case $verb in		case $verb in
list\|patch)		list\|patch)
;;		;;
*)		*)
require_clean_work_tree $verb		require_clean_work_tree "$verb"
;;		;;
esac		esac

if eval $(git config --bool --default false --get arc.browse); then		if [ "$(git config --bool --default false --get arc.browse)" != "false" ]; then
BROWSE=--browse		BROWSE=--browse
fi		fi

trap restore_head EXIT INT		trap restore_head EXIT INT

gitarc::${verb} $@		gitarc::"${verb}" "$@"

ufs: try to elide the interlock in ufs_itimesClosedPublicActions