[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20230628163958.tgwtlszadsa7zoub@green245>
Date: Wed, 28 Jun 2023 22:09:58 +0530
From: Nitesh Shetty <nj.shetty@...sung.com>
To: Damien Le Moal <dlemoal@...nel.org>
Cc: Jens Axboe <axboe@...nel.dk>, Jonathan Corbet <corbet@....net>,
Alasdair Kergon <agk@...hat.com>,
Mike Snitzer <snitzer@...nel.org>, dm-devel@...hat.com,
Keith Busch <kbusch@...nel.org>,
Christoph Hellwig <hch@....de>,
Sagi Grimberg <sagi@...mberg.me>,
Chaitanya Kulkarni <kch@...dia.com>,
Alexander Viro <viro@...iv.linux.org.uk>,
Christian Brauner <brauner@...nel.org>,
martin.petersen@...cle.com, linux-scsi@...r.kernel.org,
willy@...radead.org, hare@...e.de, djwong@...nel.org,
bvanassche@....org, ming.lei@...hat.com, nitheshshetty@...il.com,
gost.dev@...sung.com, Anuj Gupta <anuj20.g@...sung.com>,
linux-block@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org, linux-nvme@...ts.infradead.org,
linux-fsdevel@...r.kernel.org
Subject: Re: [PATCH v13 4/9] fs, block: copy_file_range for def_blk_ops for
direct block device
On 23/06/28 03:51PM, Damien Le Moal wrote:
>On 6/28/23 03:36, Nitesh Shetty wrote:
>> For direct block device opened with O_DIRECT, use copy_file_range to
>> issue device copy offload, and fallback to generic_copy_file_range incase
>> device copy offload capability is absent.
>
>...if the device does not support copy offload or the device files are not open
>with O_DIRECT.
>
>No ?
>
Yes your right. We will fallback to generic_copy_file_range in either of
these cases.
>> Modify checks to allow bdevs to use copy_file_range.
>>
>> Suggested-by: Ming Lei <ming.lei@...hat.com>
>> Signed-off-by: Anuj Gupta <anuj20.g@...sung.com>
>> Signed-off-by: Nitesh Shetty <nj.shetty@...sung.com>
>> ---
>> block/blk-lib.c | 26 ++++++++++++++++++++++++++
>> block/fops.c | 20 ++++++++++++++++++++
>> fs/read_write.c | 7 +++++--
>> include/linux/blkdev.h | 4 ++++
>> 4 files changed, 55 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-lib.c b/block/blk-lib.c
>> index 09e0d5d51d03..7d8e09a99254 100644
>> --- a/block/blk-lib.c
>> +++ b/block/blk-lib.c
>> @@ -473,6 +473,32 @@ ssize_t blkdev_copy_offload(
>> }
>> EXPORT_SYMBOL_GPL(blkdev_copy_offload);
>>
>> +/* Copy source offset from source block device to destination block
>> + * device. Returns the length of bytes copied.
>> + */
>
>Multi-line comment style: start with a "/*" line please.
>
acked
>> +ssize_t blkdev_copy_offload_failfast(
>
>What is the "failfast" in the name for ?
We dont want failed copy offload IOs to fallback to block layer copy emulation.
We wanted a API to return error, if offload fails.
>
>> + struct block_device *bdev_in, loff_t pos_in,
>> + struct block_device *bdev_out, loff_t pos_out,
>> + size_t len, gfp_t gfp_mask)
>> +{
>> + struct request_queue *in_q = bdev_get_queue(bdev_in);
>> + struct request_queue *out_q = bdev_get_queue(bdev_out);
>> + ssize_t ret = 0;
>
>You do not need this initialization.
>
we need this initialization, because __blkdev_copy_offload return number of
bytes copied or error value.
So we can not return 0, incase of success/partial completion.
blkdev_copy_offload_failfast is expected to return number of bytes copied.
>> +
>> + if (blkdev_copy_sanity_check(bdev_in, pos_in, bdev_out, pos_out, len))
>> + return 0;
>> +
>> + if (blk_queue_copy(in_q) && blk_queue_copy(out_q)) {
>
>Given that I think we do not allow copies between different devices, in_q and
>out_q should always be the same, no ?
acked, will update this.
>
>> + ret = __blkdev_copy_offload(bdev_in, pos_in, bdev_out, pos_out,
>> + len, NULL, NULL, gfp_mask);
>
>Same here. Why pass 2 bdevs if we only allow copies within the same device ?
>
acked, will update function arguments to take single bdev.
>> + if (ret < 0)
>> + return 0;
>> + }
>> +
>> + return ret;
>
>return 0;
>
Nack, explained above.
Thank you,
Nitesh Shetty
Powered by blists - more mailing lists