Received-SPF: pass (google.com: domain of linux-kernel+bounces-188009-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <024b9a30-ad3b-4063-b5c8-e6c948ad6b2e@kernel.dk>
Date: Thu, 23 May 2024 15:16:38 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] mm/filemap: invalidating pages is still necessary when io
 with IOCB_NOWAIT
To: Matthew Wilcox <willy@infradead.org>, Liu Wei <liuwei09@cestc.cn>
Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org,
 Goldwyn Rodrigues <rgoldwyn@suse.com>, Christoph Hellwig <hch@lst.de>,
 Jan Kara <jack@suse.cz>, linux-fsdevel@vger.kernel.org
References: <20240513132339.26269-1-liuwei09@cestc.cn>
 <Zk-wa_GvvrxpX9kn@casper.infradead.org>
Content-Language: en-US
From: Jens Axboe <axboe@kernel.dk>
In-Reply-To: <Zk-wa_GvvrxpX9kn@casper.infradead.org>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

On 5/23/24 3:08 PM, Matthew Wilcox wrote:
> On Mon, May 13, 2024 at 09:23:39PM +0800, Liu Wei wrote:
>> After commit (6be96d3ad3 fs: return if direct I/O will trigger writeback),
> 
> If you're reporting problems with a particular commit, it's good form
> to cc the people who actually wrote that commit.

Yeah indeed...

>> when we issuing AIO with direct I/O and IOCB_NOWAIT on a block device, the
>> process context will not be blocked.
>>
>> However, if the device already has page cache in memory, EAGAIN will be
>> returned. And even when trying to reissue the AIO with direct I/O and
>> IOCB_NOWAIT again, we consistently receive EAGAIN.

-EAGAIN doesn't mean "just try again and it'll work".

>> Maybe a better way to deal with it: filemap_fdatawrite_range dirty pages
>> with WB_SYNC_NONE flag, and invalidate_mapping_pages unmapped pages at
>> the same time.
>>
>> Signed-off-by: Liu Wei <liuwei09@cestc.cn>
>> ---
>>  mm/filemap.c | 9 ++++++++-
>>  1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/mm/filemap.c b/mm/filemap.c
>> index 30de18c4fd28..1852a00caf31 100644
>> --- a/mm/filemap.c
>> +++ b/mm/filemap.c
>> @@ -2697,8 +2697,15 @@ int kiocb_invalidate_pages(struct kiocb *iocb, size_t count)
>>  
>>  	if (iocb->ki_flags & IOCB_NOWAIT) {
>>  		/* we could block if there are any pages in the range */
>> -		if (filemap_range_has_page(mapping, pos, end))
>> +		if (filemap_range_has_page(mapping, pos, end)) {
>> +			if (mapping_needs_writeback(mapping)) {
>> +				__filemap_fdatawrite_range(mapping,
>> +						pos, end, WB_SYNC_NONE);
>> +			}

I don't think WB_SYNC_NONE tells it not to block, it just says not to
wait for it... So this won't work as-is.

>> +			invalidate_mapping_pages(mapping,
>> +					pos >> PAGE_SHIFT, end >> PAGE_SHIFT);
>>  			return -EAGAIN;
>> +		}
>>  	} else {
>>  		ret = filemap_write_and_wait_range(mapping, pos, end);
>>  		if (ret)
>> -- 
>> 2.42.1
>>
>>
>>

-- 
Jens Axboe