[blkback] blkback statistic counters are signed values

[blkback] blkback statistic counters are signed values

[Zoltan Kiss]

Hi,

One of our colleagues spotted a problem in xentop. Sometimes the 
VBD_WSECT value suddenly becomes unreasonably high, and it turned out 
xentop reads 
/sys/bus/xen-backend/devices/vbd-(domid)-(devID)/statistics/wr_sect into 
an unsigned long long. That value is exposed by blkback, and among other 
stat counters, it's a signed integer:

drivers/block/xen-blkback/common.h
struct xen_blkif {
...
	int			st_rd_req;
	int			st_wr_req;
	int			st_oo_req;
	int			st_f_req;
	int			st_ds_req;
	int			st_rd_sect;
	int			st_wr_sect;


I don't think these values should be negative ever, but when they 
overflow (which happens eventually), they do, and this leads to bad 
conversion in xentop.
I think the best solution would be to change the above mentioned values 
to unsigned long long as well. Any comments on that?

Regards,

Zoltan Kiss

Re: [blkback] blkback statistic counters are signed values

[Konrad Rzeszutek Wilk]

On Mon, Mar 04, 2013 at 03:54:42PM +0000, Zoltan Kiss wrote:
> Hi,
> 
> One of our colleagues spotted a problem in xentop. Sometimes the
> VBD_WSECT value suddenly becomes unreasonably high, and it turned
> out xentop reads
> /sys/bus/xen-backend/devices/vbd-(domid)-(devID)/statistics/wr_sect
> into an unsigned long long. That value is exposed by blkback, and
> among other stat counters, it's a signed integer:
> 
> drivers/block/xen-blkback/common.h
> struct xen_blkif {
> ...
> 	int			st_rd_req;
> 	int			st_wr_req;
> 	int			st_oo_req;
> 	int			st_f_req;
> 	int			st_ds_req;
> 	int			st_rd_sect;
> 	int			st_wr_sect;
> 
> 
> I don't think these values should be negative ever, but when they
> overflow (which happens eventually), they do, and this leads to bad
> conversion in xentop.
> I think the best solution would be to change the above mentioned
> values to unsigned long long as well. Any comments on that?

Seems like the right approach. Could you send a patch please?

Thought that won't solve the problem when we overflow an 'unsigned long long'.

> 
> Regards,
> 
> Zoltan Kiss
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> http://lists.xen.org/xen-devel
> 

Re: [blkback] blkback statistic counters are signed values

[Zoltan Kiss]

Hi,

On 04/03/13 19:22, Konrad Rzeszutek Wilk wrote:
> Seems like the right approach. Could you send a patch please?
I just sent it to you and linux-kernel@vger.kernel.org:

http://lkml.indiana.edu/hypermail/linux/kernel/1303.0/02306.html

> Thought that won't solve the problem when we overflow an 'unsigned long long'.
Well, I think it's good enough if the users of these values pay 
attention, and poll these sysfs entries often enough to detect that 
there was an overflow.

Regards,

Zoltan Kiss

Re: [blkback] blkback statistic counters are signed values

[David Vrabel]

On 05/03/13 18:07, Zoltan Kiss wrote:
> Hi,
> 
> On 04/03/13 19:22, Konrad Rzeszutek Wilk wrote:
>> Seems like the right approach. Could you send a patch please?
> I just sent it to you and linux-kernel@vger.kernel.org:
> 
> http://lkml.indiana.edu/hypermail/linux/kernel/1303.0/02306.html
> 
>> Thought that won't solve the problem when we overflow an 'unsigned
>> long long'.
> Well, I think it's good enough if the users of these values pay
> attention, and poll these sysfs entries often enough to detect that
> there was an overflow.

Unless my maths is wrong it will take millions of years for these sector
count statistics to overflow.

David

Re: [blkback] blkback statistic counters are signed values

[Konrad Rzeszutek Wilk]

On Wed, Mar 06, 2013 at 11:14:11AM +0000, David Vrabel wrote:
> On 05/03/13 18:07, Zoltan Kiss wrote:
> > Hi,
> > 
> > On 04/03/13 19:22, Konrad Rzeszutek Wilk wrote:
> >> Seems like the right approach. Could you send a patch please?
> > I just sent it to you and linux-kernel@vger.kernel.org:
> > 
> > http://lkml.indiana.edu/hypermail/linux/kernel/1303.0/02306.html
> > 
> >> Thought that won't solve the problem when we overflow an 'unsigned
> >> long long'.
> > Well, I think it's good enough if the users of these values pay
> > attention, and poll these sysfs entries often enough to detect that
> > there was an overflow.
> 
> Unless my maths is wrong it will take millions of years for these sector
> count statistics to overflow.

OK, so you are saying like Bill Gates, that this ought to be enough :-)

I will take that as an Acked-by :-)
> 
> David