Re: [PATCH] fio: add for_each_rw_ddir() macro

[Alexey Dobriyan <adobriyan@gmail.com>]

On Wed, Aug 12, 2020 at 10:01:56PM +0000, Elliott, Robert (Servers) wrote:
> > -----Original Message-----
> > From: fio-owner@vger.kernel.org <fio-owner@vger.kernel.org> On Behalf
> > Of Alexey Dobriyan
> > Sent: Wednesday, August 12, 2020 10:26 AM
> > To: axboe@kernel.dk
> > Cc: fio@vger.kernel.org
> > Subject: [PATCH] fio: add for_each_rw_ddir() macro
> > 
> > Make it slightly easier to add DDIR_APPEND as fully fledged I/O type.

> >  	/*
> >  	 * For random IO, allow blockalign offset other than min_bs.
> >  	 */
> > -	if (!o->ba[DDIR_READ] || !td_random(td))
> > -		o->ba[DDIR_READ] = o->min_bs[DDIR_READ];
> > -	if (!o->ba[DDIR_WRITE] || !td_random(td))
> > -		o->ba[DDIR_WRITE] = o->min_bs[DDIR_WRITE];
> > -	if (!o->ba[DDIR_TRIM] || !td_random(td))
> > -		o->ba[DDIR_TRIM] = o->min_bs[DDIR_TRIM];
> > +	for_each_rw_ddir(ddir) {
> > +		if (!o->ba[ddir] || !td_random(td))
> > +			o->ba[ddir] = o->min_bs[ddir];
> > +	}
> > 
> >  	if ((o->ba[DDIR_READ] != o->min_bs[DDIR_READ] ||
> >  	    o->ba[DDIR_WRITE] != o->min_bs[DDIR_WRITE] ||
> 
> continues as:
>             o->ba[DDIR_TRIM] != o->min_bs[DDIR_TRIM]) &&
>             !o->norandommap) {
> 
> which should be updated too.

Actually it should not. This error message doesn't make sense for Append:

        if ((o->ba[DDIR_READ] != o->min_bs[DDIR_READ] ||
            o->ba[DDIR_WRITE] != o->min_bs[DDIR_WRITE] ||
            o->ba[DDIR_TRIM] != o->min_bs[DDIR_TRIM]) &&
            !o->norandommap) {
                log_err("fio: Any use of blockalign= turns off randommap\n");
                o->norandommap = 1;
                ret |= warnings_fatal;
        }

ba= should be ignored for Append (maybe with a non-fatal warning message).

> > @@ -765,14 +757,12 @@ static int fixup_options(struct thread_data
> > *td)
> >  		log_err("fio: rate and rate_iops are mutually
> > exclusive\n");
> >  		ret |= 1;
> >  	}
> > -	if ((o->rate[DDIR_READ] && (o->rate[DDIR_READ] < o->ratemin[DDIR_READ])) ||
> > -	    (o->rate[DDIR_WRITE] && (o->rate[DDIR_WRITE] < o->ratemin[DDIR_WRITE])) ||
> > -	    (o->rate[DDIR_TRIM] && (o->rate[DDIR_TRIM] < o->ratemin[DDIR_TRIM])) ||
> > -	    (o->rate_iops[DDIR_READ] && (o->rate_iops[DDIR_READ] < o->rate_iops_min[DDIR_READ])) ||
> > -	    (o->rate_iops[DDIR_WRITE] && (o->rate_iops[DDIR_WRITE] < o->rate_iops_min[DDIR_WRITE])) ||
> > -	    (o->rate_iops[DDIR_TRIM] && (o->rate_iops[DDIR_TRIM] < o->rate_iops_min[DDIR_TRIM]))) {
> > -		log_err("fio: minimum rate exceeds rate\n");
> > -		ret |= 1;
> > +	for_each_rw_ddir(ddir) {
> > +		if ((o->rate[ddir] && (o->rate[ddir] < o->ratemin[ddir])) ||
> > +		    (o->rate_iops[ddir] && (o->rate_iops[ddir] < o->rate_iops_min[ddir]))) {
> > +			log_err("fio: minimum rate exceeds rate\n");
> > +			ret |= 1;
> > +		}
> >  	}
> 
> That changes the behavior slightly - you can now get up to three of
> those messages. Printing the value of ddir would help identify the
> problem(s) and be more informative than the current single message.

OK.