[PATCHSET,v2,0/15] Uncached buffered IO

Message ID	20241110152906.1747545-1-axboe@kernel.dk (mailing list archive)
Headers	show Return-Path: <owner-linux-mm@kvack.org> From: Jens Axboe <axboe@kernel.dk> To: linux-mm@kvack.org, linux-fsdevel@vger.kernel.org Cc: hannes@cmpxchg.org, clm@meta.com, linux-kernel@vger.kernel.org, willy@infradead.org Subject: [PATCHSET v2 0/15] Uncached buffered IO Date: Sun, 10 Nov 2024 08:27:52 -0700 Message-ID: <20241110152906.1747545-1-axboe@kernel.dk> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: owner-linux-mm@kvack.org Precedence: bulk
Series	Uncached buffered IO \| expand [PATCHSET,v2,0/15] Uncached buffered IO [01/15] mm/filemap: change filemap_create_folio() to take a struct kiocb [02/15] mm/readahead: add folio allocation helper [03/15] mm: add PG_uncached page flag [04/15] mm/readahead: add readahead_control->uncached member [05/15] mm/filemap: use page_cache_sync_ra() to kick off read-ahead [06/15] mm/truncate: make invalidate_complete_folio2() public [07/15] fs: add RWF_UNCACHED iocb and FOP_UNCACHED file_operations flag [09/15] mm/filemap: drop uncached pages when writeback completes [10/15] mm/filemap: make buffered writes work with RWF_UNCACHED [11/15] mm: add FGP_UNCACHED folio creation flag [12/15] ext4: add RWF_UNCACHED write support [13/15] iomap: make buffered writes work with RWF_UNCACHED [14/15] xfs: punt uncached write completions to the completion wq [15/15] xfs: flag as supporting FOP_UNCACHED

Jens Axboe Nov. 10, 2024, 3:27 p.m. UTC

Hi,

5 years ago I posted patches adding support for RWF_UNCACHED, as a way
to do buffered IO that isn't page cache persistent. The approach back
then was to have private pages for IO, and then get rid of them once IO
was done. But that then runs into all the issues that O_DIRECT has, in
terms of synchronizing with the page cache.

So here's a new approach to the same concent, but using the page cache
as synchronization. That makes RWF_UNCACHED less special, in that it's
just page cache IO, except it prunes the ranges once IO is completed.

Why do this, you may ask? The tldr is that device speeds are only
getting faster, while reclaim is not. Doing normal buffered IO can be
very unpredictable, and suck up a lot of resources on the reclaim side.
This leads people to use O_DIRECT as a work-around, which has its own
set of restrictions in terms of size, offset, and length of IO. It's
also inherently synchronous, and now you need async IO as well. While
the latter isn't necessarily a big problem as we have good options
available there, it also should not be a requirement when all you want
to do is read or write some data without caching.

Even on desktop type systems, a normal NVMe device can fill the entire
page cache in seconds. On the big system I used for testing, there's a
lot more RAM, but also a lot more devices. As can be seen in some of the
results in the following patches, you can still fill RAM in seconds even
when there's 1TB of it. Hence this problem isn't solely a "big
hyperscaler system" issue, it's common across the board.

Common for both reads and writes with RWF_UNCACHED is that they use the
page cache for IO. Reads work just like a normal buffered read would,
with the only exception being that the touched ranges will get pruned
after data has been copied. For writes, the ranges will get writeback
kicked off before the syscall returns, and then writeback completion
will prune the range. Hence writes aren't synchronous, and it's easy to
pipeline writes using RWF_UNCACHED.

File systems need to support this. The patches add support for the
generic filemap helpers, and for iomap. Then ext4 and XFS are marked as
supporting it. The amount of code here is really trivial, and the only
reason the fs opt-in is necessary is to have an RWF_UNCACHED IO return
-EOPNOTSUPP just in case the fs doesn't use either the generic paths or
iomap. Adding "support" to other file systems should be trivial, most of
the time just a one-liner adding FOP_UNCACHED to the fop_flags in the
file_operations struct.

Performance results are in patch 7 for reads and patch 10 for writes,
with the tldr being that I see about a 65% improvement in performance
for both, with fully predictable IO times. CPU reduction is substantial
as well, with no kswapd activity at all for reclaim when using uncached
IO.

Using it from applications is trivial - just set RWF_UNCACHED for the
read or write, using pwritev2(2) or preadv2(2). For io_uring, same
thing, just set RWF_UNCACHED in sqe->rw_flags for a buffered read/write
operation. And that's it.

The goal with this patchset was to make it less special than before. I
think if you look at the diffstat you'll agree that this is the case.

Patches 1..6 are just prep patches, and should have no functional
changes at all. Patch 7 adds support for the filemap path for
RWF_UNCACHED reads, patch 10 adds support for filemap RWF_UNCACHED
writes, and patches 12..15 adds ext4 and xfs/iomap support iomap.

Git tree available here:

https://git.kernel.dk/cgit/linux/log/?h=buffered-uncached.5

 fs/ext4/ext4.h                 |  1 +
 fs/ext4/file.c                 |  2 +-
 fs/ext4/inline.c               |  7 ++-
 fs/ext4/inode.c                | 18 ++++++-
 fs/ext4/page-io.c              | 28 ++++++-----
 fs/iomap/buffered-io.c         | 15 +++++-
 fs/xfs/xfs_aops.c              |  7 ++-
 fs/xfs/xfs_file.c              |  4 +-
 include/linux/fs.h             | 10 +++-
 include/linux/iomap.h          |  4 +-
 include/linux/page-flags.h     |  5 ++
 include/linux/pagemap.h        | 34 +++++++++++++
 include/trace/events/mmflags.h |  3 +-
 include/uapi/linux/fs.h        |  6 ++-
 mm/filemap.c                   | 90 ++++++++++++++++++++++++++++------
 mm/readahead.c                 | 22 +++++++--
 mm/swap.c                      |  2 +
 mm/truncate.c                  |  9 ++--
 18 files changed, 218 insertions(+), 49 deletions(-)

Since v1
- Move iocb->ki_flags checking into filemap_create_folio()
- Use __folio_set_uncached() when marking a folio as uncached right
  after creation
- Shuffle patches around to not have a bisection issue
- Combine some patches
- Add FGP_UNCACHED folio get flag. If set, newly created folios will
  get marked as uncached.
- Add foliop_uncached to be able to pass whether this is an uncached
  folio creation or not into ->write_begin(). Fixes the issue of
  uncached writes pruning existing ranges. Not the prettiest, but the
  prospect of changing ->write_begin() is daunting. I did go that
  route but it's a lot of churn. Have the patch and the scars.
- Ensure that both ext4 and xfs punt to their workqueue handling
  writeback completion for uncached writebacks, as we need workqueue
  context to prune ranges.
- Add generic_uncached_write() helper to make it easy for file systems
  to prune the ranges.
- Add folio_end_uncached() writeback completion helper, and also check
  in_task() in there. Can trigger if the fs needs hints on punting to
  a safe context for this completion, but normal writeback races with
  us and starts writeback on a range that includes uncached folios.
- Rebase on current master

Kirill A. Shutemov Nov. 11, 2024, 9:15 a.m. UTC | #1

On Sun, Nov 10, 2024 at 08:28:00AM -0700, Jens Axboe wrote:
> Add RWF_UNCACHED as a read operation flag, which means that any data
> read wil be removed from the page cache upon completion. Uses the page
> cache to synchronize, and simply prunes folios that were instantiated
> when the operation completes. While it would be possible to use private
> pages for this, using the page cache as synchronization is handy for a
> variety of reasons:
> 
> 1) No special truncate magic is needed
> 2) Async buffered reads need some place to serialize, using the page
>    cache is a lot easier than writing extra code for this
> 3) The pruning cost is pretty reasonable
> 
> and the code to support this is much simpler as a result.
> 
> You can think of uncached buffered IO as being the much more attractive
> cousing of O_DIRECT - it has none of the restrictions of O_DIRECT. Yes,
> it will copy the data, but unlike regular buffered IO, it doesn't run
> into the unpredictability of the page cache in terms of reclaim. As an
> example, on a test box with 32 drives, reading them with buffered IO
> looks as follows:
> 
> Reading bs 65536, uncached 0
>   1s: 145945MB/sec
>   2s: 158067MB/sec
>   3s: 157007MB/sec
>   4s: 148622MB/sec
>   5s: 118824MB/sec
>   6s: 70494MB/sec
>   7s: 41754MB/sec
>   8s: 90811MB/sec
>   9s: 92204MB/sec
>  10s: 95178MB/sec
>  11s: 95488MB/sec
>  12s: 95552MB/sec
>  13s: 96275MB/sec
> 
> where it's quite easy to see where the page cache filled up, and
> performance went from good to erratic, and finally settles at a much
> lower rate. Looking at top while this is ongoing, we see:
> 
>  PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND
> 7535 root      20   0  267004      0      0 S  3199   0.0   8:40.65 uncached
> 3326 root      20   0       0      0      0 R 100.0   0.0   0:16.40 kswapd4
> 3327 root      20   0       0      0      0 R 100.0   0.0   0:17.22 kswapd5
> 3328 root      20   0       0      0      0 R 100.0   0.0   0:13.29 kswapd6
> 3332 root      20   0       0      0      0 R 100.0   0.0   0:11.11 kswapd10
> 3339 root      20   0       0      0      0 R 100.0   0.0   0:16.25 kswapd17
> 3348 root      20   0       0      0      0 R 100.0   0.0   0:16.40 kswapd26
> 3343 root      20   0       0      0      0 R 100.0   0.0   0:16.30 kswapd21
> 3344 root      20   0       0      0      0 R 100.0   0.0   0:11.92 kswapd22
> 3349 root      20   0       0      0      0 R 100.0   0.0   0:16.28 kswapd27
> 3352 root      20   0       0      0      0 R  99.7   0.0   0:11.89 kswapd30
> 3353 root      20   0       0      0      0 R  96.7   0.0   0:16.04 kswapd31
> 3329 root      20   0       0      0      0 R  96.4   0.0   0:11.41 kswapd7
> 3345 root      20   0       0      0      0 R  96.4   0.0   0:13.40 kswapd23
> 3330 root      20   0       0      0      0 S  91.1   0.0   0:08.28 kswapd8
> 3350 root      20   0       0      0      0 S  86.8   0.0   0:11.13 kswapd28
> 3325 root      20   0       0      0      0 S  76.3   0.0   0:07.43 kswapd3
> 3341 root      20   0       0      0      0 S  74.7   0.0   0:08.85 kswapd19
> 3334 root      20   0       0      0      0 S  71.7   0.0   0:10.04 kswapd12
> 3351 root      20   0       0      0      0 R  60.5   0.0   0:09.59 kswapd29
> 3323 root      20   0       0      0      0 R  57.6   0.0   0:11.50 kswapd1
> [...]
> 
> which is just showing a partial list of the 32 kswapd threads that are
> running mostly full tilt, burning ~28 full CPU cores.
> 
> If the same test case is run with RWF_UNCACHED set for the buffered read,
> the output looks as follows:
> 
> Reading bs 65536, uncached 0
>   1s: 153144MB/sec
>   2s: 156760MB/sec
>   3s: 158110MB/sec
>   4s: 158009MB/sec
>   5s: 158043MB/sec
>   6s: 157638MB/sec
>   7s: 157999MB/sec
>   8s: 158024MB/sec
>   9s: 157764MB/sec
>  10s: 157477MB/sec
>  11s: 157417MB/sec
>  12s: 157455MB/sec
>  13s: 157233MB/sec
>  14s: 156692MB/sec
> 
> which is just chugging along at ~155GB/sec of read performance. Looking
> at top, we see:
> 
>  PID USER      PR  NI    VIRT    RES    SHR S  %CPU  %MEM     TIME+ COMMAND
> 7961 root      20   0  267004      0      0 S  3180   0.0   5:37.95 uncached
> 8024 axboe     20   0   14292   4096      0 R   1.0   0.0   0:00.13 top
> 
> where just the test app is using CPU, no reclaim is taking place outside
> of the main thread. Not only is performance 65% better, it's also using
> half the CPU to do it.
> 
> Signed-off-by: Jens Axboe <axboe@kernel.dk>
> ---
>  mm/filemap.c | 18 ++++++++++++++++--
>  mm/swap.c    |  2 ++
>  2 files changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 38dc94b761b7..bd698340ef24 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2474,6 +2474,8 @@ static int filemap_create_folio(struct kiocb *iocb,
>  	folio = filemap_alloc_folio(mapping_gfp_mask(mapping), min_order);
>  	if (!folio)
>  		return -ENOMEM;
> +	if (iocb->ki_flags & IOCB_UNCACHED)
> +		__folio_set_uncached(folio);
>  
>  	/*
>  	 * Protect against truncate / hole punch. Grabbing invalidate_lock
> @@ -2519,6 +2521,8 @@ static int filemap_readahead(struct kiocb *iocb, struct file *file,
>  
>  	if (iocb->ki_flags & IOCB_NOIO)
>  		return -EAGAIN;
> +	if (iocb->ki_flags & IOCB_UNCACHED)
> +		ractl.uncached = 1;
>  	page_cache_async_ra(&ractl, folio, last_index - folio->index);
>  	return 0;
>  }
> @@ -2548,6 +2552,8 @@ static int filemap_get_pages(struct kiocb *iocb, size_t count,
>  			return -EAGAIN;
>  		if (iocb->ki_flags & IOCB_NOWAIT)
>  			flags = memalloc_noio_save();
> +		if (iocb->ki_flags & IOCB_UNCACHED)
> +			ractl.uncached = 1;
>  		page_cache_sync_ra(&ractl, last_index - index);
>  		if (iocb->ki_flags & IOCB_NOWAIT)
>  			memalloc_noio_restore(flags);
> @@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
>  			}
>  		}
>  put_folios:
> -		for (i = 0; i < folio_batch_count(&fbatch); i++)
> -			folio_put(fbatch.folios[i]);
> +		for (i = 0; i < folio_batch_count(&fbatch); i++) {
> +			struct folio *folio = fbatch.folios[i];
> +
> +			if (folio_test_uncached(folio)) {
> +				folio_lock(folio);
> +				invalidate_complete_folio2(mapping, folio, 0);
> +				folio_unlock(folio);

I am not sure it is safe. What happens if it races with page fault?

The only current caller of invalidate_complete_folio2() unmaps the folio
explicitly before calling it. And folio lock prevents re-faulting.

I think we need to give up PG_uncached if we see folio_mapped(). And maybe
also mark the page accessed.

> +			}
> +			folio_put(folio);
> +		}
>  		folio_batch_init(&fbatch);
>  	} while (iov_iter_count(iter) && iocb->ki_pos < isize && !error);
>  
> diff --git a/mm/swap.c b/mm/swap.c
> index 835bdf324b76..f2457acae383 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -472,6 +472,8 @@ static void folio_inc_refs(struct folio *folio)
>   */
>  void folio_mark_accessed(struct folio *folio)
>  {
> +	if (folio_test_uncached(folio))
> +		return;

	if (folio_test_uncached(folio)) {
		if (folio_mapped(folio))
			folio_clear_uncached(folio);
		else
			return;
	}

>  	if (lru_gen_enabled()) {
>  		folio_inc_refs(folio);
>  		return;
> -- 
> 2.45.2
>

Jens Axboe Nov. 11, 2024, 2:12 p.m. UTC | #2

On 11/11/24 2:15 AM, Kirill A. Shutemov wrote:
>> @@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
>>  			}
>>  		}
>>  put_folios:
>> -		for (i = 0; i < folio_batch_count(&fbatch); i++)
>> -			folio_put(fbatch.folios[i]);
>> +		for (i = 0; i < folio_batch_count(&fbatch); i++) {
>> +			struct folio *folio = fbatch.folios[i];
>> +
>> +			if (folio_test_uncached(folio)) {
>> +				folio_lock(folio);
>> +				invalidate_complete_folio2(mapping, folio, 0);
>> +				folio_unlock(folio);
> 
> I am not sure it is safe. What happens if it races with page fault?
> 
> The only current caller of invalidate_complete_folio2() unmaps the folio
> explicitly before calling it. And folio lock prevents re-faulting.
> 
> I think we need to give up PG_uncached if we see folio_mapped(). And maybe
> also mark the page accessed.

Ok thanks, let me take a look at that and create a test case that
exercises that explicitly.

>> diff --git a/mm/swap.c b/mm/swap.c
>> index 835bdf324b76..f2457acae383 100644
>> --- a/mm/swap.c
>> +++ b/mm/swap.c
>> @@ -472,6 +472,8 @@ static void folio_inc_refs(struct folio *folio)
>>   */
>>  void folio_mark_accessed(struct folio *folio)
>>  {
>> +	if (folio_test_uncached(folio))
>> +		return;
> 
> 	if (folio_test_uncached(folio)) {
> 		if (folio_mapped(folio))
> 			folio_clear_uncached(folio);
> 		else
> 			return;
> 	}

Noted, thanks!

Christoph Hellwig Nov. 11, 2024, 3:16 p.m. UTC | #3

On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
> Ok thanks, let me take a look at that and create a test case that
> exercises that explicitly.

Please add RWF_UNCACHED to fsstress.c in xfstests also.  That is our
exerciser for concurrent issuing of different I/O types to hit these
kinds of corner cases.

Jens Axboe Nov. 11, 2024, 3:17 p.m. UTC | #4

On 11/11/24 8:16 AM, Christoph Hellwig wrote:
> On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
>> Ok thanks, let me take a look at that and create a test case that
>> exercises that explicitly.
> 
> Please add RWF_UNCACHED to fsstress.c in xfstests also.  That is our
> exerciser for concurrent issuing of different I/O types to hit these
> kinds of corner cases.

Sure, can do.

Kirill A. Shutemov Nov. 11, 2024, 3:25 p.m. UTC | #5

On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
> On 11/11/24 2:15 AM, Kirill A. Shutemov wrote:
> >> @@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
> >>  			}
> >>  		}
> >>  put_folios:
> >> -		for (i = 0; i < folio_batch_count(&fbatch); i++)
> >> -			folio_put(fbatch.folios[i]);
> >> +		for (i = 0; i < folio_batch_count(&fbatch); i++) {
> >> +			struct folio *folio = fbatch.folios[i];
> >> +
> >> +			if (folio_test_uncached(folio)) {
> >> +				folio_lock(folio);
> >> +				invalidate_complete_folio2(mapping, folio, 0);
> >> +				folio_unlock(folio);
> > 
> > I am not sure it is safe. What happens if it races with page fault?
> > 
> > The only current caller of invalidate_complete_folio2() unmaps the folio
> > explicitly before calling it. And folio lock prevents re-faulting.
> > 
> > I think we need to give up PG_uncached if we see folio_mapped(). And maybe
> > also mark the page accessed.
> 
> Ok thanks, let me take a look at that and create a test case that
> exercises that explicitly.

It might be worth generalizing it to clearing PG_uncached for any page cache
lookups that don't come from RWF_UNCACHED.

Jens Axboe Nov. 11, 2024, 3:31 p.m. UTC | #6

On 11/11/24 8:25 AM, Kirill A. Shutemov wrote:
> On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
>> On 11/11/24 2:15 AM, Kirill A. Shutemov wrote:
>>>> @@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
>>>>  			}
>>>>  		}
>>>>  put_folios:
>>>> -		for (i = 0; i < folio_batch_count(&fbatch); i++)
>>>> -			folio_put(fbatch.folios[i]);
>>>> +		for (i = 0; i < folio_batch_count(&fbatch); i++) {
>>>> +			struct folio *folio = fbatch.folios[i];
>>>> +
>>>> +			if (folio_test_uncached(folio)) {
>>>> +				folio_lock(folio);
>>>> +				invalidate_complete_folio2(mapping, folio, 0);
>>>> +				folio_unlock(folio);
>>>
>>> I am not sure it is safe. What happens if it races with page fault?
>>>
>>> The only current caller of invalidate_complete_folio2() unmaps the folio
>>> explicitly before calling it. And folio lock prevents re-faulting.
>>>
>>> I think we need to give up PG_uncached if we see folio_mapped(). And maybe
>>> also mark the page accessed.
>>
>> Ok thanks, let me take a look at that and create a test case that
>> exercises that explicitly.
> 
> It might be worth generalizing it to clearing PG_uncached for any page cache
> lookups that don't come from RWF_UNCACHED.

We can do that - you mean at lookup time? Eg have __filemap_get_folio()
do:

if (folio_test_uncached(folio) && !(fgp_flags & FGP_UNCACHED))
	folio_clear_uncached(folio);

or do you want this logic just in filemap_read()? Arguably it should
already clear it in the quoted code above, regardless, eg:

	if (folio_test_uncached(folio)) {
		folio_lock(folio);
		invalidate_complete_folio2(mapping, folio, 0);
		folio_clear_uncached(folio);
		folio_unlock(folio);
	}

in case invalidation fails.

Kirill A. Shutemov Nov. 11, 2024, 3:51 p.m. UTC | #7

On Mon, Nov 11, 2024 at 08:31:28AM -0700, Jens Axboe wrote:
> On 11/11/24 8:25 AM, Kirill A. Shutemov wrote:
> > On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
> >> On 11/11/24 2:15 AM, Kirill A. Shutemov wrote:
> >>>> @@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
> >>>>  			}
> >>>>  		}
> >>>>  put_folios:
> >>>> -		for (i = 0; i < folio_batch_count(&fbatch); i++)
> >>>> -			folio_put(fbatch.folios[i]);
> >>>> +		for (i = 0; i < folio_batch_count(&fbatch); i++) {
> >>>> +			struct folio *folio = fbatch.folios[i];
> >>>> +
> >>>> +			if (folio_test_uncached(folio)) {
> >>>> +				folio_lock(folio);
> >>>> +				invalidate_complete_folio2(mapping, folio, 0);
> >>>> +				folio_unlock(folio);
> >>>
> >>> I am not sure it is safe. What happens if it races with page fault?
> >>>
> >>> The only current caller of invalidate_complete_folio2() unmaps the folio
> >>> explicitly before calling it. And folio lock prevents re-faulting.
> >>>
> >>> I think we need to give up PG_uncached if we see folio_mapped(). And maybe
> >>> also mark the page accessed.
> >>
> >> Ok thanks, let me take a look at that and create a test case that
> >> exercises that explicitly.
> > 
> > It might be worth generalizing it to clearing PG_uncached for any page cache
> > lookups that don't come from RWF_UNCACHED.
> 
> We can do that - you mean at lookup time? Eg have __filemap_get_folio()
> do:
> 
> if (folio_test_uncached(folio) && !(fgp_flags & FGP_UNCACHED))
> 	folio_clear_uncached(folio);
> 
> or do you want this logic just in filemap_read()? Arguably it should
> already clear it in the quoted code above, regardless, eg:
> 
> 	if (folio_test_uncached(folio)) {
> 		folio_lock(folio);
> 		invalidate_complete_folio2(mapping, folio, 0);
> 		folio_clear_uncached(folio);
> 		folio_unlock(folio);
> 	}
> 
> in case invalidation fails.

The point is to leave the folio in page cache if there's a
non-RWF_UNCACHED user of it.

Putting the check in __filemap_get_folio() sounds reasonable.

But I am not 100% sure it would be enough to never get PG_uncached mapped.
Will think about it more.

Anyway, I think we need BUG_ON(folio_mapped(folio)) inside
invalidate_complete_folio2().

Jens Axboe Nov. 11, 2024, 3:57 p.m. UTC | #8

On 11/11/24 8:51 AM, Kirill A. Shutemov wrote:
> On Mon, Nov 11, 2024 at 08:31:28AM -0700, Jens Axboe wrote:
>> On 11/11/24 8:25 AM, Kirill A. Shutemov wrote:
>>> On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
>>>> On 11/11/24 2:15 AM, Kirill A. Shutemov wrote:
>>>>>> @@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
>>>>>>  			}
>>>>>>  		}
>>>>>>  put_folios:
>>>>>> -		for (i = 0; i < folio_batch_count(&fbatch); i++)
>>>>>> -			folio_put(fbatch.folios[i]);
>>>>>> +		for (i = 0; i < folio_batch_count(&fbatch); i++) {
>>>>>> +			struct folio *folio = fbatch.folios[i];
>>>>>> +
>>>>>> +			if (folio_test_uncached(folio)) {
>>>>>> +				folio_lock(folio);
>>>>>> +				invalidate_complete_folio2(mapping, folio, 0);
>>>>>> +				folio_unlock(folio);
>>>>>
>>>>> I am not sure it is safe. What happens if it races with page fault?
>>>>>
>>>>> The only current caller of invalidate_complete_folio2() unmaps the folio
>>>>> explicitly before calling it. And folio lock prevents re-faulting.
>>>>>
>>>>> I think we need to give up PG_uncached if we see folio_mapped(). And maybe
>>>>> also mark the page accessed.
>>>>
>>>> Ok thanks, let me take a look at that and create a test case that
>>>> exercises that explicitly.
>>>
>>> It might be worth generalizing it to clearing PG_uncached for any page cache
>>> lookups that don't come from RWF_UNCACHED.
>>
>> We can do that - you mean at lookup time? Eg have __filemap_get_folio()
>> do:
>>
>> if (folio_test_uncached(folio) && !(fgp_flags & FGP_UNCACHED))
>> 	folio_clear_uncached(folio);
>>
>> or do you want this logic just in filemap_read()? Arguably it should
>> already clear it in the quoted code above, regardless, eg:
>>
>> 	if (folio_test_uncached(folio)) {
>> 		folio_lock(folio);
>> 		invalidate_complete_folio2(mapping, folio, 0);
>> 		folio_clear_uncached(folio);
>> 		folio_unlock(folio);
>> 	}
>>
>> in case invalidation fails.
> 
> The point is to leave the folio in page cache if there's a
> non-RWF_UNCACHED user of it.

Right. The uncached flag should be ephemeral, hitting it should be
relatively rare. But if it does happen, yeah we should leave the page in
cache.

> Putting the check in __filemap_get_folio() sounds reasonable.

OK will do.

> But I am not 100% sure it would be enough to never get PG_uncached mapped.
> Will think about it more.

Thanks!

> Anyway, I think we need BUG_ON(folio_mapped(folio)) inside
> invalidate_complete_folio2().

Isn't that a bit rough? Maybe just a:

if (WARN_ON_ONCE(folio_mapped(folio)))
	return;

would do? I'm happy to do either one, let me know what you prefer.

Kirill A. Shutemov Nov. 11, 2024, 4:29 p.m. UTC | #9

On Mon, Nov 11, 2024 at 08:57:17AM -0700, Jens Axboe wrote:
> On 11/11/24 8:51 AM, Kirill A. Shutemov wrote:
> > On Mon, Nov 11, 2024 at 08:31:28AM -0700, Jens Axboe wrote:
> >> On 11/11/24 8:25 AM, Kirill A. Shutemov wrote:
> >>> On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
> >>>> On 11/11/24 2:15 AM, Kirill A. Shutemov wrote:
> >>>>>> @@ -2706,8 +2712,16 @@ ssize_t filemap_read(struct kiocb *iocb, struct iov_iter *iter,
> >>>>>>  			}
> >>>>>>  		}
> >>>>>>  put_folios:
> >>>>>> -		for (i = 0; i < folio_batch_count(&fbatch); i++)
> >>>>>> -			folio_put(fbatch.folios[i]);
> >>>>>> +		for (i = 0; i < folio_batch_count(&fbatch); i++) {
> >>>>>> +			struct folio *folio = fbatch.folios[i];
> >>>>>> +
> >>>>>> +			if (folio_test_uncached(folio)) {
> >>>>>> +				folio_lock(folio);
> >>>>>> +				invalidate_complete_folio2(mapping, folio, 0);
> >>>>>> +				folio_unlock(folio);
> >>>>>
> >>>>> I am not sure it is safe. What happens if it races with page fault?
> >>>>>
> >>>>> The only current caller of invalidate_complete_folio2() unmaps the folio
> >>>>> explicitly before calling it. And folio lock prevents re-faulting.
> >>>>>
> >>>>> I think we need to give up PG_uncached if we see folio_mapped(). And maybe
> >>>>> also mark the page accessed.
> >>>>
> >>>> Ok thanks, let me take a look at that and create a test case that
> >>>> exercises that explicitly.
> >>>
> >>> It might be worth generalizing it to clearing PG_uncached for any page cache
> >>> lookups that don't come from RWF_UNCACHED.
> >>
> >> We can do that - you mean at lookup time? Eg have __filemap_get_folio()
> >> do:
> >>
> >> if (folio_test_uncached(folio) && !(fgp_flags & FGP_UNCACHED))
> >> 	folio_clear_uncached(folio);
> >>
> >> or do you want this logic just in filemap_read()? Arguably it should
> >> already clear it in the quoted code above, regardless, eg:
> >>
> >> 	if (folio_test_uncached(folio)) {
> >> 		folio_lock(folio);
> >> 		invalidate_complete_folio2(mapping, folio, 0);
> >> 		folio_clear_uncached(folio);
> >> 		folio_unlock(folio);
> >> 	}
> >>
> >> in case invalidation fails.
> > 
> > The point is to leave the folio in page cache if there's a
> > non-RWF_UNCACHED user of it.
> 
> Right. The uncached flag should be ephemeral, hitting it should be
> relatively rare. But if it does happen, yeah we should leave the page in
> cache.
> 
> > Putting the check in __filemap_get_folio() sounds reasonable.
> 
> OK will do.
> 
> > But I am not 100% sure it would be enough to never get PG_uncached mapped.
> > Will think about it more.
> 
> Thanks!
> 
> > Anyway, I think we need BUG_ON(folio_mapped(folio)) inside
> > invalidate_complete_folio2().
> 
> Isn't that a bit rough? Maybe just a:
> 
> if (WARN_ON_ONCE(folio_mapped(folio)))
> 	return;
> 
> would do? I'm happy to do either one, let me know what you prefer.

I suggested BUG_ON() because current caller has it. But, yeah, WARN() is
good enough.

Jens Axboe Nov. 11, 2024, 5:09 p.m. UTC | #10

On 11/11/24 8:17 AM, Jens Axboe wrote:
> On 11/11/24 8:16 AM, Christoph Hellwig wrote:
>> On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
>>> Ok thanks, let me take a look at that and create a test case that
>>> exercises that explicitly.
>>
>> Please add RWF_UNCACHED to fsstress.c in xfstests also.  That is our
>> exerciser for concurrent issuing of different I/O types to hit these
>> kinds of corner cases.
> 
> Sure, can do.

Not familiar with fsstress at all, but something like the below? Will
use it if available, if it gets EOPNOTSUPP it'll just fallback to
using writev_f()/readv_f() instead.

Did give it a quick test spin and I see uncached reads and writes on the
kernel that supports it.

diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index 3d248ee25791..6430f10efbc7 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -82,6 +82,12 @@ static int renameat2(int dfd1, const char *path1,
 #define RENAME_WHITEOUT		(1 << 2)	/* Whiteout source */
 #endif
 
+#ifndef RWF_UNCACHED
+#define RWF_UNCACHED		0x80
+#endif
+
+static int have_rwf_uncached = 1;
+
 #define FILELEN_MAX		(32*4096)
 
 typedef enum {
@@ -117,6 +123,7 @@ typedef enum {
 	OP_COLLAPSE,
 	OP_INSERT,
 	OP_READ,
+	OP_READ_UNCACHED,
 	OP_READLINK,
 	OP_READV,
 	OP_REMOVEFATTR,
@@ -143,6 +150,7 @@ typedef enum {
 	OP_URING_READ,
 	OP_URING_WRITE,
 	OP_WRITE,
+	OP_WRITE_UNCACHED,
 	OP_WRITEV,
 	OP_EXCHANGE_RANGE,
 	OP_LAST
@@ -248,6 +256,7 @@ void	zero_f(opnum_t, long);
 void	collapse_f(opnum_t, long);
 void	insert_f(opnum_t, long);
 void	unshare_f(opnum_t, long);
+void	read_uncached_f(opnum_t, long);
 void	read_f(opnum_t, long);
 void	readlink_f(opnum_t, long);
 void	readv_f(opnum_t, long);
@@ -273,6 +282,7 @@ void	unlink_f(opnum_t, long);
 void	unresvsp_f(opnum_t, long);
 void	uring_read_f(opnum_t, long);
 void	uring_write_f(opnum_t, long);
+void	write_uncached_f(opnum_t, long);
 void	write_f(opnum_t, long);
 void	writev_f(opnum_t, long);
 void	exchangerange_f(opnum_t, long);
@@ -315,6 +325,7 @@ struct opdesc	ops[OP_LAST]	= {
 	[OP_COLLAPSE]	   = {"collapse",      collapse_f,	1, 1 },
 	[OP_INSERT]	   = {"insert",	       insert_f,	1, 1 },
 	[OP_READ]	   = {"read",	       read_f,		1, 0 },
+	[OP_READ_UNCACHED] = {"read_uncached", read_uncached_f,	1, 0 },
 	[OP_READLINK]	   = {"readlink",      readlink_f,	1, 0 },
 	[OP_READV]	   = {"readv",	       readv_f,		1, 0 },
 	/* remove (delete) extended attribute */
@@ -346,6 +357,7 @@ struct opdesc	ops[OP_LAST]	= {
 	[OP_URING_WRITE]   = {"uring_write",   uring_write_f,	1, 1 },
 	[OP_WRITE]	   = {"write",	       write_f,		4, 1 },
 	[OP_WRITEV]	   = {"writev",	       writev_f,	4, 1 },
+	[OP_WRITE_UNCACHED]= {"write_uncaced", write_uncached_f,4, 1 },
 	[OP_EXCHANGE_RANGE]= {"exchangerange", exchangerange_f,	2, 1 },
 }, *ops_end;
 
@@ -4635,6 +4647,76 @@ readv_f(opnum_t opno, long r)
 	close(fd);
 }
 
+void
+read_uncached_f(opnum_t opno, long r)
+{
+	int		e;
+	pathname_t	f;
+	int		fd;
+	int64_t		lr;
+	off64_t		off;
+	struct stat64	stb;
+	int		v;
+	char		st[1024];
+	struct iovec	iov;
+
+	if (!have_rwf_uncached) {
+		readv_f(opno, r);
+		return;
+	}
+
+	init_pathname(&f);
+	if (!get_fname(FT_REGFILE, r, &f, NULL, NULL, &v)) {
+		if (v)
+			printf("%d/%lld: read - no filename\n", procid, opno);
+		free_pathname(&f);
+		return;
+	}
+	fd = open_path(&f, O_RDONLY);
+	e = fd < 0 ? errno : 0;
+	check_cwd();
+	if (fd < 0) {
+		if (v)
+			printf("%d/%lld: read - open %s failed %d\n",
+				procid, opno, f.path, e);
+		free_pathname(&f);
+		return;
+	}
+	if (fstat64(fd, &stb) < 0) {
+		if (v)
+			printf("%d/%lld: read - fstat64 %s failed %d\n",
+				procid, opno, f.path, errno);
+		free_pathname(&f);
+		close(fd);
+		return;
+	}
+	inode_info(st, sizeof(st), &stb, v);
+	if (stb.st_size == 0) {
+		if (v)
+			printf("%d/%lld: read - %s%s zero size\n", procid, opno,
+			       f.path, st);
+		free_pathname(&f);
+		close(fd);
+		return;
+	}
+	lr = ((int64_t)random() << 32) + random();
+	off = (off64_t)(lr % stb.st_size);
+	iov.iov_len = (random() % FILELEN_MAX) + 1;
+	iov.iov_base = malloc(iov.iov_len);
+	e = preadv2(fd, &iov, 1, off, RWF_UNCACHED) < 0 ? errno : 0;
+	if (e == EOPNOTSUPP) {
+		have_rwf_uncached = 0;
+		e = 0;
+	}
+	free(iov.iov_base);
+	if (v)
+		printf("%d/%lld: read uncached %s%s [%lld,%d] %d\n",
+		       procid, opno, f.path, st, (long long)off,
+		       (int)iov.iov_len, e);
+	free_pathname(&f);
+	close(fd);
+}
+
 void
 removefattr_f(opnum_t opno, long r)
 {
@@ -5509,6 +5591,70 @@ writev_f(opnum_t opno, long r)
 	close(fd);
 }
 
+void
+write_uncached_f(opnum_t opno, long r)
+{
+	int		e;
+	pathname_t	f;
+	int		fd;
+	int64_t		lr;
+	off64_t		off;
+	struct stat64	stb;
+	int		v;
+	char		st[1024];
+	struct iovec	iov;
+
+	if (!have_rwf_uncached) {
+		writev_f(opno, r);
+		return;
+	}
+
+	init_pathname(&f);
+	if (!get_fname(FT_REGm, r, &f, NULL, NULL, &v)) {
+		if (v)
+			printf("%d/%lld: write - no filename\n", procid, opno);
+		free_pathname(&f);
+		return;
+	}
+	fd = open_path(&f, O_WRONLY);
+	e = fd < 0 ? errno : 0;
+	check_cwd();
+	if (fd < 0) {
+		if (v)
+			printf("%d/%lld: write - open %s failed %d\n",
+				procid, opno, f.path, e);
+		free_pathname(&f);
+		return;
+	}
+	if (fstat64(fd, &stb) < 0) {
+		if (v)
+			printf("%d/%lld: write - fstat64 %s failed %d\n",
+				procid, opno, f.path, errno);
+		free_pathname(&f);
+		close(fd);
+		return;
+	}
+	inode_info(st, sizeof(st), &stb, v);
+	lr = ((int64_t)random() << 32) + random();
+	off = (off64_t)(lr % MIN(stb.st_size + (1024 * 1024), MAXFSIZE));
+	off %= maxfsize;
+	iov.iov_len = (random() % FILELEN_MAX) + 1;
+	iov.iov_base = malloc(iov.iov_len);
+	memset(iov.iov_base, nameseq & 0xff, iov.iov_len);
+	e = pwritev2(fd, &iov, 1, off, RWF_UNCACHED) < 0 ? errno : 0;
+	free(iov.iov_base);
+	if (v)
+		printf("%d/%lld: write uncached %s%s [%lld,%d] %d\n",
+		       procid, opno, f.path, st, (long long)off,
+		       (int)iov.iov_len, e);
+	free_pathname(&f);
+	close(fd);
+	if (e == EOPNOTSUPP) {
+		writev_f(opno, r);
+		have_rwf_uncached = 0;
+	}
+}
+
 char *
 xattr_flag_to_string(int flag)
 {

Matthew Wilcox Nov. 11, 2024, 5:25 p.m. UTC | #11

On Sun, Nov 10, 2024 at 08:27:52AM -0700, Jens Axboe wrote:
> 5 years ago I posted patches adding support for RWF_UNCACHED, as a way
> to do buffered IO that isn't page cache persistent. The approach back
> then was to have private pages for IO, and then get rid of them once IO
> was done. But that then runs into all the issues that O_DIRECT has, in
> terms of synchronizing with the page cache.

Today's a holiday, and I suspect you're going to do a v3 before I have
a chance to do a proper review of this version of the series.

I think "uncached" isn't quite the right word.  Perhaps 'RWF_STREAMING'
so that userspace is indicating that this is a streaming I/O and the
kernel gets to choose what to do with that information.

Also, do we want to fail I/Os to filesystems which don't support
it?  I suppose really sophisticated userspace might fall back to
madvise(DONTNEED), but isn't most userspace going to just clear the flag
and retry the I/O?

Um.  Now I've looked, we also have posix_fadvise(POSIX_FADV_NOREUSE),
which is currently a noop.  But would we be better off honouring
POSIX_FADV_NOREUSE than introducing RWF_UNCACHED?  I'll think about this
some more while I'm offline.

Jens Axboe Nov. 11, 2024, 5:39 p.m. UTC | #12

On 11/11/24 10:25 AM, Matthew Wilcox wrote:
> On Sun, Nov 10, 2024 at 08:27:52AM -0700, Jens Axboe wrote:
>> 5 years ago I posted patches adding support for RWF_UNCACHED, as a way
>> to do buffered IO that isn't page cache persistent. The approach back
>> then was to have private pages for IO, and then get rid of them once IO
>> was done. But that then runs into all the issues that O_DIRECT has, in
>> terms of synchronizing with the page cache.
> 
> Today's a holiday, and I suspect you're going to do a v3 before I have
> a chance to do a proper review of this version of the series.

Probably, since I've done some fixes since v2 :-). So you can wait for
v3, I'll post it later today anyway.

> I think "uncached" isn't quite the right word.  Perhaps 'RWF_STREAMING'
> so that userspace is indicating that this is a streaming I/O and the
> kernel gets to choose what to do with that information.

Yeah not sure, it's the one I used back in the day, and I still haven't
found a more descriptive word for it. That doesn't mean one doesn't
exist, certainly taking suggestions. I don't think STREAMING is the
right one however, you could most certainly be doing random uncached IO.

> Also, do we want to fail I/Os to filesystems which don't support
> it?  I suppose really sophisticated userspace might fall back to
> madvise(DONTNEED), but isn't most userspace going to just clear the flag
> and retry the I/O?

Also something that's a bit undecided, you can make arguments for both
ways. For just ignoring the flag if not support, the argument would be
that the application just wants to do IO, uncached if available. For the
other argument, maybe you have an application that wants to fallback to
O_DIRECT if uncached isn't available. That application certainly wants
to know if it works or not.

Which is why I defaulted to return -EOPNOTSUPP if it's not available.
An applicaton may probe this upfront if it so desires, and just not set
the flag for IO. That'd keep it out of the hot path.

Seems to me that returning whether it's supported or not is the path of
least surprises for applications, which is why I went that way.

> Um.  Now I've looked, we also have posix_fadvise(POSIX_FADV_NOREUSE),
> which is currently a noop.  But would we be better off honouring
> POSIX_FADV_NOREUSE than introducing RWF_UNCACHED?  I'll think about this
> some more while I'm offline.

That would certainly work too, for synchronous IO. But per-file hints
are a bad idea for async IO, for obvious reasons. We really want per-IO
hints for that, we have a long history of messing that up. That doesn't
mean that FMODE_NOREUSE couldn't just set RWF_UNCACHED, if it's set.
That'd be trivial.

Then the next question is if setting POSIX_FADV_NOREUSE should fail of
file->f_op->fop_flags & FOP_UNCACHED isn't true. Probably not, since
it'd potentially break applications. So probably best to just set
f_iocb_flags IFF FOP_UNCACHED is true for that file.

And the bigger question is why on earth do we have this thing in the
kernel that doesn't do anything... But yeah, now we could make it do
something.

Yu Zhao Nov. 11, 2024, 9:24 p.m. UTC | #13

On Mon, Nov 11, 2024 at 10:25 AM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Sun, Nov 10, 2024 at 08:27:52AM -0700, Jens Axboe wrote:
> > 5 years ago I posted patches adding support for RWF_UNCACHED, as a way
> > to do buffered IO that isn't page cache persistent. The approach back
> > then was to have private pages for IO, and then get rid of them once IO
> > was done. But that then runs into all the issues that O_DIRECT has, in
> > terms of synchronizing with the page cache.
>
> Today's a holiday, and I suspect you're going to do a v3 before I have
> a chance to do a proper review of this version of the series.
>
> I think "uncached" isn't quite the right word.  Perhaps 'RWF_STREAMING'
> so that userspace is indicating that this is a streaming I/O and the
> kernel gets to choose what to do with that information.
>
> Also, do we want to fail I/Os to filesystems which don't support
> it?  I suppose really sophisticated userspace might fall back to
> madvise(DONTNEED), but isn't most userspace going to just clear the flag
> and retry the I/O?
>
> Um.  Now I've looked, we also have posix_fadvise(POSIX_FADV_NOREUSE),
> which is currently a noop.

Just to clarify that NOREUSE is NOT a noop since commit 17e8102 ("mm:
support POSIX_FADV_NOREUSE"). And it had at least one user (we made
the userpspace change after the kernel supported it): SVT-AV1 [1]; it
was also added to FIO for testing purposes.

[1] https://gitlab.com/AOMediaCodec/SVT-AV1

> But would we be better off honouring
> POSIX_FADV_NOREUSE than introducing RWF_UNCACHED?  I'll think about this
> some more while I'm offline.

But I guess the flag isn't honored the way UNCACHED works?

Matthew Wilcox Nov. 11, 2024, 9:48 p.m. UTC | #14

On Mon, Nov 11, 2024 at 02:24:54PM -0700, Yu Zhao wrote:
> Just to clarify that NOREUSE is NOT a noop since commit 17e8102 ("mm:

maybe you should send a patch to the manpage?

Yu Zhao Nov. 11, 2024, 10:07 p.m. UTC | #15

On Mon, Nov 11, 2024 at 2:48 PM Matthew Wilcox <willy@infradead.org> wrote:
>
> On Mon, Nov 11, 2024 at 02:24:54PM -0700, Yu Zhao wrote:
> > Just to clarify that NOREUSE is NOT a noop since commit 17e8102 ("mm:
>
> maybe you should send a patch to the manpage?

I was under the impression that our engineers took care of that. But
apparently it's still pending:
https://lore.kernel.org/linux-man/20230320222057.1976956-1-talumbau@google.com/

Will find someone else to follow up on that.

Jens Axboe Nov. 11, 2024, 11:42 p.m. UTC | #16

On 11/11/24 10:09 AM, Jens Axboe wrote:
> On 11/11/24 8:17 AM, Jens Axboe wrote:
>> On 11/11/24 8:16 AM, Christoph Hellwig wrote:
>>> On Mon, Nov 11, 2024 at 07:12:35AM -0700, Jens Axboe wrote:
>>>> Ok thanks, let me take a look at that and create a test case that
>>>> exercises that explicitly.
>>>
>>> Please add RWF_UNCACHED to fsstress.c in xfstests also.  That is our
>>> exerciser for concurrent issuing of different I/O types to hit these
>>> kinds of corner cases.
>>
>> Sure, can do.
> 
> Not familiar with fsstress at all, but something like the below? Will
> use it if available, if it gets EOPNOTSUPP it'll just fallback to
> using writev_f()/readv_f() instead.
> 
> Did give it a quick test spin and I see uncached reads and writes on the
> kernel that supports it.

Here's the slightly cleaned up version, this is the one I ran testing
with.

diff --git a/ltp/fsstress.c b/ltp/fsstress.c
index 3d248ee25791..a06ba300a1d7 100644
--- a/ltp/fsstress.c
+++ b/ltp/fsstress.c
@@ -82,6 +82,12 @@ static int renameat2(int dfd1, const char *path1,
 #define RENAME_WHITEOUT		(1 << 2)	/* Whiteout source */
 #endif
 
+#ifndef RWF_UNCACHED
+#define RWF_UNCACHED		0x80
+#endif
+
+static int have_rwf_uncached = 1;
+
 #define FILELEN_MAX		(32*4096)
 
 typedef enum {
@@ -117,6 +123,7 @@ typedef enum {
 	OP_COLLAPSE,
 	OP_INSERT,
 	OP_READ,
+	OP_READ_UNCACHED,
 	OP_READLINK,
 	OP_READV,
 	OP_REMOVEFATTR,
@@ -143,6 +150,7 @@ typedef enum {
 	OP_URING_READ,
 	OP_URING_WRITE,
 	OP_WRITE,
+	OP_WRITE_UNCACHED,
 	OP_WRITEV,
 	OP_EXCHANGE_RANGE,
 	OP_LAST
@@ -248,6 +256,7 @@ void	zero_f(opnum_t, long);
 void	collapse_f(opnum_t, long);
 void	insert_f(opnum_t, long);
 void	unshare_f(opnum_t, long);
+void	read_uncached_f(opnum_t, long);
 void	read_f(opnum_t, long);
 void	readlink_f(opnum_t, long);
 void	readv_f(opnum_t, long);
@@ -273,6 +282,7 @@ void	unlink_f(opnum_t, long);
 void	unresvsp_f(opnum_t, long);
 void	uring_read_f(opnum_t, long);
 void	uring_write_f(opnum_t, long);
+void	write_uncached_f(opnum_t, long);
 void	write_f(opnum_t, long);
 void	writev_f(opnum_t, long);
 void	exchangerange_f(opnum_t, long);
@@ -315,6 +325,7 @@ struct opdesc	ops[OP_LAST]	= {
 	[OP_COLLAPSE]	   = {"collapse",      collapse_f,	1, 1 },
 	[OP_INSERT]	   = {"insert",	       insert_f,	1, 1 },
 	[OP_READ]	   = {"read",	       read_f,		1, 0 },
+	[OP_READ_UNCACHED] = {"read_uncached", read_uncached_f,	1, 0 },
 	[OP_READLINK]	   = {"readlink",      readlink_f,	1, 0 },
 	[OP_READV]	   = {"readv",	       readv_f,		1, 0 },
 	/* remove (delete) extended attribute */
@@ -346,6 +357,7 @@ struct opdesc	ops[OP_LAST]	= {
 	[OP_URING_WRITE]   = {"uring_write",   uring_write_f,	1, 1 },
 	[OP_WRITE]	   = {"write",	       write_f,		4, 1 },
 	[OP_WRITEV]	   = {"writev",	       writev_f,	4, 1 },
+	[OP_WRITE_UNCACHED]= {"write_uncaced", write_uncached_f,4, 1 },
 	[OP_EXCHANGE_RANGE]= {"exchangerange", exchangerange_f,	2, 1 },
 }, *ops_end;
 
@@ -4635,6 +4647,71 @@ readv_f(opnum_t opno, long r)
 	close(fd);
 }
 
+void
+read_uncached_f(opnum_t opno, long r)
+{
+	int		e;
+	pathname_t	f;
+	int		fd;
+	int64_t		lr;
+	off64_t		off;
+	struct stat64	stb;
+	int		v;
+	char		st[1024];
+	struct iovec	iov;
+	int flags;
+
+	init_pathname(&f);
+	if (!get_fname(FT_REGFILE, r, &f, NULL, NULL, &v)) {
+		if (v)
+			printf("%d/%lld: read - no filename\n", procid, opno);
+		free_pathname(&f);
+		return;
+	}
+	fd = open_path(&f, O_RDONLY);
+	e = fd < 0 ? errno : 0;
+	check_cwd();
+	if (fd < 0) {
+		if (v)
+			printf("%d/%lld: read - open %s failed %d\n",
+				procid, opno, f.path, e);
+		free_pathname(&f);
+		return;
+	}
+	if (fstat64(fd, &stb) < 0) {
+		if (v)
+			printf("%d/%lld: read - fstat64 %s failed %d\n",
+				procid, opno, f.path, errno);
+		free_pathname(&f);
+		close(fd);
+		return;
+	}
+	inode_info(st, sizeof(st), &stb, v);
+	if (stb.st_size == 0) {
+		if (v)
+			printf("%d/%lld: read - %s%s zero size\n", procid, opno,
+			       f.path, st);
+		free_pathname(&f);
+		close(fd);
+		return;
+	}
+	lr = ((int64_t)random() << 32) + random();
+	off = (off64_t)(lr % stb.st_size);
+	iov.iov_len = (random() % FILELEN_MAX) + 1;
+	iov.iov_base = malloc(iov.iov_len);
+	flags = have_rwf_uncached ? RWF_UNCACHED : 0;
+	e = preadv2(fd, &iov, 1, off, flags) < 0 ? errno : 0;
+	if (have_rwf_uncached && e == EOPNOTSUPP)
+		e = preadv2(fd, &iov, 1, off, 0) < 0 ? errno : 0;
+	free(iov.iov_base);
+	if (v)
+		printf("%d/%lld: read uncached %s%s [%lld,%d] %d\n",
+		       procid, opno, f.path, st, (long long)off,
+		       (int)iov.iov_len, e);
+	free_pathname(&f);
+	close(fd);
+}
+
 void
 removefattr_f(opnum_t opno, long r)
 {
@@ -5509,6 +5586,65 @@ writev_f(opnum_t opno, long r)
 	close(fd);
 }
 
+void
+write_uncached_f(opnum_t opno, long r)
+{
+	int		e;
+	pathname_t	f;
+	int		fd;
+	int64_t		lr;
+	off64_t		off;
+	struct stat64	stb;
+	int		v;
+	char		st[1024];
+	struct iovec	iov;
+	int flags;
+
+	init_pathname(&f);
+	if (!get_fname(FT_REGm, r, &f, NULL, NULL, &v)) {
+		if (v)
+			printf("%d/%lld: write - no filename\n", procid, opno);
+		free_pathname(&f);
+		return;
+	}
+	fd = open_path(&f, O_WRONLY);
+	e = fd < 0 ? errno : 0;
+	check_cwd();
+	if (fd < 0) {
+		if (v)
+			printf("%d/%lld: write - open %s failed %d\n",
+				procid, opno, f.path, e);
+		free_pathname(&f);
+		return;
+	}
+	if (fstat64(fd, &stb) < 0) {
+		if (v)
+			printf("%d/%lld: write - fstat64 %s failed %d\n",
+				procid, opno, f.path, errno);
+		free_pathname(&f);
+		close(fd);
+		return;
+	}
+	inode_info(st, sizeof(st), &stb, v);
+	lr = ((int64_t)random() << 32) + random();
+	off = (off64_t)(lr % MIN(stb.st_size + (1024 * 1024), MAXFSIZE));
+	off %= maxfsize;
+	iov.iov_len = (random() % FILELEN_MAX) + 1;
+	iov.iov_base = malloc(iov.iov_len);
+	memset(iov.iov_base, nameseq & 0xff, iov.iov_len);
+	flags = have_rwf_uncached ? RWF_UNCACHED : 0;
+	e = pwritev2(fd, &iov, 1, off, flags) < 0 ? errno : 0;
+	if (have_rwf_uncached && e == EOPNOTSUPP)
+		e = pwritev2(fd, &iov, 1, off, 0) < 0 ? errno : 0;
+	free(iov.iov_base);
+	if (v)
+		printf("%d/%lld: write uncached %s%s [%lld,%d] %d\n",
+		       procid, opno, f.path, st, (long long)off,
+		       (int)iov.iov_len, e);
+	free_pathname(&f);
+	close(fd);
+}
+
 char *
 xattr_flag_to_string(int flag)
 {

Christoph Hellwig Nov. 12, 2024, 5:13 a.m. UTC | #17

On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
> Here's the slightly cleaned up version, this is the one I ran testing
> with.

Looks reasonable to me, but you probably get better reviews on the
fstests lists.

Jens Axboe Nov. 12, 2024, 3:14 p.m. UTC | #18

On 11/11/24 10:13 PM, Christoph Hellwig wrote:
> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
>> Here's the slightly cleaned up version, this is the one I ran testing
>> with.
> 
> Looks reasonable to me, but you probably get better reviews on the
> fstests lists.

I'll send it out once this patchset is a bit closer to integration,
there's the usual chicken and egg situation with it. For now, it's quite
handy for my testing, found a few issues with this version. So thanks
for the suggestion, sure beats writing more of your own test cases :-)

Brian Foster Nov. 12, 2024, 4:39 p.m. UTC | #19

On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
> > On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
> >> Here's the slightly cleaned up version, this is the one I ran testing
> >> with.
> > 
> > Looks reasonable to me, but you probably get better reviews on the
> > fstests lists.
> 
> I'll send it out once this patchset is a bit closer to integration,
> there's the usual chicken and egg situation with it. For now, it's quite
> handy for my testing, found a few issues with this version. So thanks
> for the suggestion, sure beats writing more of your own test cases :-)
> 

fsx support is probably a good idea as well. It's similar in idea to
fsstress, but bashes the same file with mixed operations and includes
data integrity validation checks as well. It's pretty useful for
uncovering subtle corner case issues or bad interactions..

Brian

> -- 
> Jens Axboe
>

Jens Axboe Nov. 12, 2024, 5:06 p.m. UTC | #20

On 11/12/24 9:39 AM, Brian Foster wrote:
> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
>>>> Here's the slightly cleaned up version, this is the one I ran testing
>>>> with.
>>>
>>> Looks reasonable to me, but you probably get better reviews on the
>>> fstests lists.
>>
>> I'll send it out once this patchset is a bit closer to integration,
>> there's the usual chicken and egg situation with it. For now, it's quite
>> handy for my testing, found a few issues with this version. So thanks
>> for the suggestion, sure beats writing more of your own test cases :-)
>>
> 
> fsx support is probably a good idea as well. It's similar in idea to
> fsstress, but bashes the same file with mixed operations and includes
> data integrity validation checks as well. It's pretty useful for
> uncovering subtle corner case issues or bad interactions..

Indeed, I did that too. Re-running xfstests right now with that too.

Jens Axboe Nov. 12, 2024, 5:19 p.m. UTC | #21

On 11/12/24 10:06 AM, Jens Axboe wrote:
> On 11/12/24 9:39 AM, Brian Foster wrote:
>> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
>>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
>>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
>>>>> Here's the slightly cleaned up version, this is the one I ran testing
>>>>> with.
>>>>
>>>> Looks reasonable to me, but you probably get better reviews on the
>>>> fstests lists.
>>>
>>> I'll send it out once this patchset is a bit closer to integration,
>>> there's the usual chicken and egg situation with it. For now, it's quite
>>> handy for my testing, found a few issues with this version. So thanks
>>> for the suggestion, sure beats writing more of your own test cases :-)
>>>
>>
>> fsx support is probably a good idea as well. It's similar in idea to
>> fsstress, but bashes the same file with mixed operations and includes
>> data integrity validation checks as well. It's pretty useful for
>> uncovering subtle corner case issues or bad interactions..
> 
> Indeed, I did that too. Re-running xfstests right now with that too.

Here's what I'm running right now, fwiw. It adds RWF_UNCACHED support
for both the sync read/write and io_uring paths.


diff --git a/ltp/fsx.c b/ltp/fsx.c
index 41933354..104910ff 100644
--- a/ltp/fsx.c
+++ b/ltp/fsx.c
@@ -43,6 +43,10 @@
 # define MAP_FILE 0
 #endif
 
+#ifndef RWF_UNCACHED
+#define RWF_UNCACHED	0x80
+#endif
+
 #define NUMPRINTCOLUMNS 32	/* # columns of data to print on each line */
 
 /* Operation flags (bitmask) */
@@ -101,7 +105,9 @@ int			logcount = 0;	/* total ops */
 enum {
 	/* common operations */
 	OP_READ = 0,
+	OP_READ_UNCACHED,
 	OP_WRITE,
+	OP_WRITE_UNCACHED,
 	OP_MAPREAD,
 	OP_MAPWRITE,
 	OP_MAX_LITE,
@@ -190,15 +196,16 @@ int	o_direct;			/* -Z */
 int	aio = 0;
 int	uring = 0;
 int	mark_nr = 0;
+int	rwf_uncached = 1;
 
 int page_size;
 int page_mask;
 int mmap_mask;
-int fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset);
+int fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags);
 #define READ 0
 #define WRITE 1
-#define fsxread(a,b,c,d)	fsx_rw(READ, a,b,c,d)
-#define fsxwrite(a,b,c,d)	fsx_rw(WRITE, a,b,c,d)
+#define fsxread(a,b,c,d,f)	fsx_rw(READ, a,b,c,d,f)
+#define fsxwrite(a,b,c,d,f)	fsx_rw(WRITE, a,b,c,d,f)
 
 struct timespec deadline;
 
@@ -266,7 +273,9 @@ prterr(const char *prefix)
 
 static const char *op_names[] = {
 	[OP_READ] = "read",
+	[OP_READ_UNCACHED] = "read_uncached",
 	[OP_WRITE] = "write",
+	[OP_WRITE_UNCACHED] = "write_uncached",
 	[OP_MAPREAD] = "mapread",
 	[OP_MAPWRITE] = "mapwrite",
 	[OP_TRUNCATE] = "truncate",
@@ -393,12 +402,14 @@ logdump(void)
 				prt("\t******WWWW");
 			break;
 		case OP_READ:
+		case OP_READ_UNCACHED:
 			prt("READ     0x%x thru 0x%x\t(0x%x bytes)",
 			    lp->args[0], lp->args[0] + lp->args[1] - 1,
 			    lp->args[1]);
 			if (overlap)
 				prt("\t***RRRR***");
 			break;
+		case OP_WRITE_UNCACHED:
 		case OP_WRITE:
 			prt("WRITE    0x%x thru 0x%x\t(0x%x bytes)",
 			    lp->args[0], lp->args[0] + lp->args[1] - 1,
@@ -784,9 +795,8 @@ doflush(unsigned offset, unsigned size)
 }
 
 void
-doread(unsigned offset, unsigned size)
+__doread(unsigned offset, unsigned size, int flags)
 {
-	off_t ret;
 	unsigned iret;
 
 	offset -= offset % readbdy;
@@ -818,23 +828,39 @@ doread(unsigned offset, unsigned size)
 			(monitorend == -1 || offset <= monitorend))))))
 		prt("%lld read\t0x%x thru\t0x%x\t(0x%x bytes)\n", testcalls,
 		    offset, offset + size - 1, size);
-	ret = lseek(fd, (off_t)offset, SEEK_SET);
-	if (ret == (off_t)-1) {
-		prterr("doread: lseek");
-		report_failure(140);
-	}
-	iret = fsxread(fd, temp_buf, size, offset);
+	iret = fsxread(fd, temp_buf, size, offset, flags);
 	if (iret != size) {
-		if (iret == -1)
-			prterr("doread: read");
-		else
+		if (iret == -1) {
+			if (errno == EOPNOTSUPP && flags & RWF_UNCACHED) {
+				rwf_uncached = 1;
+				return;
+			}
+			prterr("dowrite: read");
+		} else {
 			prt("short read: 0x%x bytes instead of 0x%x\n",
 			    iret, size);
+		}
 		report_failure(141);
 	}
 	check_buffers(temp_buf, offset, size);
 }
+void
+doread(unsigned offset, unsigned size)
+{
+	__doread(offset, size, 0);
+}
 
+void
+doread_uncached(unsigned offset, unsigned size)
+{
+	if (rwf_uncached) {
+		__doread(offset, size, RWF_UNCACHED);
+		if (rwf_uncached)
+			return;
+	}
+	__doread(offset, size, 0);
+}
+	
 void
 check_eofpage(char *s, unsigned offset, char *p, int size)
 {
@@ -870,7 +896,6 @@ check_contents(void)
 	unsigned map_offset;
 	unsigned map_size;
 	char *p;
-	off_t ret;
 	unsigned iret;
 
 	if (!check_buf) {
@@ -885,13 +910,7 @@ check_contents(void)
 	if (size == 0)
 		return;
 
-	ret = lseek(fd, (off_t)offset, SEEK_SET);
-	if (ret == (off_t)-1) {
-		prterr("doread: lseek");
-		report_failure(140);
-	}
-
-	iret = fsxread(fd, check_buf, size, offset);
+	iret = fsxread(fd, check_buf, size, offset, 0);
 	if (iret != size) {
 		if (iret == -1)
 			prterr("check_contents: read");
@@ -1064,9 +1083,8 @@ update_file_size(unsigned offset, unsigned size)
 }
 
 void
-dowrite(unsigned offset, unsigned size)
+__dowrite(unsigned offset, unsigned size, int flags)
 {
-	off_t ret;
 	unsigned iret;
 
 	offset -= offset % writebdy;
@@ -1101,18 +1119,18 @@ dowrite(unsigned offset, unsigned size)
 			(monitorend == -1 || offset <= monitorend))))))
 		prt("%lld write\t0x%x thru\t0x%x\t(0x%x bytes)\n", testcalls,
 		    offset, offset + size - 1, size);
-	ret = lseek(fd, (off_t)offset, SEEK_SET);
-	if (ret == (off_t)-1) {
-		prterr("dowrite: lseek");
-		report_failure(150);
-	}
-	iret = fsxwrite(fd, good_buf + offset, size, offset);
+	iret = fsxwrite(fd, good_buf + offset, size, offset, flags);
 	if (iret != size) {
-		if (iret == -1)
+		if (iret == -1) {
+			if (errno == EOPNOTSUPP && flags & RWF_UNCACHED) {
+				rwf_uncached = 0;
+				return;
+			}
 			prterr("dowrite: write");
-		else
+		} else {
 			prt("short write: 0x%x bytes instead of 0x%x\n",
 			    iret, size);
+		}
 		report_failure(151);
 	}
 	if (do_fsync) {
@@ -1126,6 +1144,22 @@ dowrite(unsigned offset, unsigned size)
 	}
 }
 
+void
+dowrite(unsigned offset, unsigned size)
+{
+	__dowrite(offset, size, 0);
+}
+
+void
+dowrite_uncached(unsigned offset, unsigned size)
+{
+	if (rwf_uncached) {
+		__dowrite(offset, size, RWF_UNCACHED);
+		if (rwf_uncached)
+			return;
+	}
+	__dowrite(offset, size, 0);
+}
 
 void
 domapwrite(unsigned offset, unsigned size)
@@ -2340,11 +2374,21 @@ have_op:
 		doread(offset, size);
 		break;
 
+	case OP_READ_UNCACHED:
+		TRIM_OFF_LEN(offset, size, file_size);
+		doread_uncached(offset, size);
+		break;
+
 	case OP_WRITE:
 		TRIM_OFF_LEN(offset, size, maxfilelen);
 		dowrite(offset, size);
 		break;
 
+	case OP_WRITE_UNCACHED:
+		TRIM_OFF_LEN(offset, size, maxfilelen);
+		dowrite_uncached(offset, size);
+		break;
+
 	case OP_MAPREAD:
 		TRIM_OFF_LEN(offset, size, file_size);
 		domapread(offset, size);
@@ -2702,7 +2746,7 @@ uring_setup()
 }
 
 int
-uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
+uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags)
 {
 	struct io_uring_sqe     *sqe;
 	struct io_uring_cqe     *cqe;
@@ -2733,6 +2777,7 @@ uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
 		} else {
 			io_uring_prep_writev(sqe, fd, &iovec, 1, o);
 		}
+		sqe->rw_flags = flags;
 
 		ret = io_uring_submit_and_wait(&ring, 1);
 		if (ret != 1) {
@@ -2781,7 +2826,7 @@ uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
 }
 #else
 int
-uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
+uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags)
 {
 	fprintf(stderr, "io_rw: need IO_URING support!\n");
 	exit(111);
@@ -2789,19 +2834,21 @@ uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
 #endif
 
 int
-fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
+fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags)
 {
 	int ret;
 
 	if (aio) {
 		ret = aio_rw(rw, fd, buf, len, offset);
 	} else if (uring) {
-		ret = uring_rw(rw, fd, buf, len, offset);
+		ret = uring_rw(rw, fd, buf, len, offset, flags);
 	} else {
+		struct iovec iov = { .iov_base = buf, .iov_len = len };
+
 		if (rw == READ)
-			ret = read(fd, buf, len);
+			ret = preadv2(fd, &iov, 1, offset, flags);
 		else
-			ret = write(fd, buf, len);
+			ret = pwritev2(fd, &iov, 1, offset, flags);
 	}
 	return ret;
 }

Brian Foster Nov. 12, 2024, 6:44 p.m. UTC | #22

On Tue, Nov 12, 2024 at 10:19:02AM -0700, Jens Axboe wrote:
> On 11/12/24 10:06 AM, Jens Axboe wrote:
> > On 11/12/24 9:39 AM, Brian Foster wrote:
> >> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
> >>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
> >>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
> >>>>> Here's the slightly cleaned up version, this is the one I ran testing
> >>>>> with.
> >>>>
> >>>> Looks reasonable to me, but you probably get better reviews on the
> >>>> fstests lists.
> >>>
> >>> I'll send it out once this patchset is a bit closer to integration,
> >>> there's the usual chicken and egg situation with it. For now, it's quite
> >>> handy for my testing, found a few issues with this version. So thanks
> >>> for the suggestion, sure beats writing more of your own test cases :-)
> >>>
> >>
> >> fsx support is probably a good idea as well. It's similar in idea to
> >> fsstress, but bashes the same file with mixed operations and includes
> >> data integrity validation checks as well. It's pretty useful for
> >> uncovering subtle corner case issues or bad interactions..
> > 
> > Indeed, I did that too. Re-running xfstests right now with that too.
> 
> Here's what I'm running right now, fwiw. It adds RWF_UNCACHED support
> for both the sync read/write and io_uring paths.
> 

Nice, thanks. Looks reasonable to me at first glance. A few randomish
comments inlined below.

BTW, I should have also mentioned that fsx is also useful for longer
soak testing. I.e., fstests will provide a decent amount of coverage as
is via the various preexisting tests, but I'll occasionally run fsx
directly and let it run overnight or something to get the op count at
least up in the 100 millions or so to have a little more confidence
there isn't some rare/subtle bug lurking. That might be helpful with
something like this. JFYI.

> 
> diff --git a/ltp/fsx.c b/ltp/fsx.c
> index 41933354..104910ff 100644
> --- a/ltp/fsx.c
> +++ b/ltp/fsx.c
> @@ -43,6 +43,10 @@
>  # define MAP_FILE 0
>  #endif
>  
> +#ifndef RWF_UNCACHED
> +#define RWF_UNCACHED	0x80
> +#endif
> +
>  #define NUMPRINTCOLUMNS 32	/* # columns of data to print on each line */
>  
>  /* Operation flags (bitmask) */
> @@ -101,7 +105,9 @@ int			logcount = 0;	/* total ops */
>  enum {
>  	/* common operations */
>  	OP_READ = 0,
> +	OP_READ_UNCACHED,
>  	OP_WRITE,
> +	OP_WRITE_UNCACHED,
>  	OP_MAPREAD,
>  	OP_MAPWRITE,
>  	OP_MAX_LITE,
> @@ -190,15 +196,16 @@ int	o_direct;			/* -Z */
>  int	aio = 0;
>  int	uring = 0;
>  int	mark_nr = 0;
> +int	rwf_uncached = 1;
>  
>  int page_size;
>  int page_mask;
>  int mmap_mask;
> -int fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset);
> +int fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags);
>  #define READ 0
>  #define WRITE 1
> -#define fsxread(a,b,c,d)	fsx_rw(READ, a,b,c,d)
> -#define fsxwrite(a,b,c,d)	fsx_rw(WRITE, a,b,c,d)
> +#define fsxread(a,b,c,d,f)	fsx_rw(READ, a,b,c,d,f)
> +#define fsxwrite(a,b,c,d,f)	fsx_rw(WRITE, a,b,c,d,f)
>  

My pattern recognition brain wants to see an 'e' here. ;)

>  struct timespec deadline;
>  
> @@ -266,7 +273,9 @@ prterr(const char *prefix)
>  
>  static const char *op_names[] = {
>  	[OP_READ] = "read",
> +	[OP_READ_UNCACHED] = "read_uncached",
>  	[OP_WRITE] = "write",
> +	[OP_WRITE_UNCACHED] = "write_uncached",
>  	[OP_MAPREAD] = "mapread",
>  	[OP_MAPWRITE] = "mapwrite",
>  	[OP_TRUNCATE] = "truncate",
> @@ -393,12 +402,14 @@ logdump(void)
>  				prt("\t******WWWW");
>  			break;
>  		case OP_READ:
> +		case OP_READ_UNCACHED:
>  			prt("READ     0x%x thru 0x%x\t(0x%x bytes)",
>  			    lp->args[0], lp->args[0] + lp->args[1] - 1,
>  			    lp->args[1]);
>  			if (overlap)
>  				prt("\t***RRRR***");
>  			break;
> +		case OP_WRITE_UNCACHED:
>  		case OP_WRITE:
>  			prt("WRITE    0x%x thru 0x%x\t(0x%x bytes)",
>  			    lp->args[0], lp->args[0] + lp->args[1] - 1,
> @@ -784,9 +795,8 @@ doflush(unsigned offset, unsigned size)
>  }
>  
>  void
> -doread(unsigned offset, unsigned size)
> +__doread(unsigned offset, unsigned size, int flags)
>  {
> -	off_t ret;
>  	unsigned iret;
>  
>  	offset -= offset % readbdy;
> @@ -818,23 +828,39 @@ doread(unsigned offset, unsigned size)
>  			(monitorend == -1 || offset <= monitorend))))))
>  		prt("%lld read\t0x%x thru\t0x%x\t(0x%x bytes)\n", testcalls,
>  		    offset, offset + size - 1, size);
> -	ret = lseek(fd, (off_t)offset, SEEK_SET);
> -	if (ret == (off_t)-1) {
> -		prterr("doread: lseek");
> -		report_failure(140);
> -	}
> -	iret = fsxread(fd, temp_buf, size, offset);
> +	iret = fsxread(fd, temp_buf, size, offset, flags);
>  	if (iret != size) {
> -		if (iret == -1)
> -			prterr("doread: read");
> -		else
> +		if (iret == -1) {
> +			if (errno == EOPNOTSUPP && flags & RWF_UNCACHED) {
> +				rwf_uncached = 1;

I assume you meant rwf_uncached = 0 here?

If so, check out test_fallocate() and friends to see how various
operations are tested for support before the test starts. Following that
might clean things up a bit.

Also it's useful to have a CLI option to enable/disable individual
features. That tends to be helpful to narrow things down when it does
happen to explode and you want to narrow down the cause.

Brian

> +				return;
> +			}
> +			prterr("dowrite: read");
> +		} else {
>  			prt("short read: 0x%x bytes instead of 0x%x\n",
>  			    iret, size);
> +		}
>  		report_failure(141);
>  	}
>  	check_buffers(temp_buf, offset, size);
>  }
> +void
> +doread(unsigned offset, unsigned size)
> +{
> +	__doread(offset, size, 0);
> +}
>  
> +void
> +doread_uncached(unsigned offset, unsigned size)
> +{
> +	if (rwf_uncached) {
> +		__doread(offset, size, RWF_UNCACHED);
> +		if (rwf_uncached)
> +			return;
> +	}
> +	__doread(offset, size, 0);
> +}
> +	
>  void
>  check_eofpage(char *s, unsigned offset, char *p, int size)
>  {
> @@ -870,7 +896,6 @@ check_contents(void)
>  	unsigned map_offset;
>  	unsigned map_size;
>  	char *p;
> -	off_t ret;
>  	unsigned iret;
>  
>  	if (!check_buf) {
> @@ -885,13 +910,7 @@ check_contents(void)
>  	if (size == 0)
>  		return;
>  
> -	ret = lseek(fd, (off_t)offset, SEEK_SET);
> -	if (ret == (off_t)-1) {
> -		prterr("doread: lseek");
> -		report_failure(140);
> -	}
> -
> -	iret = fsxread(fd, check_buf, size, offset);
> +	iret = fsxread(fd, check_buf, size, offset, 0);
>  	if (iret != size) {
>  		if (iret == -1)
>  			prterr("check_contents: read");
> @@ -1064,9 +1083,8 @@ update_file_size(unsigned offset, unsigned size)
>  }
>  
>  void
> -dowrite(unsigned offset, unsigned size)
> +__dowrite(unsigned offset, unsigned size, int flags)
>  {
> -	off_t ret;
>  	unsigned iret;
>  
>  	offset -= offset % writebdy;
> @@ -1101,18 +1119,18 @@ dowrite(unsigned offset, unsigned size)
>  			(monitorend == -1 || offset <= monitorend))))))
>  		prt("%lld write\t0x%x thru\t0x%x\t(0x%x bytes)\n", testcalls,
>  		    offset, offset + size - 1, size);
> -	ret = lseek(fd, (off_t)offset, SEEK_SET);
> -	if (ret == (off_t)-1) {
> -		prterr("dowrite: lseek");
> -		report_failure(150);
> -	}
> -	iret = fsxwrite(fd, good_buf + offset, size, offset);
> +	iret = fsxwrite(fd, good_buf + offset, size, offset, flags);
>  	if (iret != size) {
> -		if (iret == -1)
> +		if (iret == -1) {
> +			if (errno == EOPNOTSUPP && flags & RWF_UNCACHED) {
> +				rwf_uncached = 0;
> +				return;
> +			}
>  			prterr("dowrite: write");
> -		else
> +		} else {
>  			prt("short write: 0x%x bytes instead of 0x%x\n",
>  			    iret, size);
> +		}
>  		report_failure(151);
>  	}
>  	if (do_fsync) {
> @@ -1126,6 +1144,22 @@ dowrite(unsigned offset, unsigned size)
>  	}
>  }
>  
> +void
> +dowrite(unsigned offset, unsigned size)
> +{
> +	__dowrite(offset, size, 0);
> +}
> +
> +void
> +dowrite_uncached(unsigned offset, unsigned size)
> +{
> +	if (rwf_uncached) {
> +		__dowrite(offset, size, RWF_UNCACHED);
> +		if (rwf_uncached)
> +			return;
> +	}
> +	__dowrite(offset, size, 0);
> +}
>  
>  void
>  domapwrite(unsigned offset, unsigned size)
> @@ -2340,11 +2374,21 @@ have_op:
>  		doread(offset, size);
>  		break;
>  
> +	case OP_READ_UNCACHED:
> +		TRIM_OFF_LEN(offset, size, file_size);
> +		doread_uncached(offset, size);
> +		break;
> +
>  	case OP_WRITE:
>  		TRIM_OFF_LEN(offset, size, maxfilelen);
>  		dowrite(offset, size);
>  		break;
>  
> +	case OP_WRITE_UNCACHED:
> +		TRIM_OFF_LEN(offset, size, maxfilelen);
> +		dowrite_uncached(offset, size);
> +		break;
> +
>  	case OP_MAPREAD:
>  		TRIM_OFF_LEN(offset, size, file_size);
>  		domapread(offset, size);
> @@ -2702,7 +2746,7 @@ uring_setup()
>  }
>  
>  int
> -uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
> +uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags)
>  {
>  	struct io_uring_sqe     *sqe;
>  	struct io_uring_cqe     *cqe;
> @@ -2733,6 +2777,7 @@ uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
>  		} else {
>  			io_uring_prep_writev(sqe, fd, &iovec, 1, o);
>  		}
> +		sqe->rw_flags = flags;
>  
>  		ret = io_uring_submit_and_wait(&ring, 1);
>  		if (ret != 1) {
> @@ -2781,7 +2826,7 @@ uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
>  }
>  #else
>  int
> -uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
> +uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags)
>  {
>  	fprintf(stderr, "io_rw: need IO_URING support!\n");
>  	exit(111);
> @@ -2789,19 +2834,21 @@ uring_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
>  #endif
>  
>  int
> -fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset)
> +fsx_rw(int rw, int fd, char *buf, unsigned len, unsigned offset, int flags)
>  {
>  	int ret;
>  
>  	if (aio) {
>  		ret = aio_rw(rw, fd, buf, len, offset);
>  	} else if (uring) {
> -		ret = uring_rw(rw, fd, buf, len, offset);
> +		ret = uring_rw(rw, fd, buf, len, offset, flags);
>  	} else {
> +		struct iovec iov = { .iov_base = buf, .iov_len = len };
> +
>  		if (rw == READ)
> -			ret = read(fd, buf, len);
> +			ret = preadv2(fd, &iov, 1, offset, flags);
>  		else
> -			ret = write(fd, buf, len);
> +			ret = pwritev2(fd, &iov, 1, offset, flags);
>  	}
>  	return ret;
>  }
> 
> 
> -- 
> Jens Axboe
>

Jens Axboe Nov. 12, 2024, 7:08 p.m. UTC | #23

On 11/12/24 11:44 AM, Brian Foster wrote:
> On Tue, Nov 12, 2024 at 10:19:02AM -0700, Jens Axboe wrote:
>> On 11/12/24 10:06 AM, Jens Axboe wrote:
>>> On 11/12/24 9:39 AM, Brian Foster wrote:
>>>> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
>>>>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
>>>>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
>>>>>>> Here's the slightly cleaned up version, this is the one I ran testing
>>>>>>> with.
>>>>>>
>>>>>> Looks reasonable to me, but you probably get better reviews on the
>>>>>> fstests lists.
>>>>>
>>>>> I'll send it out once this patchset is a bit closer to integration,
>>>>> there's the usual chicken and egg situation with it. For now, it's quite
>>>>> handy for my testing, found a few issues with this version. So thanks
>>>>> for the suggestion, sure beats writing more of your own test cases :-)
>>>>>
>>>>
>>>> fsx support is probably a good idea as well. It's similar in idea to
>>>> fsstress, but bashes the same file with mixed operations and includes
>>>> data integrity validation checks as well. It's pretty useful for
>>>> uncovering subtle corner case issues or bad interactions..
>>>
>>> Indeed, I did that too. Re-running xfstests right now with that too.
>>
>> Here's what I'm running right now, fwiw. It adds RWF_UNCACHED support
>> for both the sync read/write and io_uring paths.
>>
> 
> Nice, thanks. Looks reasonable to me at first glance. A few randomish
> comments inlined below.
> 
> BTW, I should have also mentioned that fsx is also useful for longer
> soak testing. I.e., fstests will provide a decent amount of coverage as
> is via the various preexisting tests, but I'll occasionally run fsx
> directly and let it run overnight or something to get the op count at
> least up in the 100 millions or so to have a little more confidence
> there isn't some rare/subtle bug lurking. That might be helpful with
> something like this. JFYI.

Good suggestion, I can leave it running overnight here as well. Since
I'm not super familiar with it, what would be a good set of parameters
to run it with?

>>  #define READ 0
>>  #define WRITE 1
>> -#define fsxread(a,b,c,d)	fsx_rw(READ, a,b,c,d)
>> -#define fsxwrite(a,b,c,d)	fsx_rw(WRITE, a,b,c,d)
>> +#define fsxread(a,b,c,d,f)	fsx_rw(READ, a,b,c,d,f)
>> +#define fsxwrite(a,b,c,d,f)	fsx_rw(WRITE, a,b,c,d,f)
>>  
> 
> My pattern recognition brain wants to see an 'e' here. ;)

This is a "check if reviewer has actually looked at it" check ;-)

>> @@ -266,7 +273,9 @@ prterr(const char *prefix)
>>  
>>  static const char *op_names[] = {
>>  	[OP_READ] = "read",
>> +	[OP_READ_UNCACHED] = "read_uncached",
>>  	[OP_WRITE] = "write",
>> +	[OP_WRITE_UNCACHED] = "write_uncached",
>>  	[OP_MAPREAD] = "mapread",
>>  	[OP_MAPWRITE] = "mapwrite",
>>  	[OP_TRUNCATE] = "truncate",
>> @@ -393,12 +402,14 @@ logdump(void)
>>  				prt("\t******WWWW");
>>  			break;
>>  		case OP_READ:
>> +		case OP_READ_UNCACHED:
>>  			prt("READ     0x%x thru 0x%x\t(0x%x bytes)",
>>  			    lp->args[0], lp->args[0] + lp->args[1] - 1,
>>  			    lp->args[1]);
>>  			if (overlap)
>>  				prt("\t***RRRR***");
>>  			break;
>> +		case OP_WRITE_UNCACHED:
>>  		case OP_WRITE:
>>  			prt("WRITE    0x%x thru 0x%x\t(0x%x bytes)",
>>  			    lp->args[0], lp->args[0] + lp->args[1] - 1,
>> @@ -784,9 +795,8 @@ doflush(unsigned offset, unsigned size)
>>  }
>>  
>>  void
>> -doread(unsigned offset, unsigned size)
>> +__doread(unsigned offset, unsigned size, int flags)
>>  {
>> -	off_t ret;
>>  	unsigned iret;
>>  
>>  	offset -= offset % readbdy;
>> @@ -818,23 +828,39 @@ doread(unsigned offset, unsigned size)
>>  			(monitorend == -1 || offset <= monitorend))))))
>>  		prt("%lld read\t0x%x thru\t0x%x\t(0x%x bytes)\n", testcalls,
>>  		    offset, offset + size - 1, size);
>> -	ret = lseek(fd, (off_t)offset, SEEK_SET);
>> -	if (ret == (off_t)-1) {
>> -		prterr("doread: lseek");
>> -		report_failure(140);
>> -	}
>> -	iret = fsxread(fd, temp_buf, size, offset);
>> +	iret = fsxread(fd, temp_buf, size, offset, flags);
>>  	if (iret != size) {
>> -		if (iret == -1)
>> -			prterr("doread: read");
>> -		else
>> +		if (iret == -1) {
>> +			if (errno == EOPNOTSUPP && flags & RWF_UNCACHED) {
>> +				rwf_uncached = 1;
> 
> I assume you meant rwf_uncached = 0 here?

Indeed, good catch. Haven't tested this on a kernel without RWF_UNCACHED
yet...

> If so, check out test_fallocate() and friends to see how various
> operations are tested for support before the test starts. Following that
> might clean things up a bit.

Sure, I can do something like that instead. fsx looks pretty old school
in its design, was not expecting a static (and single) fd. But since we
have that, we can do the probe and check. Just a basic read would be
enough, with RWF_UNCACHED set.

> Also it's useful to have a CLI option to enable/disable individual
> features. That tends to be helpful to narrow things down when it does
> happen to explode and you want to narrow down the cause.

I can add a -U for "do not use uncached".

Brian Foster Nov. 12, 2024, 7:39 p.m. UTC | #24

On Tue, Nov 12, 2024 at 12:08:45PM -0700, Jens Axboe wrote:
> On 11/12/24 11:44 AM, Brian Foster wrote:
> > On Tue, Nov 12, 2024 at 10:19:02AM -0700, Jens Axboe wrote:
> >> On 11/12/24 10:06 AM, Jens Axboe wrote:
> >>> On 11/12/24 9:39 AM, Brian Foster wrote:
> >>>> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
> >>>>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
> >>>>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
> >>>>>>> Here's the slightly cleaned up version, this is the one I ran testing
> >>>>>>> with.
> >>>>>>
> >>>>>> Looks reasonable to me, but you probably get better reviews on the
> >>>>>> fstests lists.
> >>>>>
> >>>>> I'll send it out once this patchset is a bit closer to integration,
> >>>>> there's the usual chicken and egg situation with it. For now, it's quite
> >>>>> handy for my testing, found a few issues with this version. So thanks
> >>>>> for the suggestion, sure beats writing more of your own test cases :-)
> >>>>>
> >>>>
> >>>> fsx support is probably a good idea as well. It's similar in idea to
> >>>> fsstress, but bashes the same file with mixed operations and includes
> >>>> data integrity validation checks as well. It's pretty useful for
> >>>> uncovering subtle corner case issues or bad interactions..
> >>>
> >>> Indeed, I did that too. Re-running xfstests right now with that too.
> >>
> >> Here's what I'm running right now, fwiw. It adds RWF_UNCACHED support
> >> for both the sync read/write and io_uring paths.
> >>
> > 
> > Nice, thanks. Looks reasonable to me at first glance. A few randomish
> > comments inlined below.
> > 
> > BTW, I should have also mentioned that fsx is also useful for longer
> > soak testing. I.e., fstests will provide a decent amount of coverage as
> > is via the various preexisting tests, but I'll occasionally run fsx
> > directly and let it run overnight or something to get the op count at
> > least up in the 100 millions or so to have a little more confidence
> > there isn't some rare/subtle bug lurking. That might be helpful with
> > something like this. JFYI.
> 
> Good suggestion, I can leave it running overnight here as well. Since
> I'm not super familiar with it, what would be a good set of parameters
> to run it with?
> 

Most things are on by default, so I'd probably just go with that. -p is
useful to get occasional status output on how many operations have
completed and you could consider increasing the max file size with -l,
but usually I don't use more than a few MB or so if I increase it at
all.

Random other thought: I also wonder if uncached I/O should be an
exclusive mode more similar to like how O_DIRECT or AIO is implemented.
But I dunno, maybe it doesn't matter that much (or maybe others will
have opinions on the fstests list).

Brian

> >>  #define READ 0
> >>  #define WRITE 1
> >> -#define fsxread(a,b,c,d)	fsx_rw(READ, a,b,c,d)
> >> -#define fsxwrite(a,b,c,d)	fsx_rw(WRITE, a,b,c,d)
> >> +#define fsxread(a,b,c,d,f)	fsx_rw(READ, a,b,c,d,f)
> >> +#define fsxwrite(a,b,c,d,f)	fsx_rw(WRITE, a,b,c,d,f)
> >>  
> > 
> > My pattern recognition brain wants to see an 'e' here. ;)
> 
> This is a "check if reviewer has actually looked at it" check ;-)
> 
> >> @@ -266,7 +273,9 @@ prterr(const char *prefix)
> >>  
> >>  static const char *op_names[] = {
> >>  	[OP_READ] = "read",
> >> +	[OP_READ_UNCACHED] = "read_uncached",
> >>  	[OP_WRITE] = "write",
> >> +	[OP_WRITE_UNCACHED] = "write_uncached",
> >>  	[OP_MAPREAD] = "mapread",
> >>  	[OP_MAPWRITE] = "mapwrite",
> >>  	[OP_TRUNCATE] = "truncate",
> >> @@ -393,12 +402,14 @@ logdump(void)
> >>  				prt("\t******WWWW");
> >>  			break;
> >>  		case OP_READ:
> >> +		case OP_READ_UNCACHED:
> >>  			prt("READ     0x%x thru 0x%x\t(0x%x bytes)",
> >>  			    lp->args[0], lp->args[0] + lp->args[1] - 1,
> >>  			    lp->args[1]);
> >>  			if (overlap)
> >>  				prt("\t***RRRR***");
> >>  			break;
> >> +		case OP_WRITE_UNCACHED:
> >>  		case OP_WRITE:
> >>  			prt("WRITE    0x%x thru 0x%x\t(0x%x bytes)",
> >>  			    lp->args[0], lp->args[0] + lp->args[1] - 1,
> >> @@ -784,9 +795,8 @@ doflush(unsigned offset, unsigned size)
> >>  }
> >>  
> >>  void
> >> -doread(unsigned offset, unsigned size)
> >> +__doread(unsigned offset, unsigned size, int flags)
> >>  {
> >> -	off_t ret;
> >>  	unsigned iret;
> >>  
> >>  	offset -= offset % readbdy;
> >> @@ -818,23 +828,39 @@ doread(unsigned offset, unsigned size)
> >>  			(monitorend == -1 || offset <= monitorend))))))
> >>  		prt("%lld read\t0x%x thru\t0x%x\t(0x%x bytes)\n", testcalls,
> >>  		    offset, offset + size - 1, size);
> >> -	ret = lseek(fd, (off_t)offset, SEEK_SET);
> >> -	if (ret == (off_t)-1) {
> >> -		prterr("doread: lseek");
> >> -		report_failure(140);
> >> -	}
> >> -	iret = fsxread(fd, temp_buf, size, offset);
> >> +	iret = fsxread(fd, temp_buf, size, offset, flags);
> >>  	if (iret != size) {
> >> -		if (iret == -1)
> >> -			prterr("doread: read");
> >> -		else
> >> +		if (iret == -1) {
> >> +			if (errno == EOPNOTSUPP && flags & RWF_UNCACHED) {
> >> +				rwf_uncached = 1;
> > 
> > I assume you meant rwf_uncached = 0 here?
> 
> Indeed, good catch. Haven't tested this on a kernel without RWF_UNCACHED
> yet...
> 
> > If so, check out test_fallocate() and friends to see how various
> > operations are tested for support before the test starts. Following that
> > might clean things up a bit.
> 
> Sure, I can do something like that instead. fsx looks pretty old school
> in its design, was not expecting a static (and single) fd. But since we
> have that, we can do the probe and check. Just a basic read would be
> enough, with RWF_UNCACHED set.
> 
> > Also it's useful to have a CLI option to enable/disable individual
> > features. That tends to be helpful to narrow things down when it does
> > happen to explode and you want to narrow down the cause.
> 
> I can add a -U for "do not use uncached".
> 
> -- 
> Jens Axboe
>

Jens Axboe Nov. 12, 2024, 7:45 p.m. UTC | #25

On 11/12/24 12:39 PM, Brian Foster wrote:
> On Tue, Nov 12, 2024 at 12:08:45PM -0700, Jens Axboe wrote:
>> On 11/12/24 11:44 AM, Brian Foster wrote:
>>> On Tue, Nov 12, 2024 at 10:19:02AM -0700, Jens Axboe wrote:
>>>> On 11/12/24 10:06 AM, Jens Axboe wrote:
>>>>> On 11/12/24 9:39 AM, Brian Foster wrote:
>>>>>> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
>>>>>>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
>>>>>>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
>>>>>>>>> Here's the slightly cleaned up version, this is the one I ran testing
>>>>>>>>> with.
>>>>>>>>
>>>>>>>> Looks reasonable to me, but you probably get better reviews on the
>>>>>>>> fstests lists.
>>>>>>>
>>>>>>> I'll send it out once this patchset is a bit closer to integration,
>>>>>>> there's the usual chicken and egg situation with it. For now, it's quite
>>>>>>> handy for my testing, found a few issues with this version. So thanks
>>>>>>> for the suggestion, sure beats writing more of your own test cases :-)
>>>>>>>
>>>>>>
>>>>>> fsx support is probably a good idea as well. It's similar in idea to
>>>>>> fsstress, but bashes the same file with mixed operations and includes
>>>>>> data integrity validation checks as well. It's pretty useful for
>>>>>> uncovering subtle corner case issues or bad interactions..
>>>>>
>>>>> Indeed, I did that too. Re-running xfstests right now with that too.
>>>>
>>>> Here's what I'm running right now, fwiw. It adds RWF_UNCACHED support
>>>> for both the sync read/write and io_uring paths.
>>>>
>>>
>>> Nice, thanks. Looks reasonable to me at first glance. A few randomish
>>> comments inlined below.
>>>
>>> BTW, I should have also mentioned that fsx is also useful for longer
>>> soak testing. I.e., fstests will provide a decent amount of coverage as
>>> is via the various preexisting tests, but I'll occasionally run fsx
>>> directly and let it run overnight or something to get the op count at
>>> least up in the 100 millions or so to have a little more confidence
>>> there isn't some rare/subtle bug lurking. That might be helpful with
>>> something like this. JFYI.
>>
>> Good suggestion, I can leave it running overnight here as well. Since
>> I'm not super familiar with it, what would be a good set of parameters
>> to run it with?
>>
> 
> Most things are on by default, so I'd probably just go with that. -p is
> useful to get occasional status output on how many operations have
> completed and you could consider increasing the max file size with -l,
> but usually I don't use more than a few MB or so if I increase it at
> all.

When you say default, I'd run it without arguments. And then it does
nothing :-)

Not an fs guy, I never run fsx. I run xfstests if I make changes that
may impact the page cache, writeback, or file systems.

IOW, consider this a "I'm asking my mom to run fsx, I need to be pretty
specific" ;-)

> Random other thought: I also wonder if uncached I/O should be an
> exclusive mode more similar to like how O_DIRECT or AIO is implemented.
> But I dunno, maybe it doesn't matter that much (or maybe others will
> have opinions on the fstests list).

Should probably exclude it with DIO, as it should not do anything there
anyway. Eg if you ask for DIO, it gets turned off. For some of the other
exclusions, they seem kind of wonky to me. Why can you use libaio and
io_uring at the same time, for example?

io_uring will work just fine with both buffered and direct IO, and it'll
do the right thing with uncached as well. AIO is really a DIO only
thing, not useful for anything else.

Brian Foster Nov. 12, 2024, 8:21 p.m. UTC | #26

On Tue, Nov 12, 2024 at 12:45:58PM -0700, Jens Axboe wrote:
> On 11/12/24 12:39 PM, Brian Foster wrote:
> > On Tue, Nov 12, 2024 at 12:08:45PM -0700, Jens Axboe wrote:
> >> On 11/12/24 11:44 AM, Brian Foster wrote:
> >>> On Tue, Nov 12, 2024 at 10:19:02AM -0700, Jens Axboe wrote:
> >>>> On 11/12/24 10:06 AM, Jens Axboe wrote:
> >>>>> On 11/12/24 9:39 AM, Brian Foster wrote:
> >>>>>> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
> >>>>>>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
> >>>>>>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
> >>>>>>>>> Here's the slightly cleaned up version, this is the one I ran testing
> >>>>>>>>> with.
> >>>>>>>>
> >>>>>>>> Looks reasonable to me, but you probably get better reviews on the
> >>>>>>>> fstests lists.
> >>>>>>>
> >>>>>>> I'll send it out once this patchset is a bit closer to integration,
> >>>>>>> there's the usual chicken and egg situation with it. For now, it's quite
> >>>>>>> handy for my testing, found a few issues with this version. So thanks
> >>>>>>> for the suggestion, sure beats writing more of your own test cases :-)
> >>>>>>>
> >>>>>>
> >>>>>> fsx support is probably a good idea as well. It's similar in idea to
> >>>>>> fsstress, but bashes the same file with mixed operations and includes
> >>>>>> data integrity validation checks as well. It's pretty useful for
> >>>>>> uncovering subtle corner case issues or bad interactions..
> >>>>>
> >>>>> Indeed, I did that too. Re-running xfstests right now with that too.
> >>>>
> >>>> Here's what I'm running right now, fwiw. It adds RWF_UNCACHED support
> >>>> for both the sync read/write and io_uring paths.
> >>>>
> >>>
> >>> Nice, thanks. Looks reasonable to me at first glance. A few randomish
> >>> comments inlined below.
> >>>
> >>> BTW, I should have also mentioned that fsx is also useful for longer
> >>> soak testing. I.e., fstests will provide a decent amount of coverage as
> >>> is via the various preexisting tests, but I'll occasionally run fsx
> >>> directly and let it run overnight or something to get the op count at
> >>> least up in the 100 millions or so to have a little more confidence
> >>> there isn't some rare/subtle bug lurking. That might be helpful with
> >>> something like this. JFYI.
> >>
> >> Good suggestion, I can leave it running overnight here as well. Since
> >> I'm not super familiar with it, what would be a good set of parameters
> >> to run it with?
> >>
> > 
> > Most things are on by default, so I'd probably just go with that. -p is
> > useful to get occasional status output on how many operations have
> > completed and you could consider increasing the max file size with -l,
> > but usually I don't use more than a few MB or so if I increase it at
> > all.
> 
> When you say default, I'd run it without arguments. And then it does
> nothing :-)
> 
> Not an fs guy, I never run fsx. I run xfstests if I make changes that
> may impact the page cache, writeback, or file systems.
> 
> IOW, consider this a "I'm asking my mom to run fsx, I need to be pretty
> specific" ;-)
> 

Heh. In that case I'd just run something like this:

	fsx -p 100000 <file>

... and see how long it survives. It may not necessarily be an uncached
I/O problem if it fails, but depending on how reproducible a failure is,
that's where a cli knob comes in handy.

> > Random other thought: I also wonder if uncached I/O should be an
> > exclusive mode more similar to like how O_DIRECT or AIO is implemented.
> > But I dunno, maybe it doesn't matter that much (or maybe others will
> > have opinions on the fstests list).
> 
> Should probably exclude it with DIO, as it should not do anything there
> anyway. Eg if you ask for DIO, it gets turned off. For some of the other
> exclusions, they seem kind of wonky to me. Why can you use libaio and
> io_uring at the same time, for example?
> 

To your earlier point, if I had to guess it's probably just because it's
grotty test code with sharp edges.

Brian

> io_uring will work just fine with both buffered and direct IO, and it'll
> do the right thing with uncached as well. AIO is really a DIO only
> thing, not useful for anything else.
> 
> -- 
> Jens Axboe
>

Jens Axboe Nov. 12, 2024, 8:25 p.m. UTC | #27

On 11/12/24 1:21 PM, Brian Foster wrote:
> On Tue, Nov 12, 2024 at 12:45:58PM -0700, Jens Axboe wrote:
>> On 11/12/24 12:39 PM, Brian Foster wrote:
>>> On Tue, Nov 12, 2024 at 12:08:45PM -0700, Jens Axboe wrote:
>>>> On 11/12/24 11:44 AM, Brian Foster wrote:
>>>>> On Tue, Nov 12, 2024 at 10:19:02AM -0700, Jens Axboe wrote:
>>>>>> On 11/12/24 10:06 AM, Jens Axboe wrote:
>>>>>>> On 11/12/24 9:39 AM, Brian Foster wrote:
>>>>>>>> On Tue, Nov 12, 2024 at 08:14:28AM -0700, Jens Axboe wrote:
>>>>>>>>> On 11/11/24 10:13 PM, Christoph Hellwig wrote:
>>>>>>>>>> On Mon, Nov 11, 2024 at 04:42:25PM -0700, Jens Axboe wrote:
>>>>>>>>>>> Here's the slightly cleaned up version, this is the one I ran testing
>>>>>>>>>>> with.
>>>>>>>>>>
>>>>>>>>>> Looks reasonable to me, but you probably get better reviews on the
>>>>>>>>>> fstests lists.
>>>>>>>>>
>>>>>>>>> I'll send it out once this patchset is a bit closer to integration,
>>>>>>>>> there's the usual chicken and egg situation with it. For now, it's quite
>>>>>>>>> handy for my testing, found a few issues with this version. So thanks
>>>>>>>>> for the suggestion, sure beats writing more of your own test cases :-)
>>>>>>>>>
>>>>>>>>
>>>>>>>> fsx support is probably a good idea as well. It's similar in idea to
>>>>>>>> fsstress, but bashes the same file with mixed operations and includes
>>>>>>>> data integrity validation checks as well. It's pretty useful for
>>>>>>>> uncovering subtle corner case issues or bad interactions..
>>>>>>>
>>>>>>> Indeed, I did that too. Re-running xfstests right now with that too.
>>>>>>
>>>>>> Here's what I'm running right now, fwiw. It adds RWF_UNCACHED support
>>>>>> for both the sync read/write and io_uring paths.
>>>>>>
>>>>>
>>>>> Nice, thanks. Looks reasonable to me at first glance. A few randomish
>>>>> comments inlined below.
>>>>>
>>>>> BTW, I should have also mentioned that fsx is also useful for longer
>>>>> soak testing. I.e., fstests will provide a decent amount of coverage as
>>>>> is via the various preexisting tests, but I'll occasionally run fsx
>>>>> directly and let it run overnight or something to get the op count at
>>>>> least up in the 100 millions or so to have a little more confidence
>>>>> there isn't some rare/subtle bug lurking. That might be helpful with
>>>>> something like this. JFYI.
>>>>
>>>> Good suggestion, I can leave it running overnight here as well. Since
>>>> I'm not super familiar with it, what would be a good set of parameters
>>>> to run it with?
>>>>
>>>
>>> Most things are on by default, so I'd probably just go with that. -p is
>>> useful to get occasional status output on how many operations have
>>> completed and you could consider increasing the max file size with -l,
>>> but usually I don't use more than a few MB or so if I increase it at
>>> all.
>>
>> When you say default, I'd run it without arguments. And then it does
>> nothing :-)
>>
>> Not an fs guy, I never run fsx. I run xfstests if I make changes that
>> may impact the page cache, writeback, or file systems.
>>
>> IOW, consider this a "I'm asking my mom to run fsx, I need to be pretty
>> specific" ;-)
>>
> 
> Heh. In that case I'd just run something like this:
> 
> 	fsx -p 100000 <file>
> 
> ... and see how long it survives. It may not necessarily be an uncached
> I/O problem if it fails, but depending on how reproducible a failure is,
> that's where a cli knob comes in handy.

OK good, will give that a spin.

>>> Random other thought: I also wonder if uncached I/O should be an
>>> exclusive mode more similar to like how O_DIRECT or AIO is implemented.
>>> But I dunno, maybe it doesn't matter that much (or maybe others will
>>> have opinions on the fstests list).
>>
>> Should probably exclude it with DIO, as it should not do anything there
>> anyway. Eg if you ask for DIO, it gets turned off. For some of the other
>> exclusions, they seem kind of wonky to me. Why can you use libaio and
>> io_uring at the same time, for example?
>>
> 
> To your earlier point, if I had to guess it's probably just because it's
> grotty test code with sharp edges.

Yeah makes sense, unloved.

Jens Axboe Nov. 13, 2024, 2:07 p.m. UTC | #28

On 11/12/24 1:25 PM, Jens Axboe wrote:
>>>>>> BTW, I should have also mentioned that fsx is also useful for longer
>>>>>> soak testing. I.e., fstests will provide a decent amount of coverage as
>>>>>> is via the various preexisting tests, but I'll occasionally run fsx
>>>>>> directly and let it run overnight or something to get the op count at
>>>>>> least up in the 100 millions or so to have a little more confidence
>>>>>> there isn't some rare/subtle bug lurking. That might be helpful with
>>>>>> something like this. JFYI.
>>>>>
>>>>> Good suggestion, I can leave it running overnight here as well. Since
>>>>> I'm not super familiar with it, what would be a good set of parameters
>>>>> to run it with?
>>>>>
>>>>
>>>> Most things are on by default, so I'd probably just go with that. -p is
>>>> useful to get occasional status output on how many operations have
>>>> completed and you could consider increasing the max file size with -l,
>>>> but usually I don't use more than a few MB or so if I increase it at
>>>> all.
>>>
>>> When you say default, I'd run it without arguments. And then it does
>>> nothing :-)
>>>
>>> Not an fs guy, I never run fsx. I run xfstests if I make changes that
>>> may impact the page cache, writeback, or file systems.
>>>
>>> IOW, consider this a "I'm asking my mom to run fsx, I need to be pretty
>>> specific" ;-)
>>>
>>
>> Heh. In that case I'd just run something like this:
>>
>> 	fsx -p 100000 <file>
>>
>> ... and see how long it survives. It may not necessarily be an uncached
>> I/O problem if it fails, but depending on how reproducible a failure is,
>> that's where a cli knob comes in handy.
> 
> OK good, will give that a spin.

Ran overnight, no issues seen. Just terminated the process. For funsies,
I also added RWF_UNCACHED support to qemu and had the vm booted with
that as well, to get some host side testing too. Everything looks fine.
This is running:

https://git.kernel.dk/cgit/linux/log/?h=buffered-uncached.7

which is the current branch.

[PATCHSET,v2,0/15] Uncached buffered IO

Message

Comments