On Tue, 1 Feb 2022, Bart Van Assche wrote:
> On 2/1/22 10:32, Mikulas Patocka wrote:
> > /**
> > + * blk_queue_max_copy_sectors - set maximum copy offload sectors for the
> > queue
> > + * @q: the request queue for the device
> > + * @size: the maximum copy offload sectors
> > + */
> > +void blk_queue_max_copy_sectors(struct request_queue *q, unsigned int size)
> > +{
> > + q->limits.max_copy_sectors = size;
> > +}
> > +EXPORT_SYMBOL_GPL(blk_queue_max_copy_sectors);
>
> Please either change the unit of 'size' into bytes or change its type into
> sector_t.
blk_queue_chunk_sectors, blk_queue_max_discard_sectors,
blk_queue_max_write_same_sectors, blk_queue_max_write_zeroes_sectors,
blk_queue_max_zone_append_sectors also have the unit of sectors and the
argument is "unsigned int". Should blk_queue_max_copy_sectors be
different?
> > +extern int blkdev_issue_copy(struct block_device *bdev1, sector_t sector1,
> > + struct block_device *bdev2, sector_t sector2,
> > + sector_t nr_sects, sector_t *copied, gfp_t gfp_mask);
> > +
>
> Only supporting copying between contiguous LBA ranges seems restrictive to me.
> I expect garbage collection by filesystems for UFS devices to perform better
> if multiple LBA ranges are submitted as a single SCSI XCOPY command.
NVMe has a possibility to copy multiple source ranges into one destination
range. But I think that leveraging this capability would just make the
code excessively complex.
> A general comment about the approach: encoding the LBA range information in a
> bio payload is not compatible with bio splitting. How can the dm driver
> implement copy offloading without the ability to split copy offload bio's?
I don't expect the copy bios to be split.
One possibility is to just return -EOPNOTSUPP and fall back to explicit
copy if the bio crosses dm target boundary (that's what my previous patch
for SCSI XCOPY did).
Another possibility is to return the split boundary in the token and retry
both bios will smaller length. But this approach would prevent us from
submitting the REQ_OP_COPY_WRITE_TOKEN bios asynchronously.
I'm not sure which of these two approaches is better.
> > +int blkdev_issue_copy(struct block_device *bdev1, sector_t sector1,
> > + struct block_device *bdev2, sector_t sector2,
> > + sector_t nr_sects, sector_t *copied, gfp_t gfp_mask)
> > +{
> > + struct page *token;
> > + sector_t m;
> > + int r = 0;
> > + struct completion comp;
>
> Consider using DECLARE_COMPLETION_ONSTACK() instead of a separate declaration
> and init_completion() call.
OK.
> Thanks,
>
> Bart.
Mikulas
