Skip to content

CA-395093: sync between tapback slave process and xenopsd #6825

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
chunjiez wants to merge 1 commit into
base: master
Choose a base branch
from
Open

CA-395093: sync between tapback slave process and xenopsd #6825

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions ocaml/xenopsd/scripts/block
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,25 @@ syslog ()
logger -tscripts-block "$*"
}

wait_tapback_ready()
{
local statefile="/var/run/tapback.${DOMID}.statefile"
while true; do
Copy link
Contributor

@BengangY BengangY Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible the while never exits if statefile fails to be created? Consider to add a timeout for max number of retries.

Copy link
Contributor

@lindig lindig Jan 9, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A simple way would be:

seq 120 | while read i; do
  ...
  sleep 1
done

This would iterate at most 120 times or (2min)

Copy link
Contributor

@MarkSymsCtx MarkSymsCtx Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already commented on the related PR to tapdisk that I don't think this is the correct approach and it just adds even more complexity and fargility to the system which will induce even more cost of maintenance going forward.

Copy link
Contributor

@lindig lindig Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MarkSymsCtx could you link to that PR. What is a generally better approach?

Copy link
Contributor

@BengangY BengangY Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

xapi-project/blktap#435 @lindig

Copy link
Contributor

@lindig lindig Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems correct to me that this side needs to be sure that tapback is ready as otherwise tapback won't be able to process events. So checking for readiness for a limited time and failing if tapback is not ready seems to me not fragile and I would be curious how it could be avoided @MarkSymsCtx.

Copy link
Contributor

@lindig lindig Jan 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we generally expect tapback to be available and ready, we should only wait briefly before we fail. I agree with @BengangY that we should not wait indefinitely.

if [[ -f "$statefile" ]]; then
content=$(cat "$statefile")
if [[ "$content" == *"ping"* ]]; then
syslog "${XENBUS_PATH}: tapback ready"
break
fi
fi
syslog "${XENBUS_PATH}: tapback not ready"
sleep 1
done
syslog "${XENBUS_PATH}: signal tapback to continue"
echo -n "pong" > "$statefile.tmp"
mv "$statefile.tmp" "$statefile"
}

case "$1" in
add)
if [ "$XENBUS_PREFIX" = "" ]; then
Expand All @@ -27,6 +46,8 @@ add)
if [ "$TYPE" != "9pfs" ]; then
if [[ $params =~ nbd:unix:([^:]*) ]]
then
# tapback slave process should be ready to process xenstore watch event
wait_tapback_ready
physical_device_path=$(readlink -f ${BASH_REMATCH[1]})
syslog "${XENBUS_PATH}: physical-device-path=${physical_device_path}"
xenstore-exists "${XENBUS_PATH}/physical-device-path"
Expand Down
Loading