On Thu, Jan 23, 2025 at 12:10 PM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > after the --- line, but with my suggestion below, you should probably split > this to one vfs patch to change copy_file_range() interface and another > patch to implement ovl_copy_file_range() support. ack. > > I thought this could speed a lot of operations in docker/podman. > > What sort of operations are we talking about? > The only relevant use case as far as I can tell is copying > from a shared lower fs directly into overlayfs, but why is this useful? > to which scenario? > Do you have an example of use cases for copy to another direction? My use-case is the snapshot/restore command, which copies changed files from the overlay mount, and stores them into a tar file on the podman storage directory (which is on the same FS as the lowerdir and upperdir), Restore does the process in the opposite direction. Since the tar file lives on the same FS as the upperdir, the copy could use reflinks. I prototyped this at https://github.com/containers/podman/commit/dd54c885cbe6f4d08554c886389546f215af1817 (timings in the commit message), but it hardcodes a path substitution. It would be much nicer if copy_file_range() just worked. Also, copying files into and out of containers (podman container cp) would also be sped up. I have just become aware that podman/docker also supports btrfs-based storage (iso. overlayfs). If I were to use that, I suppose I don't need this at all. > > I don't really know what I'm doing, but it seems to work. I tested this > > manually using qemu; is there an official test suite where I could add > > a test? > > https://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git > See: README.overlay Awesome. I knew this existed, but didn't realize it went beyond testing XFS. > For testing your change, you will need a tests/overlay specific copy_range test. > Let's talk about that if that becomes relevant. ack. > > + bool in_overlay = file_in->f_op->copy_file_range == &ovl_copy_file_range; > > + bool out_overlay = file_out->f_op->copy_file_range == &ovl_copy_file_range; > > I prefer if we did not add the support for copying out of overlayfs > unless there is a *very* good reason to do so and then The case of copying into overlayfs is the one I'm more interested in, as we're trying to reduce app startup time by restoring a container snapshot. This involves copying FS diffs into the container. > Note that cross-sb copy_file_range() supported by cifs_copy_file_range() > and nfs4_copy_file_range() is quite close to what you are trying to do here > I will propose below a way to unify those cases. OK. I will look into this next week. > I prefer not to do that unless there is a *very* good reason. > copy from another sb source is less risky IMO. You said this up above, but could you say a little bit more about why? Are you concerned that code that will dereference the wrong type of per-FS data might still survive long enough to write garbage data to stable storage? > Thanks, > Amir. -- Han-Wen Nienhuys | hanwen@xxxxxxxxxxx | EngFlow GmbH Fischerweg 51, 82194 Gröbenzell, Germany Amtsgericht München, HRB 255664 Geschäftsführer (Managing Director): Ulf Adams https://www.engflow.com/
