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);
+			}
+			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 (lru_gen_enabled()) {
 		folio_inc_refs(folio);
 		return;
-- 
2.45.2

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
> 

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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!

-- 
Jens Axboe

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.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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.

-- 
Jens Axboe

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

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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.

-- 
Jens Axboe

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.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

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.

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.

-- 
Jens Axboe

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)
 {

-- 
Jens Axboe

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)
 {

-- 
Jens Axboe

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.

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 :-)

-- 
Jens Axboe

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
>

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

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;
 }


-- 
Jens Axboe

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
>

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

-- 
Jens Axboe

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
>

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.

-- 
Jens Axboe

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
>

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

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.

-- 
Jens Axboe