Received-SPF: pass (google.com: domain of linux-kernel+bounces-190409-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: Liu Wei <liuwei09@cestc.cn>
To: axboe@kernel.dk
Cc: akpm@linux-foundation.org,
	hch@lst.de,
	jack@suse.cz,
	linux-fsdevel@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	liuwei09@cestc.cn,
	rgoldwyn@suse.com,
	willy@infradead.org
Subject: Re: [PATCH] mm/filemap: invalidating pages is still necessary when io with IOCB_NOWAIT
Date: Mon, 27 May 2024 18:09:08 +0800
Message-ID: <20240527100908.49913-1-liuwei09@cestc.cn>
In-Reply-To: <024b9a30-ad3b-4063-b5c8-e6c948ad6b2e@kernel.dk>
References: <024b9a30-ad3b-4063-b5c8-e6c948ad6b2e@kernel.dk>
Precedence: bulk
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit

I am a newer, thanks for the reminder.

> 
> >> 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.

Yes, but I think an asynchronous writex-back is better than simply return EAGAIN.
By using __filemap_fdatawrite_range to trigger a writeback, subsequent retries
may have a higher chance of success. 

> 
> >> +			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
> >>
> >>
> >>