[CopyJob] Create clones in btrf/xfs mount
Needs ReviewPublic

Authored by chinmoyr on Dec 22 2018, 4:16 PM.

Details

Reviewers
dfaure
thiago
ossi
Group Reviewers
Frameworks
Summary

If Copy-on-Write is enabled in a btrfs/xfs mount then create a clone otherwise
fallback to standard copy.

Test Plan

Manually copied files and verified with "btrfs filesystem du" command.
All existing unit tests pass.

Diff Detail

Repository
R241 KIO
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 6731
Build 6749: arc lint + arc unit
chinmoyr created this revision.Dec 22 2018, 4:16 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptDec 22 2018, 4:16 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
chinmoyr requested review of this revision.Dec 22 2018, 4:16 PM
bruns added a subscriber: bruns.Dec 23 2018, 12:38 AM

I think it would be much simpler if you just tried to do the the FICLONE iotctl in the job, without any prior checking:

  • no possibility for races
  • less syscals
  • less code
src/core/copyjob.cpp
742 ↗(On Diff #48014)

it is *much* cheaper to compare the st_dev fields from stat for the source file and the destination directory.

src/core/kmountpoint.cpp
471 ↗(On Diff #48014)

This is not correct, xfs may support reflinks, but it is a feature only recently added, and has to be enabled at file system creation time.

src/core/kmountpoint.h
145 ↗(On Diff #48014)

The correct term here is "reflink", not CoW.

I think it would be much simpler if you just tried to do the the FICLONE iotctl in the job, without any prior checking:

  • no possibility for races
  • less syscals
  • less code

I am not sure I followed you here. To me ioctl seems to naturally fit in FileProtocol::copy(). Can I ask you to elaborate a bit?

src/core/copyjob.cpp
742 ↗(On Diff #48014)

I agree but in case of KIO::copy() the destination can be a non-existent file resulting in lstat to fail. However, I did considered using st_dev for source but couldn't find a way to map it to a block device. Maybe we can use st_dev by default and fallback to KMountPoint upon encountering this particular case?

src/core/kmountpoint.cpp
471 ↗(On Diff #48014)

Here by 'support' I meant support in its implementation. Meaning aside, unlike btrfs' "nodatacow" option which we can get using KMountPoint I couldn't find any API to get the "reflink" option set during file system creation time. How do you suggest I proceed from here?

src/core/kmountpoint.h
145 ↗(On Diff #48014)

Some documents I read on btrfs and xfs described copy-on-write as a 'feature'. So I felt the enum should contain 'Supports' followed by the feature name.
But reflink or clone fits here better. I will make the change in next update.

I think it would be much simpler if you just tried to do the the FICLONE iotctl in the job, without any prior checking:

  • no possibility for races
  • less syscals
  • less code

I am not sure I followed you here. To me ioctl seems to naturally fit in FileProtocol::copy(). Can I ask you to elaborate a bit?

I meant scrap everything but the changes from file_unix.cpp.

Worst case - the ioctl fails (for whatever reason). That is one additional ioctl/syscall. The probing code using KMountPoint::List::findByPath internally creates a QFileInfo, thus does a stat (syscall) as well.

Best case - the ioctl succeeds. You save the syscall required for the probing.

chinmoyr updated this revision to Diff 48847.Jan 7 2019, 7:53 AM

Scraped everything but the changes from file_unix.cpp

Can you update the summary - although CoW and cloning typically come together, they are two different functions (and you can e.g. have CoW disabled for a dir/file, and cloning still works).

bruns added a subscriber: ngraham.Feb 24 2019, 10:01 PM

The code looks sane to me, but I have not found time to test it.

No objection from me, but I don't know those clone APIs, so I can't really review/approve it.

Maybe Thiago or Oswald, if nobody else does.

chinmoyr edited reviewers, added: thiago, ossi; removed: davidedmundson.Fri, Aug 23, 8:17 AM
bruns added inline comments.Fri, Aug 23, 6:23 PM
src/ioslaves/file/file_unix.cpp
263

nitpick - wrong indentation

QFile::copy already implements FICLONE, sendfile() and Darwin's clonefile().

ossi added a comment.Sat, Aug 24, 4:02 AM

QFile::copy already implements FICLONE, sendfile() and Darwin's clonefile().

yes, and an api that is utterly unsuitable for interactive applications. ;)

if you just meant it as a source to steal from, fair enough.