Re: [PATCH 08/15] mm/filemap: add read support for RWF_UNCACHED

[Brian Foster <bfoster@redhat.com>]

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
>