On 9/11/23 09:07, Nitesh Shetty wrote:
On Fri, Sep 08, 2023 at 08:13:37AM +0200, Hannes Reinecke wrote:
On 9/6/23 18:38, Nitesh Shetty wrote:
Before enabling copy for dm target, check if underlying devices and
dm target support copy. Avoid split happening inside dm target.
Fail early if the request needs split, currently splitting copy
request is not supported.
And here is where I would have expected the emulation to take place;
didn't you have it in one of the earlier iterations?
No, but it was the other way round.
In dm-kcopyd we used device offload, if that was possible, before using default
dm-mapper copy. It was dropped in the current series,
to streamline the patches and make the series easier to review.
After all, device-mapper already has the infrastructure for copying
data between devices, so adding a copy-offload emulation for device-mapper
should be trivial.
I did not understand this, can you please elaborate ?
Please see my comments to patch 04.
We should only implement copy-offload if there is a dedicated
infrastructure in place. But we should not have a 'generic' copy-offload
emulation.
Problem is that 'real' copy-offload functionalities (ie for NVMe or
SCSI) are riddled with corner-cases where copy-offload does _not_ work,
and where commands might fail if particular conditions are not met.
Falling back to a generic implementation will cause applications to
assume that copy-offload worked, and that it gained performance as
the application just had to issue a single command.
Whereas in fact the opposite is true; it wasn't a single command, and
the application might have performed better by issuing the commands
itself.
Returning -EOPNOTSUPP in these cases will inform the application that
the attempt didn't work, and that it will have to fall back to the
'normal' copy.
Cheers,
Hannes
--
Dr. Hannes Reinecke Kernel Storage Architect
hare@xxxxxxx +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman
