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: yvOcG+SF2Iyy76ZvRx/rd0BwX8WA+BJEPNYwuil8iRri2wJzr3WHihFJmvL9dr+CA9p/32BGwI
 mGiZ4b8X6ytgjZJcN1c04ZXzVtN6l91sDs6fzfeBm5zH9xWuyLEClDt1Ox76k5GFdDTO85WfVD
 2mYIqPaAn99ewHkWnZ72I3jZFE39NBRYV/Hex0fiVZc0Q0/qTGwaUcf0kVyCiEN2v+D8Wb0Au7
 l57cDRJlXzP0sgdfPkT9rfQTiTa9MnEMAHzRjendnUVs7z9ycl/6tbE8EgrCRpRFYqe6PWntfv
 oes=
From:   Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
To:     Matthew Wilcox <willy@infradead.org>
CC:     "linux-fsdevel@vger.kernel.org" <linux-fsdevel@vger.kernel.org>,
        "linux-mm@kvack.org" <linux-mm@kvack.org>,
        "agruenba@redhat.com" <agruenba@redhat.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [RFC] Bypass filesystems for reading cached pages
Thread-Topic: [RFC] Bypass filesystems for reading cached pages
Thread-Index: AQHWRlFoe2caIyYlK0mh4vA1o1hS7w==
Date:   Fri, 19 Jun 2020 21:25:57 +0000
Message-ID: <BYAPR04MB4965B610300E1D73873136AB86980@BYAPR04MB4965.namprd04.prod.outlook.com>
References: <20200619155036.GZ8681@bombadil.infradead.org>
 <BYAPR04MB49655EAA09477CB716D39C8986980@BYAPR04MB4965.namprd04.prod.outlook.com>
 <20200619201216.GA8681@bombadil.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: 247201a8-6221-459d-a938-08d814975b6e
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Jun 2020 21:25:57.7652
 (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: sG2ePJEzfnfK+u3buVLA0WVdsY/D6kXckseLM7N2sqaD07QCHEMFV4KXnDOwyKYAem+3Mj6TspvBULEOCNliLJhRDH5UPv5I8jq/wnYvb8E=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR04MB5336
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Matthew,=0A=
=0A=
On 6/19/20 1:12 PM, Matthew Wilcox wrote:=0A=
> On Fri, Jun 19, 2020 at 07:06:19PM +0000, Chaitanya Kulkarni wrote:=0A=
>> On 6/19/20 8:50 AM, Matthew Wilcox wrote:=0A=
>>> This patch lifts the IOCB_CACHED idea expressed by Andreas to the VFS.=
=0A=
>>> The advantage of this patch is that we can avoid taking any filesystem=
=0A=
>>> lock, as long as the pages being accessed are in the cache (and we don'=
t=0A=
>>> need to readahead any pages into the cache).  We also avoid an indirect=
=0A=
>>> function call in these cases.=0A=
>>=0A=
>> I did a testing with NVMeOF target file backend with buffered I/O=0A=
>> enabled with your patch and setting the IOCB_CACHED for each I/O ored=0A=
>> '|' with IOCB_NOWAIT calling call_read_iter_cached() [1].=0A=
>>=0A=
>> The name was changed from call_read_iter() -> call_read_iter_cached() [2=
]).=0A=
>>=0A=
>> For the file system I've used XFS and device was null_blk with memory=0A=
>> backed so entire file was cached into the DRAM.=0A=
> =0A=
> Thanks for testing!  Can you elaborate a little more on what the test doe=
s?=0A=
> Are there many threads or tasks?  What is the I/O path?  XFS on an NVMEoF=
=0A=
> device, talking over loopback to localhost with nullblk as the server?=0A=
> =0A=
> The nullblk device will have all the data in its pagecache, but each XFS=
=0A=
> file will have an empty pagecache initially.  Then it'll be populated by=
=0A=
> the test, so does the I/O pattern revisit previously accessed data at all=
?=0A=
> =0A=
=0A=
Will share details shortly.=0A=
=0A=
>> Following are the performance numbers :-=0A=
>>=0A=
>> IOPS/Bandwidth :-=0A=
>>=0A=
>> default-page-cache:      read:  IOPS=3D1389k,  BW=3D5424MiB/s  (5688MB/s=
)=0A=
>> default-page-cache:      read:  IOPS=3D1381k,  BW=3D5395MiB/s  (5657MB/s=
)=0A=
>> default-page-cache:      read:  IOPS=3D1391k,  BW=3D5432MiB/s  (5696MB/s=
)=0A=
>> iocb-cached-page-cache:  read:  IOPS=3D1403k,  BW=3D5481MiB/s  (5747MB/s=
)=0A=
>> iocb-cached-page-cache:  read:  IOPS=3D1393k,  BW=3D5439MiB/s  (5704MB/s=
)=0A=
>> iocb-cached-page-cache:  read:  IOPS=3D1399k,  BW=3D5465MiB/s  (5731MB/s=
)=0A=
> =0A=
> That doesn't look bad at all ... about 0.7% increase in IOPS.=0A=
> =0A=
>> Submission lat :-=0A=
>>=0A=
>> default-page-cache:      slat  (usec):  min=3D2,  max=3D1076,  avg=3D  3=
.71,=0A=
>> default-page-cache:      slat  (usec):  min=3D2,  max=3D489,   avg=3D  3=
.72,=0A=
>> default-page-cache:      slat  (usec):  min=3D2,  max=3D1078,  avg=3D  3=
.70,=0A=
>> iocb-cached-page-cache:  slat  (usec):  min=3D2,  max=3D1731,  avg=3D  3=
.70,=0A=
>> iocb-cached-page-cache:  slat  (usec):  min=3D2,  max=3D2115,  avg=3D  3=
.69,=0A=
>> iocb-cached-page-cache:  slat  (usec):  min=3D2,  max=3D3055,  avg=3D  3=
.70,=0A=
> =0A=
> Average latency unchanged, max latency up a little ... makes sense,=0A=
> since we'll do a little more work in the worst case.=0A=
> =0A=
>> @@ -264,7 +267,8 @@ static void nvmet_file_execute_rw(struct nvmet_req *=
req)=0A=
>>=0A=
>>           if (req->ns->buffered_io) {=0A=
>>                   if (likely(!req->f.mpool_alloc) &&=0A=
>> -                               nvmet_file_execute_io(req, IOCB_NOWAIT))=
=0A=
>> +                               nvmet_file_execute_io(req,=0A=
>> +                                       IOCB_NOWAIT |IOCB_CACHED))=0A=
>>                           return;=0A=
>>                   nvmet_file_submit_buffered_io(req);=0A=
> =0A=
> You'll need a fallback path here, right?  IOCB_CACHED can get part-way=0A=
> through doing a request, and then need to be finished off after taking=0A=
> the mutex.=0A=
> =0A=
> =0A=
=0A=
That is what something needs to be done in the call_read_iter cached =0A=
otherwise with a flag otherwise callers will have to duplicate the code=0A=
all over the kernel. Correct me if I'm wrong.=0A=
=0A=
For now I'm populating all the data in the cache for fio runs so 2nd and =
=0A=
3rd run is coming from the cache.=0A=
=0A=
=0A=
Also, looking at the code now I don't think we need to use IOCB_CACHED=0A=
flag as long as we call call_read_iter_cached() since it takes care of=0A=
adding this flags which is a correct way of abstracting the code, than=0A=
caller duplicate this flag all over the kernel.=0A=
=0A=
Am I missing something ?=0A=
=0A=
I'll run test again and share more details.=0A=