Re: [PATCH 1/2] fsstress: add support for RWF_DONTCACHE

[Jens Axboe <axboe@kernel.dk>]

On 1/6/25 7:11 PM, Darrick J. Wong wrote:
>> +void
>> +read_dontcache_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);
> 
> Should there be a check for null iov_base after the allocation?

Nothing else in fsstress seems to bother with malloc() failures, which
at least on Linux, is probably fair game.

>> +	flags = have_rwf_dontcache ? RWF_DONTCACHE : 0;
>> +	e = preadv2(fd, &iov, 1, off, flags) < 0 ? errno : 0;
>> +	if (have_rwf_dontcache && e == EOPNOTSUPP)
> 
> ...and should this set have_rwf_dontcache = 0?

I don't think so? If we get EOPNOTSUPP and we don't have dontcache, then
it's a fatal condition. fsstress defaults to it being available, so we
may very well run into EOPNOTSUPP and then just do a regular read or
write for that case. We could probably do:

if (have_rwf_dontcache && e == EOPNOTSUPP) {
	have_rwf_dontcache = 0;
	e = preadv2(fd, &iov, 1, off, 0) < 0 ? errno : 0;
}

here and on the write side, at least then we won't repeatedly try
RWF_DONTCACHE if we hit EOPNOTSUPP. But in terms of logic, it should be
correct as-is.

-- 
Jens Axboe