Skip to content

Commit 6882beb

Browse files
marcalffmarcalff
committed
Bug#14113704 - A STAT ON A NON EXISTING FILE CREATING A FILE RECORD IN PFS
Before this fix, the file io instrumentation for the performance schema would instrument some operations as follows: - For FILE_STAT, a file instrument was created before the actual call to my_file_stat. - For FILE_OPEN, FILE_STREAM_OPEN, a file instrument was also created before the actual file open call. - For FILE_DELETE, a file instrument was possibly created before the actual call, only to be destroyed after the call. In general, this "optimistic" approach leads to several problems. When the actual call to open(), fopen() or stat() fails, the instrumented file that was created needs to be cleaned up. At best, this creates some performance overhead. At worst, this creates instrumentation leaks when the cleanup code is missing, as found initially with the FILE_STAT operation. In addition, attempting to create / destroy file instrumentations in several threads executing open / close / stat / delete on the same file concurrently has a high risk of creating race conditions inside the file instrumentation, even when such race conditions do not exist in the code instrumented. The solution is to never create files optimistically in a "start" event (before the operation), but do create file instrumentation in the "end" event only, and only if the operation actually succeeded. Specific changes: - move the find_or_create_file() call from start_file_open_wait() to end_file_open_wait() - add a name / class member to the file locker state, to hold parameters until the call to find_or_create_file() - remove all file creation / deletion logic present in start_file_wait() and end_file_wait(), for both clarity and performances - create new start_file_close_wait() and end_file_close_wait() instrumentation points, used when closing / deleting files. - created find_file(), to be used when instrumenting FILE_DELETE: if the file is not known, there is no point in creating instrumentation only to remove it after the operation.
1 parent 408ef21 commit 6882beb

File tree

8 files changed

+314
-79
lines changed

8 files changed

+314
-79
lines changed

‎include/mysql/psi/mysql_file.h‎

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -813,10 +813,10 @@ inline_mysql_file_fopen(
813813
(&state, key, PSI_FILE_STREAM_OPEN, filename, that);
814814
if (likely(locker != NULL))
815815
{
816-
that->m_psi= PSI_FILE_CALL(start_file_open_wait)
816+
PSI_FILE_CALL(start_file_open_wait)
817817
(locker, src_file, src_line);
818818
that->m_file= my_fopen(filename, flags, myFlags);
819-
PSI_FILE_CALL(end_file_open_wait)(locker, NULL);
819+
that->m_psi= PSI_FILE_CALL(end_file_open_wait)(locker, that->m_file);
820820
if (unlikely(that->m_file == NULL))
821821
{
822822
my_free(that);
@@ -854,9 +854,9 @@ inline_mysql_file_fclose(
854854
(&state, file->m_psi, PSI_FILE_STREAM_CLOSE);
855855
if (likely(locker != NULL))
856856
{
857-
PSI_FILE_CALL(start_file_wait)(locker, (size_t) 0, src_file, src_line);
857+
PSI_FILE_CALL(start_file_close_wait)(locker, src_file, src_line);
858858
result= my_fclose(file->m_file, flags);
859-
PSI_FILE_CALL(end_file_wait)(locker, (size_t) 0);
859+
PSI_FILE_CALL(end_file_close_wait)(locker, result);
860860
my_free(file);
861861
return result;
862862
}
@@ -1069,9 +1069,9 @@ inline_mysql_file_close(
10691069
(&state, file, PSI_FILE_CLOSE);
10701070
if (likely(locker != NULL))
10711071
{
1072-
PSI_FILE_CALL(start_file_wait)(locker, (size_t) 0, src_file, src_line);
1072+
PSI_FILE_CALL(start_file_close_wait)(locker, src_file, src_line);
10731073
result= my_close(file, flags);
1074-
PSI_FILE_CALL(end_file_wait)(locker, (size_t) 0);
1074+
PSI_FILE_CALL(end_file_close_wait)(locker, result);
10751075
return result;
10761076
}
10771077
#endif
@@ -1271,9 +1271,9 @@ inline_mysql_file_delete(
12711271
(&state, key, PSI_FILE_DELETE, name, &locker);
12721272
if (likely(locker != NULL))
12731273
{
1274-
PSI_FILE_CALL(start_file_wait)(locker, (size_t) 0, src_file, src_line);
1274+
PSI_FILE_CALL(start_file_close_wait)(locker, src_file, src_line);
12751275
result= my_delete(name, flags);
1276-
PSI_FILE_CALL(end_file_wait)(locker, (size_t) 0);
1276+
PSI_FILE_CALL(end_file_close_wait)(locker, result);
12771277
return result;
12781278
}
12791279
#endif
@@ -1352,9 +1352,9 @@ inline_mysql_file_delete_with_symlink(
13521352
(&state, key, PSI_FILE_DELETE, name, &locker);
13531353
if (likely(locker != NULL))
13541354
{
1355-
PSI_FILE_CALL(start_file_wait)(locker, (size_t) 0, src_file, src_line);
1355+
PSI_FILE_CALL(start_file_close_wait)(locker, src_file, src_line);
13561356
result= my_delete_with_symlink(name, flags);
1357-
PSI_FILE_CALL(end_file_wait)(locker, (size_t) 0);
1357+
PSI_FILE_CALL(end_file_close_wait)(locker, result);
13581358
return result;
13591359
}
13601360
#endif

‎include/mysql/psi/psi.h‎

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -899,6 +899,10 @@ struct PSI_file_locker_state_v1
899899
enum PSI_file_operation m_operation;
900900
/** Current file. */
901901
struct PSI_file *m_file;
902+
/** Current file name. */
903+
const char *m_name;
904+
/** Current file class. */
905+
void *m_class;
902906
/** Current thread. */
903907
struct PSI_thread *m_thread;
904908
/** Operation number of bytes. */
@@ -1575,17 +1579,18 @@ typedef void (*end_table_lock_wait_v1_t)(struct PSI_table_locker *locker);
15751579
@param op the operation to perform
15761580
@param src_file the source file name
15771581
@param src_line the source line number
1578-
@return an instrumented file handle
15791582
*/
1580-
typedef struct PSI_file* (*start_file_open_wait_v1_t)
1583+
typedef void (*start_file_open_wait_v1_t)
15811584
(struct PSI_file_locker *locker, const char *src_file, uint src_line);
15821585

15831586
/**
15841587
End a file instrumentation open operation, for file streams.
15851588
@param locker the file locker.
1589+
@param result the opened file (NULL indicates failure, non NULL success).
1590+
@return an instrumented file handle
15861591
*/
1587-
typedef void (*end_file_open_wait_v1_t)(struct PSI_file_locker *locker,
1588-
void *result);
1592+
typedef struct PSI_file* (*end_file_open_wait_v1_t)
1593+
(struct PSI_file_locker *locker, void *result);
15891594

15901595
/**
15911596
End a file instrumentation open operation, for non stream files.
@@ -1622,6 +1627,25 @@ typedef void (*start_file_wait_v1_t)
16221627
typedef void (*end_file_wait_v1_t)
16231628
(struct PSI_file_locker *locker, size_t count);
16241629

1630+
/**
1631+
Start a file instrumentation close operation.
1632+
@param locker the file locker
1633+
@param op the operation to perform
1634+
@param src_file the source file name
1635+
@param src_line the source line number
1636+
*/
1637+
typedef void (*start_file_close_wait_v1_t)
1638+
(struct PSI_file_locker *locker, const char *src_file, uint src_line);
1639+
1640+
/**
1641+
End a file instrumentation close operation.
1642+
@param locker the file locker.
1643+
@param rc the close operation return code (0 for success).
1644+
@return an instrumented file handle
1645+
*/
1646+
typedef void (*end_file_close_wait_v1_t)
1647+
(struct PSI_file_locker *locker, int rc);
1648+
16251649
/**
16261650
Start a new stage, and implicitly end the previous stage.
16271651
@param key the key of the new stage
@@ -2025,6 +2049,10 @@ struct PSI_v1
20252049
start_file_wait_v1_t start_file_wait;
20262050
/** @sa end_file_wait_v1_t. */
20272051
end_file_wait_v1_t end_file_wait;
2052+
/** @sa start_file_close_wait_v1_t. */
2053+
start_file_close_wait_v1_t start_file_close_wait;
2054+
/** @sa end_file_close_wait_v1_t. */
2055+
end_file_close_wait_v1_t end_file_close_wait;
20282056
/** @sa start_stage_v1_t. */
20292057
start_stage_v1_t start_stage;
20302058
/** @sa end_stage_v1_t. */

‎include/mysql/psi/psi_abi_v1.h.pp‎

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -221,6 +221,8 @@
221221
uint m_flags;
222222
enum PSI_file_operation m_operation;
223223
struct PSI_file *m_file;
224+
const char *m_name;
225+
void *m_class;
224226
struct PSI_thread *m_thread;
225227
size_t m_number_of_bytes;
226228
ulonglong m_timer_start;
@@ -422,17 +424,21 @@
422424
ulong flags,
423425
const char *src_file, uint src_line);
424426
typedef void (*end_table_lock_wait_v1_t)(struct PSI_table_locker *locker);
425-
typedef struct PSI_file* (*start_file_open_wait_v1_t)
427+
typedef void (*start_file_open_wait_v1_t)
426428
(struct PSI_file_locker *locker, const char *src_file, uint src_line);
427-
typedef void (*end_file_open_wait_v1_t)(struct PSI_file_locker *locker,
428-
void *result);
429+
typedef struct PSI_file* (*end_file_open_wait_v1_t)
430+
(struct PSI_file_locker *locker, void *result);
429431
typedef void (*end_file_open_wait_and_bind_to_descriptor_v1_t)
430432
(struct PSI_file_locker *locker, File file);
431433
typedef void (*start_file_wait_v1_t)
432434
(struct PSI_file_locker *locker, size_t count,
433435
const char *src_file, uint src_line);
434436
typedef void (*end_file_wait_v1_t)
435437
(struct PSI_file_locker *locker, size_t count);
438+
typedef void (*start_file_close_wait_v1_t)
439+
(struct PSI_file_locker *locker, const char *src_file, uint src_line);
440+
typedef void (*end_file_close_wait_v1_t)
441+
(struct PSI_file_locker *locker, int rc);
436442
typedef void (*start_stage_v1_t)
437443
(PSI_stage_key key, const char *src_file, int src_line);
438444
typedef void (*end_stage_v1_t) (void);
@@ -571,6 +577,8 @@
571577
end_file_open_wait_and_bind_to_descriptor;
572578
start_file_wait_v1_t start_file_wait;
573579
end_file_wait_v1_t end_file_wait;
580+
start_file_close_wait_v1_t start_file_close_wait;
581+
end_file_close_wait_v1_t end_file_close_wait;
574582
start_stage_v1_t start_stage;
575583
end_stage_v1_t end_stage;
576584
get_thread_statement_locker_v1_t get_thread_statement_locker;

‎mysys/psi_noop.c‎

Lines changed: 22 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -402,17 +402,17 @@ static void end_table_lock_wait_noop(PSI_table_locker* locker NNN)
402402
return;
403403
}
404404

405-
static PSI_file* start_file_open_wait_noop(PSI_file_locker *locker NNN,
406-
const char *src_file NNN,
407-
uint src_line NNN)
405+
static void start_file_open_wait_noop(PSI_file_locker *locker NNN,
406+
const char *src_file NNN,
407+
uint src_line NNN)
408408
{
409-
return NULL;
409+
return;
410410
}
411411

412-
static void end_file_open_wait_noop(PSI_file_locker *locker NNN,
413-
void *result NNN)
412+
static PSI_file* end_file_open_wait_noop(PSI_file_locker *locker NNN,
413+
void *result NNN)
414414
{
415-
return;
415+
return NULL;
416416
}
417417

418418
static void end_file_open_wait_and_bind_to_descriptor_noop
@@ -435,6 +435,19 @@ static void end_file_wait_noop(PSI_file_locker *locker NNN,
435435
return;
436436
}
437437

438+
static void start_file_close_wait_noop(PSI_file_locker *locker NNN,
439+
const char *src_file NNN,
440+
uint src_line NNN)
441+
{
442+
return;
443+
}
444+
445+
static void end_file_close_wait_noop(PSI_file_locker *locker NNN,
446+
int result NNN)
447+
{
448+
return;
449+
}
450+
438451
static void start_stage_noop(PSI_stage_key key NNN,
439452
const char *src_file NNN, int src_line NNN)
440453
{
@@ -698,6 +711,8 @@ static PSI PSI_noop=
698711
end_file_open_wait_and_bind_to_descriptor_noop,
699712
start_file_wait_noop,
700713
end_file_wait_noop,
714+
start_file_close_wait_noop,
715+
end_file_close_wait_noop,
701716
start_stage_noop,
702717
end_stage_noop,
703718
get_thread_statement_locker_noop,

0 commit comments

Comments
 (0)