On Mon, Dec 4, 2023 at 4:07 PM Christoph Hellwig <hch@xxxxxx> wrote: > > On Mon, Dec 04, 2023 at 03:29:43PM +0200, Amir Goldstein wrote: > > > > Shouldn't ceph be switched to use generic_copy_file_range? > > > > That does the capping of the size which we want, and doesn't update > > > > the file offsets, which would require recalculation in the ceph code. > > > > > > > > IDK. I did not want to change the logic of the ceph code. > > I am not sure that we must impose MAX_RW_COUNT limit on ceph, > > although, i_layout.object_size may already be limited? Jeff? > > We better don't go beyond it, as it is called from the copy_file_range > implementation which is expected to never return more than MAX_RW_COUNT. > So either it is a noop change, or it fixes a bug. > ok. > > > > > > But this could avoid another exported API as splice_file_range could > > > > simply be folded into generic_copy_file_range which should reduce > > > > confusion. And splice really is a mess for so many different layers > > > > of the onion being exposed. I've been wanting to reduce some of that > > > > for a while but haven't found a really nice way yet. > > > > > > (and generic_copy_file_range really should be renamed to > > > splice_copy_file_range and moved to splice.c) > > > > That depends if we are keeping two helpers. > > One with a cap of MAX_RW_COUNT and one without. > > If we are going to keep two helpers, I'd rather keep things as they are. > > If one helper, then I personally prefer splice_file_range() over > > splice_copy_file_range() and other reviewers (Jan) liked this > > name as well. > > Well, splice_file_range makes sense if it is a separate helper. But when > is the default implementation for ->copy_file_range and matches the > signature, naming it that way is not only sensible but required to keep > sanity. > It is NOT a default implementation of ->copy_file_range(), but a fallback helper. Specifically, it is never expected to have a filesystem that does .copy_file_range = generic_copy_file_range, so getting rid of generic_copy_file_range() would be good. Note also that generic_copy_file_range() gets a flags argument that is COPY_FILE_* flags (currently only for the vfs level) and this flags argument is NOT the splice flags argument, so I intentionally removed the flags argument from splice_file_range() to reduce confusion. I like the idea of moving MAX_RW_COUNT into splice_file_range() and replacing generic_copy_file_range() calls with splice_file_range(). I do not feel strongly against splice_copy_file_range() name, but I would like to get feedback from other reviewers that approved the name splice_file_range() before changing it. Thanks, Amir.
