Xen devel - Mar 2013 - [blkback] blkback statistic counters are signed values

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

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
>

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

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

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