D17737: [CopyJob] Create clones in btrf/xfs mount

[Oswald Buddenhagen]

ossi added a comment.


  In D17737#517736 , @thiago wrote:
  
  > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17737

To: chinmoyr, dfaure, #frameworks, thiago, ossi
Cc: ngraham, bruns, kde-frameworks-devel, LeGast00n, GB_2, michaelh

D17737: [CopyJob] Create clones in btrf/xfs mount

[Thiago Macieira]

thiago added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17737

To: chinmoyr, dfaure, #frameworks, thiago, ossi
Cc: ngraham, bruns, kde-frameworks-devel, LeGast00n, GB_2, michaelh

D17737: [CopyJob] Create clones in btrf/xfs mount

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> file_unix.cpp:263
> +clone_created = ioctl(dest_file.handle(), FICLONE, 
> src_file.handle()) != -1;
> +//qDebug() << "Clone created:" << clone_created;"
> +#endif

nitpick - wrong indentation

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17737

To: chinmoyr, dfaure, #frameworks, thiago, ossi
Cc: ngraham, bruns, kde-frameworks-devel, LeGast00n, GB_2, michaelh

D17737: [CopyJob] Create clones in btrf/xfs mount

[Chinmoy Ranjan Pradhan]

chinmoyr edited reviewers, added: thiago, ossi; removed: davidedmundson.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17737

To: chinmoyr, dfaure, #frameworks, thiago, ossi, davidedmundson
Cc: ngraham, bruns, kde-frameworks-devel, LeGast00n, GB_2, michaelh

D17737: [CopyJob] Create clones in btrf/xfs mount

[David Faure]

dfaure added a comment.


  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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17737

To: chinmoyr, dfaure, davidedmundson, #frameworks
Cc: ngraham, bruns, kde-frameworks-devel, michaelh

D17737: [CopyJob] Create clones in btrf/xfs mount

[Stefan Brüns]

bruns added a comment.


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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17737

To: chinmoyr, dfaure, davidedmundson, #frameworks
Cc: ngraham, bruns, kde-frameworks-devel, michaelh

D17737: [CopyJob] Create clones in btrf/xfs mount

[Stefan Brüns]

bruns added a comment.


  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).

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17737

To: chinmoyr, dfaure, davidedmundson, #frameworks
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17737: [CopyJob] Create clones in btrf/xfs mount

[Stefan Brüns]

bruns added a reviewer: Frameworks.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17737

To: chinmoyr, dfaure, davidedmundson, #frameworks
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17737: [CopyJob] Create clones in btrf/xfs mount

[Chinmoy Ranjan Pradhan]

chinmoyr added a comment.


  @bruns  ping

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17737

To: chinmoyr, dfaure, davidedmundson
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17737: [CopyJob] Create clones in btrf/xfs mount

[Chinmoy Ranjan Pradhan]

chinmoyr updated this revision to Diff 48847.
chinmoyr added a comment.


  Scraped everything but the changes from file_unix.cpp

REPOSITORY
  R241 KIO

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D17737?vs=48014=48847

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17737

AFFECTED FILES
  src/ioslaves/file/ConfigureChecks.cmake
  src/ioslaves/file/config-kioslave-file.h.cmake
  src/ioslaves/file/file_unix.cpp

To: chinmoyr, dfaure, davidedmundson
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17737: [CopyJob] Create clones in btrf/xfs mount

[Stefan Brüns]

bruns added a comment.


  In D17737#381354 , @chinmoyr wrote:
  
  > In D17737#381029 , @bruns wrote:
  >
  > > 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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17737

To: chinmoyr, dfaure, davidedmundson
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17737: [CopyJob] Create clones in btrf/xfs mount

[Chinmoy Ranjan Pradhan]

chinmoyr added a comment.


  In D17737#381029 , @bruns wrote:
  
  > 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?

INLINE COMMENTS

> bruns wrote in copyjob.cpp:742
> it is *much* cheaper to compare the st_dev fields from stat for the source 
> file and the destination directory.

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?

> bruns wrote in kmountpoint.cpp:471
> 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.

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?

> bruns wrote in kmountpoint.h:145
> The correct term here is "reflink", not CoW.

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.

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17737

To: chinmoyr, dfaure, davidedmundson
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17737: [CopyJob] Create clones in btrf/xfs mount

[Stefan Brüns]

bruns added a comment.


  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

INLINE COMMENTS

> copyjob.cpp:742
> +if (!isDir && info.linkDest.isEmpty() && m_mode == CopyJob::Copy && 
> m_bDestSupportsCoW) {
> +const QString srcDevice = 
> KMountPoint::currentMountPoints().findByPath(info.uSource.toLocalFile())->mountedFrom();
> +info.bCreateClone = (srcDevice == m_destDevice);

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

> kmountpoint.cpp:471
>  || d->mountType == QLatin1String("smb-share");
> +const bool isCoWSupported = d->mountType == QLatin1String("btrfs") || 
> d->mountType == QLatin1String("xfs");
>  

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.

> kmountpoint.h:145
>   * "foo" and "FOO" as being the same file (true for msdos systems)
> + * @li SupportsCoW: returns true if the filesystem supports Copy-on-Write
> + * (true for btrfs, xfs, etc.)

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

REPOSITORY
  R241 KIO

REVISION DETAIL
  https://phabricator.kde.org/D17737

To: chinmoyr, dfaure, davidedmundson
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D17737: [CopyJob] Create clones in btrf/xfs mount

[Chinmoy Ranjan Pradhan]

chinmoyr created this revision.
chinmoyr added reviewers: dfaure, davidedmundson.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
chinmoyr requested review of this revision.

REVISION 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.

REPOSITORY
  R241 KIO

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D17737

AFFECTED FILES
  src/core/copyjob.cpp
  src/core/copyjob.h
  src/core/kmountpoint.cpp
  src/core/kmountpoint.h
  src/ioslaves/file/file_unix.cpp

To: chinmoyr, dfaure, davidedmundson
Cc: kde-frameworks-devel, michaelh, ngraham, bruns