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: PrDjXGuBNoL9LNnDIQa3rEKCTt3Ynw1P0FMHf0wmCYu1Dl0jPQLkpuhygyRIFsPDOo1VGElf8s
 v+Nf/yLB/XosUscWu8DhbVUSbN6kJQg2AvvTPGBp00P/v9HY8FwckHXeytPJu5V/lX6e3JYy8i
 VWn7ULfa/Xiql81KtMiKvb9ECnp2KQ0u97mrVS8NMR18aBOHIMKsvLEwv1z5CJjI6cbfKszlpX
 krlcbzdZC+5bfY8RhwVT10uZRqNAH6568lqIcaALJhQKtbSeWagJCsuggYXr8taV8kgRLCsn45
 HgI=
From:   Damien Le Moal <Damien.LeMoal@wdc.com>
To:     "hch@infradead.org" <hch@infradead.org>
CC:     Kanchan Joshi <joshiiitr@gmail.com>, Jens Axboe <axboe@kernel.dk>,
        Pavel Begunkov <asml.silence@gmail.com>,
        Kanchan Joshi <joshi.k@samsung.com>,
        "viro@zeniv.linux.org.uk" <viro@zeniv.linux.org.uk>,
        "bcrl@kvack.org" <bcrl@kvack.org>,
        Matthew Wilcox <willy@infradead.org>,
        "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "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>,
        "linux-api@vger.kernel.org" <linux-api@vger.kernel.org>,
        SelvaKumar S <selvakuma.s1@samsung.com>,
        Nitesh Shetty <nj.shetty@samsung.com>,
        Javier Gonzalez <javier.gonz@samsung.com>,
        Johannes Thumshirn <Johannes.Thumshirn@wdc.com>,
        Naohiro Aota <Naohiro.Aota@wdc.com>
Subject: Re: [PATCH v4 6/6] io_uring: add support for zone-append
Thread-Topic: [PATCH v4 6/6] io_uring: add support for zone-append
Thread-Index: AQHWYdbcc0q15qREKECGO7brHi6zEg==
Date:   Wed, 5 Aug 2020 07:35:28 +0000
Message-ID: <CY4PR04MB37517D633920E4D31AC6EA0DE74B0@CY4PR04MB3751.namprd04.prod.outlook.com>
References: <CA+1E3rKxZk2CatTuPcQq5d14vXL9_9LVb2_+AfR2m9xn2WTZdg@mail.gmail.com>
 <MWHPR04MB3758DC08EA17780E498E9EC0E74E0@MWHPR04MB3758.namprd04.prod.outlook.com>
 <20200731064526.GA25674@infradead.org>
 <MWHPR04MB37581344328A42EA7F5ED13EE74E0@MWHPR04MB3758.namprd04.prod.outlook.com>
 <CA+1E3rLM4G4SwzD6RWsK6Ssp7NmhiPedZDjrqN3kORQr9fxCtw@mail.gmail.com>
 <MWHPR04MB375863C20C1EF2CB27E62703E74E0@MWHPR04MB3758.namprd04.prod.outlook.com>
 <20200731091416.GA29634@infradead.org>
 <MWHPR04MB37586D39CA389296CE0252A4E74E0@MWHPR04MB3758.namprd04.prod.outlook.com>
 <20200731094135.GA4104@infradead.org>
 <MWHPR04MB3758A4B2967DB1FABAAD9265E74E0@MWHPR04MB3758.namprd04.prod.outlook.com>
 <20200731125110.GA11500@infradead.org>
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: 7c5ee32e-3e2b-4cd0-19a8-08d83912202f
X-MS-Exchange-CrossTenant-originalarrivaltime: 05 Aug 2020 07:35:28.4097
 (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: BMpuCMfCv6xEvP9Wgnv2LeoOeK49oqnYXjEUrTXs8ol6jdDZh5ePnHtQ8euwoBHj6wJbj84ST4qzW2TIh3E9dA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY1PR04MB2316
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/31 21:51, hch@infradead.org wrote:=0A=
> On Fri, Jul 31, 2020 at 10:16:49AM +0000, Damien Le Moal wrote:=0A=
>>>=0A=
>>> Let's keep semantics and implementation separate.  For the case=0A=
>>> where we report the actual offset we need a size imitation and no=0A=
>>> short writes.=0A=
>>=0A=
>> OK. So the name of the flag confused me. The flag name should reflect "D=
o zone=0A=
>> append and report written offset", right ?=0A=
> =0A=
> Well, we already have O_APPEND, which is the equivalent to append to=0A=
> the write pointer.  The only interesting addition is that we also want=0A=
> to report where we wrote.  So I'd rather have RWF_REPORT_OFFSET or so.=0A=
=0A=
That works for me. But that rules out having the same interface for raw blo=
ck=0A=
devices since O_APPEND has no meaning in that case. So for raw block device=
s, it=0A=
will have to be through zonefs. That works for me, and I think it was your =
idea=0A=
all along. Can you confirm please ?=0A=
=0A=
>> But I think I am starting to see the picture you are drawing here:=0A=
>> 1) Introduce a fcntl() to get "maximum size for atomic append writes"=0A=
>> 2) Introduce an aio flag specifying "Do atomic append write and report w=
ritten=0A=
>> offset"=0A=
> =0A=
> I think we just need the 'report written offset flag', in fact even for=
=0A=
> zonefs with the right locking we can handle unlimited write sizes, just=
=0A=
> with lower performance.  E.g.=0A=
> =0A=
> 1) check if the write size is larger than the zone append limit=0A=
> =0A=
> if no:=0A=
> =0A=
>  - take the shared lock  and issue a zone append, done=0A=
> =0A=
> if yes:=0A=
> =0A=
>  - take the exclusive per-inode (zone) lock and just issue either normal=
=0A=
>    writes or zone append at your choice, relying on the lock to=0A=
>    serialize other writers.  For the async case this means we need a=0A=
>    lock than can be release in a different context than it was acquired,=
=0A=
>    which is a little ugly but can be done.=0A=
=0A=
Yes, that would be possible. But likely, this will also need calls to=0A=
inode_dio_wait() to avoid ending up with a mix of regular write and zone ap=
pend=0A=
writes in flight (which likely would result in the regular write failing as=
 the=0A=
zone append writes would go straight to the device without waiting for the =
zone=0A=
write lock like regular writes do).=0A=
=0A=
This all sound sensible to me. One last point though, specific to zonefs: i=
f the=0A=
user opens a zone file with O_APPEND, I do want to have that necessarily me=
an=0A=
"use zone append". And same for the "RWF_REPORT_OFFSET". The point here is =
that=0A=
both O_APPEND and RWF_REPORT_OFFSET can be used with both regular writes an=
d=0A=
zone append writes, but none of them actually clearly specify if the=0A=
application/user tolerates writing data to disk in a different order than t=
he=0A=
issuing order... So another flag to indicate "atomic out-of-order writes" (=
=3D=3D=0A=
zone append) ?=0A=
=0A=
Without a new flag, we can only have these cases to enable zone append:=0A=
=0A=
1) No O_APPEND: ignore RWF_REPORT_OFFSET and use regular writes using ->aio=
_ofst=0A=
=0A=
2) O_APPEND without RWF_REPORT_OFFSET: use regular write with file size as=
=0A=
->aio_ofst (as done today already), do not report the write offset on compl=
etion.=0A=
=0A=
3) O_APPEND with RWF_REPORT_OFFSET: use zone append, implying potentially o=
ut of=0A=
order writes.=0A=
=0A=
And case (3) is not nice. I would rather prefer something like:=0A=
=0A=
3) O_APPEND with RWF_REPORT_OFFSET: use regular write with file size as=0A=
->aio_ofst (as done today already), report the write offset on completion.=
=0A=
=0A=
4) O_APPEND with RWF_ATOMIC_APPEND: use zone append writes, do not report t=
he=0A=
write offset on completion.=0A=
=0A=
5) O_APPEND with RWF_ATOMIC_APPEND+RWF_REPORT_OFFSET: use zone append write=
s,=0A=
report the write offset on completion.=0A=
=0A=
RWF_ATOMIC_APPEND could also always imply RWF_REPORT_OFFSET. ANy aio with=
=0A=
RWF_ATOMIC_APPEND that is too large would be failed.=0A=
=0A=
Thoughts ?=0A=
=0A=
-- =0A=
Damien Le Moal=0A=
Western Digital Research=0A=