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

["Darrick J. Wong" <djwong@kernel.org>]

On Mon, Jan 06, 2025 at 07:16:17PM -0700, Jens Axboe wrote:
> 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.

lol ok.

> >> +	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;
> }

Yep, that's exactly what I was thinking. :)

--D

> 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
>