[PATCH] blk-throttle: Fix W=1 build issue in tg_prfill_limit()

[PATCH] blk-throttle: Fix W=1 build issue in tg_prfill_limit()

[John Garry]

For when using gcc 8 and above, the following warnings can be seen when
compiling blk-throttle.c with W=1:

block/blk-throttle.c: In function ‘tg_prfill_limit’:
block/blk-throttle.c:1539:53: error: ‘snprintf’ output may be truncated before the last format character [-Werror=format-truncation=]
    snprintf(idle_time, sizeof(idle_time), " idle=%lu",
                                                     ^
block/blk-throttle.c:1539:4: note: ‘snprintf’ output between 8 and 27 bytes into a destination of size 26
    snprintf(idle_time, sizeof(idle_time), " idle=%lu",
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     tg->idletime_threshold_conf);
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~
block/blk-throttle.c:1546:15: error: ‘%lu’ directive output may be truncated writing between 1 and 20 bytes into a region of size 17 [-Werror=format-truncation=]
     " latency=%lu", tg->latency_target_conf);
               ^~~
block/blk-throttle.c:1546:5: note: directive argument in the range [0, 18446744073709551614]
     " latency=%lu", tg->latency_target_conf);
     ^~~~~~~~~~~~~~
block/blk-throttle.c:1545:4: note: ‘snprintf’ output between 11 and 30 bytes into a destination of size 26
    snprintf(latency_time, sizeof(latency_time),
    ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
     " latency=%lu", tg->latency_target_conf);
     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[3]: *** [scripts/Makefile.build:244: block/blk-throttle.o] Error 1

Increase idle_time[] and latency_time[] sizes to clear the warnings.

Fixes: ec80991d6fc2c ("blk-throttle: add interface for per-cgroup target latency")
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
 block/blk-throttle.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/block/blk-throttle.c b/block/blk-throttle.c
index f4850a6f860b..ece272937792 100644
--- a/block/blk-throttle.c
+++ b/block/blk-throttle.c
@@ -1497,8 +1497,8 @@ static u64 tg_prfill_limit(struct seq_file *sf, struct blkg_policy_data *pd,
 	char bufs[4][21] = { "max", "max", "max", "max" };
 	u64 bps_dft;
 	unsigned int iops_dft;
-	char idle_time[26] = "";
-	char latency_time[26] = "";
+	char idle_time[27] = "";
+	char latency_time[30] = "";
 
 	if (!dname)
 		return 0;
-- 
2.31.1

Re: [PATCH] blk-throttle: Fix W=1 build issue in tg_prfill_limit()

[Christoph Hellwig]

On Mon, Mar 25, 2024 at 12:19:05PM +0000, John Garry wrote:
> For when using gcc 8 and above, the following warnings can be seen when
> compiling blk-throttle.c with W=1:

Why is this function even using these local buffers vs a sequence
of separate seq_printf calls that would get rid of these pointless
on-stack buffers?

Re: [PATCH] blk-throttle: Fix W=1 build issue in tg_prfill_limit()

[John Garry]

On 26/03/2024 07:03, Christoph Hellwig wrote:
> On Mon, Mar 25, 2024 at 12:19:05PM +0000, John Garry wrote:
>> For when using gcc 8 and above, the following warnings can be seen when
>> compiling blk-throttle.c with W=1:
> 
> Why is this function even using these local buffers vs a sequence
> of separate seq_printf calls that would get rid of these pointless
> on-stack buffers?

Currently a combo of snprintf and seq_printf is used, a strategy which 
seems to go as far back as 2ee867dcfa2ea (2015), when it was a much 
simpler print. All the other code in this area only uses seq_printf, so 
it seems that the author(s) prefer this way here.

Here's how the current code could look (using only seq_printf):

static u64 tg_prfill_limit(struct seq_file *sf, struct blkg_policy_data *pd,
			 int off)
{
	struct throtl_grp *tg = pd_to_tg(pd);
	const char *dname = blkg_dev_name(pd->blkg);
	u64 bps_dft;
	unsigned int iops_dft;

	if (!dname)
		return 0;

	if (off == LIMIT_LOW) {
		bps_dft = 0;
		iops_dft = 0;
	} else {
		bps_dft = U64_MAX;
		iops_dft = UINT_MAX;
	}

	if (tg->bps_conf[READ][off] == bps_dft &&
	    tg->bps_conf[WRITE][off] == bps_dft &&
	    tg->iops_conf[READ][off] == iops_dft &&
	    tg->iops_conf[WRITE][off] == iops_dft &&
	    (off != LIMIT_LOW ||
	     (tg->idletime_threshold_conf == DFL_IDLE_THRESHOLD &&
	      tg->latency_target_conf == DFL_LATENCY_TARGET)))
		return 0;

	seq_printf(sf, "%s rbps=", dname);
	if (tg->bps_conf[READ][off] == U64_MAX)
		seq_printf(sf, "max");
	else
		seq_printf(sf, "%llu", tg->bps_conf[READ][off]);

	if (tg->bps_conf[WRITE][off] == U64_MAX)
		seq_printf(sf, " wbps=max ");
	else
		seq_printf(sf, " wbps=%llu ", tg->bps_conf[WRITE][off]);

	if (tg->iops_conf[READ][off] == UINT_MAX)
		seq_printf(sf, " riops=max");
	else
		seq_printf(sf, " riops=%u", tg->iops_conf[READ][off]);

	if (tg->iops_conf[WRITE][off] == UINT_MAX)
		seq_printf(sf, " wiops=max");
	else
		seq_printf(sf, " wiops=%u", tg->iops_conf[WRITE][off]);

	if (off == LIMIT_LOW) {
		if (tg->idletime_threshold_conf == ULONG_MAX)
			seq_printf(sf, " idle=max");
		else
			seq_printf(sf, " idle=%lu", tg->idletime_threshold_conf);

		if (tg->latency_target_conf == ULONG_MAX)
			seq_printf(sf, " latency=max");
		else
			seq_printf(sf, " latency=%lu", tg->latency_target_conf);
	}

	return 0;
}

better?

Thanks,
John

Re: [PATCH] blk-throttle: Fix W=1 build issue in tg_prfill_limit()

[Christoph Hellwig]

On Tue, Mar 26, 2024 at 08:46:24AM +0000, John Garry wrote:
> On 26/03/2024 07:03, Christoph Hellwig wrote:
> > On Mon, Mar 25, 2024 at 12:19:05PM +0000, John Garry wrote:
> > > For when using gcc 8 and above, the following warnings can be seen when
> > > compiling blk-throttle.c with W=1:
> > 
> > Why is this function even using these local buffers vs a sequence
> > of separate seq_printf calls that would get rid of these pointless
> > on-stack buffers?
> 
> Currently a combo of snprintf and seq_printf is used, a strategy which seems
> to go as far back as 2ee867dcfa2ea (2015), when it was a much simpler print.
> All the other code in this area only uses seq_printf, so it seems that the
> author(s) prefer this way here.
> 
> Here's how the current code could look (using only seq_printf):

That looks much better, thanks.

Re: [PATCH] blk-throttle: Fix W=1 build issue in tg_prfill_limit()

[John Garry]

On 26/03/2024 14:49, Christoph Hellwig wrote:
>> Here's how the current code could look (using only seq_printf):
> That looks much better, thanks.

ok, I'll put it in a formal patch. It would be nice to put purely a 
"fix" for stable, but no one else has complained about this old warning 
AFAIK.

Thanks,
John