From aa150b940251ca5518a5c79a12e2b34f3835d339 Mon Sep 17 00:00:00 2001 From: Daniel Black Date: Tue, 15 Jan 2019 13:53:01 +1100 Subject: [PATCH 1/2] mysys: THR_LOCK_open reduce usage Change the following to statistic counters: * my_file_opened * my_file_total_opened * my_stream_opened * my_tmp_file_created A file descriptor is already a unique element per process - in Windows, protection occurs at fd allocation using THR_LOCK_open in my_win_{,f}open and in other OSes, a unique fd to file map exists at the OS level. So accesses to my_file_info[fd] don't need to be protected by the THR_LOCK_open. my_close/my_fclose where restructured to clear out the my_file_info before the close/my_win_close/my_win_fclose. After these calls another thread could gain the same file descriptor. So for Windows this the file_info elements available to the my_win_{,f}_open are released during the invalidate_fd call within my_win_close. No locking is needed as the my_win_{,f}open is searching for a invalidate entry which is determined by a single value change. There is one non-statistics use of my_file_opened/my_stream_opened in my_end which prints a warning if we shutdown and its still open. --- mysys/mf_tempfile.cc | 2 -- mysys/my_fopen.cc | 22 +++++++++------------- mysys/my_open.cc | 23 +++++++++++------------ 3 files changed, 20 insertions(+), 27 deletions(-) diff --git a/mysys/mf_tempfile.cc b/mysys/mf_tempfile.cc index f2bf1b18036..4ac5d1abc29 100644 --- a/mysys/mf_tempfile.cc +++ b/mysys/mf_tempfile.cc @@ -147,9 +147,7 @@ File create_temp_file(char *to, const char *dir, const char *prefix, } #endif if (file >= 0) { - mysql_mutex_lock(&THR_LOCK_open); my_tmp_file_created++; - mysql_mutex_unlock(&THR_LOCK_open); } DBUG_RETURN(file); } diff --git a/mysys/my_fopen.cc b/mysys/my_fopen.cc index c6f42215d1a..4672c74b522 100644 --- a/mysys/my_fopen.cc +++ b/mysys/my_fopen.cc @@ -84,19 +84,15 @@ FILE *my_fopen(const char *filename, int flags, myf MyFlags) { int filedesc = my_fileno(fd); if ((uint)filedesc >= my_file_limit) { - mysql_mutex_lock(&THR_LOCK_open); my_stream_opened++; - mysql_mutex_unlock(&THR_LOCK_open); DBUG_RETURN(fd); /* safeguard */ } dup_filename = my_strdup(key_memory_my_file_info, filename, MyFlags); if (dup_filename != NULL) { - mysql_mutex_lock(&THR_LOCK_open); my_file_info[filedesc].name = dup_filename; my_stream_opened++; my_file_total_opened++; my_file_info[filedesc].type = STREAM_BY_FOPEN; - mysql_mutex_unlock(&THR_LOCK_open); DBUG_PRINT("exit", ("stream: %p", fd)); DBUG_RETURN(fd); } @@ -185,11 +181,16 @@ FILE *my_freopen(const char *path, const char *mode, FILE *stream) { /* Close a stream */ int my_fclose(FILE *fd, myf MyFlags) { int err, file; + char *name = NULL; DBUG_ENTER("my_fclose"); DBUG_PRINT("my", ("stream: %p MyFlags: %d", fd, MyFlags)); - mysql_mutex_lock(&THR_LOCK_open); file = my_fileno(fd); + if ((uint)file < my_file_limit && my_file_info[file].type != UNOPEN) { + name = my_file_info[file].name; + my_file_info[file].type = UNOPEN; + my_file_info[file].name = NULL; + } #ifndef _WIN32 err = fclose(fd); #else @@ -199,16 +200,13 @@ int my_fclose(FILE *fd, myf MyFlags) { set_my_errno(errno); if (MyFlags & (MY_FAE | MY_WME)) { char errbuf[MYSYS_STRERROR_SIZE]; - my_error(EE_BADCLOSE, MYF(0), my_filename(file), my_errno(), + my_error(EE_BADCLOSE, MYF(0), name, my_errno(), my_strerror(errbuf, sizeof(errbuf), my_errno())); } } else my_stream_opened--; - if ((uint)file < my_file_limit && my_file_info[file].type != UNOPEN) { - my_file_info[file].type = UNOPEN; - my_free(my_file_info[file].name); - } - mysql_mutex_unlock(&THR_LOCK_open); + if (name) + my_free(name); DBUG_RETURN(err); } /* my_fclose */ @@ -235,7 +233,6 @@ FILE *my_fdopen(File Filedes, const char *name, int Flags, myf MyFlags) { my_strerror(errbuf, sizeof(errbuf), my_errno())); } } else { - mysql_mutex_lock(&THR_LOCK_open); my_stream_opened++; if ((uint)Filedes < (uint)my_file_limit) { if (my_file_info[Filedes].type != UNOPEN) { @@ -246,7 +243,6 @@ FILE *my_fdopen(File Filedes, const char *name, int Flags, myf MyFlags) { } my_file_info[Filedes].type = STREAM_BY_FDOPEN; } - mysql_mutex_unlock(&THR_LOCK_open); } DBUG_PRINT("exit", ("stream: %p", fd)); diff --git a/mysys/my_open.cc b/mysys/my_open.cc index ee35d133b73..3d6a2e98442 100644 --- a/mysys/my_open.cc +++ b/mysys/my_open.cc @@ -94,10 +94,15 @@ File my_open(const char *FileName, int Flags, myf MyFlags) int my_close(File fd, myf MyFlags) { int err; + char *name = NULL; DBUG_ENTER("my_close"); DBUG_PRINT("my", ("fd: %d MyFlags: %d", fd, MyFlags)); - mysql_mutex_lock(&THR_LOCK_open); + if ((uint)fd < my_file_limit && my_file_info[fd].type != UNOPEN) { + name = my_file_info[fd].name; + my_file_info[fd].type = UNOPEN; + my_file_info[fd].name = NULL; + } #ifndef _WIN32 do { err = close(fd); @@ -110,16 +115,14 @@ int my_close(File fd, myf MyFlags) { set_my_errno(errno); if (MyFlags & (MY_FAE | MY_WME)) { char errbuf[MYSYS_STRERROR_SIZE]; - my_error(EE_BADCLOSE, MYF(0), my_filename(fd), my_errno(), + my_error(EE_BADCLOSE, MYF(0), name, my_errno(), my_strerror(errbuf, sizeof(errbuf), my_errno())); } + } else + my_file_opened--; + if (name) { + my_free(name); } - if ((uint)fd < my_file_limit && my_file_info[fd].type != UNOPEN) { - my_free(my_file_info[fd].name); - my_file_info[fd].type = UNOPEN; - } - my_file_opened--; - mysql_mutex_unlock(&THR_LOCK_open); DBUG_RETURN(err); } /* my_close */ @@ -150,20 +153,16 @@ File my_register_filename(File fd, const char *FileName, #if defined(_WIN32) set_my_errno(EMFILE); #else - mysql_mutex_lock(&THR_LOCK_open); my_file_opened++; - mysql_mutex_unlock(&THR_LOCK_open); DBUG_RETURN(fd); /* safeguard */ #endif } else { dup_filename = my_strdup(key_memory_my_file_info, FileName, MyFlags); if (dup_filename != NULL) { - mysql_mutex_lock(&THR_LOCK_open); my_file_info[fd].name = dup_filename; my_file_opened++; my_file_total_opened++; my_file_info[fd].type = type_of_file; - mysql_mutex_unlock(&THR_LOCK_open); DBUG_PRINT("exit", ("fd: %d", fd)); DBUG_RETURN(fd); } From b725ea2d4d76f4775526fa6e9f39ba40acf7ccc2 Mon Sep 17 00:00:00 2001 From: Daniel Black Date: Tue, 15 Jan 2019 14:42:59 +1100 Subject: [PATCH 2/2] remove my_print_open_files my_file_opened are now my_stream_opened incremented without locks so there is a small chance that they will not be statisticly accurate. As my_end error messages should be accurate, lets not take a chance and iterate the array, displaying each open file under EXTRA_DEBUG, and display a warning if a mismatch of files/streams opened occurred. --- include/my_sys.h | 6 ------ mysys/my_file.cc | 2 +- mysys/my_init.cc | 29 ++++++++++++++++++++++++----- mysys/my_open.cc | 16 ---------------- 4 files changed, 25 insertions(+), 28 deletions(-) diff --git a/include/my_sys.h b/include/my_sys.h index 63a0aede28d..27025e3cbc5 100644 --- a/include/my_sys.h +++ b/include/my_sys.h @@ -671,12 +671,6 @@ extern char *my_filename(File fd); extern MY_MODE get_file_perm(ulong perm_flags); extern bool my_chmod(const char *filename, ulong perm_flags, myf my_flags); -#ifdef EXTRA_DEBUG -void my_print_open_files(void); -#else -#define my_print_open_files() -#endif - extern bool init_tmpdir(MY_TMPDIR *tmpdir, const char *pathlist); extern char *my_tmpdir(MY_TMPDIR *tmpdir); extern void free_tmpdir(MY_TMPDIR *tmpdir); diff --git a/mysys/my_file.cc b/mysys/my_file.cc index b25515fb7f7..46ac30a4a79 100644 --- a/mysys/my_file.cc +++ b/mysys/my_file.cc @@ -147,7 +147,7 @@ uint my_set_max_open_files(uint files) { void my_free_open_file_info() { DBUG_ENTER("my_free_file_info"); if (my_file_info != my_file_info_default) { - /* Copy data back for my_print_open_files */ + /* Copy data back for my_end */ memcpy((char *)my_file_info_default, my_file_info, sizeof(*my_file_info_default) * MY_NFILE); my_free(my_file_info); diff --git a/mysys/my_init.cc b/mysys/my_init.cc index 388b688180c..0c4209f759b 100644 --- a/mysys/my_init.cc +++ b/mysys/my_init.cc @@ -179,6 +179,7 @@ void my_end(int infoflag) { operational, so we cannot use DBUG_RETURN. */ + ulong file_opened = 0, stream_opened = 0; FILE *info_file = (DBUG_FILE ? DBUG_FILE : stderr); if (!my_init_done) return; @@ -186,13 +187,31 @@ void my_end(int infoflag) { if ((infoflag & MY_CHECK_ERROR) || (info_file != stderr)) { /* Test if some file is left open */ - if (my_file_opened | my_stream_opened) { - char ebuff[512]; - snprintf(ebuff, sizeof(ebuff), EE(EE_OPEN_WARNING), my_file_opened, - my_stream_opened); + uint i; + char ebuff[512]; + for (i = 0; i < my_file_limit; i++) { + if (my_file_info[i].type != UNOPEN) { +#ifdef EXTRA_DEBUG + my_message_local(INFORMATION_LEVEL, EE_FILE_NOT_CLOSED, + my_file_info[i].name, i); +#endif + switch (my_file_info[i].type) + { + case STREAM_BY_FOPEN: + case STREAM_BY_FDOPEN: + stream_opened++; + break; + default: + file_opened++; + } + } + + } + if (file_opened || stream_opened) { + snprintf(ebuff, sizeof(ebuff), EE(EE_OPEN_WARNING), file_opened, + stream_opened); my_message_stderr(EE_OPEN_WARNING, ebuff, MYF(0)); DBUG_PRINT("error", ("%s", ebuff)); - my_print_open_files(); } } my_error_unregister_all(); diff --git a/mysys/my_open.cc b/mysys/my_open.cc index 3d6a2e98442..22305721b94 100644 --- a/mysys/my_open.cc +++ b/mysys/my_open.cc @@ -182,19 +182,3 @@ File my_register_filename(File fd, const char *FileName, } DBUG_RETURN(-1); } - -#ifdef EXTRA_DEBUG - -void my_print_open_files(void) { - if (my_file_opened | my_stream_opened) { - uint i; - for (i = 0; i < my_file_limit; i++) { - if (my_file_info[i].type != UNOPEN) { - my_message_local(INFORMATION_LEVEL, EE_FILE_NOT_CLOSED, - my_file_info[i].name, i); - } - } - } -} - -#endif