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

[Jens Axboe <axboe@kernel.dk>]

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

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

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

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

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

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

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

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

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

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

-- 
Jens Axboe