Repository navigation
mv: keep copying xattrs after one fails in a cross-device move - #15049
Conversation
| copy_xattrs(source, dest) | ||
| }; | ||
| // Every attribute has been tried; report the first one that failed. | ||
| let copy_xattrs_result = copy_xattrs_result |
There was a problem hiding this comment.
this changes cp behavior too, please add a test in tests/by-util/test_cp.rs as well
| /// callers where xattr preservation is best-effort. | ||
| fn without_unsupported( | ||
| result: std::io::Result<Vec<(OsString, std::io::Error)>>, | ||
| ) -> std::io::Result<Vec<(OsString, std::io::Error)>> { |
There was a problem hiding this comment.
std::io::Result<Vec<(OsString, std::io::Error)>> is repeated 6 times, could you please add a type alias?
| if attr_name.as_bytes() == b"security.selinux" { | ||
| continue; | ||
| } | ||
| let result = xattr::get(&source, &attr_name).and_then(|value| match value { |
There was a problem hiding this comment.
this is almost the same loop as in copy_xattrs, could be dedup, no? (e.g. a shared helper taking a filter)
|
Thanks for the review, all three are done in the new commit.
|
Merging this PR will improve performance by 88.26%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Memory | ls_recursive_balanced_tree[(6, 4, 15)] |
1,850 KB | 111.1 KB | ×17 |
| ⚡ | Memory | ls_recursive_long_all_balanced_tree[(6, 4, 15)] |
2,059.1 KB | 320.2 KB | ×6.4 |
| ⚡ | Simulation | three_39_bit_primes |
860 ms | 481.9 ms | +78.45% |
| ⚡ | Memory | ls_recursive_deep_tree[(200, 2)] |
159.5 KB | 125 KB | +27.66% |
| ⚡ | Simulation | five_38_bit_primes |
1.9 s | 1.5 s | +27.6% |
| ⚡ | Memory | ls_recursive_long_all_mixed_tree |
142.3 KB | 118.4 KB | +20.16% |
| ⚡ | Memory | ls_recursive_mixed_tree |
141.6 KB | 117.9 KB | +20.08% |
| ⚡ | Memory | ls_recursive_long_all_deep_tree[(100, 4)] |
133.7 KB | 117 KB | +14.34% |
| ⚡ | Simulation | ls_recursive_mixed_tree |
2.8 ms | 2.7 ms | +4.98% |
| ⚡ | Simulation | ls_recursive_balanced_tree[(6, 4, 15)] |
118.9 ms | 114.6 ms | +3.72% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing Cassian433:mv-keep-copying-xattrs-after-failure (a68a26d) with main (e7c9f31)2
Footnotes
-
117 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
-
No successful run was found on
main(f56fe41) during the generation of this report, so e7c9f31 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩
The xattr copy loops in uucore returned at the first attribute they could not copy, and mv threw that error away. A cross-device move then dropped every attribute after the failing one without a word and exited 0. The copy helpers now try every attribute and return the ones that failed with their errors. mv reports each of them, except ENOTSUP/EOPNOTSUPP, and still completes the move with exit status 0, as GNU does. cp goes through the same helpers, so it now copies the remaining attributes too and still reports the first failure as before. Closes uutils#14598
cd0e604 to
a68a26d
Compare
|
GNU testsuite comparison: |
2 participants
Closes #14598
On a cross-device move,
copy_xattrs,copy_xattrs_fdandcopy_xattrs_skip_selinuxreturned at the first attribute they could not set, and mv dropped that error withlet _ =. Every attribute listed after the failing one was lost, with no message and exit status 0. Moving a file from tmpfs to ext4 with one 8000-byteuser.*value is enough to hit it.The helpers now try every attribute and return the ones that failed along with their errors. mv prints
setting attribute 'NAME': ERRORfor each one, except ENOTSUP/EOPNOTSUPP, and still finishes the move with exit 0. That is what the GNU manual describes ("Upon failure all but 'Operation not supported' warnings are output"), and GNU 9.4 does the same here, apart from printing the name twice. cp uses the same helpers, socp --preserve=xattrnow copies the remaining attributes too and still reports the first failure as before.The new test moves a file, and a directory containing a file, from /dev/shm into the test directory. Each file has five attributes, two of them too large for ext4. The names are interleaved so that a failing attribute is listed before one that must be kept, whichever way tmpfs sorts them. The test checks that both failures are reported, that the move succeeds and that the three small attributes arrive. It skips when the destination accepts large values. It fails on main.
The directory's own xattrs go through
apply_xattrs_fd, which #14971 is reworking, so I left that path alone.