Date: Mon, 31 Jan 2011 10:27:16 +0100 From: Martin Matuska <mm@FreeBSD.org> To: Bruce Evans <brde@optusnet.com.au> Cc: freebsd-fs <freebsd-fs@FreeBSD.org>, Pawel Jakub Dawidek <pjd@FreeBSD.org> Subject: Re: ZFS: clock_t overflow in l2arc_feed_thread Message-ID: <4D468074.5050402@FreeBSD.org> In-Reply-To: <20110131160644.P2539@besplex.bde.org> References: <AANLkTikciV_XHvrurytr0-r11W4u=_p5bRi-xfX3S%2BQm@mail.gmail.com> <4D443407.7010604@FreeBSD.org> <AANLkTikASEbWQRsZr%2BMHth-jzbskwt4P14mXjCjdZrPk@mail.gmail.com> <4D45F359.4040402@FreeBSD.org> <20110131160644.P2539@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
This is a problem for several places in the ZFS code, most of them in arc.c. The L2 issuse is quite clear: in arc.c: l2arc_feed_thread() we have the following statement: (void) cv_timedwait(&l2arc_feed_thr_cv, &l2arc_feed_thr_lock, next - LBOLT); next gets calculated from l2arc_write_interval() from the previous while() loop. Now a value of 0 will occur if l2arc_writ_interval() returns the same LBOLT, effectively making this a cv_wait(), negative values for the timeout are theoretically possible, too. In addition there are other areas in the code where comparative decisions based on a stored LBOLT and current LBOLT are made. A wrong decision made on clock_t overflow time might be harmful and cause unexpected behaviour. Affected are (what I have seen so far, in sys/cddl/contrib/opensolaris/uts/common/fs/zfs/): arc.c: arc_access(), arc_reclaim_thread(), l2arc_write_interval(), l2arc_feed_thread() dmu_zfetch.c: dmu_zfetch_dofetch(), dmu_zfetch_stream_reclaim() zil.c: zil_replay() In userland we have some clock_t in: lib/libzpool/common/kernel.c Dňa 31.01.2011 06:25, Bruce Evans wrote / napísal(a): > On Mon, 31 Jan 2011, Martin Matuska wrote: > >> I have re-checked OpenSolaris, and discovered that long is a int32_t. >> >> I agree, we should go for int64_t in our case. > > Why be different from both OpenSolaris and FreeBSD? clock_t is 32 bits > on all, and only bogusly signed on some. > > Also, there must be another bug for overflow to occur after only 24 days. > clock_t is only specified for holding statclock ticks, which have a > frequency of about 128 Hz, so 32-bit unsigned ints hold 388 days of > ticks. > 32-bit signed ints hold 194 days of ticks. It is a units/type error to > use clock_t for any other type of ticks. I used this an excuse to not > expand clock_t on i386 about 15 years ago when I worked on this most. > Clock ticks should be virtual and have a frequency much higher than 128, > especially 15 years later. 1 MHz would have been OK 15 years ago, and > some broken standards (XSI?) even required this. 1 GHz would have been > good to cover the next 15 years, but it actually only lasted about 5 > years since CPU frequencies exceeded it in about 2001. Now that CPU > frequencies have stopped growing rapidly, there seem to be no clock > periods much smaller than 0.1 nsec on the horizon, so 1 THz might last > more than 5 years. But even 1 GHz will overflow a 32-bit signed int > in about 2 seconds. IIRC, POSIX only requires clock_t to work for > 24 hours, so overflow after 24 days isn't necessarily a bug, but overflow > after 2 or 2000 seconds (the latter for 1 MHz) would be. POSIX mostly > uses timespecs when more resolution is required, but this has only been > standard for 23 years so support and use of them for are patchy. > > [Context lost to top posting] > > Bruce > _______________________________________________ > freebsd-fs@freebsd.org mailing list > http://lists.freebsd.org/mailman/listinfo/freebsd-fs > To unsubscribe, send any mail to "freebsd-fs-unsubscribe@freebsd.org"
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4D468074.5050402>