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: 2oYiCVJmazMRnbduVWOv+mVVie2LPFbjBmKXQLKGerIixG6Y8gKxSttga8NFL8hZLC62SRitY8
 hJYcjJsgiDvny+4tvdOSZYpP665nQFozfQFPGy1m0ONm/9b28HL9IRpbnyBaB8SQ1gLo2mrMCr
 UHurnZ1N+vJsR37ekeG35ULFdJQ91kNgtNu3MJdFpZnCwCQZ/b/vnvz+BLklfOQxkYB2IrYrir
 lsbVkBccdXboV5rShuvDAy4NgKX/DctSJfIxEClvfyI/m2cjC4r1MzhR2aVUAKwC7LPi8rzylG
 kOI=
From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     Kanchan Joshi <joshi.k@samsung.com>
CC:     "axboe@kernel.dk" <axboe@kernel.dk>,
        "kbusch@kernel.org" <kbusch@kernel.org>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "hch@lst.de" <hch@lst.de>, Selvakumar S <selvakuma.s1@samsung.com>,
        Nitesh Shetty <nj.shetty@samsung.com>,
        Javier Gonzalez <javier.gonz@samsung.com>
Subject: Re: [PATCH 2/2] block: enable zone-append for iov_iter of bvec type
Thread-Topic: [PATCH 2/2] block: enable zone-append for iov_iter of bvec type
Thread-Index: AQHWUIdfrrkmUrgUfEeGzF+AR69QhA==
Date:   Fri, 3 Jul 2020 08:07:38 +0000
Message-ID: <CY4PR04MB375124BE75E2E824F543EA11E76A0@CY4PR04MB3751.namprd04.prod.outlook.com>
References: <1593704330-11540-1-git-send-email-joshi.k@samsung.com>
 <CGME20200702154213epcas5p4e8d42861cb5ae91ddeccf1ed73107304@epcas5p4.samsung.com>
 <1593704330-11540-3-git-send-email-joshi.k@samsung.com>
 <CY4PR04MB37516E788EC8804D7DF9780EE76A0@CY4PR04MB3751.namprd04.prod.outlook.com>
 <20200703065311.GB26058@test-zns>
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: 27ee0aa2-d7c1-42dd-6508-08d81f2826d0
X-MS-Exchange-CrossTenant-originalarrivaltime: 03 Jul 2020 08:07:38.1930
 (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: SlcijtfZi0FFpKlB1kQ02PhkQp2B5rqidpD6baagyqCHkz8U3Vn42NLjJhAGZEtfGEn/OPzYGq0k+9u2u9+CgQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR04MB1048
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 2020/07/03 15:56, Kanchan Joshi wrote:=0A=
> On Fri, Jul 03, 2020 at 05:32:56AM +0000, Damien Le Moal wrote:=0A=
>> On 2020/07/03 0:42, Kanchan Joshi wrote:=0A=
>>> zone-append with bvec iov_iter gives WARN_ON, and returns -EINVAL.=0A=
>>> Add new helper to process such iov_iter and add pages in bio honoring=
=0A=
>>> zone-append specific constraints.=0A=
>>>=0A=
>>> Signed-off-by: Kanchan Joshi <joshi.k@samsung.com>=0A=
>>> Signed-off-by: Selvakumar S <selvakuma.s1@samsung.com>=0A=
>>> Signed-off-by: Nitesh Shetty <nj.shetty@samsung.com>=0A=
>>> Signed-off-by: Javier Gonzalez <javier.gonz@samsung.com>=0A=
>>> ---=0A=
>>>  block/bio.c | 31 ++++++++++++++++++++++++++++---=0A=
>>>  1 file changed, 28 insertions(+), 3 deletions(-)=0A=
>>>=0A=
>>> diff --git a/block/bio.c b/block/bio.c=0A=
>>> index 0cecdbc..ade9da7 100644=0A=
>>> --- a/block/bio.c=0A=
>>> +++ b/block/bio.c=0A=
>>> @@ -975,6 +975,30 @@ static int __bio_iov_bvec_add_pages(struct bio *bi=
o, struct iov_iter *iter)=0A=
>>>  	iov_iter_advance(iter, size);=0A=
>>>  	return 0;=0A=
>>>  }=0A=
>>> +static int __bio_iov_bvec_append_add_pages(struct bio *bio, struct iov=
_iter *iter)=0A=
>>> +{=0A=
>>> +	const struct bio_vec *bv =3D iter->bvec;=0A=
>>> +	unsigned int len;=0A=
>>> +	size_t size;=0A=
>>> +	struct request_queue *q =3D bio->bi_disk->queue;=0A=
>>> +	unsigned int max_append_sectors =3D queue_max_zone_append_sectors(q);=
=0A=
>>> +	bool same_page =3D false;=0A=
>>> +=0A=
>>> +	if (WARN_ON_ONCE(!max_append_sectors))=0A=
>>> +		return -EINVAL;=0A=
>>> +=0A=
>>> +	if (WARN_ON_ONCE(iter->iov_offset > bv->bv_len))=0A=
>>> +		return -EINVAL;=0A=
>>> +=0A=
>>> +	len =3D min_t(size_t, bv->bv_len - iter->iov_offset, iter->count);=0A=
>>> +	size =3D bio_add_hw_page(q, bio, bv->bv_page, len,=0A=
>>> +				bv->bv_offset + iter->iov_offset,=0A=
>>> +				max_append_sectors, &same_page);=0A=
>>> +	if (unlikely(size !=3D len))=0A=
>>> +		return -EINVAL;=0A=
>>> +	iov_iter_advance(iter, size);=0A=
>>> +	return 0;=0A=
>>> +}=0A=
>>>=0A=
>>>  #define PAGE_PTRS_PER_BVEC     (sizeof(struct bio_vec) / sizeof(struct=
 page *))=0A=
>>>=0A=
>>> @@ -1105,9 +1129,10 @@ int bio_iov_iter_get_pages(struct bio *bio, stru=
ct iov_iter *iter)=0A=
>>>=0A=
>>>  	do {=0A=
>>>  		if (bio_op(bio) =3D=3D REQ_OP_ZONE_APPEND) {=0A=
>>> -			if (WARN_ON_ONCE(is_bvec))=0A=
>>> -				return -EINVAL;=0A=
>>> -			ret =3D __bio_iov_append_get_pages(bio, iter);=0A=
>>> +			if (is_bvec)=0A=
>>> +				ret =3D __bio_iov_bvec_append_add_pages(bio, iter);=0A=
>>> +			else=0A=
>>> +				ret =3D __bio_iov_append_get_pages(bio, iter);=0A=
>>>  		} else {=0A=
>>>  			if (is_bvec)=0A=
>>>  				ret =3D __bio_iov_bvec_add_pages(bio, iter);=0A=
>>>=0A=
>>=0A=
>> The only user of this function that issues zone append requests is zonef=
s. The=0A=
>> issued requests are not using bvec iter but a user direct IO buffer iter=
. So=0A=
>> this change would have no user at all as far as I can see. Am I missing=
=0A=
>> something ? What IO path makes this change necessary ?=0A=
> =0A=
> Yes, zonefs does not use bvec iter. But while enabling io-uring path for=
=0A=
> zone-append, I hit into this condition returning -EINVAL. =0A=
> =0A=
> Reference (from user zone-append series cover letter):=0A=
> "Append using io_uring fixed-buffer --->=0A=
> This is flagged as not-supported at the moment. Reason being, for fixed-b=
uffer=0A=
> io-uring sends iov_iter of bvec type. But current append-infra in block-l=
ayer=0A=
> does not support such iov_iter."=0A=
> =0A=
> And zone-append doesn't have a problem in using bvec iter as well, so=0A=
> thought that this may make infra more complete/future-proof?=0A=
=0A=
As long as there is no possible user for this change, I do not see the poin=
t in=0A=
adding it.=0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=