k0sproject / k0sproject/rig

PosixFile.fsBlockSize asks stat for %s, making dd transfers to XFS hosts crawl

Open
#466 0 comments 1 reaction 0 assignees View on GitHub
Dominant language
Go
Stars
53
Forks
33
Avg merge
1d 9h
Merged PRs (30d)
29

Description

## What happens

Uploading a file to a host whose target directory is on XFS runs dd with a
block size of a few dozen bytes. A k0sctl run uploading a k0s airgap image
bundle into `/var/lib/k0s/images` produced:

```
INFO ==> Running phase: Upload files to hosts
INFO [ssh] host:22: uploading bundle_file.tar
```

with the host command:

```
sudo -- dd if=/dev/stdin of=/var/lib/k0s/images/bundle_file.tar bs=29 seek=0 conv=notrunc
```

`bs=29`, so dd copies a multi-hundred-megabyte bundle 29 bytes at a time —
roughly 18 million read/write syscall pairs for a 500 MB bundle. The upload
takes hours instead of seconds.

## Why

`PosixFile.fsBlockSize` in `remotefs/posixfile.go` asks stat for `%s`:

```go
out, err := f.fs.ExecOutput(fmt.Sprintf(
`stat -c "%%s" %[1]s 2> /dev/null || stat -f "%%k" %[1]s`,
shellescape.Quote(path.Dir(f.path))))
```

`%s` is total size in bytes. The intent was `%o`, the optimal I/O block size —
which is exactly what the BSD `%k` in the same fallback expression means.

It stats the *parent directory*, and a directory's size in bytes is
filesystem-specific:

- **ext4** stores directory data in whole blocks, so `stat -c %s` returns 4096
and coincidentally equals `%o`. The bug is invisible.
- **XFS** stores a small directory inline in the inode, so the size is the size
of that inline data.

The 29 is the XFS shortform directory layout for a directory holding exactly
one entry, which is what rig itself has just created:

| part | bytes |
| --- | --- |
| header: `count` + `i8count` + parent inode | 1 + 1 + 4 = 6 |
| entry: `namelen` + `offset` + `name` + `ftype` + inode | 1 + 2 + 15 + 1 + 4 = 23 |
| **total** | **29** |

`bundle_file.tar` is 15 characters.

## Reproducing

On any host with an XFS `/var/lib`:

```
$ stat -c "%s %o" /var/lib/k0s/images
29 4096
```

The first number is what rig uses; the second is what it meant. On ext4 both
read 4096, which is why this has not come up before.

## Also

Two things next to it in the same code path:

- If stat exits 0 but prints something that does not parse as an integer,
`fsBlockSize` leaves `blockSize` at 0 and `ddParams` panics with an integer
divide by zero (`numBytes % 0`).
- `CopyFrom` passes `f.pos` to dd's `seek=`, which dd counts in output blocks
rather than bytes, so a resumed copy writes at `f.pos * bs`. Harmless at
offset 0, which is the ordinary upload.

## Versions

`fsBlockSize` is unchanged from v2.0.0 through v2.2.0 and `main` (a69b40a).
The report above comes from a k0sctl whose bundled rig still passes
`if=/dev/stdin`, so it predates #425, but the defect is the same on current
`main`.

Contributor guide

No contributing guide indexed for this repository

Research direction

Start in remotefs/posixfile.go by reading fsBlockSize and its callers, especially ddParams and CopyFrom. Reproduce the differing stat values on XFS and ext4, then verify the transfer block size, malformed stat output, and resumed-copy offset behavior. Done means uploads no longer use the directory's byte size and the reported edge cases do not cause incorrect transfers or panics.

Written by the indexing model from the issue text.

Assessment

Tech stack
go
Domain
backend, networking
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Active
Clarity
Clearly specified
Newbie friendliness
74/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.