Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
IronPort-SDR: GDIp98SwfKumKXNxqq5OzwBzt/FJxQpR05kNgoXfdxDjq+vS8KYsFSCP+9js0DRU209HOjX4y7
 tgIS1d2xVQGuA6zekO9HqAdqEvp32pWP5jMOa9aBCCYyvXdRh6+mrZeGv9PzhpGegMb0n4vHol
 ulhrppYhtiQ2i4KIBXtJfWN/kV+H0XYcJGszMaZn9UivRSOkGCV2i1Gmj0F19zfjtXcJxuXRaR
 m9LCajiTIAZm8s5zmxp6JIlYIuuLMMVAJTsMwpudoVnptsUPNy9uwYoV4m16Z5PfCagzuzO3ao
 q8s=
From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     Selva Jove <selvajove@gmail.com>
CC:     SelvaKumar S <selvakuma.s1@samsung.com>,
        "linux-nvme@lists.infradead.org" <linux-nvme@lists.infradead.org>,
        "kbusch@kernel.org" <kbusch@kernel.org>,
        "axboe@kernel.dk" <axboe@kernel.dk>, "hch@lst.de" <hch@lst.de>,
        "sagi@grimberg.me" <sagi@grimberg.me>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "dm-devel@redhat.com" <dm-devel@redhat.com>,
        "snitzer@redhat.com" <snitzer@redhat.com>,
        "joshiiitr@gmail.com" <joshiiitr@gmail.com>,
        "nj.shetty@samsung.com" <nj.shetty@samsung.com>,
        "joshi.k@samsung.com" <joshi.k@samsung.com>,
        "javier.gonz@samsung.com" <javier.gonz@samsung.com>,
        "kch@kernel.org" <kch@kernel.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>
Subject: Re: [RFC PATCH v5 2/4] block: add simple copy support
Thread-Topic: [RFC PATCH v5 2/4] block: add simple copy support
Thread-Index: AQHXByxWhY0q9dGq/EW9B8B+rRJ+Hw==
Date:   Tue, 13 Apr 2021 00:32:14 +0000
Message-ID: <BL0PR04MB65146169A9C7527280C15D4AE74F9@BL0PR04MB6514.namprd04.prod.outlook.com>
References: <20210219124517.79359-1-selvakuma.s1@samsung.com>
 <CGME20210219124603epcas5p33add0f2c1781b2a4d71bf30c9e1ac647@epcas5p3.samsung.com>
 <20210219124517.79359-3-selvakuma.s1@samsung.com>
 <BL0PR04MB6514EA91680D33A5890ECE16E7839@BL0PR04MB6514.namprd04.prod.outlook.com>
 <CAHqX9vb6GgaU9QdTQaq3=dGuoNCuWorLrGCF8gC5LEdFBESFcA@mail.gmail.com>
 <BL0PR04MB6514B34000467FABFD6BF385E7709@BL0PR04MB6514.namprd04.prod.outlook.com>
 <CAHqX9vYvtOaVL4LG0gAGCMz+a8uha8czH==Dgg3eG+TWA+xeVQ@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 86b8edd7-995f-47f7-1b22-08d8fe1395e7
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Apr 2021 00:32:14.4505
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: q45jY7heLab0BUCytM6USTe3hTqEd8yxrYCHY9+xsAtRp+etxiabX4DsjXyIq4Dk4FtwiNDlMFa8ed9fxwucjQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN2PR04MB6416
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2021/04/12 23:35, Selva Jove wrote:=0A=
> On Mon, Apr 12, 2021 at 5:55 AM Damien Le Moal <Damien.LeMoal@wdc.com> wr=
ote:=0A=
>>=0A=
>> On 2021/04/07 20:33, Selva Jove wrote:=0A=
>>> Initially I started moving the dm-kcopyd interface to the block layer=
=0A=
>>> as a generic interface.=0A=
>>> Once I dig deeper in dm-kcopyd code, I figured that dm-kcopyd is=0A=
>>> tightly coupled with dm_io()=0A=
>>>=0A=
>>> To move dm-kcopyd to block layer, it would also require dm_io code to=
=0A=
>>> be moved to block layer.=0A=
>>> It would cause havoc in dm layer, as it is the backbone of the=0A=
>>> dm-layer and needs complete=0A=
>>> rewriting of dm-layer. Do you see any other way of doing this without=
=0A=
>>> having to move dm_io code=0A=
>>> or to have redundant code ?=0A=
>>=0A=
>> Right. Missed that. So reusing dm-kcopyd and making it a common interfac=
e will=0A=
>> take some more efforts. OK, then. For the first round of commits, let's =
forget=0A=
>> about this. But I still think that your emulation could be a lot better =
than a=0A=
>> loop doing blocking writes after blocking reads.=0A=
>>=0A=
> =0A=
> Current implementation issues read asynchronously and once all the reads =
are=0A=
> completed, then the write is issued as whole to reduce the IO traffic=0A=
> in the queue.=0A=
> I agree that things can be better. Will explore another approach of=0A=
> sending writes=0A=
> immediately once reads are completed and with  plugging to increase the c=
hances=0A=
> of merging.=0A=
> =0A=
>> [...]=0A=
>>>>> +int blkdev_issue_copy(struct block_device *src_bdev, int nr_srcs,=0A=
>>>>> +             struct range_entry *src_rlist, struct block_device *des=
t_bdev,=0A=
>>>>> +             sector_t dest, gfp_t gfp_mask, int flags)=0A=
>>>>> +{=0A=
>>>>> +     struct request_queue *q =3D bdev_get_queue(src_bdev);=0A=
>>>>> +     struct request_queue *dest_q =3D bdev_get_queue(dest_bdev);=0A=
>>>>> +     struct blk_copy_payload *payload;=0A=
>>>>> +     sector_t bs_mask, copy_size;=0A=
>>>>> +     int ret;=0A=
>>>>> +=0A=
>>>>> +     ret =3D blk_prepare_payload(src_bdev, nr_srcs, src_rlist, gfp_m=
ask,=0A=
>>>>> +                     &payload, &copy_size);=0A=
>>>>> +     if (ret)=0A=
>>>>> +             return ret;=0A=
>>>>> +=0A=
>>>>> +     bs_mask =3D (bdev_logical_block_size(dest_bdev) >> 9) - 1;=0A=
>>>>> +     if (dest & bs_mask) {=0A=
>>>>> +             return -EINVAL;=0A=
>>>>> +             goto out;=0A=
>>>>> +     }=0A=
>>>>> +=0A=
>>>>> +     if (q =3D=3D dest_q && q->limits.copy_offload) {=0A=
>>>>> +             ret =3D blk_copy_offload(src_bdev, payload, dest, gfp_m=
ask);=0A=
>>>>> +             if (ret)=0A=
>>>>> +                     goto out;=0A=
>>>>> +     } else if (flags & BLKDEV_COPY_NOEMULATION) {=0A=
>>>>=0A=
>>>> Why ? whoever calls blkdev_issue_copy() wants a copy to be done. Why w=
ould that=0A=
>>>> user say "Fail on me if the device does not support copy" ??? This is =
a weird=0A=
>>>> interface in my opinion.=0A=
>>>>=0A=
>>>=0A=
>>> BLKDEV_COPY_NOEMULATION flag was introduced to allow blkdev_issue_copy(=
) callers=0A=
>>> to use their native copying method instead of the emulated copy that I=
=0A=
>>> added. This way we=0A=
>>> ensure that dm uses the hw-assisted copy and if that is not present,=0A=
>>> it falls back to existing=0A=
>>> copy method.=0A=
>>>=0A=
>>> The other users who don't have their native emulation can use this=0A=
>>> emulated-copy implementation.=0A=
>>=0A=
>> I do not understand. Emulation or not should be entirely driven by the d=
evice=0A=
>> reporting support for simple copy (or not). It does not matter which com=
ponent=0A=
>> is issuing the simple copy call: an FS to a real device, and FS to a DM =
device=0A=
>> or a DM target driver. If the underlying device reported support for sim=
ple=0A=
>> copy, use that. Otherwise, emulate with read/write. What am I missing he=
re ?=0A=
>>=0A=
> =0A=
> blkdev_issue_copy() api will generally complete the copy-operation,=0A=
> either by using=0A=
> offloaded-copy or by using emulated-copy. The caller of the api is not=0A=
> required to=0A=
> figure the type of support. However, it can opt out of emulated-copy=0A=
> by specifying=0A=
> the flag BLKDEV_NOEMULATION. This is helpful for the case when the=0A=
> caller already=0A=
> has got a sophisticated emulation (e.g. dm-kcopyd users).=0A=
=0A=
This does not make any sense to me. If the user has already another mean of=
=0A=
doing copies, then that user will not call blkdev_issue_copy(). So I really=
 do=0A=
not understand what the "opting out of emulated copy" would be useful for. =
That=0A=
user can check the simple copy support glag in the device request queue and=
 act=0A=
accordingly: use its own block copy code when simple copy is not supported =
or=0A=
use blkdev_issue_copy() when the device has simple copy. Adding that=0A=
BLKDEV_COPY_NOEMULATION does not serve any purpose at all.=0A=
=0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=