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
>
