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: DjgDAbwsfELa4A7O5TznIZQT/665idtHhAHug4HjVqmTJ/44e8DI6vAfAzAIDv+ZjGqI8bjXw+
 h/Z8jqgdQtl2D1M4Filsj15CTptIXwSs/eOBUVtZDFNJLA/d2enV6/4pbN8T39OltsQdKW5HSE
 azrBpXbSyPyTvos3bieeP0hn4w6/yr63XNjCdVrEVCO0gkJllxg2Ba8RztxAgP3b1w9tf/32Gq
 WSzLdMtEin7/sJRDow9YDuVo6sGES7LHnuPCT+nScNwWqMCVMUmHuO7ZSrqb9fxAaMLApxgPqK
 mxY=
From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     Kanchan Joshi <joshi.k@samsung.com>,
        "axboe@kernel.dk" <axboe@kernel.dk>,
        "viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
        "bcrl@kvack.org" <bcrl@kvack.org>
CC:     "asml.silence@gmail.com" <asml.silence@gmail.com>,
        "hch@infradead.org" <hch@infradead.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "mb@lightnvm.io" <mb@lightnvm.io>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-aio@kvack.org" <linux-aio@kvack.org>,
        "io-uring@vger.kernel.org" <io-uring@vger.kernel.org>,
        "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
        "selvakuma.s1@samsung.com" <selvakuma.s1@samsung.com>,
        "nj.shetty@samsung.com" <nj.shetty@samsung.com>,
        "javier.gonz@samsung.com" <javier.gonz@samsung.com>
Subject: Re: [PATCH v2 0/2] zone-append support in io-uring and aio
Thread-Topic: [PATCH v2 0/2] zone-append support in io-uring and aio
Thread-Index: AQHWSxSr/CSwBNGukEytKjLpVVok/w==
Date:   Fri, 26 Jun 2020 03:11:55 +0000
Message-ID: <CY4PR04MB37511E3B19035012A143D006E7930@CY4PR04MB3751.namprd04.prod.outlook.com>
References: <CGME20200625171829epcas5p268486a0780571edb4999fc7b3caab602@epcas5p2.samsung.com>
 <1593105349-19270-1-git-send-email-joshi.k@samsung.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: be0f1a80-03a7-4263-f4f8-08d8197eae32
X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Jun 2020 03:11:55.0374
 (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: AR0CQazvgMODZfZ4w0uVJkzPs6UUELyKxc3yNU+x7GoYdSy09uwWzFx88ds3ZMLA6IaDxPG7bVaSNPg1sz9UpQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY4PR04MB0968
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/06/26 2:18, Kanchan Joshi wrote:=0A=
> [Revised as per feedback from Damien, Pavel, Jens, Christoph, Matias, Wil=
cox]=0A=
> =0A=
> This patchset enables zone-append using io-uring/linux-aio, on block IO p=
ath.=0A=
> Purpose is to provide zone-append consumption ability to applications whi=
ch are=0A=
> using zoned-block-device directly.=0A=
> =0A=
> The application may specify RWF_ZONE_APPEND flag with write when it wants=
 to=0A=
> send zone-append. RWF_* flags work with a certain subset of APIs e.g. uri=
ng,=0A=
> aio, and pwritev2. An error is reported if zone-append is requested using=
=0A=
> pwritev2. It is not in the scope of this patchset to support pwritev2 or =
any=0A=
> other sync write API for reasons described later.=0A=
> =0A=
> Zone-append completion result --->=0A=
> With zone-append, where write took place can only be known after completi=
on.=0A=
> So apart from usual return value of write, additional mean is needed to o=
btain=0A=
> the actual written location.=0A=
> =0A=
> In aio, this is returned to application using res2 field of io_event -=0A=
> =0A=
> struct io_event {=0A=
>         __u64           data;           /* the data field from the iocb *=
/=0A=
>         __u64           obj;            /* what iocb this event came from=
 */=0A=
>         __s64           res;            /* result code for this event */=
=0A=
>         __s64           res2;           /* secondary result */=0A=
> };=0A=
> =0A=
> In io-uring, cqe->flags is repurposed for zone-append result.=0A=
> =0A=
> struct io_uring_cqe {=0A=
>         __u64   user_data;      /* sqe->data submission passed back */=0A=
>         __s32   res;            /* result code for this event */=0A=
>         __u32   flags;=0A=
> };=0A=
> =0A=
> Since 32 bit flags is not sufficient, we choose to return zone-relative o=
ffset=0A=
> in sector/512b units. This can cover zone-size represented by chunk_secto=
rs.=0A=
> Applications will have the trouble to combine this with zone start to kno=
w=0A=
> disk-relative offset. But if more bits are obtained by pulling from res f=
ield=0A=
> that too would compel application to interpret res field differently, and=
 it=0A=
> seems more painstaking than the former option.=0A=
> To keep uniformity, even with aio, zone-relative offset is returned.=0A=
=0A=
I am really not a fan of this, to say the least. The input is byte offset, =
the=0A=
output is 512B relative sector count... Arg... We really cannot do better t=
han=0A=
that ?=0A=
=0A=
At the very least, byte relative offset ? The main reason is that this is=
=0A=
_somewhat_ acceptable for raw block device accesses since the "sector"=0A=
abstraction has a clear meaning, but once we add iomap/zonefs async zone ap=
pend=0A=
support, we really will want to have byte unit as the interface is regular=
=0A=
files, not block device file. We could argue that 512B sector unit is still=
=0A=
around even for files (e.g. block counts in file stat). Bu the different un=
it=0A=
for input and output of one operation is really ugly. This is not nice for =
the user.=0A=
=0A=
> =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=
> Block IO vs File IO --->=0A=
> For now, the user zone-append interface is supported only for zoned-block=
-device.=0A=
> Regular files/block-devices are not supported. Regular file-system (e.g. =
F2FS)=0A=
> will not need this anyway, because zone peculiarities are abstracted with=
in FS.=0A=
> At this point, ZoneFS also likes to use append implicitly rather than exp=
licitly.=0A=
> But if/when ZoneFS starts supporting explicit/on-demand zone-append, the =
check=0A=
> allowing-only-block-device should be changed.=0A=
=0A=
Sure, but I think the interface is still a problem. I am not super happy ab=
out=0A=
the 512B sector unit. Zonefs will be the only file system that will be impa=
cted=0A=
since other normal POSIX file system will not have zone append interface fo=
r=0A=
users. So this is a limited problem. Still, even for raw block device files=
=0A=
accesses, POSIX system calls use Byte unit everywhere. Let's try to use tha=
t.=0A=
=0A=
For aio, it is easy since res2 is unsigned long long. For io_uring, as disc=
ussed=0A=
already, we can still 8 bits from the cqe res. All  you need is to add a sm=
all=0A=
helper function in userspace iouring.h to simplify the work of the applicat=
ion=0A=
to get that result.=0A=
=0A=
> =0A=
> Semantics --->=0A=
> Zone-append, by its nature, may perform write on a different location tha=
n what=0A=
> was specified. It does not fit into POSIX, and trying to fit may just und=
ermine=0A=
> its benefit. It may be better to keep semantics as close to zone-append a=
s=0A=
> possible i.e. specify zone-start location, and obtain the actual-write lo=
cation=0A=
> post completion. Towards that goal, existing async APIs seem to fit fine.=
=0A=
> Async APIs (uring, linux aio) do not work on implicit write-pointer and d=
emand=0A=
> explicit write offset (which is what we need for append). Neither write-p=
ointer=0A=
=0A=
What do you mean by "implicit write pointer" ? Are you referring to the beh=
avior=0A=
of AIO write with a block device file open with O_APPEND ? The yes, it does=
 not=0A=
work. But that is perfectly fine for regular files, that is for zonefs.=0A=
=0A=
I would prefer that this paragraph simply state the semantic that is implem=
ented=0A=
first. Then explain why the choice. But first, clarify how the API works, w=
hat=0A=
is allowed, what's not etc. That will also simplify reviewing the code as o=
ne=0A=
can then check the code against the goal.=0A=
=0A=
> is taken as input, nor it is updated on completion. And there is a clear =
way to=0A=
> get zone-append result. Zone-aware applications while using these async A=
PIs=0A=
> can be fine with, for the lack of better word, zone-append semantics itse=
lf.=0A=
> =0A=
> Sync APIs work with implicit write-pointer (at least few of those), and t=
here is=0A=
> no way to obtain zone-append result, making it hard for user-space zone-a=
ppend.=0A=
=0A=
Sync API are executed under inode lock, at least for regular files. So ther=
e is=0A=
absolutely no problem to use zone append. zonefs does it already. The probl=
em is=0A=
the lack of locking for block device file.=0A=
=0A=
> =0A=
> Tests --->=0A=
> Using new interface in fio (uring and libaio engine) by extending zbd tes=
ts=0A=
> for zone-append: https://github.com/axboe/fio/pull/1026=0A=
> =0A=
> Changes since v1:=0A=
> - No new opcodes in uring or aio. Use RWF_ZONE_APPEND flag instead.=0A=
> - linux-aio changes vanish because of no new opcode=0A=
> - Fixed the overflow and other issues mentioned by Damien=0A=
> - Simplified uring support code, fixed the issues mentioned by Pavel=0A=
> - Added error checks=0A=
> =0A=
> Kanchan Joshi (1):=0A=
>   fs,block: Introduce RWF_ZONE_APPEND and handling in direct IO path=0A=
> =0A=
> Selvakumar S (1):=0A=
>   io_uring: add support for zone-append=0A=
> =0A=
>  fs/block_dev.c          | 28 ++++++++++++++++++++++++----=0A=
>  fs/io_uring.c           | 32 ++++++++++++++++++++++++++++++--=0A=
>  include/linux/fs.h      |  9 +++++++++=0A=
>  include/uapi/linux/fs.h |  5 ++++-=0A=
>  4 files changed, 67 insertions(+), 7 deletions(-)=0A=
> =0A=
=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=