On 16/06/10 09:49, Jim Meyering wrote: > Pádraig Brady wrote: > >> On 16/06/10 07:45, Jim Meyering wrote: >>> Paul Eggert wrote: >>>> On 06/15/2010 02:43 PM, Jim Meyering wrote: >>>>> I think that copying physical holes via FIEMAP should be the default, when >>>>> possible. One problem is that the current code on the fiemap-copy branch >>>>> does not honor --sparse=WHEN when in fiemap-copying mode. The solution >>>>> would seem to be to change the regular-file-copying loop in the >>>>> fiemap_copy >>>>> function to use the same hole-preserving code that is used in copy_reg. >>>> >>>> I assume that the solution would be used only with cp --sparse=always? >>>> (Otherwise, it would amount to copying logical holes by default.) >>> >>> Right. >>> I.e., use the same code that honors (or not) the "make_holes" variable. >>> >>>> If so, under this proposal, --sparse=always would copy logical holes, >>>> --sparse=never would never copy holes, and --sparse=auto (the default) >>>> would copy physical holes if supported, else it would copy logical >>>> holes if the file seems to have enough physical holes, and otherwise >>>> it would copy no holes. (Whew!) >>> >>> Right. With --sparse=always and fiemap support, it would take advantage >>> of existing extents to minimize copying time, and for the nontrivial >>> extents, it would detect/induce new holes when possible. >>> >>> Perhaps we need a new logical option that would make a difference >>> only when there are nontrivial fiemap extents: >>> - read nontrivial extents, as is done now >>> - read them, *and* search-for/induce-holes as we do in legacy >>> copy for --sparse=always. >>> >>> Or, putting it another way, perhaps we need a new command line >>> option to control whether we even attempt a fiemap copy. >>> IMHO the default should be to enable it, once all of the >>> underlying bits are deemed to be stable enough. >>> >>> --fiemap >>> --no-fiemap >>> >>> Then, --fiemap --sparse=never would do what the existing fiemap_copy >>> function does, and --fiemap --sparse=always would work once the >>> internal-to-fiemap_copy copying code is adjusted to use the >>> hole-preserving code in copy_reg. >>> >>> Then, to guarantee the legacy --sparse=never behavior, >>> one would have to specify --no-fiemap --sparse=never >> >> I would suggest not to add options like this, >> and only do what's possible without new options >> as it would push too many implementation details >> to the docs/users IMHO. > > Good point. Smaller/simpler would be better. > It'd be great if we can eliminate as never-useful one of the > new combinations. > > Currently on the fiemap-copy branch, once we get a single > successful ioctl, the code will not bother with the usual > hole-detecting/introducing technique implied by --sparse=always. > > Should it do that ever? Always? > Does this need an option?
I don't see the need for new options TBH I see fiemap just as a way to efficiently detect/read holes, and should have no bearing on the destination. cp --sparse=auto (this is currently what cp does by default) recreate the original fiemap holes or resort to existing heuristic if fiemap not available cp --sparse=never write all data, but use fiemap if available to efficiently read cp --sparse=always recreate original holes and perhaps extend add to them for other runs of zero bytes. Without having looked at the code I see this as a little tricky to mix with fiemap. Now since fiemap is only an optimization we can skip it completely for this uncommon case if too tricky (just add a FIXME for now). cheers, Pádraig.
