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
