Re: Making statCounter.syscalls.disk counters more consistent

On Fri, 2003-07-18 at 09:05, Duane Wessels wrote:
> The following patch is intended to make Squid be more consistent
> with the statCounter.syscalls.disk counters.
>
> Currently they are incremented by UFS and Diskd, but not by
> AUFS and unlinkd. I'd like to hear from a pthreads expert
> that the changes are safe for AUFS.
>
>
> Index: src/unlinkd.c
> ===================================================================
> RCS file: /server/cvs-server/squid/squid/src/unlinkd.c,v
> retrieving revision 1.44.2.1
> diff -u -3 -p -r1.44.2.1 unlinkd.c
> --- src/unlinkd.c 21 Jul 2002 00:30:03 -0000 1.44.2.1
> +++ src/unlinkd.c 17 Jul 2003 22:57:15 -0000
> @@ -54,6 +54,7 @@ main(int argc, char *argv[])
> while (fgets(buf, UNLINK_BUF_LEN, stdin)) {
> if ((t = strchr(buf, '\n')))
> *t = '\0';
> + statCounter.syscalls.disk.unlinks++;
> #if USE_TRUNCATE
> x = truncate(buf, 0);
> #else

This won't increment the counters: the statCounter used by cachemanager
is not the statCounter available to the external unlinkd process.

> Index: src/fs/aufs/aiops.c
> ===================================================================
> RCS file: /server/cvs-server/squid/squid/src/fs/aufs/aiops.c,v
> retrieving revision 1.12.2.7
> diff -u -3 -p -r1.12.2.7 aiops.c
> --- src/fs/aufs/aiops.c 9 Jan 2003 05:04:23 -0000 1.12.2.7
> +++ src/fs/aufs/aiops.c 17 Jul 2003 22:54:46 -0000
> @@ -608,6 +608,7 @@ squidaio_open(const char *path, int ofla
> static void
> squidaio_do_open(squidaio_request_t * requestp)
> {
> + statCounter.syscalls.disk.opens++;
> requestp->ret = open(requestp->path, requestp->oflag, requestp->mode);
> requestp->err = errno;
> }
> @@ -638,7 +639,9 @@ squidaio_read(int fd, char *bufp, int bu
> static void
> squidaio_do_read(squidaio_request_t * requestp)
> {
> + statCounter.syscalls.disk.seeks++;
> lseek(requestp->fd, requestp->offset, requestp->whence);
> + statCounter.syscalls.disk.reads++;
> requestp->ret = read(requestp->fd, requestp->bufferp, requestp->buflen);
> requestp->err = errno;
> }
> @@ -670,6 +673,7 @@ squidaio_write(int fd, char *bufp, int b
> static void
> squidaio_do_write(squidaio_request_t * requestp)
> {
> + statCounter.syscalls.disk.writes++;
> requestp->ret = write(requestp->fd, requestp->tmpbufp, requestp->buflen);
> requestp->err = errno;
> }
> @@ -696,6 +700,7 @@ squidaio_close(int fd, squidaio_result_t
> static void
> squidaio_do_close(squidaio_request_t * requestp)
> {
> + statCounter.syscalls.disk.closes++;
> requestp->ret = close(requestp->fd);
> requestp->err = errno;
> }
> @@ -750,6 +755,7 @@ squidaio_unlink(const char *path, squida
> static void
> squidaio_do_unlink(squidaio_request_t * requestp)
> {
> + statCounter.syscalls.disk.unlinks++;
> requestp->ret = unlink(requestp->path);
> requestp->err = errno;
> }
> @@ -776,6 +782,7 @@ squidaio_truncate(const char *path, off_
> static void
> squidaio_do_truncate(squidaio_request_t * requestp)
> {
> + statCounter.syscalls.disk.unlinks++;
> requestp->ret = truncate(requestp->path, requestp->offset);
> requestp->err = errno;
> }

None of the above are safe. They are all potentially racey on SMP
machines. You need to use interlocked increments to safe perform such
counts. However, aufs -does- record the syscall counts: during the
scheduling operation, not during the worker threads actual call. That is
thread safe today..

> Index: src/fs/aufs/store_dir_aufs.c
> ===================================================================
> RCS file: /server/cvs-server/squid/squid/src/fs/aufs/store_dir_aufs.c,v
> retrieving revision 1.40.2.7
> diff -u -3 -p -r1.40.2.7 store_dir_aufs.c
> --- src/fs/aufs/store_dir_aufs.c 9 Jan 2003 03:38:38 -0000 1.40.2.7
> +++ src/fs/aufs/store_dir_aufs.c 17 Jul 2003 22:49:16 -0000
> @@ -416,7 +416,6 @@ storeAufsDirRebuildFromDirectory(void *d
> debug(47, 3) (" %s %7d files opened so far.\n",
> rb->sd->path, rb->counts.scancount);
> debug(47, 9) ("file_in: fd=%d %08X\n", fd, filn);
> - statCounter.syscalls.disk.reads++;
> if (FD_READ_METHOD(fd, hdr_buf, SM_PAGE_SIZE) < 0) {
> debug(47, 1) ("storeAufsDirRebuildFromDirectory: read(FD %d): %s\n",
> fd, xstrerror());

Why are you removing this one? (Is it double counted?)

> Index: src/fs/ufs/store_dir_ufs.c
> ===================================================================
> RCS file: /server/cvs-server/squid/squid/src/fs/ufs/store_dir_ufs.c,v
> retrieving revision 1.39.2.7
> diff -u -3 -p -r1.39.2.7 store_dir_ufs.c
> --- src/fs/ufs/store_dir_ufs.c 9 Jan 2003 03:38:48 -0000 1.39.2.7
> +++ src/fs/ufs/store_dir_ufs.c 17 Jul 2003 22:55:31 -0000
> @@ -415,7 +415,6 @@ storeUfsDirRebuildFromDirectory(void *da
> debug(47, 3) (" %s %7d files opened so far.\n",
> rb->sd->path, rb->counts.scancount);
> debug(47, 9) ("file_in: fd=%d %08X\n", fd, filn);
> - statCounter.syscalls.disk.reads++;
> if (FD_READ_METHOD(fd, hdr_buf, SM_PAGE_SIZE) < 0) {
> debug(47, 1) ("storeUfsDirRebuildFromDirectory: read(FD %d): %s\n",
> fd, xstrerror());

Ditto ?

Cheers,
Rob

-- 
GPG key available at: <http://members.aardvark.net.au/lifeless/keys.txt>.