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: MoB0AcKdAkU1M9NU1RhB1BrRSxuYQpkuD0xqLCdDmAaHOBp4oHwjD83yvp8HiFrAlugug2NPJm
 QIXtD0T7/d/6Hp8DD7TIpOfe88oA5CJMTTn8oc1i/8vpRg4L5EKXjlqabrKl38Nf60tSuNlRr2
 ym6TJOjWUFOABZwJ1u9fgXF+L4xgSV5b4BR04EoS8KQJbeJrjmntRQ6Q3ohhVTfaskZJNGE9Ch
 LWxcFNAtjercruMhQZaU8ocKKqMDcWgP+TmHyu1eyqhaVue3DWzEiuZDnLprBu/l8Gznhuh8HY
 UvI=
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:   Mon, 12 Apr 2021 00:24:56 +0000
Message-ID: <BL0PR04MB6514B34000467FABFD6BF385E7709@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>
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: cb7d7f6b-48de-4f67-4e77-08d8fd496655
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Apr 2021 00:24:56.3345
 (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: oa66vKcD59q7C168B7+dNoSz1+UahsJaSgObpMgDTUTxV5ujbzS1mgHyik5e5dfQG1h8C8SEIdr2M52qsMObRw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL0PR04MB6546
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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 interface w=
ill=0A=
take some more efforts. OK, then. For the first round of commits, let's for=
get=0A=
about this. But I still think that your emulation could be a lot better tha=
n a=0A=
loop doing blocking writes after blocking reads.=0A=
=0A=
[...]=0A=
>>> +int blkdev_issue_copy(struct block_device *src_bdev, int nr_srcs,=0A=
>>> +             struct range_entry *src_rlist, struct block_device *dest_=
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_mas=
k,=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_mas=
k);=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 wou=
ld 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 devi=
ce=0A=
reporting support for simple copy (or not). It does not matter which compon=
ent=0A=
is issuing the simple copy call: an FS to a real device, and FS to a DM dev=
ice=0A=
or a DM target driver. If the underlying device reported support for simple=
=0A=
copy, use that. Otherwise, emulate with read/write. What am I missing here =
?=0A=
=0A=
[...]=0A=
>>> @@ -565,6 +569,12 @@ int blk_stack_limits(struct queue_limits *t, struc=
t queue_limits *b,=0A=
>>>       if (b->chunk_sectors)=0A=
>>>               t->chunk_sectors =3D gcd(t->chunk_sectors, b->chunk_secto=
rs);=0A=
>>>=0A=
>>> +     /* simple copy not supported in stacked devices */=0A=
>>> +     t->copy_offload =3D 0;=0A=
>>> +     t->max_copy_sectors =3D 0;=0A=
>>> +     t->max_copy_range_sectors =3D 0;=0A=
>>> +     t->max_copy_nr_ranges =3D 0;=0A=
>>=0A=
>> You do not need this. Limits not explicitely initialized are 0 already.=
=0A=
>> But I do not see why you can't support copy on stacked devices. That sho=
uld be=0A=
>> feasible taking the min() for each of the above limit.=0A=
>>=0A=
> =0A=
> Disabling stacked device support was feedback from v2.=0A=
> =0A=
> https://patchwork.kernel.org/project/linux-block/patch/20201204094659.127=
32-2-selvakuma.s1@samsung.com/=0A=
=0A=
Right. But the initialization to 0 is still not needed. The fields are alre=
ady=0A=
initialized to 0.=0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=