| Message ID | 20241110152906.1747545-9-axboe@kernel.dk (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Uncached buffered IO | expand |
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 >
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!
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.
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.
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.
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().
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.
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.
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) {
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) {
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 :-)
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.
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; }
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".
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.
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.
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.
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;
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(-)