On Tue, Jun 01, 2021 at 09:01:45AM +0000, Shinichiro Kawasaki wrote:
> On Jun 01, 2021 / 07:19, Damien Le Moal wrote:
> > On 2021/06/01 16:15, Niklas Cassel wrote:
> > > On Tue, Jun 01, 2021 at 07:01:18AM +0000, Damien Le Moal wrote:
> > >> On 2021/06/01 15:48, Shin'ichiro Kawasaki wrote:
> > >>> Recent commit d2f442bc0bd5 ("ioengines: add get_max_open_zones zoned
> > >>> block device operation") modified fio to compare --max_open_zoned option
> > >>
> > >> s/max_open_zoned/max_open_zones
>
> Thanks. Will reflect.
>
> > >>
> > >>> value and max_open_zones reported by the device. The device limit is
> > >>> fetched through sysfs or through an ioengine specific implementation.
> > >>>
> > >>> The test script currently try to fetch the max open zones limit using
> > >>> libzbc tools or sg_inq. If either of these fail, default value 128 is
> > >>> supplied. This default value can be too high when the test script is
> > >>> run for certain zoned block devices, and can therefore result in fio
> > >>> error and test case failure.
> > >>
> > >> If sysfs has the max_open_zones attribute file present, that value should be
> > >> retrieved first. libzbc and sg_inq should be used only if the sysfs attribute is
> > >> not present, that is, when running on older kernels.
> > >
> > > Since we can assume that the test script and fio versions are updated in sync,
> > > I don't see any point of duplicating the sysfs parsing in the test script.
> > >
> > > If the test script either specifies --max_open_zones=0 or omits the parameter,
> > > fio itself will either parse sysfs, or use libzbc to fetch the max limit.
> >
> > True. Unless the user is running the tests with some special max_open_zones, we
> > should not try to grab any value then.
> >
> > >
> > > So I think that the test script should only use those "methods" that fio
> > > itself can't do.
> > > Right now fio can parse sysfs and use libzbc.
> > > The only remaining method is sg_inq. This only I assume that we want to
> > > keep in the test. (I guess the alternative is to implement the
> > > .get_max_open_zones() fio ioengine callback in engines/sg.c, that does
> > > the same. But we probably don't want to do that, right?)
> >
> > No, we certainly do not. That would need a lot more code than that, e.g. to also
> > detect zone model. That would mean basically re-implementing what libzbc does
> > inside the sg engine. And since we now have the libzbc engine, I do not see the
> > point.
> >
> > >
> > >>
> > >>>
> > >>> To avoid the failure, modify the default value used in the test script
> > >>> from 128 to 0. With this, --max_open_zoned=0 is passed to fio, and it
> > >>> makes fio use the max_open_zones reported by the device.
> > >>
> > >> Why pass anything at all since now fio will use the device reported limit by
> > >> default if the user does not specify anything ?
> > >
> > > Sure, we can just omit the --max_open_zones parameter completely.
> > > That might be more clear.
>
> I think still we need the --max_open_zones option in case the latest fio is
> run on older kernel which does not have max_open_zones in sysfs. In such case,
> fio is not able to obtain the correct max_open_zones of the device (unless it
> uses libzbc I/O engine).
>
> There are only two test cases which refers the max_open_zones that the test
> script obtained: #31 and #32. The test case #31 has a check if the found
> max_open_zones is zero or not to decide write target zone count. The
> max_open_zones value is not used for --max_open_zones option (can not omit it).
> As for the test case #32, it specifies "--max_open_zones=$max_open_zones" to the
> fio command. I assume this option is still valid and required for the older
> kernels. We could add a check to the test script to specify this option only
> when max_open_zones is zero, but I think it's the better not to add such check
> to keep the test script simpler.
Test case 31 is just poorly designed IMHO.
I don't like that it wants to know max_open_zones, but then it doesn't even
send in that value to fio.
The only reason why it fetches max_open_zones is to save time.
We can come up with some other way, e.g. a random max limit, 10 GB, divide
that by zone_size. That is the amount of zones that we decide to write.
And each zone is written fully.
Then, since test case 31 is a random read test, just read the zones that we
wrote. Right now we will write <= (all zones)/2, but we read _all_ zones.
Since half zones will be empty, this is not an accurate test of the random
read performance.
(Perhaps you could make the argument that all zones that you read from
should be in zone state implicit open, but that does not seem to be the case
currently, as many zones will most likely be empty.
My suggestion is that we simply write each zone fully, that way, all zones
that we read from will be in zone state FULL.)
Test case 32:
On fio master the test script currently checks sg_inq (scsi generic) or libzbc,
else it defaults to 128.
The max_open_zones() could look like this (regardless of kernel version):
If sg_inq returns something: use the result as --max_open_zones.
If sq_inq doesn't return something omit --max_open_zones completely (or send
in 0), and fio will try to parse sysfs (or use libzbc if libzbc ioengine was
used). If fio fails (regardless of ioengine), it will default to 4096.
-The default of 128 can be removed from the test script. 4096 is just as good
random number when we cannot get the real value as 128.
-The libzbc parsing can be removed from the test script.
-No need to add sysfs parsing to the test script.
We still need a max_open_zones() function in the test script to call sg_inq.
So the max_open_zones() function always has to be there. It is just the
--max_open_zones parameter than sometimes can be excluded (or specified as
zero).
BTW, do we really need to keep the sg_inc call at all?
libzbc ioengine has both a SCSI and a SCSI generic backend.
So right now, it seems that the only reason max_open_zones() function has
for living is basically if someone runs fio against a SMR drive, on a kernel
older than v5.9, using an ioengine different from libzbc.
That is probably reason enough for living. But we should definitely have a
comment above the max_open_zones() function itself that states that this
function only exists in case the user runs against an SMR drive, on an old
kernel, with an ioengine != libzbc ioengine. (And libzbc ioengine is probably
the most appropriate ioengine is such a scenario.)
Kind regards,
Niklas
